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

Fix support for containment paths in nav prop bindings #1109

Merged
merged 4 commits into from Mar 19, 2018

Conversation

Projects
None yet
4 participants
@mikepizzo
Member

mikepizzo commented Mar 7, 2018

Description

Fixed numerous issues with containment nav prop bindings.
-Fixed issues with locating the proper contained entity set in FindNavigationTarget
-Fixed nav prop binding issues with nav props on complex types
-Fixed issues with reading/writing nav prop bindings for contained entity sets
-Added support for writing context for contained nav props under LibraryCompatibility.Version6

Added support for entity types used exclusively in singletons and single-valued nav props without defining a key.

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

Additional work necessary

@mikepizzo mikepizzo requested review from xuzhg, AlanWong-MS and biaol-odata Mar 16, 2018

@@ -497,7 +497,7 @@ public static void AddDefaultContainerFixup(this EdmModel model, string namespac
};
// create NavigationPropertyBinding
foreach (EdmNavigationProperty property in model.SchemaElements.OfType<IEdmEntityType>().SelectMany(entityType => entityType.DeclaredNavigationProperties()))
foreach (EdmNavigationProperty property in model.SchemaElements.OfType<IEdmEntityType>().SelectMany(entityType => entityType.DeclaredNavigationProperties()).Where(p=>p.ContainsTarget == false))

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

p=>p.ContainsTarget == false [](start = 175, length = 28)

can use p => !p.ContainsTarget

@@ -63,7 +63,7 @@ internal class ExpressionTreeToXmlSerializer : ExpressionVisitor
public static String SerializeToString(Expression expression)
{
outputText = new StringBuilder();
writer = XmlTextWriter.Create(outputText);
writer = XmlWriter.Create(outputText);

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

XmlWriter [](start = 21, length = 9)

Is it a bug?

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

XmlTextWriter.Create is not supported in some environments.


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

@@ -425,6 +423,151 @@ public void ValidateOutOfLineAnnotationWithValidTargetsInReferencedModel()
Assert.IsTrue(validationResult, "Expected no validation errors from annotation with targets in an external model.");
}
[TestMethod]
public void ValidateUnresolvedInlineAnnotationTargets()

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

public void ValidateUnresolvedInlineAnnotationTargets() [](start = 8, length = 55)

The test cases seem not related to nav prop bindings?

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

Yes; I ran across several other issues in trying to fix navpropbindings, and in order to get a build out that included all of the fixes I put them in the same PR.


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

<Annotation Term=""AnnotationNS.UnknownValueTerm"" String=""Hello world!"" />
<Annotation Term=""RefNS.UnknownValueTerm"" String=""Hello world!""/>
</EntityType>
<Action Name=""SomeFunction""><ReturnType Type=""Edm.Int32""/>

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

<ReturnType Type=""Edm.Int32""/> [](start = 32, length = 32)

put returntype at new line? #Resolved

var items = typeof(ValidationRules).GetFields().Where(f => f.Name != "NavigationPropertyEntityMustNotIndirectlyContainItself")
var items = typeof(ValidationRules).GetFields().Where(f =>
f.Name != "NavigationPropertyEntityMustNotIndirectlyContainItself" &&
f.Name != "EntityTypeKeyMissingOnEntityType")

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

"EntityTypeKeyMissingOnEntityType" [](start = 26, length = 34)

I'd like to keep it unremoved and add the key in the corresponding test cases? #ByDesign

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

We can't remove it, because it is public, but we don't want it to be part of the ValidationRules. This test verifies that all of the defined tests are part of the validation rules, so we need to omit the two that are defined but should not be part of the validation rules.


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

@@ -9,6 +9,8 @@
using Microsoft.OData.Edm;
using Microsoft.OData.Edm.Vocabularies;
using Xunit;
using System;
using System.IO;

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

move to top #Resolved

@@ -13,6 +13,7 @@
using Microsoft.OData.Edm.Validation;
using Xunit;
using ErrorStrings = Microsoft.OData.Edm.Strings;
using System.Linq;

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

System.Linq; [](start = 6, length = 12)

move to top #Resolved

@@ -17,6 +17,7 @@
using Microsoft.OData.Edm.Vocabularies.V1;
using Xunit;
using ErrorStrings = Microsoft.OData.Edm.Strings;
using System.IO;

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

System.IO; [](start = 6, length = 10)

move to top #Resolved

followupErrors.AddRange(this.ValidateStructure(new EdmVocabularyAnnotation(annotatable, annotation.Term, annotation.Qualifier, annotation.Value)));
}
}
}

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

That seems not related to the nav prop binding #ByDesign

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

Right; I ran into this while working through some related issues and just fixed it here rather than opening a separate PR.


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

foreach (IEdmVocabularyAnnotation annotation in annotatable.VocabularyAnnotations(this.model))
{
// Inline annotations have a null target.
if (annotation.Target == null)

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

annotation.Target == null [](start = 24, length = 25)

So, do we use "annotation.Target == null" to distinguish inline from outOfLine? #ByDesign

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

Yes.


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

@@ -34,7 +34,7 @@ public sealed class ValidationRuleSet : IEnumerable<ValidationRule>
ValidationRules.EntityTypeInvalidKeyNullablePart,
ValidationRules.EntityTypeEntityKeyMustBeScalar,
ValidationRules.EntityTypeInvalidKeyKeyDefinedInBaseClass,
ValidationRules.EntityTypeKeyMissingOnEntityType,
//// ValidationRules.EntityTypeKeyMissingOnEntityType,

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

ValidationRules.EntityTypeKeyMissingOnEntityType, [](start = 20, length = 49)

remove? #ByDesign

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

I wanted to leave it here, explicitly commented out, so we knew we intended it to be disabled.


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

@@ -936,6 +936,7 @@ public void WritingInFullMetadataModeWithExpandAndProjectionShouldWriteMissingMe
const string selectClause = "StreamProp1,Namespace.AlwaysBindableAction1,Namespace.AlwaysBindableFunction1,DeferredNavLink";
const string expandClause = "ExpandedNavLink($select=StreamProp1,Namespace.AlwaysBindableAction1;$expand=ExpandedNavLink($select=StreamProp2,Namespace.AlwaysBindableAction1))";
this.GetWriterOutputForContentTypeAndKnobValue("application/json;odata.metadata=full", true, itemsToWrite, Model, EntitySet, EntityType, selectClause, expandClause)

This comment has been minimized.

@xuzhg

xuzhg Mar 16, 2018

Member

maybe keep this file unchanged? #Resolved

// of the path from the contained entity set, but retained the old behavior as well
// for backward compatibility. In the next breaking change we should remove that
// behavior in FindNavigationTarget and remove this special handling of containsTarget
// by always clearing the path.

This comment has been minimized.

@AlanWong-MS

AlanWong-MS Mar 16, 2018

Contributor

Do we have any items to track this task? (along with similarly commented parts of this PR) #Resolved

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

Opened #1114.


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

@AlanWong-MS

Approved with +1 to Sam's comments.

@@ -786,13 +786,15 @@ protected override void StartNestedResourceInfoWithContent(ODataNestedResourceIn
{
// Write @odata.context annotation for navigation property
var containedEntitySet = this.CurrentScope.NavigationSource as IEdmContainedEntitySet;
if (containedEntitySet != null)
if (containedEntitySet != null && this.jsonLightOutputContext.MessageWriterSettings.LibraryCompatibility == ODataLibraryCompatibility.Version6)

This comment has been minimized.

@biaol-odata

biaol-odata Mar 18, 2018

Member

ODataLibraryCompatibility [](start = 124, length = 25)

what should we do for the case of setting.LibraryCompatiblity being Version7 or Latest? #ByDesign

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

We shouldn't write context Url for nav props unless the contextUrl doesn't follow convention; and we currently have no way for the app to tell us that it doesn't follow convention, so we should just not write it.


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

if (selectList.Any())
{
// https://github.com/OData/odata.net/issues/1104
// If the user explicitly selects and expands a nav prop, we should include both forms in contextUrl
// We can't, though, because SelectExpandBinder.AddExplicitNavPropsWhereNeeded adds all of the expanded items

This comment has been minimized.

@biaol-odata

biaol-odata Mar 18, 2018

Member

SelectExpandBinder.AddExplicitNavPropsWhereNeeded [](start = 45, length = 49)

This method doesn't exist. Not mentioned in PR #1104 either. Should it be SelectExpandClauseFinisher.AddExplicitNavPropLinksWhereNecessary instead? #Resolved

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

Yes; thanks.


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

{
return string.IsNullOrEmpty(subExpand) ? null : expandNode + ODataConstants.ContextUriProjectionStart + subExpand + ODataConstants.ContextUriProjectionEnd;
return string.IsNullOrEmpty(subExpand) && version <= ODataVersion.V4 ? null : expandNode + ODataConstants.ContextUriProjectionStart + subExpand + ODataConstants.ContextUriProjectionEnd;
}

This comment has been minimized.

@biaol-odata

biaol-odata Mar 18, 2018

Member

nit: please break this long line into two. #Resolved

/// <returns>The sub expanded node passed in.</returns>
private static SelectedPropertiesNode ProcessSubExpand(string nodeName, SelectedPropertiesNode subExpandNode)
private static SelectedPropertiesNode ProcessSubExpand(string nodeName, SelectedPropertiesNode subExpandNode, ODataVersion version)

This comment has been minimized.

@biaol-odata

biaol-odata Mar 18, 2018

Member

, ODataVersion version [](start = 116, length = 22)

This newly added "version" argument is not referenced/used in this function? #ByDesign

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

No, but we want to pass this function as the first parameter to SelectExpandClause.Traverse, which takes a Func<string, SelectedPropertiesNode, ODataVersion); it's not used in this case, but it is used in other cases so it needs to be part of the signature.


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

@@ -231,7 +231,11 @@ internal static string ParameterizedName(IEdmOperation operation)
foreach (IEdmOperationParameter parameter in operation.Parameters)
{
string typeName = "";
if (parameter.Type.IsCollection())
if (parameter.Type == null)

This comment has been minimized.

@biaol-odata

biaol-odata Mar 18, 2018

Member

parameter.Type == null [](start = 20, length = 22)

I cannot see how this typeName resolution for Untyped related to nav prop binding. Is this for a separate issue? #ByDesign

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

Yes. I combined into same PR so we could verify in the same nightly builds.


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

/// <summary>
/// Represents an EDM contained entity set.
/// </summary>
internal class EdmContainedEntitySet : EdmEntitySetBase, IEdmContainedEntitySet
{
internal readonly IEdmPathExpression NavigationPath;

This comment has been minimized.

@biaol-odata

biaol-odata Mar 18, 2018

Member

NavigationPath [](start = 45, length = 14)

nit: should we use a private member named "navigationPath", and expose it thru internal property named "NavigationPath"? #Resolved

@@ -69,7 +69,7 @@ internal class ExpressionTreeToXmlSerializer : System.Data.Test.Astoria.LateBoun
public static String Serialize(Expression exp)
{
outputText = new StringBuilder();
writer = XmlTextWriter.Create(outputText);
writer = XmlWriter.Create(outputText);

This comment has been minimized.

@biaol-odata

biaol-odata Mar 18, 2018

Member

XmlWriter.Create(outputText); [](start = 21, length = 29)

doesn't seem like this change is related to nav prop bindings. Code expression simplification instead? #ByDesign

This comment has been minimized.

@mikepizzo

mikepizzo Mar 19, 2018

Member

This was a bug that prevent compilation on certain platforms.


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

@AlanWong-MS AlanWong-MS merged commit b214dbf into OData:master Mar 19, 2018

1 of 2 checks passed

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

@mikepizzo mikepizzo deleted the mikepizzo:newContainmentNavPropBindings branch Mar 30, 2018

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