Updates navigation property expansion logic (#160)

* Remove unnecessary code * Expand nav. prop. based on identifier name and not type * Update tests * Update comment * Re-add method and add obsolete attribute * Expand all containment navigation properties Don't check on the defining type to determine whether or not to expand, but on whether the navigation property has already been navigated in the path * Update tests * Update Beta integration file and tests * Update src/Microsoft.OpenApi.OData.Reader/Edm/ODataPathProvider.cs Co-authored-by: Vincent Biret <[email protected]> Co-authored-by: Vincent Biret <[email protected]>
microsoft · Feb 10, 2022 · 4294381 · 4294381
1 parent 8ff1ab7
commit 4294381
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 58 deletions.
diff --git a/src/Microsoft.OpenApi.OData.Reader/Edm/EdmModelExtensions.cs b/src/Microsoft.OpenApi.OData.Reader/Edm/EdmModelExtensions.cs
@@ -152,6 +152,7 @@ public static IEnumerable<IEdmComplexType> FindAllBaseTypes(this IEdmComplexType
         /// <param name="baseType">Type of the base type.</param>
         /// <param name="subtype">Type of the sub type.</param>
         /// <returns>true, if the <paramref name="baseType"/> is assignable to <paramref name="subtype"/>. Otherwise returns false.</returns>
+        [Obsolete]
         public static bool IsAssignableFrom(this IEdmEntityType baseType, IEdmEntityType subtype)
         {
             Utils.CheckArgumentNull(baseType, nameof(baseType));

diff --git a/src/Microsoft.OpenApi.OData.Reader/Edm/ODataPathProvider.cs b/src/Microsoft.OpenApi.OData.Reader/Edm/ODataPathProvider.cs
@@ -324,24 +324,44 @@ private void RetrieveMediaEntityStreamPaths(IEdmEntityType entityType, ODataPath
         /// <param name="count">The count restrictions.</param>
         /// <param name="currentPath">The current OData path.</param>
         /// <param name="convertSettings">The settings for the current conversion.</param>
-        private void RetrieveNavigationPropertyPaths(IEdmNavigationProperty navigationProperty, CountRestrictionsType count, ODataPath currentPath, OpenApiConvertSettings convertSettings)
+        /// <param name="visitedNavigationProperties">A stack that holds the visited navigation properties in the <paramref name="currentPath"/>.</param>
+        private void RetrieveNavigationPropertyPaths(
+            IEdmNavigationProperty navigationProperty,
+            CountRestrictionsType count,
+            ODataPath currentPath,
+            OpenApiConvertSettings convertSettings,
+            Stack<string> visitedNavigationProperties = null)
         {
             Debug.Assert(navigationProperty != null);
             Debug.Assert(currentPath != null);
 
+            if (visitedNavigationProperties == null)
+            {
+                visitedNavigationProperties = new ();
+            }
+
+            var navPropFullyQualifiedName = $"{navigationProperty.DeclaringType.FullTypeName()}/{navigationProperty.Name}";
+
+            // Check whether the navigation property has already been navigated in the path
+            if (visitedNavigationProperties.Contains(navPropFullyQualifiedName))
+            {
+                return;
+            }
+
             // Check whether the navigation property should be part of the path
             NavigationRestrictionsType navigation = _model.GetRecord<NavigationRestrictionsType>(navigationProperty, CapabilitiesConstants.NavigationRestrictions);
             if (navigation != null && !navigation.IsNavigable)
             {
                 return;
             }
 
-            // test the expandable for the navigation property.
-            bool shouldExpand = ShouldExpandNavigationProperty(navigationProperty, currentPath);
+            // Whether to expand the navigation property
+            bool shouldExpand = navigationProperty.ContainsTarget;
 
             // append a navigation property.
             currentPath.Push(new ODataNavigationPropertySegment(navigationProperty));
             AppendPath(currentPath.Clone());
+            visitedNavigationProperties.Push(navPropFullyQualifiedName);
 
             // Check whether a collection-valued navigation property should be indexed by key value(s).
             NavigationPropertyRestriction restriction = navigation?.RestrictedProperties?.FirstOrDefault();
@@ -412,7 +432,7 @@ private void RetrieveNavigationPropertyPaths(IEdmNavigationProperty navigationPr
                         {
                             if (CanFilter(subNavProperty))
                             {
-                                RetrieveNavigationPropertyPaths(subNavProperty, count, currentPath, convertSettings);
+                                RetrieveNavigationPropertyPaths(subNavProperty, count, currentPath, convertSettings, visitedNavigationProperties);
                             }
                         }
                     }
@@ -421,35 +441,11 @@ private void RetrieveNavigationPropertyPaths(IEdmNavigationProperty navigationPr
                 if (targetsMany)
                 {
                     currentPath.Pop();
-                }
+                }                
             }
             currentPath.Pop();
-        }
-
-        private bool ShouldExpandNavigationProperty(IEdmNavigationProperty navigationProperty, ODataPath currentPath)
-        {
-            Debug.Assert(navigationProperty != null);
-            Debug.Assert(currentPath != null);
-
-            // not expand for the non-containment.
-            if (!navigationProperty.ContainsTarget)
-            {
-                return false;
-            }
-
-            // check the type is visited before, if visited, not expand it.
-            IEdmEntityType navEntityType = navigationProperty.ToEntityType();
-            foreach (ODataSegment segment in currentPath)
-            {
-                if (segment.EntityType != null && 
-                    navEntityType.IsAssignableFrom(segment.EntityType))
-                {
-                    return false;
-                }
-            }
-
-            return true;
-        }
+            visitedNavigationProperties.Pop();
+        }              
 
         /// <summary>
         /// Create $ref paths.

diff --git a/test/Microsoft.OpenAPI.OData.Reader.Tests/Edm/ODataPathProviderTests.cs b/test/Microsoft.OpenAPI.OData.Reader.Tests/Edm/ODataPathProviderTests.cs
@@ -48,7 +48,7 @@ public void GetPathsForGraphBetaModelReturnsAllPaths()
 
             // Assert
             Assert.NotNull(paths);
-            Assert.Equal(28227, paths.Count());
+            Assert.Equal(26436, paths.Count());
         }
 
         [Fact]
@@ -67,7 +67,7 @@ public void GetPathsForGraphBetaModelWithDerivedTypesConstraintReturnsAllPaths()
 
             // Assert
             Assert.NotNull(paths);
-            Assert.Equal(28193, paths.Count());
+            Assert.Equal(26394, paths.Count());
         }
 
         [Fact]

diff --git a/test/Microsoft.OpenAPI.OData.Reader.Tests/Resources/Graph.Beta.OData.xml b/test/Microsoft.OpenAPI.OData.Reader.Tests/Resources/Graph.Beta.OData.xml
@@ -7495,11 +7495,6 @@
               <String>microsoft.graph.orgContact</String>
             </Collection>
           </Annotation>
-          <Annotation Term="Org.OData.Capabilities.V1.NavigationRestrictions">
-          <Record>
-            <PropertyValue Property="Referenceable" Bool="true" />
-          </Record>
-        </Annotation>
         </NavigationProperty>
         <NavigationProperty Name="licenseDetails" Type="Collection(graph.licenseDetails)" ContainsTarget="true" />
         <NavigationProperty Name="manager" Type="graph.directoryObject" />
@@ -7804,7 +7799,7 @@
         <Property Name="uid" Type="Edm.String" />
         <Property Name="webLink" Type="Edm.String" />
         <NavigationProperty Name="attachments" Type="Collection(graph.attachment)" ContainsTarget="true" />
-        <NavigationProperty Name="calendar" Type="graph.calendar" ContainsTarget="true" />
+        <NavigationProperty Name="calendar" Type="graph.calendar" />
         <NavigationProperty Name="exceptionOccurrences" Type="Collection(graph.event)" ContainsTarget="true" />
         <NavigationProperty Name="extensions" Type="Collection(graph.extension)" ContainsTarget="true" />
         <NavigationProperty Name="instances" Type="Collection(graph.event)" ContainsTarget="true" />
@@ -8125,11 +8120,11 @@
         <NavigationProperty Name="drive" Type="graph.drive" />
         <NavigationProperty Name="drives" Type="Collection(graph.drive)" />
         <NavigationProperty Name="externalColumns" Type="Collection(graph.columnDefinition)" />
-        <NavigationProperty Name="items" Type="Collection(graph.baseItem)" ContainsTarget="true" />
+        <NavigationProperty Name="items" Type="Collection(graph.baseItem)" />
         <NavigationProperty Name="lists" Type="Collection(graph.list)" ContainsTarget="true" />
         <NavigationProperty Name="pages" Type="Collection(graph.sitePage)" ContainsTarget="true" />
         <NavigationProperty Name="permissions" Type="Collection(graph.permission)" ContainsTarget="true" />
-        <NavigationProperty Name="sites" Type="Collection(graph.site)" ContainsTarget="true" />
+        <NavigationProperty Name="sites" Type="Collection(graph.site)" />
         <NavigationProperty Name="termStore" Type="microsoft.graph.termStore.store" ContainsTarget="true" />
         <NavigationProperty Name="onenote" Type="graph.onenote" ContainsTarget="true" />
       </EntityType>
@@ -13883,7 +13878,7 @@
         <Property Name="actor" Type="graph.identitySet" />
         <Property Name="times" Type="graph.itemActivityTimeSet" />
         <NavigationProperty Name="driveItem" Type="graph.driveItem" />
-        <NavigationProperty Name="listItem" Type="graph.listItem" ContainsTarget="true" />
+        <NavigationProperty Name="listItem" Type="graph.listItem" />
       </EntityType>
       <EntityType Name="driveItem" BaseType="graph.baseItem" OpenType="true">
         <Property Name="audio" Type="graph.audio" />
@@ -13913,9 +13908,9 @@
         <Property Name="video" Type="graph.video" />
         <Property Name="webDavUrl" Type="Edm.String" />
         <NavigationProperty Name="workbook" Type="graph.workbook" ContainsTarget="true" />
-        <NavigationProperty Name="activities" Type="Collection(graph.itemActivityOLD)" ContainsTarget="true" />
+        <NavigationProperty Name="activities" Type="Collection(graph.itemActivityOLD)" />
         <NavigationProperty Name="analytics" Type="graph.itemAnalytics" />
-        <NavigationProperty Name="children" Type="Collection(graph.driveItem)" ContainsTarget="true" />
+        <NavigationProperty Name="children" Type="Collection(graph.driveItem)" />
         <NavigationProperty Name="listItem" Type="graph.listItem" ContainsTarget="true" />
         <NavigationProperty Name="permissions" Type="Collection(graph.permission)" ContainsTarget="true" />
         <NavigationProperty Name="subscriptions" Type="Collection(graph.subscription)" ContainsTarget="true" />
@@ -15729,7 +15724,7 @@
         <Property Name="requestApprovalSettings" Type="graph.approvalSettings" />
         <Property Name="requestorSettings" Type="graph.requestorSettings" />
         <NavigationProperty Name="accessPackage" Type="graph.accessPackage" />
-        <NavigationProperty Name="accessPackageCatalog" Type="graph.accessPackageCatalog" ContainsTarget="true" />
+        <NavigationProperty Name="accessPackageCatalog" Type="graph.accessPackageCatalog" />
       </EntityType>
       <EntityType Name="accessPackageAssignmentRequest" BaseType="graph.entity">
         <Property Name="answers" Type="Collection(graph.accessPackageAnswer)" />
@@ -15743,14 +15738,14 @@
         <Property Name="requestType" Type="Edm.String" />
         <Property Name="schedule" Type="graph.requestSchedule" />
         <NavigationProperty Name="accessPackage" Type="graph.accessPackage" />
-        <NavigationProperty Name="accessPackageAssignment" Type="graph.accessPackageAssignment" ContainsTarget="true" />
+        <NavigationProperty Name="accessPackageAssignment" Type="graph.accessPackageAssignment" />
         <NavigationProperty Name="requestor" Type="graph.accessPackageSubject" ContainsTarget="true" />
       </EntityType>
       <EntityType Name="accessPackageAssignmentResourceRole" BaseType="graph.entity">
         <Property Name="originId" Type="Edm.String" />
         <Property Name="originSystem" Type="Edm.String" />
         <Property Name="status" Type="Edm.String" />
-        <NavigationProperty Name="accessPackageAssignments" Type="Collection(graph.accessPackageAssignment)" ContainsTarget="true" />
+        <NavigationProperty Name="accessPackageAssignments" Type="Collection(graph.accessPackageAssignment)" />
         <NavigationProperty Name="accessPackageResourceRole" Type="graph.accessPackageResourceRole" ContainsTarget="true" />
         <NavigationProperty Name="accessPackageResourceScope" Type="graph.accessPackageResourceScope" ContainsTarget="true" />
         <NavigationProperty Name="accessPackageSubject" Type="graph.accessPackageSubject" ContainsTarget="true" />
@@ -25187,8 +25182,8 @@
         <Property Name="sectionGroupsUrl" Type="Edm.String" />
         <Property Name="sectionsUrl" Type="Edm.String" />
         <NavigationProperty Name="parentNotebook" Type="graph.notebook" />
-        <NavigationProperty Name="parentSectionGroup" Type="graph.sectionGroup" ContainsTarget="true" />
-        <NavigationProperty Name="sectionGroups" Type="Collection(graph.sectionGroup)" ContainsTarget="true" />
+        <NavigationProperty Name="parentSectionGroup" Type="graph.sectionGroup" />
+        <NavigationProperty Name="sectionGroups" Type="Collection(graph.sectionGroup)" />
         <NavigationProperty Name="sections" Type="Collection(graph.onenoteSection)" ContainsTarget="true" />
       </EntityType>
       <EntityType Name="onenoteSection" BaseType="graph.onenoteEntityHierarchyModel">
@@ -25197,7 +25192,7 @@
         <Property Name="pagesUrl" Type="Edm.String" />
         <NavigationProperty Name="pages" Type="Collection(graph.onenotePage)" ContainsTarget="true" />
         <NavigationProperty Name="parentNotebook" Type="graph.notebook" />
-        <NavigationProperty Name="parentSectionGroup" Type="graph.sectionGroup" ContainsTarget="true" />
+        <NavigationProperty Name="parentSectionGroup" Type="graph.sectionGroup" />
       </EntityType>
       <EntityType Name="operation" BaseType="graph.entity">
         <Property Name="createdDateTime" Type="Edm.DateTimeOffset" />
@@ -25221,7 +25216,7 @@
         <Property Name="title" Type="Edm.String" />
         <Property Name="userTags" Type="Collection(Edm.String)" />
         <NavigationProperty Name="parentNotebook" Type="graph.notebook" />
-        <NavigationProperty Name="parentSection" Type="graph.onenoteSection" ContainsTarget="true" />
+        <NavigationProperty Name="parentSection" Type="graph.onenoteSection" />
       </EntityType>
       <EntityType Name="onenoteResource" BaseType="graph.onenoteEntityBaseModel">
         <Property Name="content" Type="Edm.Stream" />
@@ -25516,7 +25511,7 @@
         <Property Name="registeredRoot" Type="Edm.String" />
         <Property Name="status" Type="Edm.String" />
         <Property Name="type" Type="Edm.String" />
-        <NavigationProperty Name="parent" Type="graph.governanceResource" ContainsTarget="true" />
+        <NavigationProperty Name="parent" Type="graph.governanceResource" />
         <NavigationProperty Name="roleAssignmentRequests" Type="Collection(graph.governanceRoleAssignmentRequest)" ContainsTarget="true" />
         <NavigationProperty Name="roleAssignments" Type="Collection(graph.governanceRoleAssignment)" ContainsTarget="true" />
         <NavigationProperty Name="roleDefinitions" Type="Collection(graph.governanceRoleDefinition)" ContainsTarget="true" />
@@ -25533,7 +25528,7 @@
         <Property Name="status" Type="graph.governanceRoleAssignmentRequestStatus" />
         <Property Name="subjectId" Type="Edm.String" />
         <Property Name="type" Type="Edm.String" Nullable="false" />
-        <NavigationProperty Name="resource" Type="graph.governanceResource" ContainsTarget="true" />
+        <NavigationProperty Name="resource" Type="graph.governanceResource" />
         <NavigationProperty Name="roleDefinition" Type="graph.governanceRoleDefinition" ContainsTarget="true" />
         <NavigationProperty Name="subject" Type="graph.governanceSubject" ContainsTarget="true" />
       </EntityType>
@@ -25549,7 +25544,7 @@
         <Property Name="status" Type="Edm.String" Nullable="false" />
         <Property Name="subjectId" Type="Edm.String" />
         <NavigationProperty Name="linkedEligibleRoleAssignment" Type="graph.governanceRoleAssignment" />
-        <NavigationProperty Name="resource" Type="graph.governanceResource" ContainsTarget="true" />
+        <NavigationProperty Name="resource" Type="graph.governanceResource" />
         <NavigationProperty Name="roleDefinition" Type="graph.governanceRoleDefinition" ContainsTarget="true" />
         <NavigationProperty Name="subject" Type="graph.governanceSubject" ContainsTarget="true" />
       </EntityType>
@@ -25558,8 +25553,8 @@
         <Property Name="externalId" Type="Edm.String" />
         <Property Name="resourceId" Type="Edm.String" />
         <Property Name="templateId" Type="Edm.String" />
-        <NavigationProperty Name="resource" Type="graph.governanceResource" ContainsTarget="true" />
-        <NavigationProperty Name="roleSetting" Type="graph.governanceRoleSetting" ContainsTarget="true" />
+        <NavigationProperty Name="resource" Type="graph.governanceResource" />
+        <NavigationProperty Name="roleSetting" Type="graph.governanceRoleSetting" />
       </EntityType>
       <EntityType Name="governanceRoleSetting" BaseType="graph.entity">
         <Property Name="adminEligibleSettings" Type="Collection(graph.governanceRuleSetting)" />
@@ -25571,7 +25566,7 @@
         <Property Name="roleDefinitionId" Type="Edm.String" />
         <Property Name="userEligibleSettings" Type="Collection(graph.governanceRuleSetting)" />
         <Property Name="userMemberSettings" Type="Collection(graph.governanceRuleSetting)" />
-        <NavigationProperty Name="resource" Type="graph.governanceResource" ContainsTarget="true" />
+        <NavigationProperty Name="resource" Type="graph.governanceResource" />
         <NavigationProperty Name="roleDefinition" Type="graph.governanceRoleDefinition" ContainsTarget="true" />
       </EntityType>
       <EntityType Name="governanceSubject" BaseType="graph.entity">
@@ -71201,7 +71196,7 @@
           </Record>
         </Annotation>
       </Annotations>
-      <Annotations Target="microsoft.graph.application/createdOnBehalfOf">
+            <Annotations Target="microsoft.graph.application/createdOnBehalfOf">
         <Annotation Term="Org.OData.Capabilities.V1.NavigationRestrictions">
           <Record>
             <PropertyValue Property="Referenceable" Bool="true" />