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 <[email protected]>

Author: irvinesunday

src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs

@@ -45,7 +45,7 @@ internal abstract class EdmOperationOperationHandler : OperationHandler
 
         /// <inheritdoc/>
         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<OperationRestrictionsType>(TargetPath, CapabilitiesConstants.OperationRestrictions);
-            var operationRestrictions = Context.Model.GetRecord<OperationRestrictionsType>(EdmOperation, CapabilitiesConstants.OperationRestrictions);
-            _operationRestriction?.MergePropertiesIfNull(operationRestrictions);
+            _operationRestriction = Context.Model.GetRecord<OperationRestrictionsType>(TargetPath, CapabilitiesConstants.OperationRestrictions);
+            var operationRestrictions = Context.Model.GetRecord<OperationRestrictionsType>(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<string> 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
+                    };
                 }
             }
         }

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)]