Skip to content
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

Fix filters on nested provider/aliased fields #285

Merged
merged 2 commits into from Jun 2, 2020

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jun 1, 2020

Closes #282.

@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #285 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #285      +/-   ##
==========================================
+ Coverage   90.03%   90.05%   +0.01%     
==========================================
  Files          54       54              
  Lines        2269     2273       +4     
==========================================
+ Hits         2043     2047       +4     
  Misses        226      226              
Flag Coverage Δ
#unittests 90.05% <100.00%> (+0.01%) ⬆️
Impacted Files Coverage Δ
optimade/server/mappers/entries.py 98.24% <100.00%> (+0.13%) ⬆️

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 758baac...64c8963. Read the comment docs.

@ml-evs ml-evs force-pushed the ml-evs/fix_nested_alias_fields branch from e25514a to a35785c Compare June 2, 2020 00:10
@ml-evs ml-evs marked this pull request as ready for review June 2, 2020 13:58
@ml-evs ml-evs requested review from CasperWA and fekad June 2, 2020 13:58
CasperWA
CasperWA previously approved these changes Jun 2, 2020
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. Thanks @ml-evs!

I was trying to figure out what would happen if the sub-properties matched an alias. My conclusion is nothing, since only the first "top-level" property is checked against all_aliases.
Then, why do the testing against postprocessing, and if I remember correctly, this is because the update of the filter to use aliased property names is done in your transformer during the post-processing?
Finally, since the alias_for method doesn't care about the transformer whatsoever, and simply introduces an element of sub-property-awareness there shouldn't be any issues with other implementations either (i.e., my own) :)

So all in all, great!

@ml-evs ml-evs changed the title Quick fix for nested provider fields Fix filters on nested provider/aliased fields Jun 2, 2020
@ml-evs
Copy link
Member Author

ml-evs commented Jun 2, 2020

I was trying to figure out what would happen if the sub-properties matched an alias. My conclusion is nothing, since only the first "top-level" property is checked against all_aliases.

👍

Then, why do the testing against postprocessing, and if I remember correctly, this is because the update of the filter to use aliased property names is done in your transformer during the post-processing?

Yep, I've just added some more tests directly on a mapper too.

Finally, since the alias_for method doesn't care about the transformer whatsoever, and simply introduces an element of sub-property-awareness there shouldn't be any issues with other implementations either (i.e., my own) :)

🤞

Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good.

You're catching some edge-cases, where I think the response is appropriate.
One in particular is quite philosophical: "nonsensical query".
I.e., testing a wrong input for a field property ending in . passes the (aliased) property through to the query handler and does not try to infer what was meant here. It's difficult to sometimes say whether this should be inferred, denied or whether or not it should even be aliased. But I think this result hits the exact right balance. Since the mappers job is to map, not try to be more clever than that. That's for other parts to do, so, yeah. Great! :)

@ml-evs
Copy link
Member Author

ml-evs commented Jun 2, 2020

I.e., testing a wrong input for a field property ending in . passes the (aliased) property through to the query handler and does not try to infer what was meant here. It's difficult to sometimes say whether this should be inferred, denied or whether or not it should even be aliased.

Yeah, writing this test prompted me to comment on #263 again, as I think this is the next big thing we need to do.

@ml-evs ml-evs merged commit d29f7a1 into master Jun 2, 2020
@CasperWA CasperWA deleted the ml-evs/fix_nested_alias_fields branch June 2, 2020 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Queries on aliased/provider fields are broken for nested properties
2 participants