Add the writing derived type constraint validation #1358

xuzhg · 2018-12-11T20:18:41Z

Issues

*This pull request continues on Union type feature.

Description

Enable writing derived type constraint validation on:

Resource
Nested resource info
Edm.PrimitiveType property

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.

mikepizzo · 2018-12-12T19:21:28Z

src/Microsoft.OData.Core/PropertyMetadataTypeInfo.cs

@@ -13,7 +14,11 @@ namespace Microsoft.OData
    /// </summary>
    internal class PropertyMetadataTypeInfo
    {
-        public PropertyMetadataTypeInfo(string name, IEdmStructuredType owningType)
+        private bool loadDerivedTypeConstraints;


loadDerivedTypeConstraints [](start = 21, length = 26)

loadDerivedTypeConstraints [](start = 21, length = 26)

nit: sounds like directive (to load or not to load) rather than state. perhaps rename to something like "derivedTypeConstraintsLoaded" #Closed

#resolved. #Closed

mikepizzo · 2018-12-12T19:26:30Z

src/Microsoft.OData.Core/WriterValidationUtils.cs

+        /// </summary>
+        /// <param name="propertySerializationInfo">The property serialization info.</param>
+        /// <remarks>This does NOT validate the value of the property, just the type of property.</remarks>
+        internal static void ValidateCollectionPropertyDerivedTypeConstraint(ODataCollectionValue collectionValue,


collectionValue [](start = 98, length = 15)

collectionValue [](start = 98, length = 15)

Is collectionValue used anywhere? this looks like the same logic as above, which takes only PropertySerializationInfo. #Resolved

Sorry, forgot to clean the collection. #Resolved. #Resolved

mikepizzo · 2018-12-12T19:32:34Z

src/Microsoft.OData.Core/WriterValidationUtils.cs

+        /// <param name="metadataType">The type from metadata.</param>
+        /// <param name="navigationSource">The navigation source.</param>
+        /// <param name="derivedTypeConstraints">The derived type constraints on the nested resource.</param>
+        internal static void ValidateDerivedTypeConstraintOnNavigationSource(IEdmStructuredType resourceType, IEdmStructuredType metadataType, IEdmNavigationSource navigationSource, IEnumerable<string> derivedTypeConstraints)


ValidateDerivedTypeConstraintOnNavigationSource [](start = 29, length = 47)

ValidateDerivedTypeConstraintOnNavigationSource [](start = 29, length = 47)

ValidateDerivedTypeConstraintOnNavigationSource and ValidateDerivedTypeConstraintOnNestedResourceInfo have the same logic, except that one uses the name of the navigation source in the exception and the other uses the name from the nestedresourceinfo. Instead of two overloads, unless you think you'll need the navigationsource (or additional information out of the nestedResourceInfo) in the future, how about just having a single overload that takes a string property "navigationSourceName", and pass the appropriate string from the caller? #Resolved

#resolved. #Resolved

mikepizzo · 2018-12-12T20:14:12Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ODataWriterDerivedTypeConstraintTests.cs

+            Assert.Contains("/$metadata#Me\",\"Id\":7}", customerActual);
+
+            // Singleton has the derived type constraint.
+            SetDerivedTypeAnnotation(this.edmModel, this.edmMe, "NS.VipCustomer");


SetDerivedTypeAnnotation [](start = 12, length = 24)

does a new instance of edmModel get created with each test case run, or is the derived type constraint set for all subsequent tests? #Closed

@mikepizzo For every single test, xUnit.net creates a new instance of the test class, so, in each test method, the "this.edmModel" is the basic edm model created in the constructor.

Below is the description from xUnit document at: https://xunit.github.io/docs/shared-context

xUnit.net creates a new instance of the test class for every test that is run, so any code which is placed into the constructor of the test class will be run for every single test. This makes the constructor a convenient place to put reusable context setup code where you want to share the code without sharing object instances (meaning, you get a clean copy of the context object(s) for every test that is run).

#Closed

👍 Thanks for the research @xuzhg! #Closed

mikepizzo

🕐

resource and Edm primitive type property

mikepizzo · 2018-12-17T22:56:36Z

src/Microsoft.OData.Core/WriterValidationUtils.cs

+        /// </summary>
+        /// <param name="propertySerializationInfo">The property serialization info.</param>
+        /// <remarks>This does NOT validate the value of the property, just the type of property.</remarks>
+        internal static void ValidateCollectionPropertyDerivedTypeConstraint(ODataCollectionValue collectionValue,


ValidateCollectionPropertyDerivedTypeConstraint [](start = 29, length = 47)

Can this be combined with ValidatePropertyDerivedTypeConstraint? It looks like the logic is exactly the same, and that the ODataCollectionValue is not used. #Resolved

#Resolved. That's old codes to verify the collection(Edm.PrimitiveType). Not, it's not valid. And I removed. Thanks. #Resolved

mikepizzo · 2018-12-17T22:57:52Z

src/Microsoft.OData.Core/WriterValidationUtils.cs

+        /// <summary>
+        /// Validates the value type of a property meets the derived type constraints.
+        /// </summary>
+        /// <param name="propertySerializationInfo">The property serialization info.</param>


param name [](start = 13, length = 10)

parameters in comments don't match parameters in function signature (ODataCollectionValue is missing). If we do need this overload to take an ODataCollectionValue, then we need to add that parameter to the comments. #Resolved

mikepizzo

xuzhg force-pushed the UnionTypeWriter branch from 69077b9 to 5dcb07d Compare December 11, 2018 22:11

xuzhg requested review from mikepizzo, KanishManuja-MS and raheph December 11, 2018 22:12

xuzhg added the Ready for review Use this label if a pull request is ready to be reviewed label Dec 11, 2018

xuzhg added this to the 7.6 milestone Dec 11, 2018

mikepizzo reviewed Dec 12, 2018

View reviewed changes

mikepizzo requested changes Dec 12, 2018

View reviewed changes

mikepizzo removed the Ready for review Use this label if a pull request is ready to be reviewed label Dec 12, 2018

xuzhg added 3 commits December 13, 2018 13:31

Enable Derived type constraint verification on property

695aa89

Add writing derived type contraint validation on resource, nested

f57569c

resource and Edm primitive type property

resolvce the comments

d024f82

xuzhg force-pushed the UnionTypeWriter branch from 5dcb07d to d024f82 Compare December 13, 2018 22:02

xuzhg added the Ready for review Use this label if a pull request is ready to be reviewed label Dec 13, 2018

mikepizzo reviewed Dec 17, 2018

View reviewed changes

Remove the unused codes

9672536

mikepizzo approved these changes Dec 18, 2018

View reviewed changes

xuzhg merged commit a69ab2f into OData:master Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the writing derived type constraint validation #1358

Add the writing derived type constraint validation #1358

xuzhg commented Dec 11, 2018

mikepizzo Dec 12, 2018 •

edited

Loading

xuzhg Dec 13, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 12, 2018 •

edited

Loading

xuzhg Dec 18, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 12, 2018 •

edited

Loading

xuzhg Dec 13, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 12, 2018 •

edited

Loading

xuzhg Dec 13, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 13, 2018 •

edited

Loading

mikepizzo left a comment

mikepizzo Dec 17, 2018 •

edited

Loading

xuzhg Dec 18, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 17, 2018 •

edited

Loading

mikepizzo left a comment

Add the writing derived type constraint validation #1358

Add the writing derived type constraint validation #1358

Conversation

xuzhg commented Dec 11, 2018

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

mikepizzo Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

xuzhg Dec 13, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

mikepizzo Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

xuzhg Dec 18, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

mikepizzo Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

xuzhg Dec 13, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

mikepizzo Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

xuzhg Dec 13, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

mikepizzo Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

mikepizzo left a comment

Choose a reason for hiding this comment

mikepizzo Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

xuzhg Dec 18, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

mikepizzo Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

mikepizzo left a comment

Choose a reason for hiding this comment

mikepizzo Dec 12, 2018 •

edited

Loading

xuzhg Dec 13, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 12, 2018 •

edited

Loading

xuzhg Dec 18, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 12, 2018 •

edited

Loading

xuzhg Dec 13, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 12, 2018 •

edited

Loading

xuzhg Dec 13, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 13, 2018 •

edited

Loading

mikepizzo Dec 17, 2018 •

edited

Loading

xuzhg Dec 18, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 17, 2018 •

edited

Loading