Skip to content

service: fix navigation for associations with many to one relationships and for associations with relationships to same entity #223

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 11, 2022
Merged

service: fix navigation for associations with many to one relationships and for associations with relationships to same entity #223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 6 additions & 19 deletions pyodata/v2/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,16 +874,10 @@ def nav(self, nav_property):
association_info.name,
association_info.namespace)

navigation_entity_set = None
for end in association_set.end_roles:
Copy link
Contributor

@phanak-sap phanak-sap Aug 11, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Masterchen09 Masterchen09 Aug 11, 2022

Choose a reason for hiding this comment

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

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

return next((item for item in self._end_roles if item.role == end_role))

Copy link
Contributor

@phanak-sap phanak-sap Aug 11, 2022

Choose a reason for hiding this comment

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

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.

if association_set.end_by_entity_set(end.entity_set_name).role == navigation_property.to_role.role:
navigation_entity_set = self._service.schema.entity_set(end.entity_set_name, association_info.namespace)
end = association_set.end_by_role(navigation_property.to_role.role)
navigation_entity_set = self._service.schema.entity_set(end.entity_set_name)

if not navigation_entity_set:
raise PyODataException(f'No association set for role {navigation_property.to_role}')
Comment on lines -882 to -883
Copy link
Contributor

@phanak-sap phanak-sap Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Masterchen09 Masterchen09 Aug 11, 2022

Choose a reason for hiding this comment

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

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.


roles = navigation_property.association.end_roles
if all((role.multiplicity != model.EndRole.MULTIPLICITY_ZERO_OR_MORE for role in roles)):
if navigation_property.to_role.multiplicity != model.EndRole.MULTIPLICITY_ZERO_OR_MORE:
return NavEntityProxy(self, nav_property, navigation_entity_set.entity_type, {})

return EntitySetProxy(
Expand Down Expand Up @@ -1349,17 +1343,10 @@ def nav(self, nav_property, key):
association_set = self._service.schema.association_set_by_association(
association_info.name)

navigation_entity_set = None
for end in association_set.end_roles:
if association_set.end_by_entity_set(end.entity_set_name).role == navigation_property.to_role.role:
navigation_entity_set = self._service.schema.entity_set(end.entity_set_name)

if not navigation_entity_set:
raise PyODataException(
f'No association set for role {navigation_property.to_role} {association_set.end_roles}')
end = association_set.end_by_role(navigation_property.to_role.role)
navigation_entity_set = self._service.schema.entity_set(end.entity_set_name)

roles = navigation_property.association.end_roles
if all((role.multiplicity != model.EndRole.MULTIPLICITY_ZERO_OR_MORE for role in roles)):
if navigation_property.to_role.multiplicity != model.EndRole.MULTIPLICITY_ZERO_OR_MORE:
return self._get_nav_entity(key, nav_property, navigation_entity_set)

return EntitySetProxy(
Expand Down
17 changes: 17 additions & 0 deletions tests/metadata.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@
<Property Name="City" Type="Edm.String" MaxLength="15" FixedLength="false" />
<NavigationProperty Name="Orders" Relationship="EXAMPLE_SRV.CustomerOrders" FromRole="CustomerRole"
ToRole="OrdersRole"/>
<NavigationProperty Name="ReferredBy" Relationship="EXAMPLE_SRV.CustomerReferredBy" FromRole="CustomerRole"
ToRole="ReferredByRole"/>
</EntityType>

<EntityType Name="Order">
Expand Down Expand Up @@ -186,6 +188,16 @@
</Dependent>
</ReferentialConstraint>
</Association>
<Association Name="CustomerReferredBy">
<End Type="EXAMPLE_SRV.Customer" Multiplicity="*" Role="CustomerRole"/>
<End Type="EXAMPLE_SRV.Customer" Multiplicity="0..1" Role="ReferredByRole"/>
<Principal Role="CustomerRole">
<PropertyRef Name="Name"/>
</Principal>
<Dependent Role="ReferredByRole">
<PropertyRef Name="Name"/>
</Dependent>
</Association>
<Association Name="toSelfMaster" sap:content-version="1">
<End Type="EXAMPLE_SRV.MasterEntity" Multiplicity="1" Role="FromRole_toSelfMaster"/>
<End Type="EXAMPLE_SRV.MasterEntity" Multiplicity="0..1" Role="ToRole_toSelfMaster"/>
Expand Down Expand Up @@ -396,6 +408,11 @@
<End Role="OrdersRole" EntitySet="Orders"/>
</AssociationSet>

<AssociationSet Name="CustomerReferredBy_AssocSet" Association="EXAMPLE_SRV.CustomerReferredBy">
<End Role="CustomerRole" EntitySet="Customers"/>
<End Role="ReferredByRole" EntitySet="Customers"/>
</AssociationSet>

</EntityContainer>

</Schema>
Expand Down
2 changes: 2 additions & 0 deletions tests/test_model_v2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ def test_edmx_associations(schema):
'toCarIDPic',
'toDataEntity',
'CustomerOrders',
'CustomerReferredBy',
'AssociationEmployeeAddress',
'toSelfMaster'
}
Expand Down Expand Up @@ -241,6 +242,7 @@ def test_edmx_associations(schema):
'toDataEntitySet',
'AssociationEmployeeAddress_AssocSet',
'CustomerOrder_AssocSet',
'CustomerReferredBy_AssocSet',
'toCarIDPicSet',
'toSelfMasterSet'
}
Expand Down
28 changes: 28 additions & 0 deletions tests/test_service_v2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,34 @@ def test_navigation(service):
assert addr.City == 'London'


@responses.activate
def test_navigation_multi_on1(service):
"""Check getting entity via navigation property"""

# pylint: disable=redefined-outer-name

responses.add(
responses.GET,
f"{service.url}/Customers('Mammon')/ReferredBy",
headers={'Content-type': 'application/json'},
json = { 'd': {
'Name': 'John',
}
},
status=200)

request = service.entity_sets.Customers.get_entity('Mammon').nav('ReferredBy')
assert isinstance(request, pyodata.v2.service.EntityGetRequest)

referred_by_proxy = request.execute()
assert isinstance(referred_by_proxy, pyodata.v2.service.NavEntityProxy)

assert referred_by_proxy.entity_set._name == 'Customers'
assert referred_by_proxy._entity_type.name == 'Customer'

assert referred_by_proxy.Name == 'John'


@responses.activate
def test_navigation_1on1(service):
"""Check getting entity via navigation property"""
Expand Down