allow users to override content type for aux files #8241 by pdurbin · Pull Request #8282 · IQSS/dataverse

pdurbin · 2021-12-06T20:31:59Z

What this PR does / why we need it:

Some auxiliary were being saved with the wrong content type (MIME type) but now the user can supply the content type on upload, overriding the type that would otherwise be assigned.

Which issue(s) this PR closes:

Closes #8241

Special notes for your reviewer:

Markdown files can now be given a better content type of text/markdown but on download, no file extension is added. This is because Tika's getDefaultMimeTypes doesn't include "text/markdown". I left a TODO about how perhaps we could look up more esoteric content types in the MimeTypeDetectionByFileExtension.properties file we maintain (.geojson was added recently, for example).

Suggestions on how to test this:

Run the curl commands. You should be able to set the content type to whatever you want.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

No.

Is there a release notes update needed for this change?:

Included but feel free to leave it out.

Additional documentation:

Included.

qqmyers · 2021-12-06T20:52:12Z

src/main/java/edu/harvard/iq/dataverse/api/DownloadInstanceWriter.java

        }
    }

+    // TODO: Return ".md" for "text/markdown" as well as other extensions in MimeTypeDetectionByFileExtension.properties


FWIW: We have FileUtil.generateOriginalExtension for tabular files but it uses a hardcoded list rather than the properties file.

qqmyers

Looks good!

landreev · 2021-12-07T14:27:29Z

doc/sphinx-guides/source/developers/aux-file-support.rst

  export SERVER_URL=https://demo.dataverse.org

-  curl -H X-Dataverse-key:$API_TOKEN -X POST -F "file=@$FILENAME" -F 'origin=myApp' -F 'isPublic=true' -F "type=$TYPE" "$SERVER_URL/api/access/datafile/$FILE_ID/auxiliary/$FORMAT_TAG/$FORMAT_VERSION"
+  curl -H X-Dataverse-key:$API_TOKEN -X POST -F "file=@$FILENAME;type=$FILETYPE" -F 'origin=myApp' -F 'isPublic=true' -F "type=$TYPE" "$SERVER_URL/api/access/datafile/$FILE_ID/auxiliary/$FORMAT_TAG/$FORMAT_VERSION"


I'm very mildly rubbed the wrong way by the fact that there are now 2 instances of type= in the command line above... Would have been better if we had named that type parameter something more specific (auxtype? dvtype? - idk, something indicating that this is our, proprietary type...) to differentiate it from the standard mime type... But I'm not going to propose changing it now.

This API is for software clients only; nobody should ever be using it on the command line except an occasional developer.

I had the same thought: two cases of "type" is weird and unfortunate. Perhaps we'll do a final cleanup of this API before we move its docs from the dev guide to the API guide and right some of these wrongs.

Whether this specific parameter is worth changing or not, I really like the fact that we have a note in the doc saying that this API is experimental, so we reserve the right to keep changing it without guaranteeing backward compatibility, etc.

landreev · 2021-12-07T14:32:49Z

src/main/java/edu/harvard/iq/dataverse/AuxiliaryFileServiceBean.java


-            Tika tika = new Tika();
-            auxFile.setContentType(tika.detect(storageIO.getAuxFileAsInputStream(auxExtension)));
+            if (mediaType != null) {


Quick question - is mediaType (formDataBodyPart.getMediaType();) definitely going to be null here, if not supplied by the client? - As opposed to defaulting to something like application/octet-stream?

Hmm. You're right. When I leave off ;type=WHATEVER (the old way), mediaType is non-null and its toString method prints application/octet-stream.

So it should just be if (mediaType != null && (!mediaType.equals("application/octet-stream"))) here. We are doing something similar throughout the "normal" datafile upload process.

Otherwise everything looks good in the PR.

I didn't make any code changes but I tweaked the tests so I could make assertions on what happens if you leave off ;type=WHATEVER: 787dbed

I'm happy to make changes, of course. Right now we're saying the user can change the content type to whatever we want. We could change is such that if they supply application/octet-stream as a content type we'll run it though Tika and save whatever Tika says it is. That is, we could do something like this:

if (mediaType != null && !mediaType.toString().equals("application/octet-stream")) { auxFile.setContentType(mediaType.toString()); } else { Tika tika = new Tika(); auxFile.setContentType(tika.detect(storageIO.getAuxFileAsInputStream(auxExtension))); }

Yes, that's what I was saying above, how that "if" line should look like. Nobody would ever want to supply "application/octet-stream" as the type. That type just means "type unknown" or "type not specified".

(otherwise Tika will never be called... and we want to leave this option, of not supplying the type and letting the application identify it)

Yes, sorry, I didn't see that comment until I had written mine. I just pushed the change in 985b552. Please take a look.

Yeah, I think it looks good now - but I'd like @scolapasta and @qqmyers to chime in, on whether we really want to be doing this instead, and not in addition to saving the file name, as uploaded.

"application/octet-stream" is the default when the user doesn't supply a content type. So if it's this, send it through Tika. Yes, a user can supply "application/octet-stream" and this will also be sent through Tika.

landreev · 2021-12-07T17:33:37Z

src/main/java/edu/harvard/iq/dataverse/AuxiliaryFileServiceBean.java

-            Tika tika = new Tika();
-            auxFile.setContentType(tika.detect(storageIO.getAuxFileAsInputStream(auxExtension)));
+            // The null check prevents an NPE but we expect mediaType to be non-null
+            // and to default to "application/octet-stream".


FWIW, I believe the null check is necessary, since the mediaType can still be null, if an input stream is used for the file body.

landreev

OK, the code looks good, and everybody appears to be in agreement, that this is how we want to address both of the underlying issues, the mime type and the file name.
If somehow some picky user requests being able to have non-standard extensions, and/or custom download filenames for these, we'll just addres it separately.

kcondon · 2021-12-07T21:58:23Z

@pdurbin Would you update this pr from develop? I'm getting a flyway mismatch error:
Caused by: org.flywaydb.core.api.FlywayException: Validate failed: Migration checksum mismatch for
migration version 5.8.0.2
-> Applied to database : 1609268027
-> Resolved locally : 343737208

djbrooke · 2021-12-07T22:04:07Z

Hi @kcondon - I just updated from develop.

@pdurbin apologies if I messed something up, but I also trust your git skills to revert in the morning if needed :)

allow users to override content type for aux files #8241

dda1893

pdurbin mentioned this pull request Dec 6, 2021

give expected file extension when downloading auxiliary files #8241 #8255

Closed

qqmyers reviewed Dec 6, 2021

View reviewed changes

landreev reviewed Dec 7, 2021

View reviewed changes

pdurbin added 2 commits December 7, 2021 10:22

assert behavior when user doesn't include ;type=foo/bar #8241

787dbed

allow Tika to detect content type when not supplied #8241

985b552

"application/octet-stream" is the default when the user doesn't supply a content type. So if it's this, send it through Tika. Yes, a user can supply "application/octet-stream" and this will also be sent through Tika.

landreev reviewed Dec 7, 2021

View reviewed changes

landreev approved these changes Dec 7, 2021

View reviewed changes

kcondon self-assigned this Dec 7, 2021

Merge branch 'develop' into 8241-mime

5e0b7d6

kcondon merged commit 80a42de into develop Dec 7, 2021

kcondon deleted the 8241-mime branch December 7, 2021 22:53

djbrooke added this to the 5.9 milestone Dec 7, 2021

This was referenced Dec 8, 2021

auxiliary file download: JSON file extension/type is lost #8241

Closed

Change Deposit API call: add Content-type to the header opendp/dpcreator#431

Closed

Conversation

pdurbin commented Dec 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qqmyers 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!

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!

landreev left a comment

Choose a reason for hiding this comment

Uh oh!

kcondon commented Dec 7, 2021

Uh oh!

djbrooke commented Dec 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants