Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3ae49fc
determine the content type from file extension and set content-type h…
Aug 6, 2020
002b8be
added system property to control determination of content type from f…
Aug 6, 2020
eb70394
correct checkstyle error
Aug 6, 2020
e71d0d3
fixed comment on system property (flag)
Aug 6, 2020
d456301
add conditional on existing content-type and handle JDK11 activation …
Aug 6, 2020
51a6fd7
removed dependency on javax.activation, use URLConnection rather than…
Aug 6, 2020
928a393
added a comment to indicate behavior of URLConnection.guessContentTyp…
Aug 6, 2020
4ea523b
correct comment, autocontenttype is on by default
Aug 6, 2020
c9761c1
if autoset is on SET then always set the content-type, remove optiona…
Aug 7, 2020
3643d92
moved declaration
Aug 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions wagon-providers/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,5 @@ under the License.
<scope>test</scope>
</dependency>
</dependencies>

</project>
51 changes: 51 additions & 0 deletions ...http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.io.RandomAccessFile;
import java.net.URLConnection;
import java.nio.Buffer;
import java.nio.ByteBuffer;
import java.nio.channels.Channels;
Expand Down Expand Up @@ -122,6 +123,7 @@ public abstract class AbstractHttpClientWagon
final class WagonHttpEntity
extends AbstractHttpEntity
{
public static final String DEFAULT_CONTENT_TYPE = "application/octet-stream";
private final Resource resource;

private final Wagon wagon;
Expand Down Expand Up @@ -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
chrisbeckey marked this conversation as resolved.
Show resolved Hide resolved
*/
private String determineContentType()
{
try
{
String mimeType = this.source != null
? URLConnection.guessContentTypeFromName( this.source.getName() )
: this.stream != null
? URLConnection.guessContentTypeFromStream( this.stream )
: DEFAULT_CONTENT_TYPE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// if URLConnection was unable to determine the type then default it
if ( mimeType == null )
{
mimeType = DEFAULT_CONTENT_TYPE;
}
return mimeType;
}
catch ( IOException e )
{
// will only occur when guessContentTypeFromStream gets an IOException
return DEFAULT_CONTENT_TYPE;
}
}

public Resource getResource()
Expand Down Expand Up @@ -279,6 +324,12 @@ 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" ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Maximum concurrent connections per distinct route.
Expand Down