[MNG-6975] Wagon-HTTP, set content-type when determinable from file extension #72

chrisbeckey · 2020-08-06T14:52:47Z

Set the HTTP content-type header,

using the file extension to determine value, when uploading a file.
using the header when uploading a Stream

…eader if that is not empty

…ile extension

…lib removal

michael-o · 2020-08-06T14:54:46Z

wagon-providers/pom.xml

+    <profile>
+      <id>jdk11</id>
+      <activation>
+        <jdk>[1.11,)</jdk>


Is it really 1.11? If yes, then this is a bug in Maven.

michael-o · 2020-08-06T14:56:39Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+     * <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?

chrisbeckey · 2020-08-06T15:26:10Z

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 ***@***.***> wrote: *This email was sent from an external source so please treat with caution.* ------------------------------ ***@***.**** commented on this pull request. ------------------------------ In wagon-providers/pom.xml <#72 (comment)> : > @@ -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 <#72 (comment)> : > @@ -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 <#72 (review)>, 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.

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

pzygielo · 2020-08-06T16:17:24Z

@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).

chrisbeckey · 2020-08-06T16:39:42Z

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.

… File extension to mime type map.

chrisbeckey · 2020-08-06T16:47:13Z

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.

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

michael-o · 2020-08-06T17:30:07Z

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=.....

…eFromStream()

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

pzygielo · 2020-08-06T17:58:34Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+     * 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" ) );


I understand it's pattern copied from existing code - but why in new code not use parseBoolean instead of valueOf and skip unboxing?

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.

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.

michael-o · 2020-08-06T20:15:09Z

@chrisbeckey What is your stance on my previous comment?

chrisbeckey · 2020-08-07T12:21:54Z

@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.

michael-o · 2020-08-07T12:24:41Z

@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

chrisbeckey · 2020-08-07T13:00:18Z

@michael-o Is there a better way to determine content from a Stream (not from a file name, which is handled separately)?

elharo · 2020-08-07T13:09:36Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+            Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype", "true" ) );
+
+    /**
+     * If enabled, then an when determining the content type will result in a fatal exception


an --> an error
will result --> results

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

michael-o · 2020-08-07T13:15:22Z

@chrisbeckey Not with external libraries.

chrisbeckey · 2020-08-07T14:39:39Z

@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 ...

…l exception behavior

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

michael-o · 2020-08-07T16:55:33Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+                            ? URLConnection.guessContentTypeFromName( this.source.getName() )
+                            : this.stream != null
+                                ? URLConnection.guessContentTypeFromStream( this.stream )
+                                : DEFAULT_CONTENT_TYPE;


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.

I agree this is redundant, and I think most of the interest in this change is from people using client software that's bespoke, naive, or maybe completely ignorant of the RFC.

That said, I'd recommend doing it anyway. It's becoming apparent to me that there is a lot of client software out there that doesn't know/implement RFC 7231, section 3.1.1.5. It's perfectly correct to not do this, but it's also a good way to keep getting asked the question by newbies (like I was). Unless you really want to keep educating people about it, I'd recommend just returning the default that the caller should assume.

michael-o

I am not convinced that it is the entity's task to set the content type. It is the caller's task The HttpEntity is solely designed to carry data w/o any logic at best.

michael-o · 2020-08-12T16:45:30Z

I will pick this up by the end of the week.

michael-o · 2020-08-17T09:46:40Z

There are still statements which have not been responded to. Until then this PR will remain open.

michael-o · 2020-11-19T09:45:00Z

Still interested in completing this?

booniepepper · 2021-12-08T19:11:17Z

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.

michael-o · 2021-12-09T10:42:33Z

Closed by accident

marcelstoer · 2022-09-21T15:52:33Z

I am still interested in seen this land.

In our case the WAF complained about the absence of a content-type header when uploading a JAR. I guess it would have been fine seeing application/octet-stream - had there been a header at all. (sorry, if there's a separate issue for that, didn't find one)

[DEBUG] http-outgoing-0 >> PUT /artifactory/the/rest/is/redacted.jar HTTP/1.1
[DEBUG] http-outgoing-0 >> Cache-control: no-cache
[DEBUG] http-outgoing-0 >> Pragma: no-cache
[DEBUG] http-outgoing-0 >> User-Agent: Apache-Maven/3.8.5 (Java 11.0.9.1; Mac OS X 10.16)
[DEBUG] http-outgoing-0 >> Content-Length: 17856154
[DEBUG] http-outgoing-0 >> Host: my.artifactory.host
[DEBUG] http-outgoing-0 >> Connection: Keep-Alive
[DEBUG] http-outgoing-0 >> Expect: 100-continue
[DEBUG] http-outgoing-0 >> Cookie: SCDID_S=redacted$$
[DEBUG] http-outgoing-0 >> Accept-Encoding: gzip,deflate
[DEBUG] http-outgoing-0 >> Authorization: Basic *redacted*
[DEBUG] http-outgoing-0 << HTTP/1.1 100 Continue

P.S. Maybe the description of this PR should mention that it'll fix https://issues.apache.org/jira/browse/WAGON-599.

michael-o · 2022-09-21T16:21:31Z

This is a bug in your WAF: https://www.rfc-editor.org/rfc/rfc9110.html#name-content-type
Nothing wrong with the request

marcelstoer · 2022-09-21T17:49:52Z

I didn't claim there be anything "wrong" with the request. Of course we can adjust the security profile in the WAF such that it allows PUT requests without content-type (which we did). Still, I see this PR as a useful addition to wagon.

Chris Beckey added 5 commits August 6, 2020 09:19

determine the content type from file extension and set content-type h…

3ae49fc

…eader if that is not empty

added system property to control determination of content type from f…

002b8be

…ile extension

correct checkstyle error

eb70394

fixed comment on system property (flag)

e71d0d3

add conditional on existing content-type and handle JDK11 activation …

d456301

…lib removal

michael-o reviewed Aug 6, 2020

View reviewed changes

elharo changed the title ~~Wagon-HTTP, set content-type when determinable from file extension (MNG-6975)~~ [MNG-6975] Wagon-HTTP, set content-type when determinable from file extension Aug 6, 2020

elharo requested changes Aug 6, 2020

View reviewed changes

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java Outdated Show resolved Hide resolved

removed dependency on javax.activation, use URLConnection rather than…

51a6fd7

… File extension to mime type map.

michael-o requested changes Aug 6, 2020

View reviewed changes

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java Outdated Show resolved Hide resolved

added a comment to indicate behavior of URLConnection.guessContentTyp…

928a393

…eFromStream()

pzygielo reviewed Aug 6, 2020

View reviewed changes

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java Outdated Show resolved Hide resolved

pzygielo reviewed Aug 6, 2020

View reviewed changes

correct comment, autocontenttype is on by default

4ea523b

elharo reviewed Aug 7, 2020

View reviewed changes

if autoset is on SET then always set the content-type, remove optiona…

c9761c1

…l exception behavior

elharo approved these changes Aug 7, 2020

View reviewed changes

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java Outdated Show resolved Hide resolved

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java Show resolved Hide resolved

moved declaration

3643d92

michael-o reviewed Aug 7, 2020

View reviewed changes

michael-o closed this Dec 9, 2021

michael-o reopened this Dec 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNG-6975] Wagon-HTTP, set content-type when determinable from file extension #72

[MNG-6975] Wagon-HTTP, set content-type when determinable from file extension #72

chrisbeckey commented Aug 6, 2020 •

edited

michael-o Aug 6, 2020

michael-o Aug 6, 2020

chrisbeckey commented Aug 6, 2020 via email

pzygielo commented Aug 6, 2020

chrisbeckey commented Aug 6, 2020

chrisbeckey commented Aug 6, 2020

michael-o commented Aug 6, 2020

pzygielo Aug 6, 2020

chrisbeckey Aug 6, 2020

elharo Feb 18, 2021

michael-o commented Aug 6, 2020

chrisbeckey commented Aug 7, 2020

michael-o commented Aug 7, 2020

chrisbeckey commented Aug 7, 2020

elharo Aug 7, 2020

michael-o commented Aug 7, 2020

chrisbeckey commented Aug 7, 2020 •

edited

michael-o Aug 7, 2020

booniepepper Sep 25, 2022

michael-o left a comment •

edited

michael-o commented Aug 12, 2020

michael-o commented Aug 17, 2020

michael-o commented Nov 19, 2020

booniepepper commented Dec 8, 2021 •

edited

michael-o commented Dec 9, 2021

marcelstoer commented Sep 21, 2022

michael-o commented Sep 21, 2022

marcelstoer commented Sep 21, 2022

[MNG-6975] Wagon-HTTP, set content-type when determinable from file extension #72

Are you sure you want to change the base?

[MNG-6975] Wagon-HTTP, set content-type when determinable from file extension #72

Conversation

chrisbeckey commented Aug 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbeckey commented Aug 6, 2020 via email

pzygielo commented Aug 6, 2020

chrisbeckey commented Aug 6, 2020

chrisbeckey commented Aug 6, 2020

michael-o commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-o commented Aug 6, 2020

chrisbeckey commented Aug 7, 2020

michael-o commented Aug 7, 2020

chrisbeckey commented Aug 7, 2020

Choose a reason for hiding this comment

michael-o commented Aug 7, 2020

chrisbeckey commented Aug 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-o left a comment • edited

Choose a reason for hiding this comment

michael-o commented Aug 12, 2020

michael-o commented Aug 17, 2020

michael-o commented Nov 19, 2020

booniepepper commented Dec 8, 2021 • edited

michael-o commented Dec 9, 2021

marcelstoer commented Sep 21, 2022

michael-o commented Sep 21, 2022

marcelstoer commented Sep 21, 2022

chrisbeckey commented Aug 6, 2020 •

edited

chrisbeckey commented Aug 7, 2020 •

edited

michael-o left a comment •

edited

booniepepper commented Dec 8, 2021 •

edited