-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add Modalities, Classification endpoints #1010
base: master
Are you sure you want to change the base?
Conversation
011e574
to
e736429
Compare
Codecov Report
@@ Coverage Diff @@
## master #1010 +/- ##
==========================================
+ Coverage 90.97% 91.05% +0.08%
==========================================
Files 49 50 +1
Lines 7037 7145 +108
==========================================
+ Hits 6402 6506 +104
- Misses 635 639 +4 |
95a6dbd
to
b8d2db3
Compare
@gsfr Just a head's up that this is a breaking change to gears and we should chat offline about our support strategy. |
@nagem to make change to still pass measurements key for backwards compatibility. That behavior will be deprecated and removed in future. |
3cbf035
to
728322e
Compare
@gsfr When should file modifications cause jobs to spawn? Info editing? Classification editing? Any editing? |
@kofalt, @ambrussimon I am no longer fiddling with this and would appreciate your review when you have time available. |
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.
Nice work. Easily used the tests and schemas to infer the feature/endpoint details 👍
if delete_payload: | ||
delete_payload = check_and_format_classification(modality, delete_payload) | ||
|
||
# TODO: Test to make sure $pull succeeds when key does not exist |
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 believe I've seen the test for this edge case, please remove TODO 👍
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.
Thanks 👍. We turned off the pylint warning for TODOs since there were so many in the system at that time. Would be nice to go back through and address any that can be addressed and move the rest to tickets so we can catch things that weren't meant to be longstanding TODOs. Not sure how much effort we want to put into formalizing what TODO means in the system, though 😉
@require_admin | ||
def post(self): | ||
payload = self.request.json_body | ||
# Clean this up when validate_data method is fixed to use new schemas |
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.
Does this still apply? Why can't validate_data
use the newly added modality.json
?
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.
This is also a TODO that was intended to be finished before PR review. I ended up using a lot of TODO-type comments since this branch existed for so long.
@@ -188,8 +188,20 @@ def update_el(self, _id, payload, unset_payload=None, recursive=False, r_payload | |||
raise APIStorageException(e.message) | |||
if recursive and r_payload is not None: | |||
containerutil.propagate_changes(self.cont_name, _id, {}, {'$set': util.mongo_dict(r_payload)}) | |||
|
|||
config.log.warning(update) |
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.
This should be removed.
payload = self.request.json_body | ||
# Clean this up when validate_data method is fixed to use new schemas | ||
# POST unnecessary, used to avoid run-time modification of schema | ||
#validate_data(payload, 'modality.json', 'input', 'POST', optional=True) |
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.
Also update here.
7ee12a4
to
db0f6c9
Compare
New endpoints:
GET /api/modalities
- GET list of all modalities and their allowable classifications.GET /api/modalities/<modality_name>
- GET a specific modalityPOST /api/modalities
- Add a new modality. Required:_id
(readable name) andclassification
, a map of allowable classifications. Requires superuser. Example request:PUT /api/modalities/<modality_name>
- REPLACE a specific modality. Requires superuser.DELETE /api/modalities/<modality_name>
- DELETE a specific modality. Requires superuser.POST /api/<cont_name>/<cont_id>/files/<files_name>/classification
- UPDATE an existing file's classification. Works similar to the info editing endpoint. Several options are allowed:To
push
items to lists within classification, useadd
:To
pull
items from lists within classification, usedelete
:Note:
add
anddelete
can both be used in the same request.To replace an entire classification, use
replace
:Relevant Response Formats:
Success responses when editing a file (
PUT /api/cont/cont_id/files/filename
,POST /api/cont/cont_id/files/filename/info
,POST /api/cont/cont_id/files/filename/classification
) will be formatted like:Errors when attempting to edit a file's classification:
TODO:
finalize measurements -> classification handling in reaper/engine endpointsDONEBreaking Changes:
measurements
list will no longer be an accepted or outputted field on a file, it has been replaced by aclassification
map. SDK, search, and other clients will need to be updated.measurements
. DB update replaces the key withclassification
. Format did not change, just a key rename.modality
field on a file can be any string, not just to known list of modalities in the systemCustom
keyReview Checklist