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

feat: auto content-type on blob creation #338

Merged
merged 8 commits into from
Jul 15, 2020
Merged

feat: auto content-type on blob creation #338

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9795972
feat: auto content-type on blob creation
dmitry-fa May 26, 2020
a5da742
feat: auto content-type on blob creation
dmitry-fa May 26, 2020
6320364
feat: auto content-type on blob creation
dmitry-fa May 26, 2020
3cc79cb
feat: auto content-type on blob creation
dmitry-fa May 26, 2020
15fe56c
feat: auto content-type on blob creation
dmitry-fa May 26, 2020
6b7f98b
feat: auto content-type on blob creation
dmitry-fa May 26, 2020
6b14a3e
merge
dmitry-fa Jul 14, 2020
736ed2e
detection content type is made optional
dmitry-fa Jul 14, 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
5 changes: 0 additions & 5 deletions google-cloud-storage/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,6 @@
<groupId>org.threeten</groupId>
<artifactId>threetenbp</artifactId>
</dependency>
<dependency>
<groupId>com.sun.activation</groupId>
<artifactId>javax.activation</artifactId>
<version>1.2.0</version>
</dependency>

<!-- Test dependencies -->
<dependency>
Expand Down
14 changes: 9 additions & 5 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,12 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.math.BigInteger;
import java.net.FileNameMap;
import java.net.URLConnection;
import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import javax.activation.MimetypesFileTypeMap;

public class HttpStorageRpc implements StorageRpc {
public static final String DEFAULT_PROJECTION = "full";
Expand All @@ -99,7 +100,7 @@ public class HttpStorageRpc implements StorageRpc {
private final HttpRequestInitializer batchRequestInitializer;

private static final long MEGABYTE = 1024L * 1024L;
private static final MimetypesFileTypeMap MIMETYPES_FILE_TYPE_MAP = new MimetypesFileTypeMap();
private static final FileNameMap FILE_NAME_MAP = URLConnection.getFileNameMap();

public HttpStorageRpc(StorageOptions options) {
HttpTransportOptions transportOptions = (HttpTransportOptions) options.getTransportOptions();
Expand Down Expand Up @@ -376,9 +377,12 @@ public Tuple<String, Iterable<StorageObject>> list(final String bucket, Map<Opti

private static String detectContentType(StorageObject object) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be an optional feature that's disabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about making this feature optional, but I didn't find an appropriate place to store this setting. It could be a property of a project/client/bucket/object.

I only found: https://cloud.google.com/storage/docs/metadata#content-type
which states: If the Content-Type is not specified by the uploader and cannot be determined, it is set to application/octet-stream or application/x-www-form-urlencoded, depending on how you uploaded the object.

If I read this correctly, the Storage should attempt to determine the content type if not explicitly set, so this feature should be on by default.

BTW, right now the content type of blobs created with Storage.create() methods is null, but should be application/octet-stream.

Copy link
Member

Choose a reason for hiding this comment

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

Storage doesn't provide contentType detection in the API.

If contentType is set at creation then application/octet-stream is used.

Please make this optional otherwise it's a behavior change not originally intended on uploads with the Java client.

String contentType = object.getContentType();
return contentType != null
? contentType
: MIMETYPES_FILE_TYPE_MAP.getContentType(object.getName().toLowerCase());
if (contentType != null) {
return contentType;
}
return firstNonNull(
FILE_NAME_MAP.getContentTypeFor(object.getName().toLowerCase()),
Copy link
Contributor

Choose a reason for hiding this comment

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

toLowerCase always should have a locale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"application/octet-stream");
}

private static Function<String, StorageObject> objectFromPrefix(final String bucket) {
Expand Down