Fix #1114: Fix nested sorting key breaks pagination token. #1116
Conversation
|
Oh now I understand the bug, so we are supporting filtering on nested fields but not sorting on them. It looks reasonable |
kinto/core/utils.py
Outdated
| @@ -182,6 +182,13 @@ def dict_merge(a, b): | |||
| return result | |||
|
|
|||
|
|
|||
| def find_nested_value(dict, path): | |||
| if "." not in path: | |||
There was a problem hiding this comment.
Unfortunately it is not as simple because path can contain dots: See what we did there: https://github.com/Kinto/kinto-admin/blob/62553505e3610831b6119780328dede6c0f784da/src/utils.js#L124-L165
| # we have our root key, extract the new subpath and recur | ||
| subpath = path.replace(root + '.', '', 1) | ||
| return find_nested_value(d.get(root), subpath) | ||
|
|
There was a problem hiding this comment.
In storage/memory:apply_filters() and apply_sorting() we had just this:
left = record
subfields = f.field.split('.')
for subfield in subfields:
if not isinstance(left, dict):
break
left = left.get(subfield)It might be worth unifying both :) The simpler the better IMO (unless there's a reason hehe)
There was a problem hiding this comment.
I would be in favor of using @n1k0 function there too.
There was a problem hiding this comment.
Really depends if we want to prioritize dotted field key names as my implementation does, or if we don't really care. Probably this is mostly matter of properly documenting the behavior we settle on?
There was a problem hiding this comment.
Personally, I'd be in favor of forbidding dots in field names. MongoDB does.
| token['last_record'][field] = last_record[field] | ||
| last_value = find_nested_value(last_record, field) | ||
| if last_value is not None: | ||
| token['last_record'][field] = last_value |
There was a problem hiding this comment.
What happens if last_value is None ? There is no pagination ? Would that solve #816 ?
There was a problem hiding this comment.
If last_value is None it's most probably because an erroneous sorting field key has been provided. Should we error is such a case, or just ignore it as I did? We could possibly raise a 400 error properly describing the issue to the consumer, which sounds rather good to me. I can work on that if you want.
There was a problem hiding this comment.
Also note that without a JSON schema to validate provided parameters against (in which case all fields are inherently optional), the last record from a chunk might or might not have the field matching the provided key set. In such a case, I don't have any solution in mind, sadly.
| def test_find_disambiguated_dotted_path_values(self): | ||
| # XXX: To be honest, this is a really scary use case. Probably a | ||
| # limitation of the dotted path notation we may want to document. | ||
| # At least this test acts as documentation for now. |
There was a problem hiding this comment.
Ahhh that's why... Hmm no strong opinion
|
So I'm gonna go crazy and ask for a review. r=? @Natim @leplatrem |
| if not isinstance(left, dict): | ||
| break | ||
| left = left.get(subfield) | ||
| left = find_nested_value(record, f.field) |
|
|
||
| # we have our root key, extract the new subpath and recur | ||
| subpath = path.replace(root + '.', '', 1) | ||
| return find_nested_value(d.get(root), subpath) |
There was a problem hiding this comment.
Shouldn't we pass default here?
There was a problem hiding this comment.
The default parameter has None as a default value already, which is consistent with the previous implementation.
There was a problem hiding this comment.
Woops, misread the place where you've added that comment, you're totally right! Edit: #1118
| # we have our root key, extract the new subpath and recur | ||
| subpath = path.replace(root + '.', '', 1) | ||
| return find_nested_value(d.get(root), subpath) | ||
|
|
There was a problem hiding this comment.
Personally, I'd be in favor of forbidding dots in field names. MongoDB does.
Post #1116 fix: ensure default propagation in find_nested_value.
This is a
simplisticattempt at fixing #1114. More tests are probably needed, I'd just like some feedback first.f=? @Natim @leplatrem @gabisurita