From 4136c6a1ac893ef78b1c3c847b9c6ed6322859ea Mon Sep 17 00:00:00 2001 From: Irvine Sunday <40403681+irvinesunday@users.noreply.github.com> Date: Wed, 2 Oct 2024 10:35:40 +0300 Subject: [PATCH 1/3] Creates unique operation ids for paths with composable overloaded functions (#590) * Update unit function operation test * Account for overloaded functions in any segment * Update release note Signed-off-by: Vincent Biret --- .../Operation/EdmOperationOperationHandler.cs | 75 ++++++++++--------- .../EdmFunctionOperationHandlerTests.cs | 61 ++++++++++++++- 2 files changed, 99 insertions(+), 37 deletions(-) diff --git a/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs b/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs index 72ebb225..3e4cb274 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs +++ b/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs @@ -45,7 +45,7 @@ internal abstract class EdmOperationOperationHandler : OperationHandler /// protected override void Initialize(ODataContext context, ODataPath path) - { + { base.Initialize(context, path); // It's bound operation, the first segment must be the navigaiton source. @@ -57,9 +57,9 @@ protected override void Initialize(ODataContext context, ODataPath path) HasTypeCast = path.Segments.Any(s => s is ODataTypeCastSegment); - _operationRestriction = Context.Model.GetRecord(TargetPath, CapabilitiesConstants.OperationRestrictions); - var operationRestrictions = Context.Model.GetRecord(EdmOperation, CapabilitiesConstants.OperationRestrictions); - _operationRestriction?.MergePropertiesIfNull(operationRestrictions); + _operationRestriction = Context.Model.GetRecord(TargetPath, CapabilitiesConstants.OperationRestrictions); + var operationRestrictions = Context.Model.GetRecord(EdmOperation, CapabilitiesConstants.OperationRestrictions); + _operationRestriction?.MergePropertiesIfNull(operationRestrictions); _operationRestriction ??= operationRestrictions; } @@ -81,6 +81,7 @@ protected override void SetBasicInfo(OpenApiOperation operation) // duplicates in entity vs entityset functions/actions List identifiers = new(); + string pathHash = string.Empty; foreach (ODataSegment segment in Path.Segments) { if (segment is ODataKeySegment keySegment) @@ -89,41 +90,43 @@ protected override void SetBasicInfo(OpenApiOperation operation) { identifiers.Add(segment.EntityType.Name); continue; - } - - // We'll consider alternate keys in the operation id to eliminate potential duplicates with operation id of primary path - if (segment == Path.Segments.Last()) - { - identifiers.Add("By" + string.Join("", keySegment.Identifier.Split(',').Select(static x => Utils.UpperFirstChar(x)))); - } - else - { - identifiers.Add(keySegment.Identifier); - } + } + + // We'll consider alternate keys in the operation id to eliminate potential duplicates with operation id of primary path + if (segment == Path.Segments.Last()) + { + identifiers.Add("By" + string.Join("", keySegment.Identifier.Split(',').Select(static x => Utils.UpperFirstChar(x)))); + } + else + { + identifiers.Add(keySegment.Identifier); + } + } + else if (segment is ODataOperationSegment opSegment) + { + if (opSegment.Operation is IEdmFunction function && Context.Model.IsOperationOverload(function)) + { + // Hash the segment to avoid duplicate operationIds + pathHash = segment.GetPathHash(Context.Settings); + } + + identifiers.Add(segment.Identifier); } else { - identifiers.Add(segment.Identifier); + identifiers.Add(segment.Identifier); } } string operationId = string.Join(".", identifiers); - if (EdmOperation.IsAction()) + if (!string.IsNullOrEmpty(pathHash)) { - operation.OperationId = operationId; + operation.OperationId = operationId + "-" + pathHash; } else { - if (Path.LastSegment is ODataOperationSegment operationSegment && - Context.Model.IsOperationOverload(operationSegment.Operation)) - { - operation.OperationId = operationId + "-" + Path.LastSegment.GetPathHash(Context.Settings); - } - else - { - operation.OperationId = operationId; - } + operation.OperationId = operationId; } } @@ -269,15 +272,15 @@ protected override void SetExternalDocs(OpenApiOperation operation) if (Context.Settings.ShowExternalDocs) { var externalDocs = Context.Model.GetLinkRecord(TargetPath, CustomLinkRel) ?? - Context.Model.GetLinkRecord(EdmOperation, CustomLinkRel); - - if (externalDocs != null) - { - operation.ExternalDocs = new OpenApiExternalDocs() - { - Description = CoreConstants.ExternalDocsDescription, - Url = externalDocs.Href - }; + Context.Model.GetLinkRecord(EdmOperation, CustomLinkRel); + + if (externalDocs != null) + { + operation.ExternalDocs = new OpenApiExternalDocs() + { + Description = CoreConstants.ExternalDocsDescription, + Url = externalDocs.Href + }; } } } diff --git a/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/EdmFunctionOperationHandlerTests.cs b/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/EdmFunctionOperationHandlerTests.cs index de817a52..7ad2cbf9 100644 --- a/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/EdmFunctionOperationHandlerTests.cs +++ b/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/EdmFunctionOperationHandlerTests.cs @@ -1,4 +1,4 @@ -// ------------------------------------------------------------ +// ------------------------------------------------------------ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. // ------------------------------------------------------------ @@ -299,6 +299,65 @@ public void CreateOperationForOverloadEdmFunctionReturnsCorrectOperationId(bool } } + [Theory] + [InlineData(true)] + [InlineData(false)] + public void CreateOperationForComposableOverloadEdmFunctionReturnsCorrectOperationId(bool enableOperationId) + { + // Arrange + EdmModel model = new(); + EdmEntityType customer = new("NS", "Customer"); + customer.AddKeys(customer.AddStructuralProperty("ID", EdmPrimitiveTypeKind.Int32)); + model.AddElement(customer); + + EdmFunction function = new("NS", "MyFunction", EdmCoreModel.Instance.GetString(false), true, null, false); + function.AddParameter("entity", new EdmEntityTypeReference(customer, false)); + function.AddParameter("param", EdmCoreModel.Instance.GetString(false)); + model.AddElement(function); + + function = new EdmFunction("NS", "MyFunction", EdmCoreModel.Instance.GetString(false), true, null, false); + function.AddParameter("entity", new EdmEntityTypeReference(customer, false)); + function.AddParameter("param", EdmCoreModel.Instance.GetString(false)); + function.AddParameter("param2", EdmCoreModel.Instance.GetString(false)); + model.AddElement(function); + + EdmFunction function2 = new("NS", "MyFunction2", EdmCoreModel.Instance.GetString(false), true, null, false); + function2.AddParameter("entity2", new EdmEntityTypeReference(customer, false)); + function2.AddParameter("param3", EdmCoreModel.Instance.GetString(false)); + model.AddElement(function2); + + EdmEntityContainer container = new("NS", "Default"); + EdmEntitySet customers = new(container, "Customers", customer); + model.AddElement(container); + + OpenApiConvertSettings settings = new OpenApiConvertSettings + { + EnableOperationId = enableOperationId, + AddSingleQuotesForStringParameters = true, + }; + ODataContext context = new(model, settings); + + ODataPath path = new(new ODataNavigationSourceSegment(customers), + new ODataKeySegment(customer), + new ODataOperationSegment(function), + new ODataOperationSegment(function2)); + + // Act + var operation = _operationHandler.CreateOperation(context, path); + + // Assert + Assert.NotNull(operation); + + if (enableOperationId) + { + Assert.Equal("Customers.Customer.MyFunction.MyFunction2-df74", operation.OperationId); + } + else + { + Assert.Null(operation.OperationId); + } + } + [Theory] [InlineData(true)] [InlineData(false)] From c53d3a580bf90d55d85b5c5d61bf56dea6ca2809 Mon Sep 17 00:00:00 2001 From: Irvine Sunday <40403681+irvinesunday@users.noreply.github.com> Date: Fri, 11 Oct 2024 18:17:18 +0300 Subject: [PATCH 2/3] Ensures unique generation of operation ids for paths with composable overloaded functions where all functions in path are overloaded (#595) * Ensure unique operation ids for composable functions with both overloaded * Update test to validate fix * Update release notes Signed-off-by: Vincent Biret --- .../Operation/EdmOperationOperationHandler.cs | 4 +- .../EdmFunctionOperationHandlerTests.cs | 72 ++++++++++++++----- 2 files changed, 56 insertions(+), 20 deletions(-) diff --git a/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs b/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs index 3e4cb274..d944fcf7 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs +++ b/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs @@ -107,7 +107,9 @@ protected override void SetBasicInfo(OpenApiOperation operation) if (opSegment.Operation is IEdmFunction function && Context.Model.IsOperationOverload(function)) { // Hash the segment to avoid duplicate operationIds - pathHash = segment.GetPathHash(Context.Settings); + pathHash = string.IsNullOrEmpty(pathHash) + ? opSegment.GetPathHash(Context.Settings) + : (pathHash + opSegment.GetPathHash(Context.Settings)).GetHashSHA256()[..4]; } identifiers.Add(segment.Identifier); diff --git a/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/EdmFunctionOperationHandlerTests.cs b/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/EdmFunctionOperationHandlerTests.cs index 7ad2cbf9..e899b8e4 100644 --- a/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/EdmFunctionOperationHandlerTests.cs +++ b/test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/EdmFunctionOperationHandlerTests.cs @@ -310,22 +310,29 @@ public void CreateOperationForComposableOverloadEdmFunctionReturnsCorrectOperati customer.AddKeys(customer.AddStructuralProperty("ID", EdmPrimitiveTypeKind.Int32)); model.AddElement(customer); - EdmFunction function = new("NS", "MyFunction", EdmCoreModel.Instance.GetString(false), true, null, false); - function.AddParameter("entity", new EdmEntityTypeReference(customer, false)); - function.AddParameter("param", EdmCoreModel.Instance.GetString(false)); - model.AddElement(function); + // Overloaded function 1 + EdmFunction function1 = new("NS", "MyFunction1", EdmCoreModel.Instance.GetString(false), true, null, false); + function1.AddParameter("entity", new EdmEntityTypeReference(customer, false)); + model.AddElement(function1); - function = new EdmFunction("NS", "MyFunction", EdmCoreModel.Instance.GetString(false), true, null, false); - function.AddParameter("entity", new EdmEntityTypeReference(customer, false)); - function.AddParameter("param", EdmCoreModel.Instance.GetString(false)); - function.AddParameter("param2", EdmCoreModel.Instance.GetString(false)); - model.AddElement(function); + // Overloaded function 1 + EdmFunction function2 = new("NS", "MyFunction1", EdmCoreModel.Instance.GetString(false), true, null, false); + function2.AddParameter("entity", new EdmEntityTypeReference(customer, false)); + function2.AddParameter("param", EdmCoreModel.Instance.GetString(false)); - EdmFunction function2 = new("NS", "MyFunction2", EdmCoreModel.Instance.GetString(false), true, null, false); - function2.AddParameter("entity2", new EdmEntityTypeReference(customer, false)); - function2.AddParameter("param3", EdmCoreModel.Instance.GetString(false)); model.AddElement(function2); + // Overloaded function 2 + EdmFunction function3 = new("NS", "MyFunction2", EdmCoreModel.Instance.GetString(false), true, null, false); + function3.AddParameter("entity2", new EdmEntityTypeReference(customer, false)); + model.AddElement(function3); + + // Overloaded function 2 + EdmFunction function4 = new("NS", "MyFunction2", EdmCoreModel.Instance.GetString(false), true, null, false); + function4.AddParameter("entity2", new EdmEntityTypeReference(customer, false)); + function4.AddParameter("param", EdmCoreModel.Instance.GetString(false)); + model.AddElement(function4); + EdmEntityContainer container = new("NS", "Default"); EdmEntitySet customers = new(container, "Customers", customer); model.AddElement(container); @@ -337,24 +344,51 @@ public void CreateOperationForComposableOverloadEdmFunctionReturnsCorrectOperati }; ODataContext context = new(model, settings); - ODataPath path = new(new ODataNavigationSourceSegment(customers), + ODataPath path1 = new(new ODataNavigationSourceSegment(customers), new ODataKeySegment(customer), - new ODataOperationSegment(function), - new ODataOperationSegment(function2)); + new ODataOperationSegment(function1), + new ODataOperationSegment(function3)); + + ODataPath path2 = new(new ODataNavigationSourceSegment(customers), + new ODataKeySegment(customer), + new ODataOperationSegment(function1), + new ODataOperationSegment(function4)); + + ODataPath path3 = new(new ODataNavigationSourceSegment(customers), + new ODataKeySegment(customer), + new ODataOperationSegment(function2), + new ODataOperationSegment(function3)); + + ODataPath path4 = new(new ODataNavigationSourceSegment(customers), + new ODataKeySegment(customer), + new ODataOperationSegment(function2), + new ODataOperationSegment(function4)); // Act - var operation = _operationHandler.CreateOperation(context, path); + var operation1 = _operationHandler.CreateOperation(context, path1); + var operation2 = _operationHandler.CreateOperation(context, path2); + var operation3 = _operationHandler.CreateOperation(context, path3); + var operation4 = _operationHandler.CreateOperation(context, path4); // Assert - Assert.NotNull(operation); + Assert.NotNull(operation1); + Assert.NotNull(operation2); + Assert.NotNull(operation3); + Assert.NotNull(operation4); if (enableOperationId) { - Assert.Equal("Customers.Customer.MyFunction.MyFunction2-df74", operation.OperationId); + Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-c53d", operation1.OperationId); + Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-4d93", operation2.OperationId); + Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-a2b2", operation3.OperationId); + Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-7bea", operation4.OperationId); } else { - Assert.Null(operation.OperationId); + Assert.Null(operation1.OperationId); + Assert.Null(operation2.OperationId); + Assert.Null(operation3.OperationId); + Assert.Null(operation4.OperationId); } } From 4c8eafa292f687d054ee4df3676576bdcde82ffd Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Tue, 24 Dec 2024 13:06:45 -0500 Subject: [PATCH 3/3] chore: fix merge issue --- .../Operation/EdmOperationOperationHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs b/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs index d944fcf7..f1d51875 100644 --- a/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs +++ b/src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs @@ -109,7 +109,7 @@ protected override void SetBasicInfo(OpenApiOperation operation) // Hash the segment to avoid duplicate operationIds pathHash = string.IsNullOrEmpty(pathHash) ? opSegment.GetPathHash(Context.Settings) - : (pathHash + opSegment.GetPathHash(Context.Settings)).GetHashSHA256()[..4]; + : (pathHash + opSegment.GetPathHash(Context.Settings)).GetHashSHA256().Substring(0, 4); } identifiers.Add(segment.Identifier);