Skip to content

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

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

Draft
chrisbeckey wants to merge 10 commits into
base: master
Choose a base branch
from
+52 −0
Draft

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

Show file tree
Hide file tree
Changes from 1 commit
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
15 changes: 0 additions & 15 deletions wagon-providers/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,4 @@ under the License.
</dependency>
</dependencies>

<profiles>
<profile>
<id>jdk11</id>
<activation>
<jdk>[1.11,)</jdk>
</activation>
<dependencies>
<dependency>
<groupId>com.sun.activation</groupId>
<artifactId>javax.activation</artifactId>
<version>1.2.0</version>
</dependency>
</dependencies>
</profile>
</profiles>
</project>
70 changes: 50 additions & 20 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 @@ -82,7 +82,6 @@
import org.apache.maven.wagon.resource.Resource;
import org.codehaus.plexus.util.StringUtils;

import javax.activation.MimetypesFileTypeMap;
import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.SSLContext;
import java.io.Closeable;
Expand All @@ -92,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 @@ -120,8 +120,6 @@
public abstract class AbstractHttpClientWagon
extends StreamWagon
{
private static final MimetypesFileTypeMap FILE_TYPE_MAP = new MimetypesFileTypeMap();

final class WagonHttpEntity
extends AbstractHttpEntity
{
Expand All @@ -145,19 +143,6 @@ private WagonHttpEntity( final InputStream stream, final Resource resource, fina
{
this.source = source;
this.repeatable = true;
// 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
if ( getContentType() == null )
{
final String mimeType = SET_CONTENT_TYPE_FROM_FILE_EXTENSION
? FILE_TYPE_MAP.getContentType( source )
: null;
if ( mimeType != null && !mimeType.isEmpty() )
{
setContentType( mimeType );
}
}
}
else
{
Expand All @@ -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 )
Comment thread
chrisbeckey marked this conversation as resolved.
Outdated
: null
: null;
if ( mimeType != null && !mimeType.isEmpty() )
{
setContentType( mimeType );
}

}

public Resource getResource()
Expand Down Expand Up @@ -297,12 +320,19 @@ public boolean isStreaming()
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,
* 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>
Comment thread
chrisbeckey marked this conversation as resolved.
Outdated
*/
private static final boolean AUTOSET_CONTENT_TYPE =
Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype", "true" ) );
Copy link
Copy Markdown
Contributor

@pzygielo pzygielo Aug 6, 2020

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
Copy Markdown
Author

@chrisbeckey chrisbeckey Aug 6, 2020

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
Copy Markdown
Contributor

@elharo elharo Feb 18, 2021 •
edited
Loading

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


/**
* If enabled, then an when determining the content type will result in a fatal exception
Copy link
Copy Markdown
Contributor

@elharo elharo Aug 7, 2020

Choose a reason for hiding this comment

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

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

* <b>disabled by default</b>
* This flag is only effective when uploading from a File, not directly from a Stream.
* This flag is only effective when maven.wagon.http.autocontenttype is set.
*/
private static final boolean SET_CONTENT_TYPE_FROM_FILE_EXTENSION =
Boolean.valueOf( System.getProperty( "maven.wagon.http.file.autocontenttype", "false" ) );
private static final boolean AUTOSET_CONTENT_TYPE_FATAL =
Comment thread
chrisbeckey marked this conversation as resolved.
Outdated
Boolean.valueOf( System.getProperty( "maven.wagon.http.autocontenttype.fatal", "false" ) );

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