Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uri parsing on type cast on property allowed in validation vocabulary #1295

Merged
merged 2 commits into from Dec 7, 2018

Conversation

Projects
None yet
3 participants
@xuzhg
Copy link
Member

commented Oct 18, 2018

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.

@xuzhg xuzhg requested a review from mikepizzo Oct 18, 2018

@xuzhg xuzhg force-pushed the xuzhg:ParseTypeCastWithValidataion branch from e96f5ce to 2bc0559 Oct 18, 2018

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

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Oct 22, 2018

Member

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

This comment has been minimized.

Copy link
@xuzhg

xuzhg Oct 23, 2018

Author Member

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

@@ -1432,5 +1434,50 @@ private void CheckDollarEachSegmentRestrictions(int index)
}
}
}

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

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Oct 22, 2018

Member

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.

This comment has been minimized.

Copy link
@xuzhg

xuzhg Oct 23, 2018

Author Member

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

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Nov 27, 2018

Member

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)

This comment has been minimized.

Copy link
@xuzhg

xuzhg Nov 27, 2018

Author Member

@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

@xuzhg xuzhg force-pushed the xuzhg:ParseTypeCastWithValidataion branch 2 times, most recently from b9b3c8b to aad6983 Oct 22, 2018

@xuzhg xuzhg added this to the 7.6 milestone Oct 24, 2018

@xuzhg xuzhg added the under-review label Oct 24, 2018

@xuzhg xuzhg force-pushed the xuzhg:ParseTypeCastWithValidataion branch 2 times, most recently from e173037 to 0b17729 Oct 30, 2018

@xuzhg xuzhg force-pushed the xuzhg:ParseTypeCastWithValidataion branch from 0b17729 to d59865b Nov 16, 2018

{
Debug.Assert(operation != null);

if (this.parsedSegments == null || !this.parsedSegments.Any())

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Nov 27, 2018

Member

|| !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

This comment has been minimized.

Copy link
@xuzhg

xuzhg Nov 27, 2018

Author Member

Yes. You are right. #Closed

@@ -1432,5 +1437,150 @@ private void CheckDollarEachSegmentRestrictions(int index)
}
}
}

private void CheckTypeCastSegmentRestriction(ODataPathSegment previous, IEdmType targetEdmType)

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Nov 27, 2018

Member

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.

This comment has been minimized.

Copy link
@xuzhg

xuzhg Nov 27, 2018

Author Member

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

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 4, 2018

Member

Separate offline discussion on how to support annotating ReturnType.


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

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

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Nov 27, 2018

Member

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

This comment has been minimized.

Copy link
@xuzhg

xuzhg Nov 27, 2018

Author Member

Yes. # Fixed. #Closed

@mikepizzo
Copy link
Member

left a comment

🕐

@xuzhg xuzhg force-pushed the xuzhg:ParseTypeCastWithValidataion branch from d59865b to 8d3fbe8 Nov 27, 2018

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

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 4, 2018

Member

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

@@ -819,6 +819,7 @@ PathParser_EntityReferenceNotSupported=The request URI is not valid. $ref cannot
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.

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 4, 2018

Member

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

how about "due to an"

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 7, 2018

Author Member

Changed. Thanks.

Uri parsing on type cast using DerivedTypeConstraints term
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

@xuzhg xuzhg force-pushed the xuzhg:ParseTypeCastWithValidataion branch 2 times, most recently from 546c326 to be845a3 Dec 6, 2018

@mikepizzo
Copy link
Member

left a comment

:shipit:

@xuzhg xuzhg force-pushed the xuzhg:ParseTypeCastWithValidataion branch from be845a3 to c9c9cea Dec 7, 2018

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

1 of 2 checks passed

continuous-integration/vsts The build trigger has fired.
Details
license/cla All CLA requirements met.
Details

@xuzhg xuzhg deleted the xuzhg:ParseTypeCastWithValidataion branch May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.