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

Remove the NavigationPropertyEntityMustNotIndirectlyContainItself rule #1042

Merged
merged 2 commits into from Jan 30, 2018

Conversation

Projects
None yet
2 participants
@xuzhg
Member

xuzhg commented Jan 24, 2018

Issues

  • Issue from the Graph beta validation. It shows:

Problem 3: NavigationPropertyEntityMustNotIndirectlyContainItself:The navigation property 'events' is invalid because it indirectly contains itself.

Description

When we validate the Graph beta CSDL, it gives us the following error messages:

The navigation property 'drive' is invalid because it indirectly contains itself.
The navigation property 'driveItem' is invalid because it indirectly contains itself.
The navigation property 'site' is invalid because it indirectly contains itself.
The navigation property 'worksheet' is invalid because it indirectly contains itself.
The navigation property 'worksheet' is invalid because it indirectly contains itself.
The navigation property 'charts' is invalid because it indirectly contains itself.

As discussed with Mike, it's better to remove the in-correct validation rule.

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

This comment has been minimized.

Member

xuzhg commented Jan 25, 2018

@mikepizzo #Closed

@@ -3115,7 +3114,6 @@ public sealed class Microsoft.OData.Edm.Validation.ValidationRules {
public static readonly Microsoft.OData.Edm.Validation.ValidationRule`1[[Microsoft.OData.Edm.IEdmNavigationProperty]] NavigationPropertyDependentPropertiesMustBelongToDependentEntity = Microsoft.OData.Edm.Validation.ValidationRule`1[Microsoft.OData.Edm.IEdmNavigationProperty]
public static readonly Microsoft.OData.Edm.Validation.ValidationRule`1[[Microsoft.OData.Edm.IEdmNavigationProperty]] NavigationPropertyDuplicateDependentProperty = Microsoft.OData.Edm.Validation.ValidationRule`1[Microsoft.OData.Edm.IEdmNavigationProperty]
public static readonly Microsoft.OData.Edm.Validation.ValidationRule`1[[Microsoft.OData.Edm.IEdmNavigationProperty]] NavigationPropertyEndWithManyMultiplicityCannotHaveOperationsSpecified = Microsoft.OData.Edm.Validation.ValidationRule`1[Microsoft.OData.Edm.IEdmNavigationProperty]
public static readonly Microsoft.OData.Edm.Validation.ValidationRule`1[[Microsoft.OData.Edm.IEdmNavigationProperty]] NavigationPropertyEntityMustNotIndirectlyContainItself = Microsoft.OData.Edm.Validation.ValidationRule`1[Microsoft.OData.Edm.IEdmNavigationProperty]

This comment has been minimized.

@mikepizzo

mikepizzo Jan 25, 2018

Member

Removing this is a breaking change that would break apps that attempt to access this property, no? Should we still support the property and just return a dummy rule that always validates to true? Or retain the rule but not include it as part of the default set of validations? Or modify the rule to enforce the semantics introduced in 4.0, namely: "If the containment is recursive, the partner navigation property MUST be nullable and specify a single entity type (i.e. have a cardinality of 0..1). If the containment is not recursive, the partner navigation property MUST NOT be nullable (i.e. have a cardinality of 1)."

This comment has been minimized.

@xuzhg

xuzhg Jan 27, 2018

Member

Thanks for pointing out. I am prefer to remove the validate rule from the default set and keep the rule unchanged.

/// A navigation property without direct containment cannot contain its declaring entity indirectly.
/// </summary>
NavigationPropertyEntityMustNotIndirectlyContainItself = 222,
// /// <summary>

This comment has been minimized.

@mikepizzo

mikepizzo Jan 25, 2018

Member

Is this commented, rather than removed, in order to make sure we don't re-use the value 222? Should we add a comment to that effect?

This comment has been minimized.

@xuzhg

xuzhg Jan 27, 2018

Member

Yes. removing it is a breaking change. I will keep it unchanged.

@xuzhg

This comment has been minimized.

Member

xuzhg commented Jan 27, 2018

@mikepizzo I changed the codes. Please take a look. #Closed

@xuzhg xuzhg merged commit a725043 into OData:master Jan 30, 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:RemoveRules branch Jan 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment