-
Notifications
You must be signed in to change notification settings - Fork 501
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
allow users to override content type for aux files #8241 #8282
Changes from all commits
dda1893
787dbed
985b552
5e0b7d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ | |
import javax.ws.rs.ClientErrorException; | ||
import javax.ws.rs.InternalServerErrorException; | ||
import javax.ws.rs.ServerErrorException; | ||
import javax.ws.rs.core.MediaType; | ||
import javax.ws.rs.core.Response; | ||
|
||
import org.apache.tika.Tika; | ||
|
@@ -67,10 +68,11 @@ public AuxiliaryFile save(AuxiliaryFile auxiliaryFile) { | |
* @param origin - name of the tool/system that created the file | ||
* @param isPublic boolean - is this file available to any user? | ||
* @param type how to group the files such as "DP" for "Differentially | ||
* @param mediaType user supplied content type (MIME type) | ||
* Private Statistics". | ||
* @return success boolean - returns whether the save was successful | ||
*/ | ||
public AuxiliaryFile processAuxiliaryFile(InputStream fileInputStream, DataFile dataFile, String formatTag, String formatVersion, String origin, boolean isPublic, String type) { | ||
public AuxiliaryFile processAuxiliaryFile(InputStream fileInputStream, DataFile dataFile, String formatTag, String formatVersion, String origin, boolean isPublic, String type, MediaType mediaType) { | ||
|
||
StorageIO<DataFile> storageIO = null; | ||
AuxiliaryFile auxFile = new AuxiliaryFile(); | ||
|
@@ -97,8 +99,14 @@ public AuxiliaryFile processAuxiliaryFile(InputStream fileInputStream, DataFile | |
storageIO.saveInputStreamAsAux(di, auxExtension); | ||
auxFile.setChecksum(FileUtil.checksumDigestToString(di.getMessageDigest().digest())); | ||
|
||
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". | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
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))); | ||
} | ||
auxFile.setFormatTag(formatTag); | ||
auxFile.setFormatVersion(formatVersion); | ||
auxFile.setOrigin(origin); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -577,6 +577,7 @@ private boolean isAuxiliaryObjectCached(StorageIO storageIO, String auxiliaryTag | |
} | ||
} | ||
|
||
// TODO: Return ".md" for "text/markdown" as well as other extensions in MimeTypeDetectionByFileExtension.properties | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW: We have FileUtil.generateOriginalExtension for tabular files but it uses a hardcoded list rather than the properties file. |
||
private String getFileExtension(AuxiliaryFile auxFile) { | ||
String fileExtension = ""; | ||
if (auxFile == null) { | ||
|
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.
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 thattype
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.
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.
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.
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.
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.