Fix wrong links generated for derived type with navigation property when odata.metadata=full by gathogojr · Pull Request #2592 · OData/odata.net

gathogojr · 2023-01-11T09:32:33Z

Issues

This pull request fixes OData/AspNetCoreOData#797.

Description

Fix wrong links generated for derived type with navigation property when odata.metadata=full.

There are scenarios where the resource id already contains the cast segment but ComputeEditLink method defined in ODataConventionalEntityMetadataBuilder does not detected that so it ends up adding an extra cast segment.

When that happens, you end up with the response payload looking as follows:

{
    "@odata.context": "http://localhost:5219/odata/$metadata#Customers/ODataAlternateKeySample.Models.GoldCustomer/$entity",
    "@odata.type": "#ODataAlternateKeySample.Models.GoldCustomer",
    "@odata.id": "http://localhost:5219/odata/Customers(1)/ODataAlternateKeySample.Models.GoldCustomer",
    "@odata.editLink": "Customers(1)/ODataAlternateKeySample.Models.GoldCustomer/ODataAlternateKeySample.Models.GoldCustomer",
    "Id": 1,
    "Name": "Tom",
    "CountryOrRegion": null,
    "Passport": null,
    "SSN": "SSN-1-101",
    "Titles@odata.type": "#Collection(String)",
    "Titles": [ "abc", null, "efg" ],
    "Contact@odata.associationLink": "http://localhost:5219/odata/Customers(1)/ODataAlternateKeySample.Models.GoldCustomer/ODataAlternateKeySample.Models.GoldCustomer/Contact/$ref",
    "Contact@odata.navigationLink": "http://localhost:5219/odata/Customers(1)/ODataAlternateKeySample.Models.GoldCustomer/ODataAlternateKeySample.Models.GoldCustomer/Contact"
}

This pull request fixes the generation of EditLink, AssociationLink and NagivationLink such that the response payload looks as follows:

{
    "@odata.context": "http://localhost:5219/odata/$metadata#Customers/ODataAlternateKeySample.Models.GoldCustomer/$entity",
    "@odata.type": "#ODataAlternateKeySample.Models.GoldCustomer",
    "@odata.id": "http://localhost:5219/odata/Customers(1)/ODataAlternateKeySample.Models.GoldCustomer",
    "@odata.editLink": "Customers(1)/ODataAlternateKeySample.Models.GoldCustomer",
    "Id": 1,
    "Name": "Tom",
    "CountryOrRegion": null,
    "Passport": null,
    "SSN": "SSN-1-101",
    "Titles@odata.type": "#Collection(String)",
    "Titles": [ "abc", null, "efg" ],
    "Contact@odata.associationLink": "http://localhost:5219/odata/Customers(1)/ODataAlternateKeySample.Models.GoldCustomer/Contact/$ref",
    "Contact@odata.navigationLink": "http://localhost:5219/odata/Customers(1)/ODataAlternateKeySample.Models.GoldCustomer/Contact"
}

Explanation for the refactor of code in test classes

The code in the following test classes has been significantly refactored:

ODataConventionalEntityMetadataBuilderTests
ODataConventionalUriBuilderTests
ODataNavigationLinkTests
TestModel

Subsequent to the introduction of ODataUriParser and ODataPath classes to determine whether the resource id already contains the cast segment, tests in some classes started failing. An investigation revealed that in most scenarios, the test logic was impractical.

In the ODataConventionalEntityMetadataBuilderTests test class for example, the tests for multi-property key were impractical since the entity type that the tests were based upon wasn't defined with a multi-property key. A refactor was therefore necessary since the ODataUriParser validates the supplied keys against the Edm model. The test class setup code had a lot of logical inconsistencies.

In the ODataNavigationLinkTests test class, the refactor involved setting the delegates that are invoked to determine the model and the base Uri.

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

gathogojr · 2023-01-11T10:57:33Z

+                if (!uri.ToString().EndsWith(string.Concat('/', this.ResourceMetadataContext.ActualResourceTypeName), StringComparison.Ordinal))
+                {
+                    uri = this.UriBuilder.AppendTypeSegment(uri, this.ResourceMetadataContext.ActualResourceTypeName);
+                }


I considered using the following logic as an alternative but was concerned about increase in allocations and the appearance of overkill:

var odataUriParser = new OData.UriParser.ODataUriParser(this.MetadataContext.Model, this.MetadataContext.ServiceBaseUri, uri); var odataPath = odataUriParser.ParsePath(); if (!(odataPath.LastSegment is OData.UriParser.TypeSegment typeSegment && typeSegment.TargetEdmType == this.ResourceMetadataContext.ActualResourceType)) { uri = this.UriBuilder.AppendTypeSegment(uri, this.ResourceMetadataContext.ActualResourceTypeName); }

Eventually adopted this approach since it made it easy to cater for the scenario raised here

xuzhg · 2023-01-18T18:37:40Z

            if (this.ResourceMetadataContext.ActualResourceTypeName != this.ResourceMetadataContext.TypeContext.NavigationSourceEntityTypeName)
            {
-                uri = this.UriBuilder.AppendTypeSegment(uri, this.ResourceMetadataContext.ActualResourceTypeName);
+                // In some scenarios, the resource Id will already contain the cast segment


what scenarios? a little bit details?

@xuzhg Added an example of a scenario

habbes · 2023-01-19T05:36:16Z

            {
-                uri = this.UriBuilder.AppendTypeSegment(uri, this.ResourceMetadataContext.ActualResourceTypeName);
+                // In some scenarios, the resource Id will already contain the cast segment
+                if (!uri.ToString().EndsWith(string.Concat('/', this.ResourceMetadataContext.ActualResourceTypeName), StringComparison.Ordinal))


string.Concat does not have an overload that accepts a char as the first argument, so this method call will probably invoke an overload that takes object as an argument, which means the / char will be boxed and cause an allocation.

I suspect it would be more efficient to call string.Concat("/", ...) instead.

Here's an illustration of the IL generated for both variants: https://sharplab.io/#v2:C4LglgNgNAJiDUAfAAgJgIwFgBQyDMABGgQMIEDeOB1Rhy6ADKQPYB2AxgIbAkAWnAJwAU9JgGcAlBSo1ZyAOwEAysAFhWAcwB0JNl2BCA5AHpDUApIDcM6gF8bBB/iKMWHbirWaRrydOyycoqiOnrcQgBExhHmVg722LZAA

Also, it may be possible to match the string ending without performing a concatenation, but maybe the code complexity this would introduce is not worth it for this edge case (and it's not guaranteed that it would perform better). e.g.,: if (foo.EndsWith(bar) && foo[foo.length - bar.length - 1] == '/' ...) (there's probably an off-by-one error in there, but you get the idea).

This logic seems to hinge on the fact ActualResourceTypeName is a fully-qualified name, and in this context only a type segment would be a fully qualified name. Is that correct?

KenitoInc · 2023-01-19T08:34:07Z

+            if (actualResourceTypeName != this.ResourceMetadataContext.TypeContext.NavigationSourceEntityTypeName)
            {
-                uri = this.UriBuilder.AppendTypeSegment(uri, this.ResourceMetadataContext.ActualResourceTypeName);
+                // In some scenarios, the resource id will already contain the cast segment. For example,


Have you considered a scenario where the id comes after the typesegment?
e.g ~/Customers/My.Customer(1)

This is supported in the library

odata.net/test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/PathFunctionalTests.cs

Line 779 in ecd0850

public void KeysExpressionsCanAppearOnTypeSegments()

@KenitoInc I made use of the ODataUriParser and ODataPath classes to take care of this scenario. Please check the updated logic

uffelauesen · 2023-01-19T20:15:03Z

“No type-cast segment is added to the canonical URL, even if the entity is an instance of a type derived from the declared entity type of its entity set.”
Taken from the spec: http://docs.oasis-open.org/odata/odata/v4.0/errata03/os/complete/part2-url-conventions/odata-v4.0-errata03-os-part2-url-conventions-complete.html#_Toc453752341
To me this equals to odata.id not having a type cast segment as well.
Think about it - the Canonical URL already uniquely identifies the entity, a type cast segment does nothing good in identifying the entity. Services are required to and do return the correct derived object with all its additions included, no need to have the server perform a type filter on top of an id/key look up. The issue reported is in AspNetCoreOData not in OData repos.

Thanks for looking into it.
uffe

mikepizzo · 2023-01-20T03:57:22Z

+                // when full metadata is requested and a derived entity containing one or more navigation properties
+                // is being serialized, and the association and navigation links need to be serialized as well, the cast
+                // segment will already be contained in the resource id.
+                string uriToString = uri.ToString();


ToString()

I wonder if we should trim any trailing forward-slash.

mikepizzo

…hen odata.metadata=full

pull-request-quantifier-deprecated · 2023-01-24T13:36:16Z

This PR has 298 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Large
Size       : +235 -63
Percentile : 69.8%

Total files changed: 6

Change summary by file extension:
.cs : +235 -63

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

gathogojr · 2023-01-24T13:55:12Z

        private readonly ODataConventionalUriBuilder uriBuilder = new ODataConventionalUriBuilder(DefaultBaseUri,
            ODataUrlKeyDelimiter.Parentheses);
-        private readonly TestMetadataContext metadataContext = new TestMetadataContext { GetMetadataDocumentUriFunc = () => MetadataDocumentUri, GetModelFunc = () => TestModel.Model, OperationsBoundToStructuredTypeMustBeContainerQualifiedFunc = type => false };
+        private readonly TestMetadataContext metadataContext = new TestMetadataContext


Note: The explanation for the refactor of the code in this test class in provided in the PR description

gathogojr · 2023-01-24T13:55:39Z

        public IEdmEntityType ProductType { get; set; }
        public IEdmEntityType DerivedProductType { get; set; }
        public IEdmEntityType MultipleKeyType { get; set; }
+        public IEdmEntityType DerivedMleMultiKeyType { get; set; }


Note: The explanation for the refactor of the code in this test class in provided in the PR description

gathogojr · 2023-01-24T13:56:11Z


        public ODataNavigationLinkTests()
        {
+            var model = new EdmModel();


Note: The explanation for the refactor of the code in this test class in provided in the PR description

KenitoInc

LGTM

gathogojr · 2023-01-31T08:25:19Z

Replaced by OData/AspNetCoreOData#831

gathogojr requested review from ElizabethOkerio, KenitoInc, corranrogue9, habbes, lisicase, mikepizzo and xuzhg January 11, 2023 09:32

pull-request-quantifier-deprecated Bot added the Medium label Jan 11, 2023

gathogojr commented Jan 11, 2023

View reviewed changes

xuzhg previously approved these changes Jan 18, 2023

View reviewed changes

habbes reviewed Jan 19, 2023

View reviewed changes

gathogojr dismissed xuzhg’s stale review via b603cac January 19, 2023 07:49

gathogojr force-pushed the fix/797-wrong-links-for-derived-type-with-navigation-property branch from 4576f25 to b603cac Compare January 19, 2023 07:49

habbes previously approved these changes Jan 19, 2023

View reviewed changes

KenitoInc reviewed Jan 19, 2023

View reviewed changes

KenitoInc mentioned this pull request Jan 19, 2023

Prevent multiple cast segments #2585

Closed

2 tasks

mikepizzo reviewed Jan 20, 2023

View reviewed changes

mikepizzo previously approved these changes Jan 20, 2023

View reviewed changes

ElizabethOkerio previously approved these changes Jan 20, 2023

View reviewed changes

Fix wrong links generated for derived type with navigation property w…

6762f75

…hen odata.metadata=full

gathogojr dismissed stale reviews from ElizabethOkerio, mikepizzo, and habbes via 6762f75 January 23, 2023 12:48

gathogojr force-pushed the fix/797-wrong-links-for-derived-type-with-navigation-property branch from b603cac to 6762f75 Compare January 23, 2023 12:48

Fix failing tests

7016331

pull-request-quantifier-deprecated Bot added Large and removed Medium labels Jan 24, 2023

gathogojr commented Jan 24, 2023

View reviewed changes

KenitoInc approved these changes Jan 26, 2023

View reviewed changes

gathogojr closed this Jan 31, 2023

gathogojr deleted the fix/797-wrong-links-for-derived-type-with-navigation-property branch January 31, 2023 08:25

Uh oh!

Conversation

gathogojr commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Description

Explanation for the refactor of code in test classes

Checklist (Uncheck if it is not completed)

Additional work necessary

Uh oh!

gathogojr Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uffelauesen commented Jan 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikepizzo left a comment

Choose a reason for hiding this comment

Uh oh!

pull-request-quantifier-deprecated Bot commented Jan 24, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KenitoInc left a comment

Choose a reason for hiding this comment

Uh oh!

gathogojr commented Jan 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gathogojr commented Jan 11, 2023 •

edited

Loading

gathogojr Jan 11, 2023 •

edited

Loading