Issue2467: Support Core.AlternateKeys #2470

xuzhg · 2022-07-26T21:56:51Z

Issues

This pull request fixes #2467.

Description

Support Org.OData.Core.V1.AlternateKeys.

Checklist (Uncheck if it is not completed)

[x ] Test cases added
[x ] 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.

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

src/Microsoft.OData.Edm/Vocabularies/CoreVocabularyConstants.cs

Co-authored-by: Clément Habinshuti <clhabins@microsoft.com>

pull-request-quantifier-deprecated · 2022-08-08T15:29:00Z

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

Quantification details

Label      : Medium
Size       : +82 -30
Percentile : 42.4%

Total files changed: 8

Change summary by file extension:
.cs : +67 -27
.bsl : +15 -3

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.

xuzhg · 2022-08-08T15:30:31Z

@habbes Thanks for your review and suggestions.

syprieur · 2022-09-01T22:21:24Z

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

@@ -2146,14 +2146,22 @@ public static bool IsKey(this IEdmProperty property)
        /// <param name="model">The model to be used.</param>
        /// <param name="type">Reference to the calling object.</param>
        /// <param name="alternateKey">Dictionary of alias and structural properties for the alternate key.</param>
-        public static void AddAlternateKeyAnnotation(this EdmModel model, IEdmEntityType type, IDictionary<string, IEdmProperty> alternateKey)
+        /// <param name="useCore">A flag to indicate which alternate term to use.
+        /// If ture, 'Org.OData.Core.V1.AlternateKeys' is used, otherwise, 'OData.Community.Keys.V1.AlternateKeys' is used.


syprieur · 2022-09-01T22:22:04Z

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

        {
            EdmUtil.CheckArgumentNull(model, "model");
            EdmUtil.CheckArgumentNull(type, "type");
            EdmUtil.CheckArgumentNull(alternateKey, "alternateKey");

+            IEdmTerm alternateKeysTerm = useCore ? CoreVocabularyModel.AlternateKeysTerm : AlternateKeysVocabularyModel.AlternateKeysTerm;
+
+            IEdmComplexType propertyRefType = useCore ? CoreVocabularyModel.PropertyRefType : AlternateKeysVocabularyModel.PropertyRefType;


UT and handling of duplicate entries under each annotation namespace?

syprieur · 2022-09-01T22:41:18Z

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

+
+                // For backwards-compability, we merge the alternate keys from community and core vocabulary annotations.
+
+                retrieveAnnotationAction(annotationValue);


The output of GetAlternateKeysAnnotation(this IEdmModel model, IEdmEntityType type) does not honor the unicity requirements from the community vocabulary spec:

Services SHOULD NOT return multiple alternate key definitions for the same entity type that are composed of the exact same set of properties.

The alternate keys are not "merged" as the comment line 3483 calls out, it's instead an union that occurs.

syprieur · 2022-09-01T22:44:00Z

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

-                Debug.Assert(keys != null, "expected IEdmCollectionExpression for alternate key annotation value");
-
-                foreach (IEdmRecordExpression key in keys.Elements.OfType<IEdmRecordExpression>())
+                Action<IEdmVocabularyAnnotation> retrieveAnnotationAction = ann =>


Define a private static function rather than this action closure which will incur an extra allocation for each call.

xuzhg added 4 commits July 26, 2022 14:56

Issue2467: Support Core.AlternateKeys

2986fc5

Resolve the failing

97064ab

merge the alternate keys together when retrieving

8ba72e2

Update the public API base line

038ef9c

xuzhg requested review from mikepizzo, corranrogue9, gathogojr, habbes and KenitoInc July 28, 2022 20:43

Remove unused paramater

07e1196

xuzhg force-pushed the AlternateCoreVocabulary branch from a84ea21 to 07e1196 Compare July 28, 2022 20:46

xuzhg requested a review from ElizabethOkerio July 29, 2022 17:59

habbes reviewed Aug 8, 2022

View reviewed changes

xuzhg and others added 2 commits August 8, 2022 08:14

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

c5a831f

Co-authored-by: Clément Habinshuti <clhabins@microsoft.com>

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

8bffaa3

Co-authored-by: Clément Habinshuti <clhabins@microsoft.com>

pull-request-quantifier-deprecated bot added the Medium label Aug 8, 2022

xuzhg added 2 commits August 8, 2022 08:26

Resolve the comments

2aca55d

Resolve the failing

629d547

habbes approved these changes Aug 8, 2022

View reviewed changes

xuzhg merged commit 7275ffa into master Aug 8, 2022

syprieur reviewed Sep 1, 2022

View reviewed changes

syprieur mentioned this pull request Sep 1, 2022

Update ODL to use configurable alternate key vocabulary #2467

Open

syprieur reviewed Sep 1, 2022

View reviewed changes

gathogojr deleted the AlternateCoreVocabulary branch October 13, 2022 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue2467: Support Core.AlternateKeys #2470

Issue2467: Support Core.AlternateKeys #2470

xuzhg commented Jul 26, 2022

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

What can I do to optimize my changes

How to interpret the change counts in git diff output

xuzhg commented Aug 8, 2022

syprieur Sep 1, 2022

syprieur Sep 1, 2022

syprieur Sep 1, 2022

syprieur Sep 1, 2022


		// For backwards-compability, we merge the alternate keys from community and core vocabulary annotations.

		retrieveAnnotationAction(annotationValue);

Issue2467: Support Core.AlternateKeys #2470

Issue2467: Support Core.AlternateKeys #2470

Conversation

xuzhg commented Jul 26, 2022

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

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

What can I do to optimize my changes

How to interpret the change counts in git diff output

xuzhg commented Aug 8, 2022

syprieur Sep 1, 2022

Choose a reason for hiding this comment

syprieur Sep 1, 2022

Choose a reason for hiding this comment

syprieur Sep 1, 2022

Choose a reason for hiding this comment

syprieur Sep 1, 2022

Choose a reason for hiding this comment