Support NavigationPropertyBinding paths ending in cast segments by mikepizzo · Pull Request #2504 · OData/odata.net

mikepizzo · 2022-09-06T16:34:00Z

Issues

This pull request fixes #2502.

Description

OData 4.01 adds the ability to distinguish nav prop bindings to the same navigation property by type by appending a type segment.

This PR adds support to ODL to have different navigation property bindings to the same navigation property that vary by type.

Checklist (Uncheck if it is not completed)

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

Additional work necessary

Consider only writing nav prop bindings that terminate in type segment for 4.01 responses.

…ent for V4.01 or greater

xuzhg · 2022-09-19T23:36:33Z

-                        return new UnresolvedNavigationPropertyPath(definingType, binding.Path, binding.Location);
+                       if (lastNavProp != null && !lastNavProp.ContainsTarget)
+                       {
+                            // TODO: Improve error message #644.


shall we improve the error message as well?

xuzhg · 2022-09-19T23:37:29Z

+            Assert.NotNull(navProp);
+            Assert.Equal(2, setA.NavigationPropertyBindings.Count());
+
+            // NavPropBinding for for EntityX1 should be SetX1


duplicate 'for' #Resolved

xuzhg · 2022-09-19T23:38:39Z

+            Assert.Equal(setX1, X1NavPropBinding.Target);
+            Assert.Equal(setX1, setA.FindNavigationTarget(navProp, new EdmPathExpression("Nav/NS.EntityX1")));
+
+            // NavPropBinding for for EntityX2 should be SetX2


duplicate 'for' #Resolved

xuzhg · 2022-09-19T23:39:58Z

+            var X1NavPropBinding = setA.NavigationPropertyBindings.FirstOrDefault(b => b.Path.PathSegments.Last() == "NS.EntityX1");
+            Assert.NotNull(X1NavPropBinding);
+            var setX1 = model.FindDeclaredNavigationSource("SetX1");
+            Assert.Equal(setX1, X1NavPropBinding.Target);


shall we use 'Assert.Same' to make sure they are referencing the same instance? #Resolved

habbes · 2022-11-09T04:37:32Z

-            if (navigationProperty.Name != bindingPath.PathSegments.Last())
+            // Last segment must be the navigation segment or a type cast segment
+            string lastSegment = null;
+            string penultimateSegment = null;


Word of the day :)

# Conflicts: # test/FunctionalTests/Microsoft.OData.Edm.Tests/Microsoft.OData.Edm.Tests.csproj

pull-request-quantifier-deprecated · 2022-11-15T03:41:48Z

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

Quantification details

Label      : Extra Large
Size       : +372 -91
Percentile : 82.1%

Total files changed: 12

Change summary by file extension:
.sln : +0 -0
.csproj : +51 -68
.cs : +321 -23

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.

mikepizzo · 2022-11-15T05:14:28Z

@habbes, @xuzhg -- addressed a couple comments and updated tests for 4.0 versus 4.01. please re-review. thanks!

xuzhg · 2022-11-15T06:00:41Z

-      True
-      True
-      Microsoft.OData.Core.tt
-    


Are they not needed again?

xuzhg · 2022-11-15T06:02:58Z

-      True
-      Microsoft.OData.Edm.tt
-    
-    


removed accidently?

xuzhg · 2022-11-15T06:04:12Z

+            // Last segment must be the navigation segment or a type cast segment
+            string lastSegment = null;
+            string penultimateSegment = null;
+            foreach (string segment in bindingPath.PathSegments)


Cannot use "LastOrDefault()"?
Then check the return is null or not?

xuzhg · 2022-11-15T06:05:50Z

+        [Theory]
+        public void ValidateNavigationPropertyBindingPathEndingInTypeCast(string odataVersion)
+        {
+            var entitySetWithNavProperties =


maybe name it as " entitySetWithNavBindings"

xuzhg · 2022-11-15T06:06:00Z

+                              "" +
+                              "" +
+                            "";
+            var entitySetWithoutNavProperties =


maybe name it as " entitySetWithoutNavBindings"

xuzhg · 2022-11-15T06:08:38Z

@@ -81,20 +81,6 @@
    
  


It seems there's no source code changed in OData.Core project.

Support NavigationPropertyBinding paths ending in cast segments

7ad56a3

pull-request-quantifier-deprecated Bot added the Large label Sep 6, 2022

mikepizzo marked this pull request as draft September 6, 2022 16:34

Merge branch 'master' into Fix2502

0b6e661

pull-request-quantifier-deprecated Bot added Extra Large and removed Large labels Sep 6, 2022

mikepizzo added 3 commits September 13, 2022 13:01

In XML, only write the nav prop bindings that end in a type cast segm…

2259613

…ent for V4.01 or greater

Merge branch 'Fix2502' of https://github.com/odata/odata.net

b4591e7

Improve error checking

c35d1e9

mikepizzo marked this pull request as ready for review September 13, 2022 21:31

xuzhg reviewed Sep 19, 2022

View reviewed changes

xuzhg previously approved these changes Sep 19, 2022

View reviewed changes

habbes previously approved these changes Nov 11, 2022

View reviewed changes

Address review feedback

78052a3

mikepizzo dismissed stale reviews from habbes and xuzhg via 78052a3 November 14, 2022 20:41

mikepizzo added 2 commits November 14, 2022 12:51

Merge branch 'master' of https://github.com/odata/odata.net

a40e262

# Conflicts: # test/FunctionalTests/Microsoft.OData.Edm.Tests/Microsoft.OData.Edm.Tests.csproj

Fix CsdlReaderTests for different versions

6158b83

mikepizzo requested review from habbes and xuzhg November 15, 2022 05:12

xuzhg approved these changes Nov 15, 2022

View reviewed changes

mikepizzo merged commit 370cd96 into master Nov 16, 2022

mikepizzo deleted the Fix2502 branch November 16, 2022 16:23

Conversation

mikepizzo commented Sep 6, 2022

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg Sep 19, 2022 • edited by mikepizzo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg Sep 19, 2022 • edited by mikepizzo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg Sep 19, 2022 • edited by mikepizzo 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!

pull-request-quantifier-deprecated Bot commented Nov 15, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

mikepizzo commented Nov 15, 2022

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xuzhg Sep 19, 2022 •

edited by mikepizzo

Loading

xuzhg Sep 19, 2022 •

edited by mikepizzo

Loading

xuzhg Sep 19, 2022 •

edited by mikepizzo

Loading