/api/v1/study/1/samples accepts new categories #3235
Conversation
calling PATCH on /api/v1/study/1/samples now silently accepts metadata with new categories in them when updating a sample or adding a new one. Existing samples will be assigned a null value while samples in the metadata will be assigned the value or empty string if one was not given.
charles-cowart
force-pushed
the
new_category_support_v4
branch
from
a1a1d51
to
8c39e79
Compare
When a PATCH includes new samples as well as existing ones, 201 should be returned on success, rather than 200.
|
singlularly, the tests complete successfully, but locally as a whole 'nosetests' fails on these tests as well. It seems like an issue with setup and teardown. Will look into it tomorrow night if I have time. |
There was a problem hiding this comment.
Looks good, thank you @charles-cowart. Just some minor comments.
|
@charles-cowart, this PR has some (minor) real errors on CI. |
Tests ran fine individually, but failed when ran as a whole suite because categories and samples added for one test would affect tests downstream.
| # 640201 is an existing sample. blank.a2 is a new sample | ||
| body = _sample_creator(['1.SKM8.640201', | ||
| 'blank.a2'], categories=current) | ||
| # body['blank.a2']['DOES_NOT_EXIST'] will be '', not None. |
There was a problem hiding this comment.
is this True? I think is foo, no? Same for WHAT/bar ...
There was a problem hiding this comment.
Nope. Confirmed Friday that whenever you add a new category like we're doing here, all samples mentioned in the body will have their values set to the value you explicitly define or ''. All samples NOT mentioned in the body will have their values set to None.
| self.fail("Some categories do not exist in the sample " | ||
| "information", 400, | ||
| categories_not_found=sorted(unknown)) | ||
| if set(data.columns) != categories and set(data.columns).issubset( |
There was a problem hiding this comment.
I'm not sure if this matters in practical terms, but if the intent is to fail whenever there are missing fields, I think this code would miss a situation where a sample had new fields but was also missing existing fields, since it would neither match categories, nor be a subset.
For example, if a study had fields named FOO1, FOO2, and FOO3, and we submitted a sample with FOO1, FOO2, and FOO4, it would be missing a field but wouldn't reach this failure state.
There was a problem hiding this comment.
What 171 would do is catch all subsets of categories up to but not including an exact match for categories. Sample_template.update() handles the condition you mentioned.
Although now that you bring it up, after swapping .extend() and .update(), new categories could still be added successfully and the rest call would still fail on updates - leaving the system in an unexpected state after an error. I'll take a look.
There was a problem hiding this comment.
Resolved. Thanks for the catch! We now test for superset and raise a 400 if False.
|
Thank you @cassidysymons and @charles-cowart ! |
calling PATCH on /api/v1/study/1/samples now silently accepts metadata with new categories in them when updating a sample or adding a new one. Existing samples will be assigned a null value while samples in the metadata will be assigned the value or empty string if one was not given.