-
Notifications
You must be signed in to change notification settings - Fork 160
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
Generate valid id/read/edit links for expanded non-contained resource with a contained navigation source #1088
Conversation
7e1560e
to
071f640
Compare
test/Microsoft.AspNetCore.OData.E2E.Tests/DollarExpand/DollarExpandDataModel.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.AspNetCore.OData.E2E.Tests/DollarExpand/DollarExpandEdmModel.cs
Outdated
Show resolved
Hide resolved
e3180be
to
6069bdb
Compare
@@ -0,0 +1,98 @@ | |||
//----------------------------------------------------------------------------- | |||
// <copyright file="DollarExpandController.cs" company=".NET Foundation"> |
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.
This name doesn't really give an overview of what these tests are for.. Use a more descriptive name for this scenario for all the files.
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.
@ElizabethOkerio What would you propose?
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.
LinksForExpandedResourcesWithContainedNavSources .. this is too long but something in those lines.
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.
@ElizabethOkerio None of the existing directories that categorize tests under it has this level of detail - LinksForExpandedResourcesWithContainedNavSources
. The directory name is a just pointer to the kind of tests one should expect under it. I renamed the directory to MetadataProperties
which in essence is what the fix is all about. The tests are a combination of many scenarios (contained properties, non-contained properties, expanded navigation properties, nested expansion, etc.) - and tests for more scenario could be added with time. The key thing we're testing is that the generated metadata properties (@odata.id
, @odata.editLink
, @odata.associationLink
, @odata.navigationLink
, etc.) are valid.
test/Microsoft.AspNetCore.OData.E2E.Tests/DollarExpand/DollarExpandEdmModel.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.AspNetCore.OData.E2E.Tests/DollarExpand/DollarExpandEdmModel.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.AspNetCore.OData.E2E.Tests/DollarExpand/DollarExpandEdmModel.cs
Outdated
Show resolved
Hide resolved
Assert.EndsWith($"{plantResourceBase}", plant.Value<string>("@odata.editLink")); | ||
Assert.Equal(1, plant.Value<int>("Id")); | ||
Assert.Equal("Plant 1", plant.Value<string>("Name")); | ||
Assert.EndsWith($"{scenario}/{plantResourceBase}/Site/$ref", plant.Value<string>("[email protected]")); |
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.
Here there's no type segment?
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.
@habbes There shouldn't be a type segment there. The Site
navigation property is not initialized to a derived type as is the case for the Pipelines
navigation property.
… with contained navigation source
6069bdb
to
3c2dd70
Compare
This pull request fixes OData/odata.net#2273.
Description
In the described scenario, a
Site
entity type has a containedPlants
navigation property. ThePlant
entity type in turn has a containedPipelines
navigation property. ThePipeline
entity type in turn has a non-contained single-valuedPlant
navigation property.The non-contained
Plant
navigation property is bound to thePlants
contained entity set via the Edm pathSites/Plants
:The
Sites
entity set and the navigation property bindings are as follows:If the
$format
orAccept
request headers specifyodata.metadata=full
, and we attempt to expand thePlant
navigation property, i.e.,/Sites(1)/Plants(1)/Pipelines(1)?$expand=Plant
- wherePlant
is non-contained, failure to use the link builder to build the id/read/edit links results into invalid links since we generate the links by appending/Plants(1)
to the parentPipeline
resource id -/Sites(1)/Plants(1)/Pipelines(1)
=>/Sites(1)/Plants(1)/Pipelines(1)/Plants(1)
. This link is invalid since there's noPlants
navigation property onPipeline
entity type.The following line causes us not to use the link builder to generate the id/read/edit links if the
resourceContext.NavigationSource
is a contained entity set:AspNetCoreOData/src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataResourceSerializer.cs
Line 584 in d94daad
This PR fixes the issue by causing the link builder to be used if
resourceContext.NavigationSource
is a contained entity set and an expanded non-contained resource is being written. A navigation property binding should exist and a navigation link builder should be on hand to build the links.Tests verify that expected behaviour is observed even when the non-contained navigation property is in a nested expand.
Checklist (Uncheck if it is not completed)