Skip to content

Don't return file when some derived info is requested. #6244

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.

Sign up for GitHub

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

Merged
kcondon merged 3 commits into from
Oct 4, 2019
Merged

Don't return file when some derived info is requested. #6244

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e91866
Don't return file when some derived info is requested.
qqmyers Oct 1, 2019
e756933
fix logic for #6244
qqmyers Oct 2, 2019
9e14d7d
Merge remote-tracking branch 'IQSS/develop' into IQSS/6240
qqmyers Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 59 additions & 46 deletions src/main/java/edu/harvard/iq/dataverse/api/Access.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ private DataFile findDataFileOrDieWrapper(String fileId){
df = findDataFileOrDie(fileId);
} catch (WrappedResponse ex) {
logger.warning("Access: datafile service could not locate a DataFile object for id "+fileId+"!");
logger.warning(ex.getWrappedMessageWhenJson());
throw new NotFoundException();
}
return df;
Expand Down Expand Up @@ -312,66 +313,78 @@ public DownloadInstance datafile(@PathParam("fileId") String fileId, @QueryParam
downloadInstance.setDataverseRequestService(dvRequestService);
downloadInstance.setCommand(engineSvc);
}
boolean serviceRequested = false;
boolean serviceFound = false;
Comment on lines +316 to +317
Copy link
Member Author

@qqmyers qqmyers Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main change is to run through the loop of query params and to set serviceRequested = true if any of the ones required by a service are sent, and to then run the checkIfServiceSupportedAndSetConverter ~as before (now only run on service params) to decide if the specific service needed exists.

After the loop, a check to see if a service was requested but not found, and, if so, to throw a not found exception. Otherwise, we're good to go (as before)


for (String key : uriInfo.getQueryParameters().keySet()) {
String value = uriInfo.getQueryParameters().getFirst(key);
logger.fine("is download service supported? key="+key+", value="+value);

if (downloadInstance.checkIfServiceSupportedAndSetConverter(key, value)) {
// this automatically sets the conversion parameters in
// the download instance to key and value;
// TODO: I should probably set these explicitly instead.
logger.fine("yes!");

if (downloadInstance.getConversionParam().equals("subset")) {
String subsetParam = downloadInstance.getConversionParamValue();
String variableIdParams[] = subsetParam.split(",");
if (variableIdParams != null && variableIdParams.length > 0) {
logger.fine(variableIdParams.length + " tokens;");
for (int i = 0; i < variableIdParams.length; i++) {
logger.fine("token: " + variableIdParams[i]);
String token = variableIdParams[i].replaceFirst("^v", "");
Long variableId = null;
try {
variableId = new Long(token);
} catch (NumberFormatException nfe) {
variableId = null;
}
if (variableId != null) {
logger.fine("attempting to look up variable id " + variableId);
if (variableService != null) {
DataVariable variable = variableService.find(variableId);
if (variable != null) {
if (downloadInstance.getExtraArguments() == null) {
downloadInstance.setExtraArguments(new ArrayList<Object>());
logger.fine("is download service supported? key=" + key + ", value=" + value);
// The loop goes through all query params (e.g. including key, gbrecs, persistentId, etc. )
// So we need to identify when a service is being called and then let checkIfServiceSupportedAndSetConverter see if the required one exists
if (key.equals("imageThumb") || key.equals("format") || key.equals("variables")) {
serviceRequested = true;
//Only need to check if this key is associated with a service
if (downloadInstance.checkIfServiceSupportedAndSetConverter(key, value)) {
Copy link
Member Author

@qqmyers qqmyers Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because this used to run for any params like :persistentId, gbrecs, key it wasn't possible to just put an exception in an else clause (as was commented out in the previous code.

// this automatically sets the conversion parameters in
// the download instance to key and value;
// TODO: I should probably set these explicitly instead.
logger.fine("yes!");

if (downloadInstance.getConversionParam().equals("subset")) {
String subsetParam = downloadInstance.getConversionParamValue();
String variableIdParams[] = subsetParam.split(",");
if (variableIdParams != null && variableIdParams.length > 0) {
logger.fine(variableIdParams.length + " tokens;");
for (int i = 0; i < variableIdParams.length; i++) {
logger.fine("token: " + variableIdParams[i]);
String token = variableIdParams[i].replaceFirst("^v", "");
Long variableId = null;
try {
variableId = new Long(token);
} catch (NumberFormatException nfe) {
variableId = null;
}
if (variableId != null) {
logger.fine("attempting to look up variable id " + variableId);
if (variableService != null) {
DataVariable variable = variableService.find(variableId);
if (variable != null) {
if (downloadInstance.getExtraArguments() == null) {
downloadInstance.setExtraArguments(new ArrayList<Object>());
}
logger.fine("putting variable id " + variable.getId() + " on the parameters list of the download instance.");
downloadInstance.getExtraArguments().add(variable);

// if (!variable.getDataTable().getDataFile().getId().equals(sf.getId())) {
// variableList.add(variable);
// }
}
logger.fine("putting variable id "+variable.getId()+" on the parameters list of the download instance.");
downloadInstance.getExtraArguments().add(variable);

//if (!variable.getDataTable().getDataFile().getId().equals(sf.getId())) {
//variableList.add(variable);
//}
} else {
logger.fine("variable service is null.");
}
} else {
logger.fine("variable service is null.");
}
}
}
}
}

logger.fine("downloadInstance: "+downloadInstance.getConversionParam()+","+downloadInstance.getConversionParamValue());

break;
} else {
// Service unknown/not supported/bad arguments, etc.:
// TODO: throw new ServiceUnavailableException();
logger.fine("downloadInstance: " + downloadInstance.getConversionParam() + "," + downloadInstance.getConversionParamValue());
serviceFound = true;
break;
}
}
}

if (serviceRequested && !serviceFound) {
// Service not supported/bad arguments, etc.:
// One could return
// a ServiceNotAvailableException. However, since the returns are all files of
// some sort, it seems reasonable, and more standard, to just return
// a NotFoundException.
throw new NotFoundException();
} // Else - the file itself was requested or we have the info needed to invoke the service and get the derived info
logger.warning("Returning download instance");
/*
* Provide some browser-friendly headers: (?)
*/
//return retValue;
return downloadInstance;
}

Expand Down