Sorting on unknown properties: returning Bad Request when appropriate

[ml-evs]

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):

1. User requests sorting on exclusively unknown fields -> Bad Request. e.g. sort=foo,bar.
2. User requests sorting on exclusively "other provider" fields -> Warning, e.g. sort=_exmpl_foo,_exmpl_bar.
3. User requests sorting on known field and unknown field -> Bad Request, e.g. sort=nelements,foo.
4. 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]

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]

@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]

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):

1. User requests sorting on exclusively unknown fields -> Bad Request. e.g. sort=foo,bar.
2. User requests sorting on exclusively "other provider" fields -> Bad Request, e.g. sort=_exmpl_foo,_exmpl_bar.
3. User requests sorting on known field and unknown field -> Bad Request, e.g. sort=nelements,foo.
4. 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]

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):

1. User requests sorting on exclusively unknown fields -> Bad Request. e.g. sort=foo,bar.
2. 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.

3. 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 :)

4. 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]

2. 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]

1. 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.

[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?

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.

[CasperWA]

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]

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?

[CasperWA]

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 :)

[ml-evs]

@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.

[CasperWA]

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