From f78a4e2d55fb4d9d61c42418a10fe5660de10af7 Mon Sep 17 00:00:00 2001 From: Sam Xu Date: Fri, 22 Apr 2022 11:56:41 -0700 Subject: [PATCH 1/4] issue #568: Error when function/action name contains keyword "On" --- .../Conventions/OperationRoutingConvention.cs | 57 +++++++++++------ .../FunctionRoutingConventionTests.cs | 62 ++++++++++++++++++- 2 files changed, 97 insertions(+), 22 deletions(-) diff --git a/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs b/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs index 4fe40cfb1..41cfb4caf 100644 --- a/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs +++ b/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs @@ -65,31 +65,39 @@ protected void ProcessOperations(ODataControllerActionContext context, IEdmEntit return; } - // OperationNameOnCollectionOfEntityType - string operationName = SplitActionName(actionName, out string cast, out bool isOnCollection); - + bool isOnCollection = false; IEdmEntityType castTypeFromActionName = null; - if (cast != null) + + // Let's find the operations using the action first, + // If not founding, let's split the action name, search again + IEdmOperation[] candidates = FindCandidates(context, actionName); + + if (candidates.Length == 0) { - if (cast.Length == 0) - { - // Early return for the following cases: - // - {OperationName}On - // - {OperationName}OnCollectionOf - return; - } + // OperationNameOnCollectionOfEntityType + string operationName = SplitActionName(actionName, out string cast, out isOnCollection); - castTypeFromActionName = entityType.FindTypeInInheritance(context.Model, cast, context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true) as IEdmEntityType; - if (castTypeFromActionName == null) + castTypeFromActionName = null; + if (cast != null) { - return; + if (cast.Length == 0) + { + // Early return for the following cases: + // - {OperationName}On + // - {OperationName}OnCollectionOf + return; + } + + castTypeFromActionName = entityType.FindTypeInInheritance(context.Model, cast, context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true) as IEdmEntityType; + if (castTypeFromActionName == null) + { + return; + } } + + candidates = FindCandidates(context, operationName); } - // TODO: refactor here - // If we have multiple same function defined, we should match the best one? - StringComparison actionNameComparison = context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; - IEnumerable candidates = context.Model.SchemaElements.OfType().Where(f => f.IsBound && f.Name.Equals(operationName, actionNameComparison)); foreach (IEdmOperation edmOperation in candidates) { IEdmOperationParameter bindingParameter = edmOperation.Parameters.FirstOrDefault(); @@ -172,6 +180,15 @@ protected void ProcessOperations(ODataControllerActionContext context, IEdmEntit } } + private static IEdmOperation[] FindCandidates(ODataControllerActionContext context, string operationName) + { + // TODO: refactor here + // If we have multiple same function defined, we should match the best one? + + StringComparison actionNameComparison = context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; + return context.Model.SchemaElements.OfType().Where(f => f.IsBound && f.Name.Equals(operationName, actionNameComparison)).ToArray(); + } + /// /// Split the action based on supporting pattern. /// @@ -190,7 +207,7 @@ internal static string SplitActionName(string actionName, out string cast, out b cast = null; isOnCollection = false; string operation; - int index = actionName.IndexOf("OnCollectionOf", StringComparison.Ordinal); + int index = actionName.LastIndexOf("OnCollectionOf", StringComparison.Ordinal); if (index > 0) { operation = actionName.Substring(0, index); @@ -199,7 +216,7 @@ internal static string SplitActionName(string actionName, out string cast, out b return operation; } - index = actionName.IndexOf("On", StringComparison.Ordinal); + index = actionName.LastIndexOf("On", StringComparison.Ordinal); if (index > 0) { operation = actionName.Substring(0, index); diff --git a/test/Microsoft.AspNetCore.OData.Tests/Routing/Conventions/FunctionRoutingConventionTests.cs b/test/Microsoft.AspNetCore.OData.Tests/Routing/Conventions/FunctionRoutingConventionTests.cs index 34d95d5a9..618d11fad 100644 --- a/test/Microsoft.AspNetCore.OData.Tests/Routing/Conventions/FunctionRoutingConventionTests.cs +++ b/test/Microsoft.AspNetCore.OData.Tests/Routing/Conventions/FunctionRoutingConventionTests.cs @@ -154,11 +154,47 @@ public static TheoryDataSet FunctionRoutingConventionTes "/Customers/NS.GetWholeSalary(minSalary={minSalary},maxSalary={maxSalary},aveSalary={aveSalary})", "/Customers/GetWholeSalary(minSalary={minSalary},maxSalary={maxSalary},aveSalary={aveSalary})" } + }, + { + typeof(CustomersController), + "GetStatusOnLineOfflineUser", + new[] + { + "/Customers/NS.GetStatusOnLineOfflineUser()", + "/Customers/GetStatusOnLineOfflineUser()" + } + }, + { + typeof(CustomersController), + "GetStatusOnLineOfflineUser", + new[] + { + "/Customers/NS.GetStatusOnLineOfflineUser()", + "/Customers/GetStatusOnLineOfflineUser()" + } + }, + { + typeof(CustomersController), + "StatusLineOfflineUserOn", + new[] + { + "/Customers/NS.StatusLineOfflineUserOn()", + "/Customers/StatusLineOfflineUserOn()" + } + }, + { + typeof(CustomersController), + "GetStatusOnLineOfflineUserOnVipCustomer", + new[] + { + "/Customers/NS.VipCustomer/NS.GetStatusOnLineOfflineUser(param={param})", + "/Customers/NS.VipCustomer/GetStatusOnLineOfflineUser(param={param})" + } } }; } - } - + } + public static TheoryDataSet FunctionRoutingConventionCaseInsensitiveTestData { get @@ -448,6 +484,19 @@ private static IEdmModel GetEdmModel() getSalaray.AddOptionalParameter("aveSalary", intType, "129"); model.AddElement(getSalaray); + EdmFunction f = new EdmFunction("NS", "GetStatusOnLineOfflineUser", intType, isBound: true, entitySetPathExpression: null, isComposable: false); + f.AddParameter("entityset", new EdmCollectionTypeReference(new EdmCollectionType(new EdmEntityTypeReference(customer, false)))); + model.AddElement(f); + + EdmFunction f2 = new EdmFunction("NS", "StatusLineOfflineUserOn", intType, isBound: true, entitySetPathExpression: null, isComposable: false); + f2.AddParameter("entityset", new EdmCollectionTypeReference(new EdmCollectionType(new EdmEntityTypeReference(customer, false)))); + model.AddElement(f2); + + EdmFunction f3 = new EdmFunction("NS", "GetStatusOnLineOfflineUser", intType, isBound: true, entitySetPathExpression: null, isComposable: false); + f3.AddParameter("entityset", new EdmCollectionTypeReference(new EdmCollectionType(new EdmEntityTypeReference(vipCustomer, false)))); + f3.AddParameter("param", intType); + model.AddElement(f3); + EdmEntityContainer container = new EdmEntityContainer("NS", "Default"); container.AddEntitySet("Customers", customer); container.AddEntitySet("CustomersCaseInsensitive", customer); @@ -461,6 +510,15 @@ private class CustomersController public void Get() { } + [HttpGet] + public void GetStatusOnLineOfflineUser() { } + + [HttpGet] + public void GetStatusOnLineOfflineUserOnVipCustomer(int param) { } + + [HttpGet] + public void StatusLineOfflineUserOn() { } + [HttpGet] public void IsBaseUpgraded(int key, CancellationToken cancellation) { } From 2dd6f262dffdef79162c60f15cd6ea0c0f06220e Mon Sep 17 00:00:00 2001 From: Sam Xu Date: Mon, 25 Apr 2022 14:06:53 -0700 Subject: [PATCH 2/4] Add the comments --- .../Microsoft.AspNetCore.OData.xml | 30 ++++---- .../Conventions/OperationRoutingConvention.cs | 68 ++++++++++++------- 2 files changed, 58 insertions(+), 40 deletions(-) diff --git a/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml b/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml index b5aa272f8..b34e9348e 100644 --- a/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml +++ b/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml @@ -1094,20 +1094,6 @@ The type to test. True if the type is a DateTime; false otherwise. - - - Determine if a type is a . - - The type to test. - True if the type is a DateOnly; false otherwise. - - - - Determine if a type is a . - - The type to test. - True if the type is a TimeOnly; false otherwise. - Determine if a type is a TimeSpan. @@ -14118,3 +14104,19 @@ +ummary> + The value segment. + + + + Gets the value segment. + + + + + + + + + + diff --git a/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs b/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs index 41cfb4caf..23e874410 100644 --- a/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs +++ b/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs @@ -68,34 +68,17 @@ protected void ProcessOperations(ODataControllerActionContext context, IEdmEntit bool isOnCollection = false; IEdmEntityType castTypeFromActionName = null; - // Let's find the operations using the action first, - // If not founding, let's split the action name, search again IEdmOperation[] candidates = FindCandidates(context, actionName); - if (candidates.Length == 0) { - // OperationNameOnCollectionOfEntityType - string operationName = SplitActionName(actionName, out string cast, out isOnCollection); - - castTypeFromActionName = null; - if (cast != null) - { - if (cast.Length == 0) - { - // Early return for the following cases: - // - {OperationName}On - // - {OperationName}OnCollectionOf - return; - } - - castTypeFromActionName = entityType.FindTypeInInheritance(context.Model, cast, context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true) as IEdmEntityType; - if (castTypeFromActionName == null) - { - return; - } - } + // If we can't find any Edm operation using the action name directly, + // Let's split the action name and use part of it to search again. + candidates = FindCandidates(context, entityType, actionName, out castTypeFromActionName, out isOnCollection); + } - candidates = FindCandidates(context, operationName); + if (candidates == null || candidates.Length == 0) + { + return; } foreach (IEdmOperation edmOperation in candidates) @@ -185,8 +168,41 @@ private static IEdmOperation[] FindCandidates(ODataControllerActionContext conte // TODO: refactor here // If we have multiple same function defined, we should match the best one? - StringComparison actionNameComparison = context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; - return context.Model.SchemaElements.OfType().Where(f => f.IsBound && f.Name.Equals(operationName, actionNameComparison)).ToArray(); + StringComparison actionNameComparison = context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true ? + StringComparison.OrdinalIgnoreCase : + StringComparison.Ordinal; + + return context.Model.SchemaElements + .OfType() + .Where(f => f.IsBound && f.Name.Equals(operationName, actionNameComparison)) + .ToArray(); + } + + private static IEdmOperation[] FindCandidates(ODataControllerActionContext context, IEdmEntityType entityType, string actionName, + out IEdmEntityType castTypeFromActionName, out bool isOnCollection) + { + // OperationNameOnCollectionOfEntityType + string operationName = SplitActionName(actionName, out string cast, out isOnCollection); + + castTypeFromActionName = null; + if (cast != null) + { + if (cast.Length == 0) + { + // Early return for the following cases: + // - {OperationName}On + // - {OperationName}OnCollectionOf + return null; + } + + castTypeFromActionName = entityType.FindTypeInInheritance(context.Model, cast, context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true) as IEdmEntityType; + if (castTypeFromActionName == null) + { + return null; + } + } + + return FindCandidates(context, operationName); } /// From a7e8adae2872acdf5c1b9bfbf34ef08f91d93cbc Mon Sep 17 00:00:00 2001 From: Sam Xu Date: Mon, 25 Apr 2022 17:59:55 -0700 Subject: [PATCH 3/4] Address the comments. --- .../Routing/Conventions/OperationRoutingConvention.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs b/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs index 23e874410..47b0bbecc 100644 --- a/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs +++ b/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs @@ -76,11 +76,6 @@ protected void ProcessOperations(ODataControllerActionContext context, IEdmEntit candidates = FindCandidates(context, entityType, actionName, out castTypeFromActionName, out isOnCollection); } - if (candidates == null || candidates.Length == 0) - { - return; - } - foreach (IEdmOperation edmOperation in candidates) { IEdmOperationParameter bindingParameter = edmOperation.Parameters.FirstOrDefault(); @@ -192,13 +187,13 @@ private static IEdmOperation[] FindCandidates(ODataControllerActionContext conte // Early return for the following cases: // - {OperationName}On // - {OperationName}OnCollectionOf - return null; + return Array.Empty(); } castTypeFromActionName = entityType.FindTypeInInheritance(context.Model, cast, context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true) as IEdmEntityType; if (castTypeFromActionName == null) { - return null; + return Array.Empty(); } } From 780340421f4e97bb6ea0b63772113a12e95fb40c Mon Sep 17 00:00:00 2001 From: Sam Xu Date: Tue, 26 Apr 2022 17:09:00 -0700 Subject: [PATCH 4/4] Address the comments. --- .../Microsoft.AspNetCore.OData.xml | 33 +++++++++---------- .../Conventions/OperationRoutingConvention.cs | 14 +++++--- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml b/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml index b34e9348e..b6c01f6a4 100644 --- a/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml +++ b/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml @@ -1094,6 +1094,20 @@ The type to test. True if the type is a DateTime; false otherwise. + + + Determine if a type is a . + + The type to test. + True if the type is a DateOnly; false otherwise. + + + + Determine if a type is a . + + The type to test. + True if the type is a TimeOnly; false otherwise. + Determine if a type is a TimeSpan. @@ -12413,13 +12427,14 @@ The Edm entity type. The Edm navigation source. - + Split the action based on supporting pattern. The input action name. The out of cast type name. The out of collection binding flag. + The case comparision flag. The operation name. @@ -14104,19 +14119,3 @@ -ummary> - The value segment. - - - - Gets the value segment. - - - - - - - - - - diff --git a/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs b/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs index 47b0bbecc..a31a81851 100644 --- a/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs +++ b/src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs @@ -177,7 +177,11 @@ private static IEdmOperation[] FindCandidates(ODataControllerActionContext conte out IEdmEntityType castTypeFromActionName, out bool isOnCollection) { // OperationNameOnCollectionOfEntityType - string operationName = SplitActionName(actionName, out string cast, out isOnCollection); + StringComparison caseComparision = context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true ? + StringComparison.OrdinalIgnoreCase : + StringComparison.Ordinal; + + string operationName = SplitActionName(actionName, out string cast, out isOnCollection, caseComparision); castTypeFromActionName = null; if (cast != null) @@ -206,8 +210,10 @@ private static IEdmOperation[] FindCandidates(ODataControllerActionContext conte /// The input action name. /// The out of cast type name. /// The out of collection binding flag. + /// The case comparision flag. /// The operation name. - internal static string SplitActionName(string actionName, out string cast, out bool isOnCollection) + internal static string SplitActionName(string actionName, out string cast, out bool isOnCollection, + StringComparison comparison = StringComparison.Ordinal) { Contract.Assert(actionName != null); @@ -218,7 +224,7 @@ internal static string SplitActionName(string actionName, out string cast, out b cast = null; isOnCollection = false; string operation; - int index = actionName.LastIndexOf("OnCollectionOf", StringComparison.Ordinal); + int index = actionName.LastIndexOf("OnCollectionOf", comparison); if (index > 0) { operation = actionName.Substring(0, index); @@ -227,7 +233,7 @@ internal static string SplitActionName(string actionName, out string cast, out b return operation; } - index = actionName.LastIndexOf("On", StringComparison.Ordinal); + index = actionName.LastIndexOf("On", comparison); if (index > 0) { operation = actionName.Substring(0, index);