method to get all navigation property bindings #2157

chrisspre · 2021-08-12T00:57:20Z

Add an officially supported method to get all navigation property bindings.

Retrieving all navigation property bindings requires quite some knowledge of the edm model implementation and casting.
This method encapsulated this logic.

Issues

addresses #2060

mikepizzo · 2021-08-12T21:22:10Z

You'll need to update the publicapi baseline for the new method.

mikepizzo · 2021-08-12T21:23:26Z

Is the common pattern to get all nav prop bindings for the entire container, or to get the nav prop bindings for a particular entityset/singleton?

src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs

chrisspre · 2021-08-12T21:40:10Z

Is the common pattern to get all nav prop bindings for the entire container, or to get the nav prop bindings for a particular entityset/singleton?

In my experience, yes. For example, when Graph needs to decide if and how deep insert is possible, they are building a sort of map where to "find" an entity of a given type. This came up in a bug a few month back and I believe it would be good to encapsulate and support a way to do this without understanding the full interface hierarchy of EDM.

More info in the associated issue#2060

corranrogue9 · 2021-10-13T21:41:13Z

src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs

+        /// </summary>
+        /// <param name="container">Reference to the calling object.</param>
+        /// <returns>collection of pairs of container element and NavigationPropertyBindings.</returns>
+        public static IEnumerable<(IEdmEntityContainerElement, IEdmNavigationPropertyBinding)> GetNavigationPropertyBindings(this IEdmEntityContainer container)


Will the caller be interested in the specific kind of IEdmEntityContainerElement returned for each element? We are already casting inside this method, it would be a shame for the caller to have to do so as well if we can avoid it

it is true that a user would need to cast the first element of the pair if they are interested in more than what IEdmEntityContainerElement provides (which is the name, container, kind and a few extension methods).
But I doubt that this is common since this API is specifically about all bindings, independent of the type of "bindable" container element.

corranrogue9 · 2021-10-13T21:47:22Z

src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs

+                        }
+                        break;
+                    case IEdmSingleton singleton:
+                        foreach (var binding in singleton.NavigationPropertyBindings)


It's a more fundamental change, but I wonder if it would make sense for IEdmEntitySet and IEdmSingleton to derive from a common interface that has NavigationPropertyBindings on it. Then this whole method is sort of obsolete (or at least very derivative):

return container .AllElements() .OfType<IEdmWithNavPropertyBindings>() .SelectMany(element => element.NavigationPropertyBindings.Select(binding => (element, binding)));

src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs

…dings

...unctionalTests/Microsoft.OData.Edm.Tests/Csdl/Semantics/CsdlSemanticsEntityContainerTests.cs

habbes · 2022-08-05T05:51:59Z

...unctionalTests/Microsoft.OData.Edm.Tests/Csdl/Semantics/CsdlSemanticsEntityContainerTests.cs

+            var bindings = container.GetNavigationPropertyBindings().ToList();
+
+            // assert
+            Assert.Equal(2, bindings.Count);


I think we should also assert the actual bindings are what's expected?

@habbes Added requested asserts

pull-request-quantifier-deprecated · 2022-08-05T09:10:11Z

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

Quantification details

Label      : Small
Size       : +57 -0
Percentile : 22.8%

Total files changed: 5

Change summary by file extension:
.cs : +45 -0
.bsl : +12 -0

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.

pull-request-quantifier-deprecated bot added the Extra Small label Aug 12, 2021

mikepizzo reviewed Aug 12, 2021

View reviewed changes

src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs Outdated Show resolved Hide resolved

KenitoInc requested review from KenitoInc, ElizabethOkerio, gathogojr, habbes, marabooy and xuzhg August 16, 2021 11:03

corranrogue9 reviewed Oct 13, 2021

View reviewed changes

corranrogue9 previously approved these changes Oct 13, 2021

View reviewed changes

gathogojr reviewed May 5, 2022

View reviewed changes

src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs Outdated Show resolved Hide resolved

gathogojr reviewed May 5, 2022

View reviewed changes

src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs Outdated Show resolved Hide resolved

gathogojr reviewed May 5, 2022

View reviewed changes

src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs Outdated Show resolved Hide resolved

gathogojr dismissed corranrogue9’s stale review via 54d3aeb July 21, 2022 13:12

chrisspre and others added 2 commits July 21, 2022 17:42

add an officially supported method to get all navigation property bin…

5b18c01

…dings

Replace unsupported concise syntax

ca43c4e

gathogojr force-pushed the get-navprop-bindings branch from 54d3aeb to ca43c4e Compare July 21, 2022 14:43

Update baseline

1940668

gathogojr reviewed Jul 21, 2022

View reviewed changes

...unctionalTests/Microsoft.OData.Edm.Tests/Csdl/Semantics/CsdlSemanticsEntityContainerTests.cs Outdated Show resolved Hide resolved

gathogojr added 2 commits August 4, 2022 14:50

Fix test

8af9ba2

Remove use of var

d937cd2

KenitoInc previously approved these changes Aug 4, 2022

View reviewed changes

Improve method doc

2e3d3f7

gathogojr dismissed KenitoInc’s stale review via 2e3d3f7 August 4, 2022 12:33

gathogojr requested review from gathogojr and corranrogue9 August 4, 2022 13:28

gathogojr requested review from KenitoInc and mikepizzo August 4, 2022 13:28

habbes reviewed Aug 5, 2022

View reviewed changes

KenitoInc previously approved these changes Aug 5, 2022

View reviewed changes

Address review comments

e3d36cd

gathogojr dismissed KenitoInc’s stale review via e3d36cd August 5, 2022 09:10

pull-request-quantifier-deprecated bot added Small and removed Extra Small labels Aug 5, 2022

KenitoInc approved these changes Aug 5, 2022

View reviewed changes

habbes approved these changes Aug 5, 2022

View reviewed changes

gathogojr approved these changes Aug 5, 2022

View reviewed changes

gathogojr merged commit 9e40c7d into master Aug 8, 2022

gathogojr deleted the get-navprop-bindings branch August 8, 2022 04:52

KenitoInc mentioned this pull request Aug 18, 2022

Add method to retrieve all NavigationProperyBindings. #2060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

method to get all navigation property bindings #2157

method to get all navigation property bindings #2157

chrisspre commented Aug 12, 2021

mikepizzo commented Aug 12, 2021

mikepizzo commented Aug 12, 2021

chrisspre commented Aug 12, 2021

corranrogue9 Oct 13, 2021

chrisspre May 4, 2022

corranrogue9 Oct 13, 2021 •

edited

Loading

habbes Aug 5, 2022

gathogojr Aug 5, 2022

pull-request-quantifier-deprecated bot commented Aug 5, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

method to get all navigation property bindings #2157

method to get all navigation property bindings #2157

Conversation

chrisspre commented Aug 12, 2021

Issues

mikepizzo commented Aug 12, 2021

mikepizzo commented Aug 12, 2021

chrisspre commented Aug 12, 2021

corranrogue9 Oct 13, 2021

Choose a reason for hiding this comment

chrisspre May 4, 2022

Choose a reason for hiding this comment

corranrogue9 Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

habbes Aug 5, 2022

Choose a reason for hiding this comment

gathogojr Aug 5, 2022

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Aug 5, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

corranrogue9 Oct 13, 2021 •

edited

Loading