Uri parsing on type cast on property allowed in validation vocabulary by xuzhg · Pull Request #1295 · OData/odata.net

xuzhg · 2018-10-18T23:48:16Z

Issues

This pull request is part of Union type supporting.

Description

To parse the type cast on a property (structural or navigation property), the allowed type case segment should within the Validation vocabulary DerivedTypeConstraint annotation if it applied on such property, otherwise the Uri parse should throw exception.

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-10-22T14:54:58Z

+            if (annotation != null)
+            {
+                IEdmCollectionExpression collectionExpression = annotation.Value as IEdmCollectionExpression;
+                if (collectionExpression != null && collectionExpression.Elements != null)


collectionExpression.Elements != null [](start = 52, length = 37)

It should be legal to have the annotation with an empty list of derived types, in which case the type of the element must be exactly the expected type (no derived types allowed). Can we add a test for this case? #Closed

Thanks. I added a new test. Please take a look. #Closed

mikepizzo · 2018-10-22T15:13:03Z

            }
        }
+
+        private static void CheckTypeCastSegmentRestrictions(IEdmModel model, ODataPathSegment previous, IEdmType targetEdmType)


CheckTypeCastSegmentRestrictions [](start = 28, length = 32)

Note; this doesn't support scenarios where the immediately previous segment was not a property or a navigation property; for example, Customers/1/ns.PreferredCustomer. Probably okay just to note this in the code.

Thanks @mikepizzo .
I noticed the "AppliesTo" includes the "TypeDefinnition" as below:

<Term Name="DerivedTypeConstraint" Type="Collection(Core.QualifiedTypeName)" Nullable="false" AppliesTo="Property TypeDefinition">

What does it mean for 'TypeDefinition'? #Closed

If the underlying type of the TypeDefinition is Edm.Primitive, then the DerivedTypeConstraint can be used to restrict the set of primitive types (i.e., to string and int). If the DerivedTypeConstraint is defined on both a TypeDefinition and a Property where that DerivedTypeConstraint is used, then only those types that are in both lists are valid.

In reply to: 227070272 [](ancestors = 227070272)

@mikepizzo Thanks for the explanation. In this PR, i left some commented codes for type definition, because ODL doesn't support the Uri parse for the type definition, see issue at: #1326. So, i will un-comment type definition codes after i fix #1326. Is it ok? #Closed

mikepizzo · 2018-11-27T17:48:07Z

+        {
+            Debug.Assert(operation != null);
+
+            if (this.parsedSegments == null || !this.parsedSegments.Any())


|| !this.parsedSegments.Any() [](start = 44, length = 29)

|| !this.parsedSegments.Any() [](start = 44, length = 29)

This would also be caught in the this.parsedSegments.LastOrDefault() check on line 1546 below, right? #Closed

Yes. You are right. #Closed

mikepizzo · 2018-11-27T17:51:08Z

            }
        }
+
+        private void CheckTypeCastSegmentRestriction(ODataPathSegment previous, IEdmType targetEdmType)


CheckTypeCastSegmentRestriction [](start = 21, length = 31)

We should also validate, if the previous segment is an operation, that the return type of the operation can be cast to the specified type.

@mikepizzo I saw you closed the related issues (#52) as not-fixed. But, how to annotation the ReturnType of the operation?

Separate offline discussion on how to support annotating ReturnType.

In reply to: 236788996 [](ancestors = 236788996)

mikepizzo · 2018-11-27T17:55:09Z

+        /// The model referenced to.
+        /// The target annotatable to find annotation.
+        /// Null or a collection string of qualifed type name.
+        public static IList GetDerivedTypeConstraints(this IEdmModel model, IEdmVocabularyAnnotatable target)


IList [](start = 22, length = 5)

IList [](start = 22, length = 5)

Should this be an IEnumerable, rather than an IList? We don't expect the caller to add/remove from the list. #Closed

Yes. # Fixed. #Closed

mikepizzo

🕐

mikepizzo · 2018-12-04T18:31:09Z

+	[
+	ExtensionAttribute(),
+	]
+	public static System.Collections.Generic.IList`1[[System.String]] GetDerivedTypeConstraints (Microsoft.OData.Edm.IEdmModel model, Microsoft.OData.Edm.Vocabularies.IEdmVocabularyAnnotatable target)


IList [](start = 42, length = 5)

IList [](start = 42, length = 5)

This should be updated to IEnumerable rather than IList as per the updated code. #Closed

mikepizzo · 2018-12-04T18:58:45Z

 PathParser_CannotUseValueOnCollection=$value cannot be applied to a collection.
 PathParser_TypeMustBeRelatedToSet=The type '{0}' does not inherit from and is not a base type of '{1}'. The type of '{2}' must be related to the Type of the EntitySet.
 PathParser_TypeCastOnlyAllowedAfterStructuralCollection=Type cast segment '{0}' after a collection which is not of entity or complex type is not allowed.
+PathParser_TypeCastOnlyAllowedInDerivedTypeConstraint=Type cast segment '{0}' on {1} '{2}' is not allowed from the Org.OData.Validation.V1.DerivedTypeConstraint annotation.


from the [](start = 106, length = 8)

how about "due to an"

Changed. Thanks.

1. Type cast on singleton: ~/Me/NS.Cast 2. Type cast on entity set: /Users/NS.Cast 3. Type cast on entity: ~/users(1)/NS.Cast 4. Type cast on property: ~/user(1)/property/NS.Cast 5. Type cast on navigation property: ~/users(1)/navigationproperty/NS.Cast 6. Type cast on binding parameter

mikepizzo

xuzhg requested a review from mikepizzo October 18, 2018 23:49

xuzhg force-pushed the ParseTypeCastWithValidataion branch from e96f5ce to 2bc0559 Compare October 18, 2018 23:54

mikepizzo reviewed Oct 22, 2018

View reviewed changes

xuzhg force-pushed the ParseTypeCastWithValidataion branch 2 times, most recently from b9b3c8b to aad6983 Compare October 22, 2018 18:52

xuzhg requested review from KanishManuja-MS, biaol-odata, mikepizzo and raheph October 24, 2018 20:44

xuzhg added this to the 7.6 milestone Oct 24, 2018

xuzhg added the under-review label Oct 24, 2018

xuzhg force-pushed the ParseTypeCastWithValidataion branch 2 times, most recently from e173037 to 0b17729 Compare October 30, 2018 18:34

KanishManuja-MS removed the Ready for review label Nov 6, 2018

xuzhg force-pushed the ParseTypeCastWithValidataion branch from 0b17729 to d59865b Compare November 16, 2018 23:47

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

xuzhg assigned mikepizzo Nov 16, 2018

mikepizzo reviewed Nov 27, 2018

View reviewed changes

mikepizzo suggested changes Nov 27, 2018

View reviewed changes

xuzhg force-pushed the ParseTypeCastWithValidataion branch from d59865b to 8d3fbe8 Compare November 27, 2018 18:39

mikepizzo reviewed Dec 4, 2018

View reviewed changes

xuzhg force-pushed the ParseTypeCastWithValidataion branch 2 times, most recently from 546c326 to be845a3 Compare December 7, 2018 00:53

mikepizzo approved these changes Dec 7, 2018

View reviewed changes

Resolve the comments

c9c9cea

xuzhg force-pushed the ParseTypeCastWithValidataion branch from be845a3 to c9c9cea Compare December 7, 2018 17:33

xuzhg merged commit 8ee60e5 into OData:master Dec 7, 2018

xuzhg deleted the ParseTypeCastWithValidataion branch May 14, 2019 17:40

Conversation

xuzhg commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

Uh oh!

mikepizzo Oct 22, 2018 • 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!

xuzhg Oct 22, 2018 • 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!

mikepizzo Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

xuzhg Oct 22, 2018 • 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!

mikepizzo Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

xuzhg Nov 27, 2018 • 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!

mikepizzo Nov 27, 2018 • 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!

xuzhg Nov 27, 2018 • 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!

mikepizzo Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

xuzhg Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

mikepizzo Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

mikepizzo Nov 27, 2018 • 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!

xuzhg Nov 27, 2018 • 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!

mikepizzo left a comment

Choose a reason for hiding this comment

Uh oh!

mikepizzo Dec 4, 2018 • 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!

mikepizzo Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

xuzhg Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

mikepizzo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xuzhg commented Oct 18, 2018 •

edited

Loading

mikepizzo Oct 22, 2018 •

edited

Loading

xuzhg Oct 22, 2018 •

edited by mikepizzo

Loading

xuzhg Oct 22, 2018 •

edited by mikepizzo

Loading

xuzhg Nov 27, 2018 •

edited by mikepizzo

Loading

mikepizzo Nov 27, 2018 •

edited

Loading

xuzhg Nov 27, 2018 •

edited by mikepizzo

Loading

mikepizzo Nov 27, 2018 •

edited

Loading

xuzhg Nov 27, 2018 •

edited by mikepizzo

Loading

mikepizzo Dec 4, 2018 •

edited

Loading