From f7611ae9ad399672a28653fe6173bc8e506a3d16 Mon Sep 17 00:00:00 2001 From: Andrew Omondi Date: Wed, 15 Jan 2025 12:20:05 +0300 Subject: [PATCH] Fixes potential duplicate operation ids of navigation property paths with overloaded composable functions (#597) --- .../Common/EdmModelHelper.cs | 121 +++++++++++------- .../Microsoft.OpenAPI.OData.Reader.csproj | 9 +- .../ComplexPropertyGetOperationHandler.cs | 2 +- .../ComplexPropertyPostOperationHandler.cs | 2 +- .../ComplexPropertyUpdateOperationHandler.cs | 2 +- .../DollarCountGetOperationHandler.cs | 6 +- .../NavigationPropertyOperationHandler.cs | 2 +- .../ODataTypeCastGetOperationHandler.cs | 2 +- ...igationPropertyGetOperationHandlerTests.cs | 64 +++++++++ 9 files changed, 152 insertions(+), 58 deletions(-) diff --git a/src/Microsoft.OpenApi.OData.Reader/Common/EdmModelHelper.cs b/src/Microsoft.OpenApi.OData.Reader/Common/EdmModelHelper.cs index d636fa0d..030baba1 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Common/EdmModelHelper.cs +++ b/src/Microsoft.OpenApi.OData.Reader/Common/EdmModelHelper.cs @@ -6,6 +6,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Security.Cryptography.X509Certificates; using Microsoft.OData.Edm; using Microsoft.OData.Edm.Csdl; using Microsoft.OData.Edm.Vocabularies; @@ -93,38 +94,40 @@ internal static bool NavigationRestrictionsAllowsNavigability( /// Generates the operation id from a navigation property path. /// /// The target . + /// The OData context. /// Optional: Identifier indicating whether it is a collection-valued non-indexed or single-valued navigation property. /// The operation id generated from the given navigation property path. - internal static string GenerateNavigationPropertyPathOperationId(ODataPath path, string prefix = null) + internal static string GenerateNavigationPropertyPathOperationId(ODataPath path, ODataContext context, string prefix = null) { - IList items = RetrieveNavigationPropertyPathsOperationIdSegments(path); + IList items = RetrieveNavigationPropertyPathsOperationIdSegments(path, context); if (!items.Any()) return null; - int lastItemIndex = items.Count - 1; + int lastItemIndex = items[items.Count-1].StartsWith("-") ? items.Count - 2 : items.Count - 1; if (!string.IsNullOrEmpty(prefix)) { - items[lastItemIndex] = prefix + Utils.UpperFirstChar(items.Last()); + items[lastItemIndex] = prefix + Utils.UpperFirstChar(items[lastItemIndex]); } else { - items[lastItemIndex] = Utils.UpperFirstChar(items.Last()); + items[lastItemIndex] = Utils.UpperFirstChar(items[lastItemIndex]); } - return string.Join(".", items); + return GenerateNavigationPropertyPathOperationId(items); } /// /// Generates the operation id from a complex property path. /// /// The target . + /// The OData context. /// Optional: Identifier indicating whether it is a collection-valued or single-valued complex property. /// The operation id generated from the given complex property path. - internal static string GenerateComplexPropertyPathOperationId(ODataPath path, string prefix = null) + internal static string GenerateComplexPropertyPathOperationId(ODataPath path, ODataContext context, string prefix = null) { - IList items = RetrieveNavigationPropertyPathsOperationIdSegments(path); + IList items = RetrieveNavigationPropertyPathsOperationIdSegments(path, context); if (!items.Any()) return null; @@ -141,15 +144,29 @@ internal static string GenerateComplexPropertyPathOperationId(ODataPath path, st items.Add(Utils.UpperFirstChar(lastSegment?.Identifier)); } - return string.Join(".", items); + return GenerateNavigationPropertyPathOperationId(items); + } + + /// + /// Generates a navigation property operation id from a list of string values. + /// + /// The list of string values. + /// The generated navigation property operation id. + private static string GenerateNavigationPropertyPathOperationId(IList items) + { + if (!items.Any()) + return null; + + return string.Join(".", items).Replace(".-", "-"); // Format any hashed value appropriately (this will be the last value) } /// /// Retrieves the segments of an operation id generated from a navigation property path. /// /// The target . + /// The OData context. /// The segments of an operation id generated from the given navigation property path. - internal static IList RetrieveNavigationPropertyPathsOperationIdSegments(ODataPath path) + internal static IList RetrieveNavigationPropertyPathsOperationIdSegments(ODataPath path, ODataContext context) { Utils.CheckArgumentNull(path, nameof(path)); @@ -173,6 +190,8 @@ s is ODataOperationSegment || Utils.CheckArgumentNull(segments, nameof(segments)); string previousTypeCastSegmentId = null; + string pathHash = string.Empty; + foreach (var segment in segments) { if (segment is ODataNavigationPropertySegment navPropSegment) @@ -189,25 +208,38 @@ s is ODataOperationSegment || previousTypeCastSegmentId = "As" + Utils.UpperFirstChar(schemaElement.Name); items.Add(previousTypeCastSegmentId); } - else if (segment is ODataOperationSegment operationSegment) - { - // Navigation property generated via composable function - items.Add(operationSegment.Identifier); + else if (segment is ODataOperationSegment operationSegment) + { + // Navigation property generated via composable function + if (operationSegment.Operation is IEdmFunction function && context.Model.IsOperationOverload(function)) + { + // Hash the segment to avoid duplicate operationIds + pathHash = string.IsNullOrEmpty(pathHash) + ? operationSegment.GetPathHash(context.Settings) + : (pathHash + operationSegment.GetPathHash(context.Settings)).GetHashSHA256().Substring(0,4); + } + + items.Add(operationSegment.Identifier); } - else if (segment is ODataKeySegment keySegment && keySegment.IsAlternateKey) - { - // We'll consider alternate keys in the operation id to eliminate potential duplicates with operation id of primary path - if (segment == segments.Last()) - { - items.Add("By" + string.Join("", keySegment.Identifier.Split(',').Select(static x => Utils.UpperFirstChar(x)))); - } - else - { - items.Add(keySegment.Identifier); - } + else if (segment is ODataKeySegment keySegment && keySegment.IsAlternateKey) + { + // We'll consider alternate keys in the operation id to eliminate potential duplicates with operation id of primary path + if (segment == segments.Last()) + { + items.Add("By" + string.Join("", keySegment.Identifier.Split(',').Select(static x => Utils.UpperFirstChar(x)))); + } + else + { + items.Add(keySegment.Identifier); + } } } + if (!string.IsNullOrEmpty(pathHash)) + { + items.Add("-" + pathHash); + } + return items; } @@ -320,9 +352,10 @@ internal static string GenerateComplexPropertyPathTagName(ODataPath path, ODataC /// Generates the operation id prefix from an OData type cast path. /// /// The target . + /// The OData context. /// Optional: Whether to include the List or Get prefix to the generated operation id. /// The operation id prefix generated from the OData type cast path. - internal static string GenerateODataTypeCastPathOperationIdPrefix(ODataPath path, bool includeListOrGetPrefix = true) + internal static string GenerateODataTypeCastPathOperationIdPrefix(ODataPath path, ODataContext context, bool includeListOrGetPrefix = true) { // Get the segment before the last OData type cast segment ODataTypeCastSegment typeCastSegment = path.Segments.OfType()?.Last(); @@ -352,7 +385,7 @@ internal static string GenerateODataTypeCastPathOperationIdPrefix(ODataPath path if (secondLastSegment is ODataComplexPropertySegment complexSegment) { string listOrGet = includeListOrGetPrefix ? (complexSegment.Property.Type.IsCollection() ? "List" : "Get") : null; - operationId = GenerateComplexPropertyPathOperationId(path, listOrGet); + operationId = GenerateComplexPropertyPathOperationId(path, context, listOrGet); } else if (secondLastSegment is ODataNavigationPropertySegment navPropSegment) { @@ -362,27 +395,27 @@ internal static string GenerateODataTypeCastPathOperationIdPrefix(ODataPath path prefix = navPropSegment?.NavigationProperty.TargetMultiplicity() == EdmMultiplicity.Many ? "List" : "Get"; } - operationId = GenerateNavigationPropertyPathOperationId(path, prefix); + operationId = GenerateNavigationPropertyPathOperationId(path, context, prefix); } else if (secondLastSegment is ODataKeySegment keySegment) { - if (isIndexedCollValuedNavProp) - { - operationId = GenerateNavigationPropertyPathOperationId(path, "Get"); + if (isIndexedCollValuedNavProp) + { + operationId = GenerateNavigationPropertyPathOperationId(path, context, "Get"); + } + else + { + string entityTypeName = keySegment.EntityType.Name; + string getPrefix = includeListOrGetPrefix ? "Get" : null; + string operationName = $"{getPrefix}{Utils.UpperFirstChar(entityTypeName)}"; + if (keySegment.IsAlternateKey) + { + string alternateKeyName = string.Join("", keySegment.Identifier.Split(',').Select(static x => Utils.UpperFirstChar(x))); + operationName = $"{operationName}By{alternateKeyName}"; + } + operationId = (entitySet != null) ? entitySet.Name : singleton.Name; + operationId += $".{entityTypeName}.{operationName}"; } - else - { - string entityTypeName = keySegment.EntityType.Name; - string getPrefix = includeListOrGetPrefix ? "Get" : null; - string operationName = $"{getPrefix}{Utils.UpperFirstChar(entityTypeName)}"; - if (keySegment.IsAlternateKey) - { - string alternateKeyName = string.Join("", keySegment.Identifier.Split(',').Select(static x => Utils.UpperFirstChar(x))); - operationName = $"{operationName}By{alternateKeyName}"; - } - operationId = (entitySet != null) ? entitySet.Name : singleton.Name; - operationId += $".{entityTypeName}.{operationName}"; - } } else if (secondLastSegment is ODataNavigationSourceSegment) { diff --git a/src/Microsoft.OpenApi.OData.Reader/Microsoft.OpenAPI.OData.Reader.csproj b/src/Microsoft.OpenApi.OData.Reader/Microsoft.OpenAPI.OData.Reader.csproj index 54adaa18..f35693c8 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Microsoft.OpenAPI.OData.Reader.csproj +++ b/src/Microsoft.OpenApi.OData.Reader/Microsoft.OpenAPI.OData.Reader.csproj @@ -15,17 +15,14 @@ netstandard2.0 Microsoft.OpenApi.OData true - 1.7.0 + 1.7.1 This package contains the codes you need to convert OData CSDL to Open API Document of Model. © Microsoft Corporation. All rights reserved. Microsoft OpenApi OData EDM https://github.com/Microsoft/OpenAPI.NET.OData - - Updates tag names for actions/functions operations #627 - - Adds nullable to double schema conversion #628 - - Updates PUT operation ID prefix from Update to Set #629 - - Ensures unique operation IDs for composable functions #630 - + - Further fix for generating unique operation ids for navigation property paths with composable overloaded functions #596 + Microsoft.OpenApi.OData.Reader ..\..\tool\Microsoft.OpenApi.OData.snk ..\..\bin\Debug\ diff --git a/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyGetOperationHandler.cs b/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyGetOperationHandler.cs index 072ac9e8..1f8d3124 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyGetOperationHandler.cs +++ b/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyGetOperationHandler.cs @@ -39,7 +39,7 @@ protected override void SetBasicInfo(OpenApiOperation operation) if (Context.Settings.EnableOperationId) { string prefix = ComplexPropertySegment.Property.Type.IsCollection() ? "List" : "Get"; - operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, prefix); + operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, Context, prefix); } // Summary and Description diff --git a/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyPostOperationHandler.cs b/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyPostOperationHandler.cs index 50b58e12..6fd736c6 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyPostOperationHandler.cs +++ b/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyPostOperationHandler.cs @@ -42,7 +42,7 @@ protected override void SetBasicInfo(OpenApiOperation operation) // OperationId if (Context.Settings.EnableOperationId) { - operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, "Set"); + operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, Context, "Set"); } // Summary and Description diff --git a/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyUpdateOperationHandler.cs b/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyUpdateOperationHandler.cs index 0d3e61e9..5973f59a 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyUpdateOperationHandler.cs +++ b/src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyUpdateOperationHandler.cs @@ -41,7 +41,7 @@ protected override void SetBasicInfo(OpenApiOperation operation) if (Context.Settings.EnableOperationId) { string prefix = OperationType == OperationType.Patch ? "Update" : "Set"; - operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, prefix); + operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, Context, prefix); } } diff --git a/src/Microsoft.OpenApi.OData.Reader/Operation/DollarCountGetOperationHandler.cs b/src/Microsoft.OpenApi.OData.Reader/Operation/DollarCountGetOperationHandler.cs index 74082736..0e78ac2e 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Operation/DollarCountGetOperationHandler.cs +++ b/src/Microsoft.OpenApi.OData.Reader/Operation/DollarCountGetOperationHandler.cs @@ -128,17 +128,17 @@ protected override void SetBasicInfo(OpenApiOperation operation) } else if (SecondLastSegment is ODataNavigationPropertySegment) { - var navPropOpId = string.Join(".", EdmModelHelper.RetrieveNavigationPropertyPathsOperationIdSegments(Path)); + var navPropOpId = string.Join(".", EdmModelHelper.RetrieveNavigationPropertyPathsOperationIdSegments(Path, Context)); operation.OperationId = $"{navPropOpId}.GetCount-{Path.GetPathHash(Context.Settings)}"; } else if (SecondLastSegment is ODataTypeCastSegment odataTypeCastSegment) { IEdmNamedElement targetStructuredType = odataTypeCastSegment.StructuredType as IEdmNamedElement; - operation.OperationId = $"{EdmModelHelper.GenerateODataTypeCastPathOperationIdPrefix(Path, false)}.GetCount.As{Utils.UpperFirstChar(targetStructuredType.Name)}-{Path.GetPathHash(Context.Settings)}"; + operation.OperationId = $"{EdmModelHelper.GenerateODataTypeCastPathOperationIdPrefix(Path, Context, false)}.GetCount.As{Utils.UpperFirstChar(targetStructuredType.Name)}-{Path.GetPathHash(Context.Settings)}"; } else if (SecondLastSegment is ODataComplexPropertySegment) { - operation.OperationId = $"{EdmModelHelper.GenerateComplexPropertyPathOperationId(Path)}.GetCount-{Path.GetPathHash(Context.Settings)}"; + operation.OperationId = $"{EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, Context)}.GetCount-{Path.GetPathHash(Context.Settings)}"; } } diff --git a/src/Microsoft.OpenApi.OData.Reader/Operation/NavigationPropertyOperationHandler.cs b/src/Microsoft.OpenApi.OData.Reader/Operation/NavigationPropertyOperationHandler.cs index 8bdbacc7..743a10e8 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Operation/NavigationPropertyOperationHandler.cs +++ b/src/Microsoft.OpenApi.OData.Reader/Operation/NavigationPropertyOperationHandler.cs @@ -107,7 +107,7 @@ protected override void SetExtensions(OpenApiOperation operation) internal string GetOperationId(string prefix = null) { - return EdmModelHelper.GenerateNavigationPropertyPathOperationId(Path, prefix); + return EdmModelHelper.GenerateNavigationPropertyPathOperationId(Path, Context, prefix); } /// diff --git a/src/Microsoft.OpenApi.OData.Reader/Operation/ODataTypeCastGetOperationHandler.cs b/src/Microsoft.OpenApi.OData.Reader/Operation/ODataTypeCastGetOperationHandler.cs index 604f31d3..ee30e3e9 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Operation/ODataTypeCastGetOperationHandler.cs +++ b/src/Microsoft.OpenApi.OData.Reader/Operation/ODataTypeCastGetOperationHandler.cs @@ -188,7 +188,7 @@ protected override void SetBasicInfo(OpenApiOperation operation) // OperationId if (Context.Settings.EnableOperationId) - operation.OperationId = EdmModelHelper.GenerateODataTypeCastPathOperationIdPrefix(Path) + $".As{Utils.UpperFirstChar(TargetSchemaElement.Name)}"; + operation.OperationId = EdmModelHelper.GenerateODataTypeCastPathOperationIdPrefix(Path, Context) + $".As{Utils.UpperFirstChar(TargetSchemaElement.Name)}"; base.SetBasicInfo(operation); } diff --git a/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/NavigationPropertyGetOperationHandlerTests.cs b/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/NavigationPropertyGetOperationHandlerTests.cs index 5e6c401d..88766ac5 100644 --- a/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/NavigationPropertyGetOperationHandlerTests.cs +++ b/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/NavigationPropertyGetOperationHandlerTests.cs @@ -131,6 +131,70 @@ public void CreateNavigationGetOperationViaComposableFunctionReturnsCorrectOpera Assert.Contains(operation.Parameters, x => x.Name == "path"); } + [Fact] + public void CreateNavigationGetOperationViaOverloadedComposableFunctionReturnsCorrectOperation() + { + // Arrange + IEdmModel model = EdmModelHelper.GraphBetaModel; + ODataContext context = new(model, new OpenApiConvertSettings() + { + EnableOperationId = true + }); + + IEdmEntitySet drives = model.EntityContainer.FindEntitySet("drives"); + IEdmEntityType drive = model.SchemaElements.OfType().First(c => c.Name == "drive"); + IEdmNavigationProperty items = drive.DeclaredNavigationProperties().First(c => c.Name == "items"); + IEdmEntityType driveItem = model.SchemaElements.OfType().First(c => c.Name == "driveItem"); + IEdmNavigationProperty workbook = driveItem.DeclaredNavigationProperties().First(c => c.Name == "workbook"); + IEdmEntityType workbookEntity = model.SchemaElements.OfType().First(c => c.Name == "workbook"); + IEdmNavigationProperty worksheets = workbookEntity.DeclaredNavigationProperties().First(c => c.Name == "worksheets"); + IEdmEntityType workbookWorksheet = model.SchemaElements.OfType().First(c => c.Name == "workbookWorksheet"); + IEdmOperation usedRangeWithParams = model.SchemaElements.OfType().First(f => f.Name == "usedRange" && f.Parameters.Any(x => x.Name.Equals("valuesOnly"))); + IEdmOperation usedRange = model.SchemaElements.OfType().First(f => f.Name == "usedRange" && f.Parameters.Count() == 1); + IEdmEntityType workbookRange = model.SchemaElements.OfType().First(c => c.Name == "workbookRange"); + IEdmNavigationProperty format = workbookRange.DeclaredNavigationProperties().First(c => c.Name == "format"); + + + ODataPath path1 = new(new ODataNavigationSourceSegment(drives), + new ODataKeySegment(drive), + new ODataNavigationPropertySegment(items), + new ODataKeySegment(driveItem), + new ODataNavigationPropertySegment(workbook), + new ODataNavigationPropertySegment(worksheets), + new ODataKeySegment(workbookWorksheet), + new ODataOperationSegment(usedRangeWithParams), + new ODataNavigationPropertySegment(format)); + + ODataPath path2 = new(new ODataNavigationSourceSegment(drives), + new ODataKeySegment(drive), + new ODataNavigationPropertySegment(items), + new ODataKeySegment(driveItem), + new ODataNavigationPropertySegment(workbook), + new ODataNavigationPropertySegment(worksheets), + new ODataKeySegment(workbookWorksheet), + new ODataOperationSegment(usedRange), + new ODataNavigationPropertySegment(format)); + + // Act + var operation1 = _operationHandler.CreateOperation(context, path1); + var operation2 = _operationHandler.CreateOperation(context, path2); + + // Assert + Assert.NotNull(operation1); + Assert.NotNull(operation2); + + Assert.Equal("drives.items.workbook.worksheets.usedRange.GetFormat-206d", operation1.OperationId); + Assert.Equal("drives.items.workbook.worksheets.usedRange.GetFormat-ec2c", operation2.OperationId); + + Assert.NotNull(operation1.Parameters); + Assert.Equal(6, operation1.Parameters.Count); + Assert.Contains(operation1.Parameters, x => x.Name == "valuesOnly"); + + Assert.NotNull(operation2.Parameters); + Assert.Equal(5, operation2.Parameters.Count); + Assert.DoesNotContain(operation2.Parameters, x => x.Name == "valuesOnly"); + } + [Theory] [InlineData(true)] [InlineData(false)]