service: fix navigation for associations with many to one relationships and for associations with relationships to same entity #223
Conversation
|
|
…ps and for associations with relationships to same entity
Masterchen09
force-pushed
the
fix-many-to-one-navigation
branch
from
98acc08 to
34825a9
Compare
|
Hi @ Masterchen09 If you want to improve small things, you can ammend the commit message (so it is not half-splitted), e.g. b678d0a, and update the changelog.md as well. I will look into the proper code review, but on first glance it looks good. I think after this PR merge I will immediately release 1.9.2 with this fix. |
| if not navigation_entity_set: | ||
| raise PyODataException(f'No association set for role {navigation_property.to_role}') |
There was a problem hiding this comment.
If the bug is about "many to one relationships", why remove this exception rise? Even the second part of the PR description - "it is not possible to use the nav method with associations where both ends are of the same entity type." - as I understand it, the case that you would like to refer to something that does not exist at all is still valid.
The exception rise is removed multiple times, line 1358 (original).
As an example - consider metadata file, that is invalid, but forced to be parsed anyway - and then you can end easily with trying to access something which is not there at all.
It bothers me a bit that there is not a test that would start to fail by removing these lines - that for specific input we expect this exception to be thrown. But if such test existed, this PR would be IMHO backward-incompatible change.
There was a problem hiding this comment.
I have removed this exception because the end_by_role method is raising a KeyError in case the end role was not found. I have thought about to handle the KeyError and raising the PyODataException, but this would be a bit redundant. However, due to the possible KeyError of end_by_role the navigation_entity_set can never be None (e.g. in case of an invalid metadata file) and the PyODataException would never be raised.
| association_info.namespace) | ||
|
|
||
| navigation_entity_set = None | ||
| for end in association_set.end_roles: |
There was a problem hiding this comment.
why was the for loop removed? Again, all tests are still passing, but this may be a problem of the test suite - that the loop was not covered so far, just simple cases around navigational properties.
There was a problem hiding this comment.
The loop is not necessary (anymore) because the end_by_role method of the association_set is "looping" over the association_set.end_roles using the generator expression here:
python-pyodata/pyodata/v2/model.py
Line 2163 in 9a72f93
There was a problem hiding this comment.
OK, that's true. And also, since now test metadata.xml contains multimple association sets ending to same end role (Customer), it is proven by tests.
2 participants
When playing around with the OData API of the SAP Analytics Cloud to retrieve some stories, I have noticed that the nav method did not work with many to one relationships. The SAC API specifies such an relationship between a story and users (createdByUser, a story can be created only by one user, but a user can create many stories): Either you have to specify a key for the nav method or to use the get_entities method which both not works as the target entity is not a list of entities (the entity is returned directly and not in a results array) or as an invalid url is generated (the target entity is being indexed, which does not work as it is not an list).
I have tracked down this issue down to line 886 (and 1362) where the multiplicity for all roles is checked and when the multiplicity for one of the role is "*" the check fails. Here only the multiplicity of the "ToRole" should be checked as this role is the relevant role for the navigation.
Additionally when writing a test for this case I have found another issue: It is not possible to use the nav method with associations where both ends are of the same entity type. The logic for finding the navigation_entity_set in line 877-880 (and 1352-1359) is using the entity set name for finding the role in the association set: This does not work if both roles has the same entity set name. Here the method end_by_role method of the association set can be used to find the correct role (and then the correct navigation_entity_set).