search

scitran / core

RESTful API
https://scitran.github.io
MIT License
18 stars 18 forks source link

Simplified download path logic #1023

Closed hkethi002 closed 6 years ago

hkethi002 commented 6 years ago

Ref #1005

Changes

Not a huge improvement and may not be worth the extra risk, but simplified _path_from_container by using the prefix and ids

Review Checklist

nagem commented 6 years ago

Another thing we could add to this is removing the optional flag from the request. It seems as if there was a time where files would be marked "optional" and the download endpoint would not download them until the flag optional was true: https://github.com/scitran/core/pull/1023/files#diff-5f6b428f8ab6dc5134ff7b9cbcad2062R49

It is safe to remove this option, as there are no "optional" files.

nagem commented 6 years ago

One more thing - we skip missing files if we cannot resolve their filepath. We should add a warning log message to that skip to make it a little less silent. @ryansanford

hkethi002 commented 6 years ago

For removing the optional, should I remove it from the schemas, making this a breaking change?

nagem commented 6 years ago

We can try coordinating with the clients (frontend, I don't think the SDK and CLI use that option), or remove it's reference in the download logic, wait for clients to make the change, and then remove it from the schemas. I think the second option will allow us to more forward more quickly so that sounds alright with me.

codecov-io commented 6 years ago

Codecov Report

Merging #1023 into master will decrease coverage by 0.02%. The diff coverage is 95.12%.

@@            Coverage Diff             @@
##           master    #1023      +/-   ##
==========================================
- Coverage   90.64%   90.61%   -0.03%     
==========================================
  Files          50       50              
  Lines        6763     6766       +3     
==========================================
+ Hits         6130     6131       +1     
- Misses        633      635       +2
nagem commented 6 years ago

This fixes an issue that is being experienced on master by a few users. I'm going to write one more test for the specific error that is being experienced and then I'd say it's ready to merge.

nagem commented 6 years ago

I loaded a mock dataset similar to the one the customer is seeing problems with and tested its download against this branch - everything looks great. Feel free to merge this as soon as CI is green.