You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/12/09 10:42:46 UTC
[GitHub] [maven-wagon] chrisbeckey opened a new pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
chrisbeckey opened a new pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72
Set the HTTP content-type header,
- using the file extension to determine value, when uploading a file.
- using the header when uploading a Stream
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] pzygielo commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
pzygielo commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670029018
@chrisbeckey
> Just to be sure I tested it with "1.11" and with "11" and they both worked.
Yeah, but why it did?
I think that 1.11 works only because when
```
[DEBUG] Detected Java String: '11.0.8'
[DEBUG] Normalized Java String: '11.0.8'
```
normalized string is tested against range `[1.11,)` - it is accepted due to 11 (of `11.0.8`) being greater than 1 of (`1.11`).
I am pretty sure it shall be `[11,)`, as you see, with `[1.11,)` for example JDK 10 is also accepted (as `10 > 1`).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670504127
@michael-o Is there a better way to determine content from a Stream (not from a file name, which is handled separately)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466554768
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -169,6 +154,44 @@ private WagonHttpEntity( final InputStream stream, final Resource resource, fina
this.wagon = wagon;
+ // if the autoset content flag is SET and the content type is blank
+ // then try to determine what the content type is and set it
+ if ( AUTOSET_CONTENT_TYPE && getContentType() == null )
+ {
+ try
+ {
+ autosetContentType();
+ }
+ catch ( IOException ioX )
+ {
+ if ( AUTOSET_CONTENT_TYPE_FATAL )
+ {
+ throw new TransferFailedException(
+ "Failed to determine content type, "
+ + " unset 'maven.wagon.http.autocontenttype.fatal' to allow continued processing",
+ ioX );
+ }
+ }
+ }
+ }
+
+ private void autosetContentType() throws IOException
+ {
+ // if the content-type has not been set then
+ // if the option flag is TRUE and the content type is determinable from the file extension
+ // then set it from the file extension
+ final String mimeType = AUTOSET_CONTENT_TYPE
+ ? this.source != null
+ ? URLConnection.guessContentTypeFromName( source.getName() )
+ : this.stream != null
+ ? URLConnection.guessContentTypeFromStream( this.stream )
Review comment:
You cannot do this. This will consume a few magic bytes from the stream making it useless. You can utilize this only with a pushback stream.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670510743
@chrisbeckey Not with external libraries.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670041840
Regarding the version 1.11 vs 11: in following up on the suggestion by @elharo to use URLConnection.guess... the need for javax.activation has been negated. Update to the PR is coming soon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r467157672
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -152,6 +154,49 @@ private WagonHttpEntity( final InputStream stream, final Resource resource, fina
this.length = resource == null ? -1 : resource.getContentLength();
this.wagon = wagon;
+
+ // if the autoset content flag is SET and the content type is blank
+ // then try to determine what the content type is and set it
+ if ( AUTOSET_CONTENT_TYPE && getContentType() == null )
+ {
+ setContentType( determineContentType() );
+ }
+ }
+
+ /**
+ * Best effort to determine the content type.
+ *
+ * if the content is coming from a file and the content type is determinable from the file extension
+ * or
+ * if the content is coming from a stream and the content type is determinable from the stream
+ * (guessContentTypeFromStream will return null if the InputStream does not support mark())
+ * then determine and return the content type
+ * if the content type is not determinable then return "application/octet-stream"
+ *
+ * NOTE: this method is expected to always return a non-empty String
+ */
+ private String determineContentType()
+ {
+ try
+ {
+ String mimeType = this.source != null
+ ? URLConnection.guessContentTypeFromName( this.source.getName() )
+ : this.stream != null
+ ? URLConnection.guessContentTypeFromStream( this.stream )
+ : DEFAULT_CONTENT_TYPE;
Review comment:
I consider this like to be redundant: RFC 7231, section 3.1.1.5:
> If a Content-Type header field is not present, the recipient
MAY either assume a media type of "application/octet-stream"
([RFC2046], Section 4.5.1) or examine the data to determine its type.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] elharo commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r467029478
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +332,20 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension
+ * or the stream header to determine the type, <b>enabled by default</b>
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE =
+ Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype", "true" ) );
+
+ /**
+ * If enabled, then an when determining the content type will result in a fatal exception
Review comment:
an --> an error
will result --> results
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +332,20 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension
+ * or the stream header to determine the type, <b>enabled by default</b>
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE =
+ Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype", "true" ) );
+
+ /**
+ * If enabled, then an when determining the content type will result in a fatal exception
+ * <b>disabled by default</b>
+ * This flag is only effective when maven.wagon.http.autocontenttype is set.
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE_FATAL =
Review comment:
YAGNI. No one needs this. Failure should unconditionally set the content-type to application/octet-stream.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670490920
@chrisbeckey I was referring to this block: https://github.com/battleblow/openjdk-jdk8u/blob/bsd-port/jdk/src/share/classes/java/net/URLConnection.java#L1434-L1562
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey edited a comment on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey edited a comment on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670550158
@michael-o assuming you meant "Not WITHOUT external libraries." ?
IMHO, URLConnection functions over the activation library has the benefit of simplicity. Changing the POM to add the JDK11 profile (and the activation within that) has maintenance implications that are not commensurate with the benefit, considering that this feature has not been previously requested.
In effect using URLConnection will address the majority of use cases with about a dozen lines of code and little expected maintenance.
Of course, reasonable opinions may differ ...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] pzygielo commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
pzygielo commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466589590
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +332,20 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension
+ * or the stream header to determine the type, <b>disabled by default</b>
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE =
+ Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype", "true" ) );
Review comment:
I understand it's pattern copied from existing code - but why in new code not use `parseBoolean` instead of `valueOf` and skip unboxing?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r467133000
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -152,6 +154,51 @@ private WagonHttpEntity( final InputStream stream, final Resource resource, fina
this.length = resource == null ? -1 : resource.getContentLength();
this.wagon = wagon;
+
+ // if the autoset content flag is SET and the content type is blank
+ // then try to determine what the content type is and set it
+ if ( AUTOSET_CONTENT_TYPE && getContentType() == null )
+ {
+ setContentType( determineContentType() );
+ }
+ }
+
+ /**
+ * Best effort to determine the content type.
+ *
+ * if the content is coming from a file and the content type is determinable from the file extension
+ * or
+ * if the content is coming from a stream and the content type is determinable from the stream
+ * (guessContentTypeFromStream will return null if the InputStream does not support mark())
+ * then determine and return the content type
+ * if the content type is not determinable then return "application/octet-stream"
+ *
+ * NOTE: this method is expected to always return a non-empty String
Review comment:
I worded it this way so serve as a warning to future maintainers rather than a statement of current state.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] hiljusti commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
hiljusti commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-989117811
As a note: My (minor) issue could have benefited from this: https://issues.apache.org/jira/browse/MDEPLOY-289
The summary is a custom maven repository proxy, where it would be nice to not default/guess content type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670071258
The stream guessing is almost pointless, look at its code: https://github.com/battleblow/openjdk-jdk8u/blob/bsd-port/jdk/src/share/classes/java/net/URLConnection.java
Path based approach looks better:
```
#sun.net.www MIME content-types table
#
# Property fields:
#
# <description> ::= 'description' '=' <descriptive string>
# <extensions> ::= 'file_extensions' '=' <comma-delimited list, include '.'>
# <image> ::= 'icon' '=' <filename of icon image>
# <action> ::= 'browser' | 'application' | 'save' | 'unknown'
# <application> ::= 'application' '=' <command line template>
#
#
# The "we don't know anything about this data" type(s).
# Used internally to mark unrecognized types.
#
content/unknown: description=Unknown Content
unknown/unknown: description=Unknown Data Type
#
# The template we should use for temporary files when launching an application
# to view a document of given type.
#
temp.file.template: c:\\temp\\%s
#
# The "real" types.
#
application/octet-stream: \
description=Generic Binary Stream;\
file_extensions=.saveme,.dump,.hqx,.arc,.obj,.lib,.bin,.exe,.zip,.gz
application/oda: \
description=ODA Document;\
file_extensions=.oda
application/pdf: \
description=Adobe PDF Format;\
file_extensions=.pdf
application/postscript: \
description=Postscript File;\
file_extensions=.eps,.ai,.ps;\
icon=ps
application/rtf: \
description=Wordpad Document;\
file_extensions=.rtf;\
action=application;\
application=wordpad.exe %s
application/x-dvi: \
description=TeX DVI File;\
file_extensions=.dvi
application/x-hdf: \
description=Hierarchical Data Format;\
file_extensions=.hdf;\
action=save
application/x-latex: \
description=LaTeX Source;\
file_extensions=.latex
application/x-netcdf: \
description=Unidata netCDF Data Format;\
file_extensions=.nc,.cdf;\
action=save
application/x-tex: \
description=TeX Source;\
file_extensions=.tex
application/x-texinfo: \
description=Gnu Texinfo;\
file_extensions=.texinfo,.texi
application/x-troff: \
description=Troff Source;\
file_extensions=.t,.tr,.roff
application/x-troff-man: \
description=Troff Manpage Source;\
file_extensions=.man
application/x-troff-me: \
description=Troff ME Macros;\
file_extensions=.me
application/x-troff-ms: \
description=Troff MS Macros;\
file_extensions=.ms
application/x-wais-source: \
description=Wais Source;\
file_extensions=.src,.wsrc
application/zip: \
description=Zip File;\
file_extensions=.zip;\
icon=zip;\
action=save
application/x-bcpio: \
description=Old Binary CPIO Archive;\
file_extensions=.bcpio;\
action=save
application/x-cpio: \
description=Unix CPIO Archive;\
file_extensions=.cpio;\
action=save
application/x-gtar: \
description=Gnu Tar Archive;\
file_extensions=.gtar;\
icon=tar;\
action=save
application/x-shar: \
description=Shell Archive;\
file_extensions=.sh,.shar;\
action=save
application/x-sv4cpio: \
description=SVR4 CPIO Archive;\
file_extensions=.sv4cpio;\
action=save
application/x-sv4crc: \
description=SVR4 CPIO with CRC;\
file_extensions=.sv4crc;\
action=save
application/x-tar: \
description=Tar Archive;\
file_extensions=.tar;\
icon=tar;\
action=save
application/x-ustar: \
description=US Tar Archive;\
file_extensions=.ustar;\
action=save
audio/basic: \
description=Basic Audio;\
file_extensions=.snd,.au;\
icon=audio
audio/x-aiff: \
description=Audio Interchange Format File;\
file_extensions=.aifc,.aif,.aiff;\
icon=aiff
audio/x-wav: \
description=Wav Audio;\
file_extensions=.wav;\
icon=wav;\
action=application;\
application=mplayer.exe %s
image/gif: \
description=GIF Image;\
file_extensions=.gif;\
icon=gif;\
action=browser
image/ief: \
description=Image Exchange Format;\
file_extensions=.ief
image/jpeg: \
description=JPEG Image;\
file_extensions=.jfif,.jfif-tbnl,.jpe,.jpg,.jpeg;\
icon=jpeg;\
action=browser
image/tiff: \
description=TIFF Image;\
file_extensions=.tif,.tiff;\
icon=tiff
image/vnd.fpx: \
description=FlashPix Image;\
file_extensions=.fpx,.fpix
image/x-cmu-rast: \
description=CMU Raster Image;\
file_extensions=.ras
image/x-portable-anymap: \
description=PBM Anymap Image;\
file_extensions=.pnm
image/x-portable-bitmap: \
description=PBM Bitmap Image;\
file_extensions=.pbm
image/x-portable-graymap: \
description=PBM Graymap Image;\
file_extensions=.pgm
image/x-portable-pixmap: \
description=PBM Pixmap Image;\
file_extensions=.ppm
image/x-rgb: \
description=RGB Image;\
file_extensions=.rgb
image/x-xbitmap: \
description=X Bitmap Image;\
file_extensions=.xbm,.xpm
image/x-xwindowdump: \
description=X Window Dump Image;\
file_extensions=.xwd
image/png: \
description=PNG Image;\
file_extensions=.png;\
icon=png;\
action=browser
image/bmp: \
description=Bitmap Image;\
file_extensions=.bmp;
text/html: \
description=HTML Document;\
file_extensions=.htm,.html;\
icon=html
text/plain: \
description=Plain Text;\
file_extensions=.text,.c,.cc,.c++,.h,.pl,.txt,.java,.el;\
icon=text;\
action=browser
text/tab-separated-values: \
description=Tab Separated Values Text;\
file_extensions=.tsv
text/x-setext: \
description=Structure Enhanced Text;\
file_extensions=.etx
video/mpeg: \
description=MPEG Video Clip;\
file_extensions=.mpg,.mpe,.mpeg;\
icon=mpeg
video/quicktime: \
description=QuickTime Video Clip;\
file_extensions=.mov,.qt
application/x-troff-msvideo: \
description=AVI Video;\
file_extensions=.avi;\
icon=avi;\
action=application;\
application=mplayer.exe %s
video/x-sgi-movie: \
description=SGI Movie;\
file_extensions=.movie,.mv
message/rfc822: \
description=Internet Email Message;\
file_extensions=.mime
application/xml: \
description=XML document;\
file_extensions=.xml
```
You can even supply a custom file with `-Dcontent.types.user.table=....`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] pzygielo commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
pzygielo commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466588481
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +332,20 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension
+ * or the stream header to determine the type, <b>disabled by default</b>
Review comment:
I'm not sure - is it disabled when property is set to `true` (which is set as default for `getProperty` call)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] elharo commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466503929
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -82,6 +82,7 @@
import org.apache.maven.wagon.resource.Resource;
import org.codehaus.plexus.util.StringUtils;
+import javax.activation.MimetypesFileTypeMap;
Review comment:
This class is not significant enough to pull in the javax.activation package. I'm sure there's an alternative somewhere. Or just use `URLConnection.guessContentTypeFromName`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670489868
@michael-o I'm not sure I understand the comment. "guessContentTypeFromStream" is invoked if the source is an InputStream, not when reading from a File, there is no extension available to determine the content from.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] hiljusti edited a comment on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
hiljusti edited a comment on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-989117811
As a note: My (minor) issue could have benefited from this: https://issues.apache.org/jira/browse/MDEPLOY-289
The summary is our enterprise (amazon) used custom maven repository proxies. It would be nice to not default/guess content type when missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] elharo commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r578455690
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +332,20 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension
+ * or the stream header to determine the type, <b>disabled by default</b>
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE =
+ Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype", "true" ) );
Review comment:
A lot of Maven code is 10+ years old and dates back to Java 1.4 and earlier. It can be quite crufty, and consistency alone is not a string argument in this context. The things we really care about are written down in the docs. Everything else should assume best current practices for Java 1.7.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670046025
Updated to use URLConnection.guess***, which also allows this to work with streaming. Added a second property to control behavior when an error in content identification occurs. Default to creating content-type and not failing if determining content-type fails. Thanks to @elharo for the suggestion, this is better than before.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466590246
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +332,20 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension
+ * or the stream header to determine the type, <b>disabled by default</b>
Review comment:
Nice catch, fixed the comment. property is TRUE by default (i.e. content-type will be determined and set)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] elharo commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r467118904
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -152,6 +154,51 @@ private WagonHttpEntity( final InputStream stream, final Resource resource, fina
this.length = resource == null ? -1 : resource.getContentLength();
this.wagon = wagon;
+
+ // if the autoset content flag is SET and the content type is blank
+ // then try to determine what the content type is and set it
+ if ( AUTOSET_CONTENT_TYPE && getContentType() == null )
+ {
+ setContentType( determineContentType() );
+ }
+ }
+
+ /**
+ * Best effort to determine the content type.
+ *
+ * if the content is coming from a file and the content type is determinable from the file extension
+ * or
+ * if the content is coming from a stream and the content type is determinable from the stream
+ * (guessContentTypeFromStream will return null if the InputStream does not support mark())
+ * then determine and return the content type
+ * if the content type is not determinable then return "application/octet-stream"
+ *
+ * NOTE: this method is expected to always return a non-empty String
Review comment:
is expected to always return --> always returns
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -152,6 +154,51 @@ private WagonHttpEntity( final InputStream stream, final Resource resource, fina
this.length = resource == null ? -1 : resource.getContentLength();
this.wagon = wagon;
+
+ // if the autoset content flag is SET and the content type is blank
+ // then try to determine what the content type is and set it
+ if ( AUTOSET_CONTENT_TYPE && getContentType() == null )
+ {
+ setContentType( determineContentType() );
+ }
+ }
+
+ /**
+ * Best effort to determine the content type.
+ *
+ * if the content is coming from a file and the content type is determinable from the file extension
+ * or
+ * if the content is coming from a stream and the content type is determinable from the stream
+ * (guessContentTypeFromStream will return null if the InputStream does not support mark())
+ * then determine and return the content type
+ * if the content type is not determinable then return "application/octet-stream"
+ *
+ * NOTE: this method is expected to always return a non-empty String
+ */
+ private String determineContentType()
+ {
+ String mimeType;
Review comment:
I'd push this into the try block to avoid splitting declaration and initialization and return from there. The catch block can return the default.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466558697
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -169,6 +154,44 @@ private WagonHttpEntity( final InputStream stream, final Resource resource, fina
this.wagon = wagon;
+ // if the autoset content flag is SET and the content type is blank
+ // then try to determine what the content type is and set it
+ if ( AUTOSET_CONTENT_TYPE && getContentType() == null )
+ {
+ try
+ {
+ autosetContentType();
+ }
+ catch ( IOException ioX )
+ {
+ if ( AUTOSET_CONTENT_TYPE_FATAL )
+ {
+ throw new TransferFailedException(
+ "Failed to determine content type, "
+ + " unset 'maven.wagon.http.autocontenttype.fatal' to allow continued processing",
+ ioX );
+ }
+ }
+ }
+ }
+
+ private void autosetContentType() throws IOException
+ {
+ // if the content-type has not been set then
+ // if the option flag is TRUE and the content type is determinable from the file extension
+ // then set it from the file extension
+ final String mimeType = AUTOSET_CONTENT_TYPE
+ ? this.source != null
+ ? URLConnection.guessContentTypeFromName( source.getName() )
+ : this.stream != null
+ ? URLConnection.guessContentTypeFromStream( this.stream )
Review comment:
If the stream does not support mark then guessContentTypeFromStream will return null. Here is the code:
static public String guessContentTypeFromStream(InputStream is)
throws IOException {
// If we can't read ahead safely, just give up on guessing
if (!is.markSupported())
return null;
...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670169900
@chrisbeckey What is your stance on my previous comment?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466592978
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +332,20 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension
+ * or the stream header to determine the type, <b>disabled by default</b>
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE =
+ Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype", "true" ) );
Review comment:
I've noticed that in OSS contributions, at least others I've made, consistency is valued highly. No other reason and, FWIW and IMHO, unboxing/autoboxing should be banished from decent society.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r467149051
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -152,6 +154,51 @@ private WagonHttpEntity( final InputStream stream, final Resource resource, fina
this.length = resource == null ? -1 : resource.getContentLength();
this.wagon = wagon;
+
+ // if the autoset content flag is SET and the content type is blank
+ // then try to determine what the content type is and set it
+ if ( AUTOSET_CONTENT_TYPE && getContentType() == null )
+ {
+ setContentType( determineContentType() );
+ }
+ }
+
+ /**
+ * Best effort to determine the content type.
+ *
+ * if the content is coming from a file and the content type is determinable from the file extension
+ * or
+ * if the content is coming from a stream and the content type is determinable from the stream
+ * (guessContentTypeFromStream will return null if the InputStream does not support mark())
+ * then determine and return the content type
+ * if the content type is not determinable then return "application/octet-stream"
+ *
+ * NOTE: this method is expected to always return a non-empty String
+ */
+ private String determineContentType()
+ {
+ String mimeType;
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466568033
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -169,6 +154,44 @@ private WagonHttpEntity( final InputStream stream, final Resource resource, fina
this.wagon = wagon;
+ // if the autoset content flag is SET and the content type is blank
+ // then try to determine what the content type is and set it
+ if ( AUTOSET_CONTENT_TYPE && getContentType() == null )
+ {
+ try
+ {
+ autosetContentType();
+ }
+ catch ( IOException ioX )
+ {
+ if ( AUTOSET_CONTENT_TYPE_FATAL )
+ {
+ throw new TransferFailedException(
+ "Failed to determine content type, "
+ + " unset 'maven.wagon.http.autocontenttype.fatal' to allow continued processing",
+ ioX );
+ }
+ }
+ }
+ }
+
+ private void autosetContentType() throws IOException
+ {
+ // if the content-type has not been set then
+ // if the option flag is TRUE and the content type is determinable from the file extension
+ // then set it from the file extension
+ final String mimeType = AUTOSET_CONTENT_TYPE
+ ? this.source != null
+ ? URLConnection.guessContentTypeFromName( source.getName() )
+ : this.stream != null
+ ? URLConnection.guessContentTypeFromStream( this.stream )
Review comment:
Javadoc does not even mentioned that. Please add a comment above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466547343
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -82,6 +82,7 @@
import org.apache.maven.wagon.resource.Resource;
import org.codehaus.plexus.util.StringUtils;
+import javax.activation.MimetypesFileTypeMap;
Review comment:
fixed in latest push
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-674779314
There are still statements which have not been responded to. Until then this PR will remain open.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] elharo commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r467062110
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +332,20 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension
+ * or the stream header to determine the type, <b>enabled by default</b>
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE =
+ Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype", "true" ) );
+
+ /**
+ * If enabled, then an when determining the content type will result in a fatal exception
+ * <b>disabled by default</b>
+ * This flag is only effective when maven.wagon.http.autocontenttype is set.
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE_FATAL =
Review comment:
Maintaining current behavior might be important. I'm OK with not setting this is that's what this if it's necessary to maintain compatibility (though won't this detect and set in cases where it's not detected and set now?) But please don't throw a fatal exception here. This is a recoverable case with reasonable defaults.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-989731213
> As a note: My (minor) issue could have benefited from this: https://issues.apache.org/jira/browse/MDEPLOY-289
>
> The summary is our enterprise (amazon) used custom maven repository proxies. It would be nice to not default/guess content type when missing.
Since the detector
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o removed a comment on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o removed a comment on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-989731213
> As a note: My (minor) issue could have benefited from this: https://issues.apache.org/jira/browse/MDEPLOY-289
>
> The summary is our enterprise (amazon) used custom maven repository proxies. It would be nice to not default/guess content type when missing.
Since the detector
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o closed pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o closed pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-670550158
@michael-o assuming you meant "Not WITHOUT external libraries." ?
IMHO, URLConnection functions over the activation library has the benefit of simplicity. Changing the POM to add the JDK11 profile (and the activation within that) has maintenance implications that are not commensurate with the benefit, considering that this feature has not been previously requested.
In effect using URLConnection will address the majority of use cases with about a dozen lines of code and little expected maintenance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-672987918
I will pick this up by the end of the week.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r467047485
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +332,20 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension
+ * or the stream header to determine the type, <b>enabled by default</b>
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE =
+ Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype", "true" ) );
+
+ /**
+ * If enabled, then an when determining the content type will result in a fatal exception
+ * <b>disabled by default</b>
+ * This flag is only effective when maven.wagon.http.autocontenttype is set.
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE_FATAL =
Review comment:
@elharo "Failure should unconditionally set the content-type to application/octet-stream." that is a slight difference from current functionality which does not set the content-type. Just to confirm the resulting behavior is what you are asking for:
if the autoset content type is enabled then a content-type will always be added and the default is application/octet-stream.
If autoset content type is not enabled then the content-type is not set. (current functionality)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on pull request #72: Wagon-HTTP, set content-type when determinable from file extension (MNG-6975)
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-669998232
regarding: "Is it really 1.11? If yes, then this is a bug in Maven."
Just to be sure I tested it with "1.11" and with "11" and they both worked.
I then tried with "12" (I'm running JDK 11) and it failed with the expected
missing package. I think it works either way.
regarding: "per-server config rather than global."
I agree, is there an example you can point me to that I can start from?
On Thu, Aug 6, 2020 at 11:17 AM Michael Osipov <no...@github.com>
wrote:
>
> *This email was sent from an external source so please treat with caution.*
> ------------------------------
> *@michael-o* commented on this pull request.
>
> ------------------------------
>
> In wagon-providers/pom.xml
> <https://github.com/apache/maven-wagon/pull/72#discussion_r466472750>
> :
>
> > @@ -78,4 +78,20 @@ under the License.
> <scope>test</scope>
> </dependency>
> </dependencies>
> +
> + <profiles>
> + <profile>
> + <id>jdk11</id>
> + <activation>
> + <jdk>[1.11,)</jdk>
>
> Is it really 1.11? If yes, then this is a bug in Maven.
> ------------------------------
>
> In
> wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
> <https://github.com/apache/maven-wagon/pull/72#discussion_r466474091>
> :
>
> > @@ -279,6 +296,13 @@ public boolean isStreaming()
> private static final boolean SSL_ALLOW_ALL =
> Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
>
> + /**
> + * If enabled, then the content-type HTTP header will be set using the file extension to determine the type,
> + * <b>disabled by default</b>
> + * This flag is only effective when uploading from a File, not directly from a Stream.
> + */
> + private static final boolean SET_CONTENT_TYPE_FROM_FILE_EXTENSION =
>
> I wonder whether this should be a per-server config rather than global. In
> general, sys props are either globally or for a technical reason. WDYT?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/maven-wagon/pull/72#pullrequestreview-462598973>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AMYZJ7EINPE3U7SSVUIEOODR7LCITANCNFSM4PWVNA4A>
> .
>
About Ascential plc: Ascential (LSE:ASCL.L) is a specialist information, data and analytics company that helps the world’s most ambitious businesses win in the digital economy. Our information, insights, connections, data and digital tools solve customer problems in three disciplines:
Product Design via global trend forecasting service WGSN;
Marketing via global benchmark for creative excellence and effectiveness Cannes Lions and WARC and strategic advisory firm MediaLink; and
Sales via ecommerce-driven data, insights and advisory service Edge by Ascential, leading managed services provider for Amazon Flywheel Digital, the world's premier payments and Fin Tech congress Money20/20, global retail industry summit World Retail Congress and retail news outlet Retail Week.
Ascential also powers political, construction and environmental intelligence brands DeHavilland, Glenigan and Groundsure.
The information in or attached to this email is confidential and may be legally privileged. If you are not the intended recipient of this message, any use, disclosure, copying, distribution or any action taken in reliance on it is prohibited and may be unlawful.
If you have received this message in error, please notify the sender immediately by return email and delete this message and any copies from your computer and network. Ascential does not warrant that this email and any attachments are free from viruses and accepts no liability for any loss resulting from infected email transmissions.
Ascential reserves the right to monitor all email through its networks. Any view expressed may be those of the originator and not necessarily of Ascential plc.
Please be advised all phone calls may be recorded for training and quality purposes and by accepting and/or making calls to us you acknowledge and agree to calls being recorded.
Ascential plc, number 9934451 (England and Wales). Registered Office: The Prow, 1 Wilder Walk, London W1B 5AP.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on a change in pull request #72: Wagon-HTTP, set content-type when determinable from file extension (MNG-6975)
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466472750
##########
File path: wagon-providers/pom.xml
##########
@@ -78,4 +78,20 @@ under the License.
<scope>test</scope>
</dependency>
</dependencies>
+
+ <profiles>
+ <profile>
+ <id>jdk11</id>
+ <activation>
+ <jdk>[1.11,)</jdk>
Review comment:
Is it really 1.11? If yes, then this is a bug in Maven.
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +296,13 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension to determine the type,
+ * <b>disabled by default</b>
+ * This flag is only effective when uploading from a File, not directly from a Stream.
+ */
+ private static final boolean SET_CONTENT_TYPE_FROM_FILE_EXTENSION =
Review comment:
I wonder whether this should be a per-server config rather than global. In general, sys props are either globally or for a technical reason. WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-989731423
Closed by accident
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] chrisbeckey commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
chrisbeckey commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r467070592
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -279,6 +332,20 @@ public boolean isStreaming()
private static final boolean SSL_ALLOW_ALL =
Boolean.valueOf( System.getProperty( "maven.wagon.http.ssl.allowall", "false" ) );
+ /**
+ * If enabled, then the content-type HTTP header will be set using the file extension
+ * or the stream header to determine the type, <b>enabled by default</b>
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE =
+ Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype", "true" ) );
+
+ /**
+ * If enabled, then an when determining the content type will result in a fatal exception
+ * <b>disabled by default</b>
+ * This flag is only effective when maven.wagon.http.autocontenttype is set.
+ */
+ private static final boolean AUTOSET_CONTENT_TYPE_FATAL =
Review comment:
I think that the following behavior is simple to explain and therefore favored:
1.) if autoset is enabled then a content-type header will always be written and defaults to application/octet-stream when the type cannot be determined
2.) if autoset is not enabled then the content-type is not written (unless it is done externally to the wagon library)
So, no exception and the second flag to control the exception behavior is not needed.
How does that sound?
Finally, should autoset behavior be enabled or disabled by default? disabled maintains current behavior, enabled could be said to be consistent with the content-length header, which is always provided.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on a change in pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#discussion_r466554768
##########
File path: wagon-providers/wagon-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
##########
@@ -169,6 +154,44 @@ private WagonHttpEntity( final InputStream stream, final Resource resource, fina
this.wagon = wagon;
+ // if the autoset content flag is SET and the content type is blank
+ // then try to determine what the content type is and set it
+ if ( AUTOSET_CONTENT_TYPE && getContentType() == null )
+ {
+ try
+ {
+ autosetContentType();
+ }
+ catch ( IOException ioX )
+ {
+ if ( AUTOSET_CONTENT_TYPE_FATAL )
+ {
+ throw new TransferFailedException(
+ "Failed to determine content type, "
+ + " unset 'maven.wagon.http.autocontenttype.fatal' to allow continued processing",
+ ioX );
+ }
+ }
+ }
+ }
+
+ private void autosetContentType() throws IOException
+ {
+ // if the content-type has not been set then
+ // if the option flag is TRUE and the content type is determinable from the file extension
+ // then set it from the file extension
+ final String mimeType = AUTOSET_CONTENT_TYPE
+ ? this.source != null
+ ? URLConnection.guessContentTypeFromName( source.getName() )
+ : this.stream != null
+ ? URLConnection.guessContentTypeFromStream( this.stream )
Review comment:
You cannot do this. This will confuse a few magic bytes from the stream making it useless. You can utilize this only with a pushback stream.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wagon] michael-o commented on pull request #72: [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #72:
URL: https://github.com/apache/maven-wagon/pull/72#issuecomment-730253846
Still interested in completing this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org