feat: auto content-type on blob creation by dmitry-fa · Pull Request #338 · googleapis/java-storage

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

 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

      <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

    }
  }

+  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

    }
  }

+  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 chingor13, elharo and frankyn 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 suggested changes May 29, 2020

View reviewed changes

frankyn suggested 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

Conversation

dmitry-fa commented May 26, 2020

Uh oh!

codecov bot commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-fa commented Jun 30, 2020

Uh oh!

frankyn commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitry-fa commented Jun 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 26, 2020 •

edited

Loading

frankyn commented Jun 30, 2020 •

edited

Loading