-
Notifications
You must be signed in to change notification settings - Fork 42
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
e25514a
to
a35785c
Compare
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.
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!
👍
Yep, I've just added some more tests directly on a mapper too.
🤞 |
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.
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! :)
Yeah, writing this test prompted me to comment on #263 again, as I think this is the next big thing we need to do. |
Closes #282.