feat: auto content-type on blob creation #338

dmitry-fa · 2020-05-26T12:40:50Z

Fixes #47

Proposed fix of the auto detection of the content type based on the blob name, like other clients do (php, nodejs, go).

The class javax.activation.MimetypesFileTypeMap from JDK is used to detect the content type. Using this class with Java 11+ requires the dependency on Java Activation Framework.

codecov · 2020-05-26T12:42:11Z

Codecov Report

Merging #338 into master will decrease coverage by 0.06%.
The diff coverage is 28.57%.

@@             Coverage Diff              @@
##             master     #338      +/-   ##
============================================
- Coverage     63.27%   63.20%   -0.07%     
- Complexity      609      619      +10     
============================================
  Files            32       32              
  Lines          5113     5123      +10     
  Branches        487      488       +1     
============================================
+ Hits           3235     3238       +3     
- Misses         1713     1720       +7     
  Partials        165      165

Impacted Files	Coverage Δ	Complexity Δ
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java	`1.60% <11.11%> (+0.09%)`	`2.00 <1.00> (+1.00)`
...rc/main/java/com/google/cloud/storage/Storage.java	`80.54% <33.33%> (-0.36%)`	`0.00 <0.00> (ø)`
...va/com/google/cloud/storage/spi/v1/StorageRpc.java	`65.33% <100.00%> (+0.46%)`	`0.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12bd62c...736ed2e. Read the comment docs.

elharo · 2020-05-26T12:55:16Z

google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java

@@ -81,6 +81,7 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import javax.activation.MimetypesFileTypeMap;


Use com.google.common.net.MediaType so you don't introduce another dependency, and ping the guava team to get this out of beta

com.google.common.net.MediaType doesn't provide mapping extension to the meida type. Its parse method recognizes strings like "text/plain".

What about Files.probeContentType(Path)?

or URLConnection

Both Files.probeContentType(Path) and URLConnection work! I will use it instead. Thanks for the hint.

elharo · 2020-05-26T15:10:31Z

google-cloud-storage/pom.xml

@@ -88,6 +88,11 @@
      <groupId>org.threeten</groupId>
      <artifactId>threetenbp</artifactId>
    </dependency>
+    <dependency>


isn't this included in java 6 and later?

it works with jdk7,8. This dependency is required for java9+

elharo · 2020-05-26T19:07:55Z

google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java

+      return contentType;
+    }
+    return firstNonNull(
+        FILE_NAME_MAP.getContentTypeFor(object.getName().toLowerCase()),


toLowerCase always should have a locale

elharo · 2020-05-26T20:26:26Z

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

+      case "create":
+        return storage.create(blobInfo);
+      case "writer":
+        {


why the braces here?

elharo · 2020-05-26T20:26:56Z

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

+    String customType = "custom/type";
+    BlobId blobId = BlobId.of(BUCKET, names[0]);
+    BlobInfo blobInfo = BlobInfo.newBuilder(blobId).setContentType(customType).build();
+    blob = createBlob(method, blobInfo);


don't reuse local variable. There are two separate blob objects here that coincidentally share a variable.

I had a reason to reuse the variable but after rewriting the test that reason is not actual anymore. Thanks for catching that.

elharo · 2020-05-26T20:27:25Z

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

+  private void testAutoContentType(String method) throws IOException {
+    String[] names = {"file1.txt", "dir with spaces/Pic.Jpg", "no_extension"};
+    String[] types = {"text/plain", "image/jpeg", "application/octet-stream"};
+    Blob blob = null;


I think you can push this into the for loop

frankyn

Hi @dmitry-fa, thanks for addressing the request. Have two requests.

frankyn · 2020-05-26T19:02:11Z

google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java

@@ -372,6 +375,16 @@ public StorageObject create(
    }
  }

+  private static String detectContentType(StorageObject object) {


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

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.

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.

frankyn · 2020-05-26T19:03:39Z

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

+  }
+
+  private void testAutoContentType(String method) throws IOException {
+    String[] names = {"file1.txt", "dir with spaces/Pic.Jpg", "no_extension"};


Is there a way to infer the type based on bytes rather than file extension? That's how Golang does it.

It's not always possible. In case of resumeable upload a blob is created first and the content is available later:

writer = storage.writer(blobInfo); // creating a blob, detecting content-type, no contect writer.write(content); writer.close();

I did an experiment:
I created a file PDF.txt with a pdf content. Then I uploaded it as PDF.jpg with various clients and checked the resulting content type. I got the following:

gsutil: text/plain
NodeJS: image/jpeg
Go: application/pdf

To achieve consistent behavior across all the clients it should be implemented on the server-side at the moment when the upload is finished.

Until this feature is implemented I suggest type detecting based on file extension, as it has been done recently in nodejs (googleapis/nodejs-storage#1190)

frankyn · 2020-06-25T15:31:47Z

google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java

@@ -372,6 +375,16 @@ public StorageObject create(
    }
  }

+  private static String detectContentType(StorageObject object) {


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.

dmitry-fa · 2020-06-30T15:48:56Z

@frankyn,

For the time being, objects created with Storage.create() method have content-type set to null if not explicitly set, not application/octet-stream.

Is it possible to store custom properties on the project level?
I mean something like: storage.setProperty('java.contentTypeDetection', 'extension')

frankyn · 2020-06-30T17:23:21Z

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

Whoops I meant, If contentType is not set at creation then application/octet-stream is used by the service.

Please introduce this as a new option in the request instead of a client level option for now.

dmitry-fa · 2020-06-30T19:04:51Z

Not sure if it will be convenient to specify 'I want to detect the content type' each time when a new object is created. The content type could be provided explicitly instead.

Okay, I'll think about that. If the user specifies explicitly that he wants to detect the content type, he can also select a strategy for that: 'by extension', 'by content' or 'by both'.

dmitry-fa added 3 commits May 26, 2020 11:58

feat: auto content-type on blob creation

9795972

feat: auto content-type on blob creation

a5da742

feat: auto content-type on blob creation

6320364

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 26, 2020

dmitry-fa requested review from frankyn, elharo and chingor13 May 26, 2020 12:41

elharo reviewed May 26, 2020

View reviewed changes

feat: auto content-type on blob creation

3cc79cb

elharo reviewed May 26, 2020

View reviewed changes

feat: auto content-type on blob creation

15fe56c

elharo reviewed May 26, 2020

View reviewed changes

feat: auto content-type on blob creation

6b7f98b

frankyn requested changes May 29, 2020

View reviewed changes

frankyn requested changes Jun 25, 2020

View reviewed changes

dmitry-fa added 2 commits July 14, 2020 14:11

merge

6b14a3e

detection content type is made optional

736ed2e

frankyn approved these changes Jul 15, 2020

View reviewed changes

frankyn merged commit 66d1eb7 into googleapis:master Jul 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auto content-type on blob creation #338

feat: auto content-type on blob creation #338

dmitry-fa commented May 26, 2020

codecov bot commented May 26, 2020 •

edited

Loading

elharo May 26, 2020

dmitry-fa May 26, 2020

elharo May 26, 2020

elharo May 26, 2020

dmitry-fa May 26, 2020

elharo May 26, 2020

dmitry-fa May 26, 2020

elharo May 26, 2020

dmitry-fa May 26, 2020

elharo May 26, 2020

elharo May 26, 2020

dmitry-fa May 26, 2020

elharo May 26, 2020

frankyn left a comment

frankyn May 26, 2020

dmitry-fa Jun 1, 2020

frankyn Jun 25, 2020

frankyn May 26, 2020

dmitry-fa Jun 1, 2020

frankyn Jun 25, 2020

dmitry-fa commented Jun 30, 2020

frankyn commented Jun 30, 2020 •

edited

Loading

dmitry-fa commented Jun 30, 2020

feat: auto content-type on blob creation #338

feat: auto content-type on blob creation #338

Conversation

dmitry-fa commented May 26, 2020

codecov bot commented May 26, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-fa commented Jun 30, 2020

frankyn commented Jun 30, 2020 • edited Loading

dmitry-fa commented Jun 30, 2020

codecov bot commented May 26, 2020 •

edited

Loading

frankyn commented Jun 30, 2020 •

edited

Loading