Skip to content

Commit

Permalink
Removed: not essential findDataFileOrDie call to avoid extra query
Browse files Browse the repository at this point in the history
  • Loading branch information
@GPortas
GPortas committed Jul 9, 2023
1 parent 9f35bf7 commit a2bc4d4
Showing 1 changed file with 2 additions and 8 deletions.
10 changes: 2 additions & 8 deletions src/main/java/edu/harvard/iq/dataverse/api/Files.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -827,14 +827,8 @@ public Response getFixityAlgorithm() {

@GET
@Path("{id}/guestbookResponses/count")
public Response getCountGuestbookResponses(@PathParam("id") String dataFileId) {
DataFile dataFile;
try {
dataFile = findDataFileOrDie(dataFileId);
} catch (WrappedResponse wr) {
return wr.getResponse();
}
return ok(guestbookResponseService.getCountGuestbookResponsesByDataFileId(dataFile.getId()).toString());
public Response getCountGuestbookResponses(@PathParam("id") long dataFileId) {
return ok(guestbookResponseService.getCountGuestbookResponsesByDataFileId(dataFileId).toString());

This comment has been minimized.

Sign in to view

Copy link
@qqmyers

qqmyers Jul 9, 2023

Member

This call will throw a NoResultException for a bad ID which, unlike the findDataFileOrDie call, won't return a 404 response as it should (I assume an uncaught exception will be a 500 error?). I think you should still be able to avoid the extra query if you handle the exception.

This comment has been minimized.

Sign in to view

Copy link
@GPortas

GPortas Jul 10, 2023

Author Contributor

Since I'm specifying long as the parameter type, any call that sends "a bad ID" will be recognized as an endpoint not found API 404 error. For example:

Sending string instead of long.

http://localhost:8080/api/files/test/guestbookResponses/count

Will result in:

{"status":"ERROR","code":404,"message":"API endpoint does not exist on this server. Please check your code for typos, or consult our API guide at http://guides.dataverse.org.","requestUrl":"http://localhost:8080/api/v1/files/test/guestbookResponses/count","requestMethod":"GET"}

Only if you send a long param the call will enter the endpoint. And a long param, even of a non-existent datafile, is a valid parameter.

The query that the guestbookResponseService executes does not check that the data file id sent corresponds to an existing file and is independent of the datafile table. Therefore, by removing the extra query of findDataFileOrDie as I did, we lose that level of verification and therefore we cannot return that information to the user. (If the identifier sent does not exist, the returned value for the count will simply be 0).

Since it's an endpoint that will potentially be executed for every file in a list, I've opted to keep the number of queries to a minimum to make it faster (in exchange for reduced error handling).

What do you think? Do you think it is better to include all verifications?
}

@GET
Expand Down

0 comments on commit a2bc4d4

Please sign in to comment.