-
Notifications
You must be signed in to change notification settings - Fork 481
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.
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
Add mime-type guessing logic based on the filename, with default mime-types as fallbacks. #1265
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
...e/src/main/java/org/datatransferproject/datatransfer/google/mediaModels/GoogleMediaItem.java
Outdated
Show resolved
Hide resolved
Map<String, String> filenameToMimeTypeMap = Map.of( | ||
"file.jpg", "image/jpeg", | ||
"file.png", "image/png", | ||
"file.apng", "image/apng", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI it looks like the gradle test is failing on this file type:
GoogleMediaItemTest > getMimeType_photoModel_mimeTypeFromFilename FAILED
org.opentest4j.AssertionFailedError: expected: <image/apng> but was: <image/vnd.mozilla.apng>
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1141)
at org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I believe I found the issue here. According to the docs, probeContentType "uses the installed FileTypeDetector implementation to probe the given file to determine its content type."
Locally, my installed FileTypeDetector returns "image/apng" for 'file-name.apng', but it seems like in our pipeline we might have a different FileTypeDetector? I am not sure where this FileTypeDetector is coming from, maybe it depends on the OS running the pipeline? Or perhaps it is a Gradle specified detail? I can't seem to find more info on this, but I think the most important part of this is that it indicates that we must decide on a mimeType conversion convention to prescribe to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of focusing on testing the probeContentType implementation, i think its fine to focus on testing the fallback logic. so i think its fine to remove this file type from the test.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me! I do think we should make note of this though, since it feels like indeterministic code. On one machine the mimetype may be x, on another it may be y.
New mime-type logic which guesses the extension based on the file name using probeContentType in java.nio.file.Files. If we cannot guess the mimetype, we fallback on the item's specified mimetype. If that is empty, we then fallback on a default. The default for photos is 'image/jpg' and for videos 'video/mp4'.