Sorting on unknown properties: returning Bad Request when appropriate #424

ml-evs · 2020-07-22T16:08:08Z

This PR enables 400: Bad Requests to be returned when requesting unknown fields. The specification provides a little bit of flexibility here, so I'll illustrate my approach with a few cases, with three classes of fields: known, unknown and "other provider" (those prefixed by _ with no known alias):

User requests sorting on exclusively unknown fields -> Bad Request. e.g. sort=foo,bar.
User requests sorting on exclusively "other provider" fields -> Warning, e.g. sort=_exmpl_foo,_exmpl_bar.
User requests sorting on known field and unknown field -> Bad Request, e.g. sort=nelements,foo.
User requests sorting on known field and "other provider" field -> sort on known field only. e.g. sort=nelements,_exmpl_foo.

Closes #276.

To-do:

Turn case 2/ above into a warning

codecov · 2020-07-22T16:13:48Z

Codecov Report

Merging #424 into master will increase coverage by 0.44%.
The diff coverage is 98.66%.

@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
+ Coverage   90.95%   91.40%   +0.44%     
==========================================
  Files          56       56              
  Lines        2632     2653      +21     
==========================================
+ Hits         2394     2425      +31     
+ Misses        238      228      -10

Flag	Coverage Δ
#unittests	`91.40% <98.66%> (+0.44%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...made/server/entry_collections/entry_collections.py	`98.79% <98.38%> (+9.90%)`	⬆️
optimade/server/entry_collections/mongo.py	`96.82% <100.00%> (+0.90%)`	⬆️
optimade/server/exceptions.py	`100.00% <100.00%> (ø)`
optimade/server/mappers/entries.py	`98.36% <100.00%> (+0.11%)`	⬆️
optimade/server/warnings.py	`85.71% <100.00%> (+1.09%)`	⬆️
optimade/server/middleware.py	`92.85% <0.00%> (+4.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2e8847...9e7d2d5. Read the comment docs.

CasperWA · 2020-07-23T10:21:35Z

@ml-evs Concerning 1411a13 you need to also add the structures parameter to the test function, since it's a fixture you need to invoke, and then use (as you do).

CasperWA · 2020-07-27T11:52:52Z

This PR enables 400: Bad Requests to be returned when requesting unknown fields. The specification provides a little bit of flexibility here, so I'll illustrate my approach with a few cases, with three classes of fields: known, unknown and "other provider" (those prefixed by _ with no known alias):

User requests sorting on exclusively unknown fields -> Bad Request. e.g. sort=foo,bar.

User requests sorting on exclusively "other provider" fields -> Bad Request, e.g. sort=_exmpl_foo,_exmpl_bar.

User requests sorting on known field and unknown field -> Bad Request, e.g. sort=nelements,foo.

User requests sorting on known field and "other provider" field -> sort on known field only. e.g. sort=nelements,_exmpl_foo.

Just to be certain, your definitions of the three classes are something like the following:

Known: Fields standardized by the OPTIMADE API specification including implementation-specific fields for the current provider, i.e., this list should/can be found by visiting /info/<entry>.
Other provider: Implementation-specific fields for a non-current provider.
Unknown: Everything else not covered by the first two classes.

CasperWA · 2020-07-27T11:59:41Z

This PR enables 400: Bad Requests to be returned when requesting unknown fields. The specification provides a little bit of flexibility here, so I'll illustrate my approach with a few cases, with three classes of fields: known, unknown and "other provider" (those prefixed by _ with no known alias):

User requests sorting on exclusively unknown fields -> Bad Request. e.g. sort=foo,bar.

User requests sorting on exclusively "other provider" fields -> Bad Request, e.g. sort=_exmpl_foo,_exmpl_bar.

I don't think I agree with this response, as it represents a well-formed and correctly valued request from the point of view of the specification, but will result in the return of different response codes depending on the implementation that is queried.
While that may be true of some filter queries as well, I consider this on the same level as a filter query where one uses fields that are on the SHOULD level of being filterable...
I.e., here I would maybe expect a 200 OK response with an added warning in the output that while these values are not supported by the current implementation they are (possibly) still valid in another implementation.

User requests sorting on known field and unknown field -> Bad Request, e.g. sort=nelements,foo.

Yes, for any unknown (not "other provider" field) a 400 Bad Request should definitely be returned :)

User requests sorting on known field and "other provider" field -> sort on known field only. e.g. sort=nelements,_exmpl_foo.

As with my suggestion for 2. above, I think this should return exactly what you have intended here, but also include a warning that _empl_foo, while (possibly) being a valid field in another OPTIMADE API implementation, it is not valid in the current implementation.

I think I should really start working on that warnings addition system ...

ml-evs · 2020-07-27T12:48:46Z

User requests sorting on exclusively "other provider" fields -> Bad Request, e.g. sort=_exmpl_foo,_exmpl_bar.

I don't think I agree with this response, as it represents a well-formed and correctly valued request from the point of view of the specification, but will result in the return of different response codes depending on the implementation that is queried.

Agree this one can be interpreted many different ways. Thinking about use cases, if sorting is requested, I agree that at least a warning should be returned, but until we have that in, I would rather return a bad request. This is perhaps something that should be clarified in 1.0.1 of the spec.

As with my suggestion for 2. above, I think this should return exactly what you have intended here, but also include a warning that _empl_foo, while (possibly) being a valid field in another OPTIMADE API implementation, it is not valid in the current implementation.

I think I should really start working on that warnings addition system ...

Agreed and agreed 😁

CasperWA · 2020-07-28T09:30:03Z

User requests sorting on exclusively "other provider" fields -> Bad Request, e.g. sort=_exmpl_foo,_exmpl_bar.

I don't think I agree with this response, as it represents a well-formed and correctly valued request from the point of view of the specification, but will result in the return of different response codes depending on the implementation that is queried.

Agree this one can be interpreted many different ways. Thinking about use cases, if sorting is requested, I agree that at least a warning should be returned, but until we have that in, I would rather return a bad request. This is perhaps something that should be clarified in 1.0.1 of the spec.

Warnings will be implemented by #431 :)
And even if not, think of a case where one does aggregated searches. Should this return a bad request for any of the searched implementations? I think that's a bit silly. Instead, I think implementations should merely ignore otherwise valid and well-formed values to sort.

As with my suggestion for 2. above, I think this should return exactly what you have intended here, but also include a warning that _empl_foo, while (possibly) being a valid field in another OPTIMADE API implementation, it is not valid in the current implementation.
I think I should really start working on that warnings addition system ...

Agreed and agreed grin

See #431 ! 😄

CasperWA

Thank @ml-evs !

So I've digged quite hard into the doc strings in this one.
I think I would prefer American diction and syntax as this was also the choice for the specification. If you strongly feel for the British syntax, then that's also fine by me, we just need to do one or the other.
After colons, I think starting with a capital letter looks nicer in the docs and makes more sense as well syntactically, no?

Concerning the move to an abstract class that's fine. However, I think you've moved over a bit more than what is needed? As soon as something is pymongo-specific, it should probably be kept in the MongoCollection? Or what is your rationale here?
I see that for much of it, it could actually serve as a general method, however, since it's not compeltely severed, I would prefer either moving it back or doing a mix of the two and properly splitting up the backend-specific stuff from the -agnostic.

optimade/server/entry_collections/entry_collections.py

optimade/server/entry_collections/mongo.py

optimade/server/mappers/entries.py

ml-evs · 2020-07-28T22:53:34Z

So I've digged quite hard into the doc strings in this one.
I think I would prefer American diction and syntax as this was also the choice for the specification. If you strongly feel for the British syntax, then that's also fine by me, we just need to do one or the other.
After colons, I think starting with a capital letter looks nicer in the docs and makes more sense as well syntactically, no?

I can't say I'd given these much thought, so happy either way.

Concerning the move to an abstract class that's fine. However, I think you've moved over a bit more than what is needed? As soon as something is pymongo-specific, it should probably be kept in the MongoCollection? Or what is your rationale here?
I see that for much of it, it could actually serve as a general method, however, since it's not compeltely severed, I would prefer either moving it back or doing a mix of the two and properly splitting up the backend-specific stuff from the -agnostic.

My rationale was that the "pymongo-specific" handling is probably already a useful start point for other collections, and I wanted to avoid spawning loads of methods for every query parameter. I'll see how tasty the mixed approach gets, as I update this PR to include warnings for case 2. discussed in the OP.

tests/server/query_params/test_sort.py

CasperWA · 2020-07-29T18:28:40Z

To-do:

Turn case 2/ above into a warning

I would more say to add a warning and use the known sort values.

And the same for case 4 above. But here, since there are no valid sort values (for this particular implementation), it just ignore the sort parameter all-together. But of course adds a warning. Maybe even for each unknown (but generally valid) sort parameter? :)

ml-evs · 2020-07-29T19:03:00Z

To-do:

Turn case 2/ above into a warning

I would more say to add a warning and use the known sort values.

And the same for case 4 above. But here, since there are no valid sort values (for this particular implementation), it just ignore the sort parameter all-together. But of course adds a warning. Maybe even for each unknown (but generally valid) sort parameter? :)

That's what I've implemented right?

CasperWA

Using a regex here should be a bit better?

optimade/server/entry_collections/entry_collections.py

CasperWA · 2020-07-29T19:36:15Z

To-do:

Turn case 2/ above into a warning

I would more say to add a warning and use the known sort values.
And the same for case 4 above. But here, since there are no valid sort values (for this particular implementation), it just ignore the sort parameter all-together. But of course adds a warning. Maybe even for each unknown (but generally valid) sort parameter? :)

That's what I've implemented right?

Yes you have. Sorry :)

…own field

- Moved many generic methods from `MongoCollection` into base class - Moved and renamed `MongoCollection._parse_params(...)` to base class `EntryCollection.handle_query_params(...). - Refactored query params parsing into multiple methods. - Cache list of field names with property `.all_fields`. - `EntryCollection` no longer inherits from typing.Collection. - Removed dunder methods required to inherit from typing.Collection. - Improved docstrings

Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

…fic fields

Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>

ml-evs · 2020-07-30T12:24:43Z

@CasperWA think this is ready for a final review? I've made your original docstring suggestions, think we'll need to do another pass once #430 is merged though.

CasperWA

I would approve, if it wasn't for an unnecessary use of the structures fixture in one of the tests, which should definitely be removed.
Then it's essentially good to go.

All the other comments are merely suggestions.

optimade/server/entry_collections/entry_collections.py

optimade/server/entry_collections/mongo.py

optimade/server/mappers/entries.py

tests/server/query_params/test_sort.py

Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>

CasperWA

This is good to go from my side now, thanks for the hard work @ml-evs ! 👍 💪

ml-evs force-pushed the ml-evs/sorting_on_unknown_properties branch from 1fe2861 to 419341c Compare July 22, 2020 16:18

CasperWA self-requested a review July 22, 2020 23:32

ml-evs force-pushed the ml-evs/sorting_on_unknown_properties branch from c6f5fc4 to a8163ed Compare July 23, 2020 10:02

ml-evs force-pushed the ml-evs/sorting_on_unknown_properties branch from 1411a13 to ad2aa50 Compare July 23, 2020 10:29

CasperWA requested changes Jul 28, 2020

View reviewed changes

ml-evs force-pushed the ml-evs/sorting_on_unknown_properties branch from ad2aa50 to 454b271 Compare July 29, 2020 16:13

ml-evs commented Jul 29, 2020

View reviewed changes

tests/server/query_params/test_sort.py Outdated Show resolved Hide resolved

ml-evs force-pushed the ml-evs/sorting_on_unknown_properties branch from e5ab87a to 0911b18 Compare July 29, 2020 17:55

CasperWA requested changes Jul 29, 2020

View reviewed changes

optimade/server/entry_collections/entry_collections.py Outdated Show resolved Hide resolved

ml-evs force-pushed the ml-evs/sorting_on_unknown_properties branch from 120c508 to ef8948d Compare July 30, 2020 12:24

ml-evs and others added 10 commits July 30, 2020 13:24

Add mapper method .alias_of(...) as opposite of .alias_for(...)

29eea54

Enable returning 400: Bad Request when sorting is requested on unkn…

11ad8d3

…own field

Updated tests after pytest changes

9035eda

Docstring updates from code review

a816419

Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

Return a warning in case of sorting requested on other provider-speci…

c42850d

…fic fields

Apply suggestions from code review

0456464

Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

Add custom FieldNotRecognised warning and reorganise tests

e3fd10a

Added custom Forbidden exception model

002d60f

Removed some git guff and updated docstrings

594a63a

ml-evs and others added 4 commits July 30, 2020 13:24

Replace string match with regexp for provider fields

8aa2646

Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>

Replace filterwarnings decorator with pytest.warns(...)

f43adeb

Succumbing to American hegemony

aecbf8c

Revert docstrings pending docs PR

0210427

ml-evs force-pushed the ml-evs/sorting_on_unknown_properties branch from ef8948d to 0210427 Compare July 30, 2020 12:24

ml-evs requested a review from CasperWA July 30, 2020 12:24

CasperWA requested changes Jul 30, 2020

View reviewed changes

Final changes from code review

9e7d2d5

Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>

CasperWA approved these changes Jul 30, 2020

View reviewed changes

ml-evs merged commit 59bba9e into master Jul 30, 2020

ml-evs deleted the ml-evs/sorting_on_unknown_properties branch July 30, 2020 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sorting on unknown properties: returning Bad Request when appropriate #424

Sorting on unknown properties: returning Bad Request when appropriate #424

ml-evs commented Jul 22, 2020 •

edited by CasperWA

Loading

codecov bot commented Jul 22, 2020 •

edited

Loading

CasperWA commented Jul 23, 2020

CasperWA commented Jul 27, 2020

CasperWA commented Jul 27, 2020

ml-evs commented Jul 27, 2020

CasperWA commented Jul 28, 2020

CasperWA left a comment

ml-evs commented Jul 28, 2020

CasperWA commented Jul 29, 2020

ml-evs commented Jul 29, 2020

CasperWA left a comment

CasperWA commented Jul 29, 2020

ml-evs commented Jul 30, 2020

CasperWA left a comment

CasperWA left a comment •

edited

Loading

Sorting on unknown properties: returning Bad Request when appropriate #424

Sorting on unknown properties: returning Bad Request when appropriate #424

Conversation

ml-evs commented Jul 22, 2020 • edited by CasperWA Loading

codecov bot commented Jul 22, 2020 • edited Loading

Codecov Report

CasperWA commented Jul 23, 2020

CasperWA commented Jul 27, 2020

CasperWA commented Jul 27, 2020

ml-evs commented Jul 27, 2020

CasperWA commented Jul 28, 2020

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Jul 28, 2020

CasperWA commented Jul 29, 2020

ml-evs commented Jul 29, 2020

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA commented Jul 29, 2020

ml-evs commented Jul 30, 2020

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA left a comment • edited Loading

Choose a reason for hiding this comment

ml-evs commented Jul 22, 2020 •

edited by CasperWA

Loading

codecov bot commented Jul 22, 2020 •

edited

Loading

CasperWA left a comment •

edited

Loading