Ensure correct types for QueryBuilder().dict() with multiple projections
#3695
Conversation
aiida/orm/querybuilder.py
Outdated
| @@ -1994,15 +1995,20 @@ def get_query(self): | |||
| self._hash = queryhelp_hash | |||
| return query | |||
|
|
|||
| @staticmethod | |||
| def get_aiida_entity_res(value): | |||
| def get_aiida_entity_res(self, value): | |||
There was a problem hiding this comment.
could be a @classmethod, right?
There was a problem hiding this comment.
could be, but I think staticmethod makes more sense. Now that I fixed the problem in the correct place (thanks for catching this 👍) I think we can keep this as was
aiida/orm/querybuilder.py
Outdated
| """Convert a projected query result to front end class if it is an instance of a `BackendEntity`. | ||
|
|
||
| Values that are not an `BackendEntity` instance will be returned unaltered | ||
|
|
||
| :param value: a projected query result to convert | ||
| :return: the converted value | ||
| """ | ||
| if isinstance(value, collections.abc.Mapping): | ||
| return {key: self.get_aiida_entity_res(sub_value) for key, sub_value in value.items()} |
There was a problem hiding this comment.
I wonder, should this not rather be fixed here?
aiida-core/aiida/orm/convert.py
Lines 26 to 28 in 81d736c
Also, this code looks very similar, so I wonder why this is not already working...
Is just the list/tuple stuff missing from there?
There was a problem hiding this comment.
You are right, it should be handled there but the problem is that there a TypeError can be raised if some of the elements are not node types, which is then caught by get_aiida_entity_res and it then returns the original value which is the original dictionary with unconverted values. I will fix it correctly
sphuber
force-pushed
the
fix_3679_query_builder_dict_multiple_projections
branch
2 times, most recently
from
cf326e5
to
e0d83cc
Compare
…tions The returned results by the backend implementation of `QueryBuilder` are passed through `get_aiida_entity_res` to convert backend instances to front end class instances. It calls `aiida.orm.convert.get_orm_entity` which is a singledispatch to convert all known backend types to its corresponding front-end ORM analogue. The registered implementation for `Mapping` contained the bug. It used a simple comprehension and did not catch any `TypeError` that might be thrown from values that could not be converted. This would bubble up to `get_aiida_entity_res` which would then simply return the original value. If the mapping contains a mixture of backend entities and normal types, the entire converting would be undone. This was surfaced when calling `dict` on a query builder instance with `project=['*', 'id']`. The returned value for each match is a dictionary with one value an integer corresponding to the `id` and the other value a backend node instance. The integer would raise the `TypeError` in the `Mapping` converter and since it wasn't caught, the backend node was also not converted.
sphuber
force-pushed
the
fix_3679_query_builder_dict_multiple_projections
branch
from
e0d83cc
to
9280fd5
Compare
I know, but I cannot reproduce it locally or in other PRs. I have been trying to fix it and already merged two more PRs in a desperate attempt to tackle this. It's infuriating. In any case it is not a problem iwth capitalization but just the fact that it is 32 characters instead of the "max allowed" 30. |
|
What if we increase the max allowed to 32?
…On Fri, Dec 20, 2019 at 4:10 PM Sebastiaan Huber ***@***.***> wrote:
seems pylint tripped over again...
can we actually fix this warning by using the proper capitalization?
I know, but I cannot reproduce it locally or in other PRs. I have been
trying to fix it and already merged two more PRs in a desperate attempt to
tackle this. It's infuriating. In any case it is not a problem iwth
capitalization but just the fact that it is 32 characters instead of the
"max allowed" 30.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#3695?email_source=notifications&email_token=AAIBEPIGLMT3THHFJW4YDPDQZTN6PA5CNFSM4J53SBH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHNFPTI#issuecomment-567957453>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIBEPME74BFDLYBCSVGITTQZTN6PANCNFSM4J53SBHQ>
.
|
|
The last commit I did may have done it: https://github.com/aiidateam/aiida-core/runs/358302396 Edit: no dice... |
As a last defense I just snuck this in this last commit. Let's hope to all that is holy that that will do it |
Fixes #3679
The returned results by the backend implementation of
QueryBuilderarepassed through
get_aiida_entity_resto convert backend instances tofront end class instances. However, in the
iterdictaccumulator thiswas not accounting for mappings and sequences. This would cause problems
when multiple projections were used, such as
project=['*', 'id']. The'*' notation will cause the entire node instance to be loaded, however
due to the fact that it was inside a dictionary in the response it would
not be converted.