From 497ebd6d8d1cf59eccf580730ee86a25be88c417 Mon Sep 17 00:00:00 2001 From: KanishManuja-MS <41647051+KanishManuja-MS@users.noreply.github.com> Date: Sat, 30 Nov 2019 21:43:41 -0800 Subject: [PATCH 1/9] Performance improvements --- .../Metadata/EdmLibraryExtensions.cs | 216 ++++++++++++++---- .../Parsers/FunctionOverloadResolver.cs | 68 ++---- .../UriParser/Resolver/ODataUriResolver.cs | 32 ++- .../ExtensionMethods/ExtensionMethods.cs | 13 +- 4 files changed, 217 insertions(+), 112 deletions(-) diff --git a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs index 1073f91c72..d2d3dc2760 100644 --- a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs +++ b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs @@ -27,6 +27,8 @@ namespace Microsoft.OData.Metadata using Microsoft.OData.JsonLight; using ErrorStrings = Microsoft.OData.Strings; using PlatformHelper = Microsoft.OData.PlatformHelper; + using System.Collections; + using Microsoft.OData.UriParser; #endif #endregion Namespaces @@ -265,6 +267,111 @@ internal static IEnumerable FindBestOverloadBasedOnParameters(thi return exactMatches.Count() > 0 ? exactMatches : functions; } + /// + /// Filters the operations by parameter names. + /// + /// The operations. + /// The list of non-binding parameter names to match. + /// Whether case insensitive. + /// The best matching operations based on parameters. + internal static IList FindBestOverloadBasedOnParameters(this IList functions, IList parameters, bool caseInsensitive = false) + { + IList exactMatches = new List(); + + // The best match out of a list of candidates is the one that has the same number of (non-binding) parameters as specified. + for(int i=0; i < functions.Count; i++) + { + if (functions[i].Parameters.Count() == parameters.Count + (functions[i].IsBound ? 1 : 0)) + { + exactMatches.Add(functions[i]); + } + } + + return exactMatches.Count > 0 ? exactMatches : functions; + } + + /// + /// Filters the operations by parameter names. + /// + /// The operations. + /// The parameters. + /// Whether case insensitive. + /// Operations filtered by parameter. + internal static IList FindBestCandidate(this IEnumerable operations, IEdmType bindingType, IList parameterNameList, bool caseInsensitive, out IList actionItems) + { + Debug.Assert(operations != null, "operations"); + Debug.Assert(parameterNameList != null, "parameters"); + + bool hasParameters = parameterNameList.Count > 0; + actionItems = new List(); + IList operationCandidates = new List(); + + foreach (var operation in operations) + { + bool suitableCandidate = true; + if (bindingType != null) + { + operation.EnsureOperationBoundWithBindingParameter(); + } + + // If the number of parameters > 0 then this has to be a function as actions can't have parameters on the uri only in the payload. Filter further by parameters in this case, otherwise don't. + if (hasParameters) + { + if (operation.IsAction()) + { + actionItems.Add(operation); + suitableCandidate = false; + } + + if (!ParametersSatisfyFunction(operation, parameterNameList, caseInsensitive)) + { + suitableCandidate = false; + } + } + else if (bindingType != null) + { + // If it is a bound function without required parameters, then the parameter list should only have one parameter and any others as optional. + if (operation.IsFunction()) + { + int count = 0; + foreach (var param in operation.Parameters) + { + if (count > 0 && !(param is IEdmOptionalParameter)) + { + suitableCandidate = false; + } + + count++; + } + + if (count == 0) + { + suitableCandidate = false; + } + } + else if (!operation.IsAction()) + { + // If IsFunction and !IsAction are the same then this may not be required. To be confirmed. + suitableCandidate = false; + } + } + else + { + if (operation.IsFunction() && operation.Parameters.HasAny()) + { + suitableCandidate = false; + } + } + + if (suitableCandidate) + { + operationCandidates.Add(operation); + } + } + + return operationCandidates; + } + /// /// Filters the operations by parameter names. /// @@ -292,22 +399,38 @@ internal static IEnumerable FilterOperationsByParameterNames(this } /// - /// Ensures that operations are bound and have a binding parameter, other wise throws an exception. + /// Filters the operations by parameter names. /// /// The operations. - internal static void EnsureOperationsBoundWithBindingParameter(this IEnumerable operations) + /// The parameters. + /// Whether case insensitive. + /// Operations filtered by parameter. + internal static IEnumerable FilterOperationsByParameterNames(this IList operations, IList parameterNameList, bool caseInsensitive) { - foreach (IEdmOperation operation in operations) + Debug.Assert(operations != null, "operations"); + Debug.Assert(parameterNameList != null, "parameters"); + + // TODO: update code that is duplicate between operation and operation import, add more tests. + for (int i=0; i < operations.Count; i++) { - if (!operation.IsBound) + if (!ParametersSatisfyFunction(operations[i], parameterNameList, caseInsensitive)) { - throw new ODataException(ErrorStrings.EdmLibraryExtensions_UnBoundOperationsFoundFromIEdmModelFindMethodIsInvalid(operation.Name)); + continue; } - if (operation.Parameters.FirstOrDefault() == null) - { - throw new ODataException(ErrorStrings.EdmLibraryExtensions_NoParameterBoundOperationsFoundFromIEdmModelFindMethodIsInvalid(operation.Name)); - } + yield return operations[i]; + } + } + + /// + /// Ensures that operations are bound and have a binding parameter, other wise throws an exception. + /// + /// The operations. + internal static void EnsureOperationsBoundWithBindingParameter(this IEnumerable operations) + { + foreach (var operation in operations) + { + operation.EnsureOperationBoundWithBindingParameter(); } } @@ -429,32 +552,6 @@ internal static string FullNameWithParameters(this IEdmOperationImport operation return operationImport.FullName() + operationImport.ParameterTypesToString(); } - /// - /// Removes the actions. - /// - /// The source. - /// The action items. - /// Only the functions from the operation sequence. - internal static IEnumerable RemoveActions(this IEnumerable source, out IList actionItems) - { - List functions = new List(); - - actionItems = new List(); - foreach (var item in source) - { - if (item.IsAction()) - { - actionItems.Add(item); - } - else - { - functions.Add(item); - } - } - - return functions; - } - /// /// Removes the action imports. /// @@ -1829,20 +1926,38 @@ private static bool ParametersSatisfyFunction(IEdmOperation operation, IList functionParameters = parametersToMatch.ToList(); // if any required parameters are missing, don't consider it a match. - if (functionParameters.Where( - p => !(p is IEdmOptionalParameter)).Any( - p => parameterNameList.All( - k => !string.Equals(k, p.Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)))) + BitArray bitmask = new BitArray(parameterNameList.Count); + for (int i=0; i < functionParameters.Count; i++) { - return false; + if (functionParameters[i] is IEdmOptionalParameter) + { + continue; + } + + bool matched = false; + for (int j=0; j < parameterNameList.Count; j++) + { + if (string.Equals(parameterNameList[j], functionParameters[i].Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)) + { + matched = true; + bitmask[j] = true; + break; + } + } + + if (!matched) + { + return false; + } } // if any specified parameters don't match, don't consider it a match. - if (parameterNameList.Any( - k => functionParameters.All( - p => !string.Equals(k, p.Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)))) + for (int i=0; i < parameterNameList.Count; i++) { - return false; + if (!bitmask[i]) + { + return false; + } } return true; @@ -1868,6 +1983,19 @@ private static int InheritanceLevelFromSpecifiedInheritedType(this IEdmStructure return inheritanceLevelsFromBase; } + + private static void EnsureOperationBoundWithBindingParameter(this IEdmOperation operation) + { + if (!operation.IsBound) + { + throw new ODataException(ErrorStrings.EdmLibraryExtensions_UnBoundOperationsFoundFromIEdmModelFindMethodIsInvalid(operation.Name)); + } + + if (operation.Parameters.FirstOrDefault() == null) + { + throw new ODataException(ErrorStrings.EdmLibraryExtensions_NoParameterBoundOperationsFoundFromIEdmModelFindMethodIsInvalid(operation.Name)); + } + } #endif #endregion diff --git a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs index 5b4737abd9..fa0085fa0b 100644 --- a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs @@ -124,7 +124,7 @@ internal static bool ResolveOperationImportFromList(string identifier, IListThe single matching function found. /// Resolver to be used. /// True if a function was matched, false otherwise. Will throw if the model has illegal operation imports. - internal static bool ResolveOperationFromList(string identifier, IEnumerable parameterNames, IEdmType bindingType, IEdmModel model, out IEdmOperation matchingOperation, ODataUriResolver resolver) + internal static bool ResolveOperationFromList(string identifier, IList parameterNames, IEdmType bindingType, IEdmModel model, out IEdmOperation matchingOperation, ODataUriResolver resolver) { // TODO: update code that is duplicate between operation and operation import, add more tests. // If the previous segment is an open type, the service action name is required to be fully qualified or else we always treat it as an open property name. @@ -139,19 +139,20 @@ internal static bool ResolveOperationFromList(string identifier, IEnumerable candidateMatchingOperations = null; + IEnumerable operationsFromModel = null; // The extension method FindBoundOperations & FindOperations call IEdmModel.FindDeclaredBoundOperations which can be implemented by anyone and it could throw any type of exception // so catching all of them and simply putting it in the inner exception. + // It is better to cast the results of ResolveBoundOperations as IList instead of looping over this for basic things such as count. try { if (bindingType != null) { - candidateMatchingOperations = resolver.ResolveBoundOperations(model, identifier, bindingType); + operationsFromModel = resolver.ResolveBoundOperations(model, identifier, bindingType); } else { - candidateMatchingOperations = resolver.ResolveUnboundOperations(model, identifier); + operationsFromModel = resolver.ResolveUnboundOperations(model, identifier); } } catch (Exception exc) @@ -165,44 +166,22 @@ internal static bool ResolveOperationFromList(string identifier, IEnumerable foundActionsWhenLookingForFunctions = new List(); - bool hasParameters = parameterNames.Count() > 0; + bool hasParameters = parameterNames.Count > 0; - if (bindingType != null) - { - candidateMatchingOperations.EnsureOperationsBoundWithBindingParameter(); - } - - // If the number of parameters > 0 then this has to be a function as actions can't have parameters on the uri only in the payload. Filter further by parameters in this case, otherwise don't. - if (hasParameters) - { - // can only be a function as only functions have parameters on the uri. - candidateMatchingOperations = candidateMatchingOperations.RemoveActions(out foundActionsWhenLookingForFunctions) - .FilterOperationsByParameterNames(parameterNames, resolver.EnableCaseInsensitive); - } - else if (bindingType != null) - { - // Filter out functions with more than one parameter. Actions should not be filtered as the parameters are in the payload not the uri - candidateMatchingOperations = candidateMatchingOperations.Where(o => - (o.IsFunction() && (o.Parameters.Count() == 1 || o.Parameters.Skip(1).All(p => p is IEdmOptionalParameter))) || o.IsAction()); - } - else - { - // Filter out functions with any parameters - candidateMatchingOperations = candidateMatchingOperations.Where(o => (o.IsFunction() && !o.Parameters.Any()) || o.IsAction()); - } + IList candidatesMatchingOperations = operationsFromModel.FindBestCandidate(bindingType, parameterNames, resolver.EnableCaseInsensitive, out foundActionsWhenLookingForFunctions); // Only filter if there is more than one and its needed. - if (candidateMatchingOperations.Count() > 1) + if (candidatesMatchingOperations.Count > 1) { - candidateMatchingOperations = candidateMatchingOperations.FilterBoundOperationsWithSameTypeHierarchyToTypeClosestToBindingType(bindingType); + candidatesMatchingOperations = candidatesMatchingOperations.FilterBoundOperationsWithSameTypeHierarchyToTypeClosestToBindingType(bindingType) as IList; } // If any of the things returned are an action, it better be the only thing returned, and there can't be parameters in the URL - if (candidateMatchingOperations.Any(f => f.IsAction())) + if (candidatesMatchingOperations.Any(f => f.IsAction())) { - if (candidateMatchingOperations.Count() > 1) + if (candidatesMatchingOperations.Count > 1) { - if (candidateMatchingOperations.Any(o => o.IsFunction())) + if (candidatesMatchingOperations.Any(o => o.IsFunction())) { throw new ODataException(ODataErrorStrings.FunctionOverloadResolver_MultipleOperationOverloads(identifier)); } @@ -217,7 +196,7 @@ internal static bool ResolveOperationFromList(string identifier, IEnumerable 0 ? candidatesMatchingOperations[0] : null; return true; } @@ -227,35 +206,28 @@ internal static bool ResolveOperationFromList(string identifier, IEnumerable 1) + if (candidatesMatchingOperations.Count > 1) { - candidateMatchingOperations = candidateMatchingOperations.FindBestOverloadBasedOnParameters(parameterNames); + candidatesMatchingOperations = candidatesMatchingOperations.FindBestOverloadBasedOnParameters(parameterNames); } - if (candidateMatchingOperations.Count() > 1) + if (candidatesMatchingOperations.Count > 1) { throw new ODataException(ODataErrorStrings.FunctionOverloadResolver_NoSingleMatchFound(identifier, string.Join(",", parameterNames.ToArray()))); } - matchingOperation = candidateMatchingOperations.SingleOrDefault(); + matchingOperation = candidatesMatchingOperations.Count > 0 ? candidatesMatchingOperations[0] : null; return matchingOperation != null; } internal static bool HasAny(this IEnumerable enumerable) where T : class { - IList list = enumerable as IList; - if (list != null) + if (enumerable != null && enumerable.GetEnumerator().MoveNext()) { - return list.Count > 0; - } - - T[] array = enumerable as T[]; - if (array != null) - { - return array.Length > 0; + return true; } - return enumerable.FirstOrDefault() != null; + return false; } } } \ No newline at end of file diff --git a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs index 32d9e2aa9a..5704a75040 100644 --- a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs @@ -5,6 +5,7 @@ //--------------------------------------------------------------------- using System; +using System.Collections; using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Linq; @@ -198,10 +199,10 @@ public virtual IEnumerable ResolveBoundOperations(IEdmModel model } IList operations = FindAcrossModels(model, identifier, /*caseInsensitive*/ true); - if (operations != null && operations.Count() > 0) + if (operations != null && operations.Count > 0) { IList matchedOperation = new List(); - for (int i = 0; i < operations.Count(); i++) + for (int i = 0; i < operations.Count; i++) { if (operations[i].HasEquivalentBindingType(bindingType)) { @@ -230,10 +231,10 @@ public virtual IEnumerable ResolveUnboundOperations(IEdmModel mod } IList operations = FindAcrossModels(model, identifier, /*caseInsensitive*/ true); - if (operations != null && operations.Count() > 0) + if (operations != null && operations.Count > 0) { IList matchedOperation = new List(); - for (int i = 0; i < operations.Count(); i++) + for (int i = 0; i < operations.Count; i++) { if (!operations[i].IsBound) { @@ -438,22 +439,33 @@ internal static ODataUriResolver GetUriResolver(IServiceProvider container) return container.GetRequiredService(); } - private static List FindAcrossModels(IEdmModel model, String qualifiedName, bool caseInsensitive) where T : IEdmSchemaElement + private static IList FindAcrossModels(IEdmModel model, String qualifiedName, bool caseInsensitive) where T : IEdmSchemaElement { - List results = FindSchemaElements(model, qualifiedName, caseInsensitive).ToList(); + IList results = new List(); + FindSchemaElements(model, qualifiedName, caseInsensitive, ref results); foreach (IEdmModel reference in model.ReferencedModels) { - results.AddRange(FindSchemaElements(reference, qualifiedName, caseInsensitive)); + FindSchemaElements(reference, qualifiedName, caseInsensitive, ref results); } return results; } - private static IEnumerable FindSchemaElements(IEdmModel model, string qualifiedName, bool caseInsensitive) where T : IEdmSchemaElement + private static IList FindSchemaElements(IEdmModel model, string qualifiedName, bool caseInsensitive, ref IList results) where T : IEdmSchemaElement { - return model.SchemaElements.OfType() - .Where(e => string.Equals(qualifiedName, e.FullName(), caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)); + foreach(var schema in model.SchemaElements) + { + if (string.Equals(qualifiedName, schema.FullName(), caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)) + { + if (schema is T item) + { + results.Add(item); + } + } + } + + return results; } } } diff --git a/src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs b/src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs index cb3f768e24..f5d50973d7 100644 --- a/src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs +++ b/src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs @@ -3182,19 +3182,12 @@ internal static bool TryGetStaticEntitySet(this IEdmPathExpression pathExpressio internal static bool HasAny(this IEnumerable enumerable) where T : class { - IList list = enumerable as IList; - if (list != null) + if (enumerable != null && enumerable.GetEnumerator().MoveNext()) { - return list.Count > 0; - } - - T[] array = enumerable as T[]; - if (array != null) - { - return array.Length > 0; + return true; } - return enumerable.FirstOrDefault() != null; + return false; } /// From 79895adde0c715d721b7af6ed8ff8dd2f3d4e297 Mon Sep 17 00:00:00 2001 From: KanishManuja-MS <41647051+KanishManuja-MS@users.noreply.github.com> Date: Sun, 1 Dec 2019 02:01:03 -0800 Subject: [PATCH 2/9] fix issues with implementation --- src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs | 8 +------- .../UriParser/Resolver/ODataUriResolver.cs | 6 +++--- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs index d2d3dc2760..7614e5fc49 100644 --- a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs +++ b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs @@ -1929,11 +1929,6 @@ private static bool ParametersSatisfyFunction(IEdmOperation operation, IList FindAcrossModels(IEdmModel model, String qualifiedNam private static IList FindSchemaElements(IEdmModel model, string qualifiedName, bool caseInsensitive, ref IList results) where T : IEdmSchemaElement { - foreach(var schema in model.SchemaElements) + foreach (var schema in model.SchemaElements) { if (string.Equals(qualifiedName, schema.FullName(), caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)) { - if (schema is T item) + if (schema is T) { - results.Add(item); + results.Add((T)schema); } } } From 5adb8e8309ae55795b60ab4a6e2c34ad58d5d304 Mon Sep 17 00:00:00 2001 From: KanishManuja-MS <41647051+KanishManuja-MS@users.noreply.github.com> Date: Mon, 2 Dec 2019 09:39:30 -0800 Subject: [PATCH 3/9] Microoptimize a little further in the Hot Path --- .../Parsers/FunctionOverloadResolver.cs | 19 +++++++++++++++++-- .../UriParser/Resolver/ODataUriResolver.cs | 4 ++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs index fa0085fa0b..933bd03fed 100644 --- a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs @@ -177,11 +177,26 @@ internal static bool ResolveOperationFromList(string identifier, IList p } // If any of the things returned are an action, it better be the only thing returned, and there can't be parameters in the URL - if (candidatesMatchingOperations.Any(f => f.IsAction())) + bool actionExists = false; + bool functionExists = false; + for (int i=0; i < candidatesMatchingOperations.Count; i++) + { + if (candidatesMatchingOperations[i].IsFunction()) + { + functionExists = true; + } + + if (candidatesMatchingOperations[i].IsAction()) + { + actionExists = true; + } + } + + if (actionExists) { if (candidatesMatchingOperations.Count > 1) { - if (candidatesMatchingOperations.Any(o => o.IsFunction())) + if (functionExists) { throw new ODataException(ODataErrorStrings.FunctionOverloadResolver_MultipleOperationOverloads(identifier)); } diff --git a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs index 655ba904be..bea716e3fa 100644 --- a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs @@ -157,7 +157,7 @@ public virtual IEdmTerm ResolveTerm(IEdmModel model, string termName) throw new ODataException(Strings.UriParserMetadata_MultipleMatchingTypesFound(termName)); } - return results.SingleOrDefault(); + return results.Count == 1 ? results[0] : null; } /// @@ -180,7 +180,7 @@ public virtual IEdmSchemaType ResolveType(IEdmModel model, string typeName) throw new ODataException(Strings.UriParserMetadata_MultipleMatchingTypesFound(typeName)); } - return results.SingleOrDefault(); + return results.Count == 1 ? results[0] : null; } /// From 8b1a788e4c877a51439feb53c22eca51bde764b8 Mon Sep 17 00:00:00 2001 From: KanishManuja-MS <41647051+KanishManuja-MS@users.noreply.github.com> Date: Wed, 4 Dec 2019 12:41:27 -0800 Subject: [PATCH 4/9] Address PR comments from JP and Sam --- .../Metadata/EdmLibraryExtensions.cs | 63 ++++++---- .../Binders/SelectPathSegmentTokenBinder.cs | 2 +- .../Parsers/FunctionOverloadResolver.cs | 10 +- .../UriParser/Resolver/ODataUriResolver.cs | 118 ++++++++++++------ 4 files changed, 122 insertions(+), 71 deletions(-) diff --git a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs index 7614e5fc49..12175b93dc 100644 --- a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs +++ b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs @@ -8,6 +8,7 @@ namespace Microsoft.OData.Metadata { #region Namespaces using System; + using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; @@ -25,10 +26,9 @@ namespace Microsoft.OData.Metadata #endif #if !ODATA_SERVICE && !ODATA_CLIENT using Microsoft.OData.JsonLight; + using Microsoft.OData.UriParser; using ErrorStrings = Microsoft.OData.Strings; using PlatformHelper = Microsoft.OData.PlatformHelper; - using System.Collections; - using Microsoft.OData.UriParser; #endif #endregion Namespaces @@ -254,71 +254,82 @@ internal static IEnumerable FilterBoundOperationsWithSameTypeHier } /// - /// Filters the operations by parameter names. + /// Filters the operations by parameter count. /// /// The operations. - /// The list of non-binding parameter names to match. - /// Whether case insensitive. + /// The count of non-binding parameter names to match. /// The best matching operations based on parameters. - internal static IEnumerable FindBestOverloadBasedOnParameters(this IEnumerable functions, IEnumerable parameters, bool caseInsensitive = false) + internal static IEnumerable FindBestOverloadBasedOnParameterCount(this IEnumerable functions, int parameterCount) { // The best match out of a list of candidates is the one that has the same number of (non-binding) parameters as specified. - IEnumerable exactMatches = functions.Where(f => f.Parameters.Count() == parameters.Count() + (f.IsBound ? 1 : 0)); + IEnumerable exactMatches = functions.Where(f => f.Parameters.Count() == parameterCount + (f.IsBound ? 1 : 0)); return exactMatches.Count() > 0 ? exactMatches : functions; } /// - /// Filters the operations by parameter names. + /// Filters the operations by parameter count. /// /// The operations. - /// The list of non-binding parameter names to match. - /// Whether case insensitive. + /// The count of non-binding parameter names to match. /// The best matching operations based on parameters. - internal static IList FindBestOverloadBasedOnParameters(this IList functions, IList parameters, bool caseInsensitive = false) + internal static IList FindBestOverloadBasedOnParameterCount(this IList functions, int parameterCount) { - IList exactMatches = new List(); + IList exactMatches = null; // The best match out of a list of candidates is the one that has the same number of (non-binding) parameters as specified. - for(int i=0; i < functions.Count; i++) + for (int i = 0; i < functions.Count; i++) { - if (functions[i].Parameters.Count() == parameters.Count + (functions[i].IsBound ? 1 : 0)) + if (functions[i].Parameters.Count() == parameterCount + (functions[i].IsBound ? 1 : 0)) { + if (exactMatches == null) + { + exactMatches = new List(); + } + exactMatches.Add(functions[i]); } } - return exactMatches.Count > 0 ? exactMatches : functions; + return exactMatches == null ? functions : exactMatches; } /// - /// Filters the operations by parameter names. + /// Filters the operations candidates based on parameters. /// /// The operations. + /// Binding type for the operation. /// The parameters. /// Whether case insensitive. + /// outputs the actions found while matching functions /// Operations filtered by parameter. - internal static IList FindBestCandidate(this IEnumerable operations, IEdmType bindingType, IList parameterNameList, bool caseInsensitive, out IList actionItems) + internal static IList FilterOperationCandidatesBasedOnParameterList(this IEnumerable operations, IEdmType bindingType, IList parameterNameList, bool caseInsensitive, out IList actionItems) { Debug.Assert(operations != null, "operations"); Debug.Assert(parameterNameList != null, "parameters"); bool hasParameters = parameterNameList.Count > 0; - actionItems = new List(); + actionItems = null; IList operationCandidates = new List(); foreach (var operation in operations) { - bool suitableCandidate = true; if (bindingType != null) { operation.EnsureOperationBoundWithBindingParameter(); } + bool suitableCandidate = true; + // If the number of parameters > 0 then this has to be a function as actions can't have parameters on the uri only in the payload. Filter further by parameters in this case, otherwise don't. if (hasParameters) { if (operation.IsAction()) { + if (actionItems == null) + { + actionItems = new List(); + } + actionItems.Add(operation); suitableCandidate = false; } @@ -411,7 +422,7 @@ internal static IEnumerable FilterOperationsByParameterNames(this Debug.Assert(parameterNameList != null, "parameters"); // TODO: update code that is duplicate between operation and operation import, add more tests. - for (int i=0; i < operations.Count; i++) + for (int i = 0; i < operations.Count; i++) { if (!ParametersSatisfyFunction(operations[i], parameterNameList, caseInsensitive)) { @@ -1923,30 +1934,28 @@ private static bool ParametersSatisfyFunction(IEdmOperation operation, IList functionParameters = parametersToMatch.ToList(); - // if any required parameters are missing, don't consider it a match. BitArray bitmask = new BitArray(parameterNameList.Count); - for (int i=0; i < functionParameters.Count; i++) + foreach (var functionParameter in parametersToMatch) { bool matched = false; - for (int j=0; j < parameterNameList.Count; j++) + for (int j = 0; j < parameterNameList.Count; j++) { - if (string.Equals(parameterNameList[j], functionParameters[i].Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)) + if (string.Equals(parameterNameList[j], functionParameter.Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)) { matched = true; bitmask[j] = true; } } - if (!matched && !(functionParameters[i] is IEdmOptionalParameter)) + if (!matched && !(functionParameter is IEdmOptionalParameter)) { return false; } } // if any specified parameters don't match, don't consider it a match. - for (int i=0; i < parameterNameList.Count; i++) + for (int i = 0; i < parameterNameList.Count; i++) { if (!bitmask[i]) { diff --git a/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs b/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs index ed841c78c6..b193877844 100644 --- a/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs +++ b/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs @@ -185,7 +185,7 @@ internal static bool TryBindAsOperation(PathSegmentToken pathToken, IEdmModel mo // If more than one overload matches, try to select based on optional parameters if (possibleFunctions.Count() > 1 && parameterNames.Count > 0) { - possibleFunctions = possibleFunctions.FindBestOverloadBasedOnParameters(parameterNames); + possibleFunctions = possibleFunctions.FindBestOverloadBasedOnParameterCount(parameterNames.Count); } if (!possibleFunctions.HasAny()) diff --git a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs index 933bd03fed..1a75c7e6b4 100644 --- a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs @@ -165,10 +165,10 @@ internal static bool ResolveOperationFromList(string identifier, IList p throw; } - IList foundActionsWhenLookingForFunctions = new List(); + IList foundActionsWhenLookingForFunctions = null; bool hasParameters = parameterNames.Count > 0; - IList candidatesMatchingOperations = operationsFromModel.FindBestCandidate(bindingType, parameterNames, resolver.EnableCaseInsensitive, out foundActionsWhenLookingForFunctions); + IList candidatesMatchingOperations = operationsFromModel.FilterOperationCandidatesBasedOnParameterList(bindingType, parameterNames, resolver.EnableCaseInsensitive, out foundActionsWhenLookingForFunctions); // Only filter if there is more than one and its needed. if (candidatesMatchingOperations.Count > 1) @@ -179,7 +179,7 @@ internal static bool ResolveOperationFromList(string identifier, IList p // If any of the things returned are an action, it better be the only thing returned, and there can't be parameters in the URL bool actionExists = false; bool functionExists = false; - for (int i=0; i < candidatesMatchingOperations.Count; i++) + for (int i = 0; i < candidatesMatchingOperations.Count; i++) { if (candidatesMatchingOperations[i].IsFunction()) { @@ -215,7 +215,7 @@ internal static bool ResolveOperationFromList(string identifier, IList p return true; } - if (foundActionsWhenLookingForFunctions.Count > 0) + if (foundActionsWhenLookingForFunctions != null && foundActionsWhenLookingForFunctions.Count > 0) { throw ExceptionUtil.CreateBadRequestError(ODataErrorStrings.RequestUriProcessor_SegmentDoesNotSupportKeyPredicates(identifier)); } @@ -223,7 +223,7 @@ internal static bool ResolveOperationFromList(string identifier, IList p // If more than one overload matches, try to select based on optional parameters if (candidatesMatchingOperations.Count > 1) { - candidatesMatchingOperations = candidatesMatchingOperations.FindBestOverloadBasedOnParameters(parameterNames); + candidatesMatchingOperations = candidatesMatchingOperations.FindBestOverloadBasedOnParameterCount(parameterNames.Count); } if (candidatesMatchingOperations.Count > 1) diff --git a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs index bea716e3fa..db7ec9006a 100644 --- a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs @@ -100,14 +100,23 @@ public virtual IEdmNavigationSource ResolveNavigationSource(IEdmModel model, str } var result = container.Elements.OfType() - .Where(source => string.Equals(identifier, source.Name, StringComparison.OrdinalIgnoreCase)).ToList(); + .Where(source => string.Equals(identifier, source.Name, StringComparison.OrdinalIgnoreCase)); + + IEdmNavigationSource resolvedNavigationSource = default(IEdmNavigationSource); - if (result.Count > 1) + foreach (var candidate in result) { - throw new ODataException(Strings.UriParserMetadata_MultipleMatchingNavigationSourcesFound(identifier)); + if (resolvedNavigationSource == default(IEdmNavigationSource)) + { + resolvedNavigationSource = candidate; + } + else + { + throw new ODataException(Strings.UriParserMetadata_MultipleMatchingNavigationSourcesFound(identifier)); + } } - return result.SingleOrDefault(); + return resolvedNavigationSource; } /// @@ -125,15 +134,23 @@ public virtual IEdmProperty ResolveProperty(IEdmStructuredType type, string prop } var result = type.Properties() - .Where(_ => string.Equals(propertyName, _.Name, StringComparison.OrdinalIgnoreCase)) - .ToList(); + .Where(_ => string.Equals(propertyName, _.Name, StringComparison.OrdinalIgnoreCase)); + + IEdmProperty resolvedProperty = default(IEdmProperty); - if (result.Count > 1) + foreach(var candidate in result) { - throw new ODataException(Strings.UriParserMetadata_MultipleMatchingPropertiesFound(propertyName, type.FullTypeName())); + if (resolvedProperty == default(IEdmProperty)) + { + resolvedProperty = candidate; + } + else + { + throw new ODataException(Strings.UriParserMetadata_MultipleMatchingPropertiesFound(propertyName, type.FullTypeName())); + } } - return result.SingleOrDefault(); + return resolvedProperty; } /// @@ -151,6 +168,11 @@ public virtual IEdmTerm ResolveTerm(IEdmModel model, string termName) } IList results = FindAcrossModels(model, termName, /*caseInsensitive*/ true); + + if (results == null) + { + return null; + } if (results.Count > 1) { @@ -175,6 +197,12 @@ public virtual IEdmSchemaType ResolveType(IEdmModel model, string typeName) } IList results = FindAcrossModels(model, typeName, /*caseInsensitive*/ true); + + if (results == null) + { + return null; + } + if (results.Count > 1) { throw new ODataException(Strings.UriParserMetadata_MultipleMatchingTypesFound(typeName)); @@ -314,22 +342,20 @@ public virtual IDictionary ResolveOpera /// The resolved key list. public virtual IEnumerable> ResolveKeys(IEdmEntityType type, IList positionalValues, Func convertFunc) { - var keyProperties = type.Key().ToList(); - // Throw an error if key size from url doesn't match that from model. // Other derived ODataUriResolver intended for alternative key resolution, such as the built in AlternateKeysODataUriResolver, // should override this ResolveKeys method. - if (keyProperties.Count != positionalValues.Count) + if (type.Key().Count() != positionalValues.Count) { throw ExceptionUtil.CreateBadRequestError(Strings.BadRequest_KeyCountMismatch(type.FullName())); } var keyPairList = new List>(positionalValues.Count); - - for (int i = 0; i < keyProperties.Count; i++) + + int i = 0; + foreach (IEdmProperty keyProperty in type.Key()) { - string valueText = positionalValues[i]; - IEdmProperty keyProperty = keyProperties[i]; + string valueText = positionalValues[i++]; object convertedValue = convertFunc(keyProperty.Type, valueText); if (convertedValue == null) { @@ -352,17 +378,16 @@ public virtual IEnumerable> ResolveKeys(IEdmEntityT public virtual IEnumerable> ResolveKeys(IEdmEntityType type, IDictionary namedValues, Func convertFunc) { var convertedPairs = new Dictionary(StringComparer.Ordinal); - var keyProperties = type.Key().ToList(); // Throw an error if key size from url doesn't match that from model. // Other derived ODataUriResolver intended for alternative key resolution, such as the built in AlternateKeysODataUriResolver, // should override this ResolveKeys method. - if (keyProperties.Count != namedValues.Count) + if (type.Key().Count() != namedValues.Count) { throw ExceptionUtil.CreateBadRequestError(Strings.BadRequest_KeyCountMismatch(type.FullName())); } - foreach (IEdmStructuralProperty property in keyProperties) + foreach (IEdmStructuralProperty property in type.Key()) { string valueText; @@ -370,17 +395,27 @@ public virtual IEnumerable> ResolveKeys(IEdmEntityT { if (EnableCaseInsensitive) { - var list = namedValues.Keys.Where(key => string.Equals(property.Name, key, StringComparison.OrdinalIgnoreCase)).ToList(); - if (list.Count > 1) + var list = namedValues.Keys.Where(key => string.Equals(property.Name, key, StringComparison.OrdinalIgnoreCase)); + + string caseInsensitiveKey = string.Empty; + int count = 0; + foreach (var key in list) { - throw new ODataException(Strings.UriParserMetadata_MultipleMatchingKeysFound(property.Name)); + if (count >= 1) + { + throw new ODataException(Strings.UriParserMetadata_MultipleMatchingKeysFound(property.Name)); + } + + caseInsensitiveKey = key; + count++; } - else if (list.Count == 0) + + if (count == 0) { throw ExceptionUtil.CreateSyntaxError(); } - valueText = namedValues[list.Single()]; + valueText = namedValues[caseInsensitiveKey]; } else { @@ -409,24 +444,28 @@ public virtual IEnumerable> ResolveKeys(IEdmEntityT internal static IEdmOperationParameter ResolveOperationParameterNameCaseInsensitive(IEdmOperation operation, string identifier) { // first look for a case-sensitive match - var list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.Ordinal)).ToList(); - if (list.Count == 0) + var list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.Ordinal)); + if (!list.Any()) { // if no case sensitive, try case-insensitive - list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.OrdinalIgnoreCase)).ToList(); + list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.OrdinalIgnoreCase)); } - if (list.Count > 1) - { - throw new ODataException(Strings.UriParserMetadata_MultipleMatchingParametersFound(identifier)); - } + IEdmOperationParameter resolvedOperationParameter = null; - if (list.Count == 1) + foreach(var parameter in list) { - return list.Single(); + if (resolvedOperationParameter == null) + { + resolvedOperationParameter = parameter; + } + else + { + throw new ODataException(Strings.UriParserMetadata_MultipleMatchingParametersFound(identifier)); + } } - return null; + return resolvedOperationParameter; } internal static ODataUriResolver GetUriResolver(IServiceProvider container) @@ -441,7 +480,7 @@ internal static ODataUriResolver GetUriResolver(IServiceProvider container) private static IList FindAcrossModels(IEdmModel model, String qualifiedName, bool caseInsensitive) where T : IEdmSchemaElement { - IList results = new List(); + IList results = null; FindSchemaElements(model, qualifiedName, caseInsensitive, ref results); foreach (IEdmModel reference in model.ReferencedModels) @@ -452,7 +491,7 @@ private static IList FindAcrossModels(IEdmModel model, String qualifiedNam return results; } - private static IList FindSchemaElements(IEdmModel model, string qualifiedName, bool caseInsensitive, ref IList results) where T : IEdmSchemaElement + private static void FindSchemaElements(IEdmModel model, string qualifiedName, bool caseInsensitive, ref IList results) where T : IEdmSchemaElement { foreach (var schema in model.SchemaElements) { @@ -460,12 +499,15 @@ private static IList FindSchemaElements(IEdmModel model, string qualifiedN { if (schema is T) { + if (results == null) + { + results = new List(); + } + results.Add((T)schema); } } } - - return results; } } } From b3cc79bc5d4cafca4275b2aa4017791cb0677f06 Mon Sep 17 00:00:00 2001 From: KanishManuja-MS <41647051+KanishManuja-MS@users.noreply.github.com> Date: Sat, 30 Nov 2019 21:43:41 -0800 Subject: [PATCH 5/9] Performance improvements --- .../Metadata/EdmLibraryExtensions.cs | 216 ++++++++++++++---- .../Parsers/FunctionOverloadResolver.cs | 68 ++---- .../UriParser/Resolver/ODataUriResolver.cs | 32 ++- .../ExtensionMethods/ExtensionMethods.cs | 13 +- 4 files changed, 217 insertions(+), 112 deletions(-) diff --git a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs index 1073f91c72..d2d3dc2760 100644 --- a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs +++ b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs @@ -27,6 +27,8 @@ namespace Microsoft.OData.Metadata using Microsoft.OData.JsonLight; using ErrorStrings = Microsoft.OData.Strings; using PlatformHelper = Microsoft.OData.PlatformHelper; + using System.Collections; + using Microsoft.OData.UriParser; #endif #endregion Namespaces @@ -265,6 +267,111 @@ internal static IEnumerable FindBestOverloadBasedOnParameters(thi return exactMatches.Count() > 0 ? exactMatches : functions; } + /// + /// Filters the operations by parameter names. + /// + /// The operations. + /// The list of non-binding parameter names to match. + /// Whether case insensitive. + /// The best matching operations based on parameters. + internal static IList FindBestOverloadBasedOnParameters(this IList functions, IList parameters, bool caseInsensitive = false) + { + IList exactMatches = new List(); + + // The best match out of a list of candidates is the one that has the same number of (non-binding) parameters as specified. + for(int i=0; i < functions.Count; i++) + { + if (functions[i].Parameters.Count() == parameters.Count + (functions[i].IsBound ? 1 : 0)) + { + exactMatches.Add(functions[i]); + } + } + + return exactMatches.Count > 0 ? exactMatches : functions; + } + + /// + /// Filters the operations by parameter names. + /// + /// The operations. + /// The parameters. + /// Whether case insensitive. + /// Operations filtered by parameter. + internal static IList FindBestCandidate(this IEnumerable operations, IEdmType bindingType, IList parameterNameList, bool caseInsensitive, out IList actionItems) + { + Debug.Assert(operations != null, "operations"); + Debug.Assert(parameterNameList != null, "parameters"); + + bool hasParameters = parameterNameList.Count > 0; + actionItems = new List(); + IList operationCandidates = new List(); + + foreach (var operation in operations) + { + bool suitableCandidate = true; + if (bindingType != null) + { + operation.EnsureOperationBoundWithBindingParameter(); + } + + // If the number of parameters > 0 then this has to be a function as actions can't have parameters on the uri only in the payload. Filter further by parameters in this case, otherwise don't. + if (hasParameters) + { + if (operation.IsAction()) + { + actionItems.Add(operation); + suitableCandidate = false; + } + + if (!ParametersSatisfyFunction(operation, parameterNameList, caseInsensitive)) + { + suitableCandidate = false; + } + } + else if (bindingType != null) + { + // If it is a bound function without required parameters, then the parameter list should only have one parameter and any others as optional. + if (operation.IsFunction()) + { + int count = 0; + foreach (var param in operation.Parameters) + { + if (count > 0 && !(param is IEdmOptionalParameter)) + { + suitableCandidate = false; + } + + count++; + } + + if (count == 0) + { + suitableCandidate = false; + } + } + else if (!operation.IsAction()) + { + // If IsFunction and !IsAction are the same then this may not be required. To be confirmed. + suitableCandidate = false; + } + } + else + { + if (operation.IsFunction() && operation.Parameters.HasAny()) + { + suitableCandidate = false; + } + } + + if (suitableCandidate) + { + operationCandidates.Add(operation); + } + } + + return operationCandidates; + } + /// /// Filters the operations by parameter names. /// @@ -292,22 +399,38 @@ internal static IEnumerable FilterOperationsByParameterNames(this } /// - /// Ensures that operations are bound and have a binding parameter, other wise throws an exception. + /// Filters the operations by parameter names. /// /// The operations. - internal static void EnsureOperationsBoundWithBindingParameter(this IEnumerable operations) + /// The parameters. + /// Whether case insensitive. + /// Operations filtered by parameter. + internal static IEnumerable FilterOperationsByParameterNames(this IList operations, IList parameterNameList, bool caseInsensitive) { - foreach (IEdmOperation operation in operations) + Debug.Assert(operations != null, "operations"); + Debug.Assert(parameterNameList != null, "parameters"); + + // TODO: update code that is duplicate between operation and operation import, add more tests. + for (int i=0; i < operations.Count; i++) { - if (!operation.IsBound) + if (!ParametersSatisfyFunction(operations[i], parameterNameList, caseInsensitive)) { - throw new ODataException(ErrorStrings.EdmLibraryExtensions_UnBoundOperationsFoundFromIEdmModelFindMethodIsInvalid(operation.Name)); + continue; } - if (operation.Parameters.FirstOrDefault() == null) - { - throw new ODataException(ErrorStrings.EdmLibraryExtensions_NoParameterBoundOperationsFoundFromIEdmModelFindMethodIsInvalid(operation.Name)); - } + yield return operations[i]; + } + } + + /// + /// Ensures that operations are bound and have a binding parameter, other wise throws an exception. + /// + /// The operations. + internal static void EnsureOperationsBoundWithBindingParameter(this IEnumerable operations) + { + foreach (var operation in operations) + { + operation.EnsureOperationBoundWithBindingParameter(); } } @@ -429,32 +552,6 @@ internal static string FullNameWithParameters(this IEdmOperationImport operation return operationImport.FullName() + operationImport.ParameterTypesToString(); } - /// - /// Removes the actions. - /// - /// The source. - /// The action items. - /// Only the functions from the operation sequence. - internal static IEnumerable RemoveActions(this IEnumerable source, out IList actionItems) - { - List functions = new List(); - - actionItems = new List(); - foreach (var item in source) - { - if (item.IsAction()) - { - actionItems.Add(item); - } - else - { - functions.Add(item); - } - } - - return functions; - } - /// /// Removes the action imports. /// @@ -1829,20 +1926,38 @@ private static bool ParametersSatisfyFunction(IEdmOperation operation, IList functionParameters = parametersToMatch.ToList(); // if any required parameters are missing, don't consider it a match. - if (functionParameters.Where( - p => !(p is IEdmOptionalParameter)).Any( - p => parameterNameList.All( - k => !string.Equals(k, p.Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)))) + BitArray bitmask = new BitArray(parameterNameList.Count); + for (int i=0; i < functionParameters.Count; i++) { - return false; + if (functionParameters[i] is IEdmOptionalParameter) + { + continue; + } + + bool matched = false; + for (int j=0; j < parameterNameList.Count; j++) + { + if (string.Equals(parameterNameList[j], functionParameters[i].Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)) + { + matched = true; + bitmask[j] = true; + break; + } + } + + if (!matched) + { + return false; + } } // if any specified parameters don't match, don't consider it a match. - if (parameterNameList.Any( - k => functionParameters.All( - p => !string.Equals(k, p.Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)))) + for (int i=0; i < parameterNameList.Count; i++) { - return false; + if (!bitmask[i]) + { + return false; + } } return true; @@ -1868,6 +1983,19 @@ private static int InheritanceLevelFromSpecifiedInheritedType(this IEdmStructure return inheritanceLevelsFromBase; } + + private static void EnsureOperationBoundWithBindingParameter(this IEdmOperation operation) + { + if (!operation.IsBound) + { + throw new ODataException(ErrorStrings.EdmLibraryExtensions_UnBoundOperationsFoundFromIEdmModelFindMethodIsInvalid(operation.Name)); + } + + if (operation.Parameters.FirstOrDefault() == null) + { + throw new ODataException(ErrorStrings.EdmLibraryExtensions_NoParameterBoundOperationsFoundFromIEdmModelFindMethodIsInvalid(operation.Name)); + } + } #endif #endregion diff --git a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs index 5b4737abd9..fa0085fa0b 100644 --- a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs @@ -124,7 +124,7 @@ internal static bool ResolveOperationImportFromList(string identifier, IListThe single matching function found. /// Resolver to be used. /// True if a function was matched, false otherwise. Will throw if the model has illegal operation imports. - internal static bool ResolveOperationFromList(string identifier, IEnumerable parameterNames, IEdmType bindingType, IEdmModel model, out IEdmOperation matchingOperation, ODataUriResolver resolver) + internal static bool ResolveOperationFromList(string identifier, IList parameterNames, IEdmType bindingType, IEdmModel model, out IEdmOperation matchingOperation, ODataUriResolver resolver) { // TODO: update code that is duplicate between operation and operation import, add more tests. // If the previous segment is an open type, the service action name is required to be fully qualified or else we always treat it as an open property name. @@ -139,19 +139,20 @@ internal static bool ResolveOperationFromList(string identifier, IEnumerable candidateMatchingOperations = null; + IEnumerable operationsFromModel = null; // The extension method FindBoundOperations & FindOperations call IEdmModel.FindDeclaredBoundOperations which can be implemented by anyone and it could throw any type of exception // so catching all of them and simply putting it in the inner exception. + // It is better to cast the results of ResolveBoundOperations as IList instead of looping over this for basic things such as count. try { if (bindingType != null) { - candidateMatchingOperations = resolver.ResolveBoundOperations(model, identifier, bindingType); + operationsFromModel = resolver.ResolveBoundOperations(model, identifier, bindingType); } else { - candidateMatchingOperations = resolver.ResolveUnboundOperations(model, identifier); + operationsFromModel = resolver.ResolveUnboundOperations(model, identifier); } } catch (Exception exc) @@ -165,44 +166,22 @@ internal static bool ResolveOperationFromList(string identifier, IEnumerable foundActionsWhenLookingForFunctions = new List(); - bool hasParameters = parameterNames.Count() > 0; + bool hasParameters = parameterNames.Count > 0; - if (bindingType != null) - { - candidateMatchingOperations.EnsureOperationsBoundWithBindingParameter(); - } - - // If the number of parameters > 0 then this has to be a function as actions can't have parameters on the uri only in the payload. Filter further by parameters in this case, otherwise don't. - if (hasParameters) - { - // can only be a function as only functions have parameters on the uri. - candidateMatchingOperations = candidateMatchingOperations.RemoveActions(out foundActionsWhenLookingForFunctions) - .FilterOperationsByParameterNames(parameterNames, resolver.EnableCaseInsensitive); - } - else if (bindingType != null) - { - // Filter out functions with more than one parameter. Actions should not be filtered as the parameters are in the payload not the uri - candidateMatchingOperations = candidateMatchingOperations.Where(o => - (o.IsFunction() && (o.Parameters.Count() == 1 || o.Parameters.Skip(1).All(p => p is IEdmOptionalParameter))) || o.IsAction()); - } - else - { - // Filter out functions with any parameters - candidateMatchingOperations = candidateMatchingOperations.Where(o => (o.IsFunction() && !o.Parameters.Any()) || o.IsAction()); - } + IList candidatesMatchingOperations = operationsFromModel.FindBestCandidate(bindingType, parameterNames, resolver.EnableCaseInsensitive, out foundActionsWhenLookingForFunctions); // Only filter if there is more than one and its needed. - if (candidateMatchingOperations.Count() > 1) + if (candidatesMatchingOperations.Count > 1) { - candidateMatchingOperations = candidateMatchingOperations.FilterBoundOperationsWithSameTypeHierarchyToTypeClosestToBindingType(bindingType); + candidatesMatchingOperations = candidatesMatchingOperations.FilterBoundOperationsWithSameTypeHierarchyToTypeClosestToBindingType(bindingType) as IList; } // If any of the things returned are an action, it better be the only thing returned, and there can't be parameters in the URL - if (candidateMatchingOperations.Any(f => f.IsAction())) + if (candidatesMatchingOperations.Any(f => f.IsAction())) { - if (candidateMatchingOperations.Count() > 1) + if (candidatesMatchingOperations.Count > 1) { - if (candidateMatchingOperations.Any(o => o.IsFunction())) + if (candidatesMatchingOperations.Any(o => o.IsFunction())) { throw new ODataException(ODataErrorStrings.FunctionOverloadResolver_MultipleOperationOverloads(identifier)); } @@ -217,7 +196,7 @@ internal static bool ResolveOperationFromList(string identifier, IEnumerable 0 ? candidatesMatchingOperations[0] : null; return true; } @@ -227,35 +206,28 @@ internal static bool ResolveOperationFromList(string identifier, IEnumerable 1) + if (candidatesMatchingOperations.Count > 1) { - candidateMatchingOperations = candidateMatchingOperations.FindBestOverloadBasedOnParameters(parameterNames); + candidatesMatchingOperations = candidatesMatchingOperations.FindBestOverloadBasedOnParameters(parameterNames); } - if (candidateMatchingOperations.Count() > 1) + if (candidatesMatchingOperations.Count > 1) { throw new ODataException(ODataErrorStrings.FunctionOverloadResolver_NoSingleMatchFound(identifier, string.Join(",", parameterNames.ToArray()))); } - matchingOperation = candidateMatchingOperations.SingleOrDefault(); + matchingOperation = candidatesMatchingOperations.Count > 0 ? candidatesMatchingOperations[0] : null; return matchingOperation != null; } internal static bool HasAny(this IEnumerable enumerable) where T : class { - IList list = enumerable as IList; - if (list != null) + if (enumerable != null && enumerable.GetEnumerator().MoveNext()) { - return list.Count > 0; - } - - T[] array = enumerable as T[]; - if (array != null) - { - return array.Length > 0; + return true; } - return enumerable.FirstOrDefault() != null; + return false; } } } \ No newline at end of file diff --git a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs index 32d9e2aa9a..5704a75040 100644 --- a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs @@ -5,6 +5,7 @@ //--------------------------------------------------------------------- using System; +using System.Collections; using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Linq; @@ -198,10 +199,10 @@ public virtual IEnumerable ResolveBoundOperations(IEdmModel model } IList operations = FindAcrossModels(model, identifier, /*caseInsensitive*/ true); - if (operations != null && operations.Count() > 0) + if (operations != null && operations.Count > 0) { IList matchedOperation = new List(); - for (int i = 0; i < operations.Count(); i++) + for (int i = 0; i < operations.Count; i++) { if (operations[i].HasEquivalentBindingType(bindingType)) { @@ -230,10 +231,10 @@ public virtual IEnumerable ResolveUnboundOperations(IEdmModel mod } IList operations = FindAcrossModels(model, identifier, /*caseInsensitive*/ true); - if (operations != null && operations.Count() > 0) + if (operations != null && operations.Count > 0) { IList matchedOperation = new List(); - for (int i = 0; i < operations.Count(); i++) + for (int i = 0; i < operations.Count; i++) { if (!operations[i].IsBound) { @@ -438,22 +439,33 @@ internal static ODataUriResolver GetUriResolver(IServiceProvider container) return container.GetRequiredService(); } - private static List FindAcrossModels(IEdmModel model, String qualifiedName, bool caseInsensitive) where T : IEdmSchemaElement + private static IList FindAcrossModels(IEdmModel model, String qualifiedName, bool caseInsensitive) where T : IEdmSchemaElement { - List results = FindSchemaElements(model, qualifiedName, caseInsensitive).ToList(); + IList results = new List(); + FindSchemaElements(model, qualifiedName, caseInsensitive, ref results); foreach (IEdmModel reference in model.ReferencedModels) { - results.AddRange(FindSchemaElements(reference, qualifiedName, caseInsensitive)); + FindSchemaElements(reference, qualifiedName, caseInsensitive, ref results); } return results; } - private static IEnumerable FindSchemaElements(IEdmModel model, string qualifiedName, bool caseInsensitive) where T : IEdmSchemaElement + private static IList FindSchemaElements(IEdmModel model, string qualifiedName, bool caseInsensitive, ref IList results) where T : IEdmSchemaElement { - return model.SchemaElements.OfType() - .Where(e => string.Equals(qualifiedName, e.FullName(), caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)); + foreach(var schema in model.SchemaElements) + { + if (string.Equals(qualifiedName, schema.FullName(), caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)) + { + if (schema is T item) + { + results.Add(item); + } + } + } + + return results; } } } diff --git a/src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs b/src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs index cb3f768e24..f5d50973d7 100644 --- a/src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs +++ b/src/Microsoft.OData.Edm/ExtensionMethods/ExtensionMethods.cs @@ -3182,19 +3182,12 @@ internal static bool TryGetStaticEntitySet(this IEdmPathExpression pathExpressio internal static bool HasAny(this IEnumerable enumerable) where T : class { - IList list = enumerable as IList; - if (list != null) + if (enumerable != null && enumerable.GetEnumerator().MoveNext()) { - return list.Count > 0; - } - - T[] array = enumerable as T[]; - if (array != null) - { - return array.Length > 0; + return true; } - return enumerable.FirstOrDefault() != null; + return false; } /// From ded75ced6603713eaab1b8ff2c04d68bf3ab89cb Mon Sep 17 00:00:00 2001 From: KanishManuja-MS <41647051+KanishManuja-MS@users.noreply.github.com> Date: Sun, 1 Dec 2019 02:01:03 -0800 Subject: [PATCH 6/9] fix issues with implementation --- src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs | 8 +------- .../UriParser/Resolver/ODataUriResolver.cs | 6 +++--- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs index d2d3dc2760..7614e5fc49 100644 --- a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs +++ b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs @@ -1929,11 +1929,6 @@ private static bool ParametersSatisfyFunction(IEdmOperation operation, IList FindAcrossModels(IEdmModel model, String qualifiedNam private static IList FindSchemaElements(IEdmModel model, string qualifiedName, bool caseInsensitive, ref IList results) where T : IEdmSchemaElement { - foreach(var schema in model.SchemaElements) + foreach (var schema in model.SchemaElements) { if (string.Equals(qualifiedName, schema.FullName(), caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)) { - if (schema is T item) + if (schema is T) { - results.Add(item); + results.Add((T)schema); } } } From 17c0eba86cbbb6ce10f091b7f3b2e53a0653db1f Mon Sep 17 00:00:00 2001 From: KanishManuja-MS <41647051+KanishManuja-MS@users.noreply.github.com> Date: Mon, 2 Dec 2019 09:39:30 -0800 Subject: [PATCH 7/9] Microoptimize a little further in the Hot Path --- .../Parsers/FunctionOverloadResolver.cs | 19 +++++++++++++++++-- .../UriParser/Resolver/ODataUriResolver.cs | 4 ++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs index fa0085fa0b..933bd03fed 100644 --- a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs @@ -177,11 +177,26 @@ internal static bool ResolveOperationFromList(string identifier, IList p } // If any of the things returned are an action, it better be the only thing returned, and there can't be parameters in the URL - if (candidatesMatchingOperations.Any(f => f.IsAction())) + bool actionExists = false; + bool functionExists = false; + for (int i=0; i < candidatesMatchingOperations.Count; i++) + { + if (candidatesMatchingOperations[i].IsFunction()) + { + functionExists = true; + } + + if (candidatesMatchingOperations[i].IsAction()) + { + actionExists = true; + } + } + + if (actionExists) { if (candidatesMatchingOperations.Count > 1) { - if (candidatesMatchingOperations.Any(o => o.IsFunction())) + if (functionExists) { throw new ODataException(ODataErrorStrings.FunctionOverloadResolver_MultipleOperationOverloads(identifier)); } diff --git a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs index 655ba904be..bea716e3fa 100644 --- a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs @@ -157,7 +157,7 @@ public virtual IEdmTerm ResolveTerm(IEdmModel model, string termName) throw new ODataException(Strings.UriParserMetadata_MultipleMatchingTypesFound(termName)); } - return results.SingleOrDefault(); + return results.Count == 1 ? results[0] : null; } /// @@ -180,7 +180,7 @@ public virtual IEdmSchemaType ResolveType(IEdmModel model, string typeName) throw new ODataException(Strings.UriParserMetadata_MultipleMatchingTypesFound(typeName)); } - return results.SingleOrDefault(); + return results.Count == 1 ? results[0] : null; } /// From 35164fc3f28158c5af18758a01a921ece8b32ca9 Mon Sep 17 00:00:00 2001 From: KanishManuja-MS <41647051+KanishManuja-MS@users.noreply.github.com> Date: Wed, 4 Dec 2019 12:41:27 -0800 Subject: [PATCH 8/9] Address PR comments from JP and Sam --- .../Metadata/EdmLibraryExtensions.cs | 63 ++++++---- .../Binders/SelectPathSegmentTokenBinder.cs | 2 +- .../Parsers/FunctionOverloadResolver.cs | 10 +- .../UriParser/Resolver/ODataUriResolver.cs | 118 ++++++++++++------ 4 files changed, 122 insertions(+), 71 deletions(-) diff --git a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs index 7614e5fc49..12175b93dc 100644 --- a/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs +++ b/src/Microsoft.OData.Core/Metadata/EdmLibraryExtensions.cs @@ -8,6 +8,7 @@ namespace Microsoft.OData.Metadata { #region Namespaces using System; + using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; @@ -25,10 +26,9 @@ namespace Microsoft.OData.Metadata #endif #if !ODATA_SERVICE && !ODATA_CLIENT using Microsoft.OData.JsonLight; + using Microsoft.OData.UriParser; using ErrorStrings = Microsoft.OData.Strings; using PlatformHelper = Microsoft.OData.PlatformHelper; - using System.Collections; - using Microsoft.OData.UriParser; #endif #endregion Namespaces @@ -254,71 +254,82 @@ internal static IEnumerable FilterBoundOperationsWithSameTypeHier } /// - /// Filters the operations by parameter names. + /// Filters the operations by parameter count. /// /// The operations. - /// The list of non-binding parameter names to match. - /// Whether case insensitive. + /// The count of non-binding parameter names to match. /// The best matching operations based on parameters. - internal static IEnumerable FindBestOverloadBasedOnParameters(this IEnumerable functions, IEnumerable parameters, bool caseInsensitive = false) + internal static IEnumerable FindBestOverloadBasedOnParameterCount(this IEnumerable functions, int parameterCount) { // The best match out of a list of candidates is the one that has the same number of (non-binding) parameters as specified. - IEnumerable exactMatches = functions.Where(f => f.Parameters.Count() == parameters.Count() + (f.IsBound ? 1 : 0)); + IEnumerable exactMatches = functions.Where(f => f.Parameters.Count() == parameterCount + (f.IsBound ? 1 : 0)); return exactMatches.Count() > 0 ? exactMatches : functions; } /// - /// Filters the operations by parameter names. + /// Filters the operations by parameter count. /// /// The operations. - /// The list of non-binding parameter names to match. - /// Whether case insensitive. + /// The count of non-binding parameter names to match. /// The best matching operations based on parameters. - internal static IList FindBestOverloadBasedOnParameters(this IList functions, IList parameters, bool caseInsensitive = false) + internal static IList FindBestOverloadBasedOnParameterCount(this IList functions, int parameterCount) { - IList exactMatches = new List(); + IList exactMatches = null; // The best match out of a list of candidates is the one that has the same number of (non-binding) parameters as specified. - for(int i=0; i < functions.Count; i++) + for (int i = 0; i < functions.Count; i++) { - if (functions[i].Parameters.Count() == parameters.Count + (functions[i].IsBound ? 1 : 0)) + if (functions[i].Parameters.Count() == parameterCount + (functions[i].IsBound ? 1 : 0)) { + if (exactMatches == null) + { + exactMatches = new List(); + } + exactMatches.Add(functions[i]); } } - return exactMatches.Count > 0 ? exactMatches : functions; + return exactMatches == null ? functions : exactMatches; } /// - /// Filters the operations by parameter names. + /// Filters the operations candidates based on parameters. /// /// The operations. + /// Binding type for the operation. /// The parameters. /// Whether case insensitive. + /// outputs the actions found while matching functions /// Operations filtered by parameter. - internal static IList FindBestCandidate(this IEnumerable operations, IEdmType bindingType, IList parameterNameList, bool caseInsensitive, out IList actionItems) + internal static IList FilterOperationCandidatesBasedOnParameterList(this IEnumerable operations, IEdmType bindingType, IList parameterNameList, bool caseInsensitive, out IList actionItems) { Debug.Assert(operations != null, "operations"); Debug.Assert(parameterNameList != null, "parameters"); bool hasParameters = parameterNameList.Count > 0; - actionItems = new List(); + actionItems = null; IList operationCandidates = new List(); foreach (var operation in operations) { - bool suitableCandidate = true; if (bindingType != null) { operation.EnsureOperationBoundWithBindingParameter(); } + bool suitableCandidate = true; + // If the number of parameters > 0 then this has to be a function as actions can't have parameters on the uri only in the payload. Filter further by parameters in this case, otherwise don't. if (hasParameters) { if (operation.IsAction()) { + if (actionItems == null) + { + actionItems = new List(); + } + actionItems.Add(operation); suitableCandidate = false; } @@ -411,7 +422,7 @@ internal static IEnumerable FilterOperationsByParameterNames(this Debug.Assert(parameterNameList != null, "parameters"); // TODO: update code that is duplicate between operation and operation import, add more tests. - for (int i=0; i < operations.Count; i++) + for (int i = 0; i < operations.Count; i++) { if (!ParametersSatisfyFunction(operations[i], parameterNameList, caseInsensitive)) { @@ -1923,30 +1934,28 @@ private static bool ParametersSatisfyFunction(IEdmOperation operation, IList functionParameters = parametersToMatch.ToList(); - // if any required parameters are missing, don't consider it a match. BitArray bitmask = new BitArray(parameterNameList.Count); - for (int i=0; i < functionParameters.Count; i++) + foreach (var functionParameter in parametersToMatch) { bool matched = false; - for (int j=0; j < parameterNameList.Count; j++) + for (int j = 0; j < parameterNameList.Count; j++) { - if (string.Equals(parameterNameList[j], functionParameters[i].Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)) + if (string.Equals(parameterNameList[j], functionParameter.Name, caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal)) { matched = true; bitmask[j] = true; } } - if (!matched && !(functionParameters[i] is IEdmOptionalParameter)) + if (!matched && !(functionParameter is IEdmOptionalParameter)) { return false; } } // if any specified parameters don't match, don't consider it a match. - for (int i=0; i < parameterNameList.Count; i++) + for (int i = 0; i < parameterNameList.Count; i++) { if (!bitmask[i]) { diff --git a/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs b/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs index 3620cc10d1..b54ebd806f 100644 --- a/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs +++ b/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs @@ -186,7 +186,7 @@ internal static bool TryBindAsOperation(PathSegmentToken pathToken, IEdmModel mo // If more than one overload matches, try to select based on optional parameters if (possibleFunctions.Count() > 1 && parameterNames.Count > 0) { - possibleFunctions = possibleFunctions.FindBestOverloadBasedOnParameters(parameterNames); + possibleFunctions = possibleFunctions.FindBestOverloadBasedOnParameterCount(parameterNames.Count); } if (!possibleFunctions.HasAny()) diff --git a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs index 933bd03fed..1a75c7e6b4 100644 --- a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs @@ -165,10 +165,10 @@ internal static bool ResolveOperationFromList(string identifier, IList p throw; } - IList foundActionsWhenLookingForFunctions = new List(); + IList foundActionsWhenLookingForFunctions = null; bool hasParameters = parameterNames.Count > 0; - IList candidatesMatchingOperations = operationsFromModel.FindBestCandidate(bindingType, parameterNames, resolver.EnableCaseInsensitive, out foundActionsWhenLookingForFunctions); + IList candidatesMatchingOperations = operationsFromModel.FilterOperationCandidatesBasedOnParameterList(bindingType, parameterNames, resolver.EnableCaseInsensitive, out foundActionsWhenLookingForFunctions); // Only filter if there is more than one and its needed. if (candidatesMatchingOperations.Count > 1) @@ -179,7 +179,7 @@ internal static bool ResolveOperationFromList(string identifier, IList p // If any of the things returned are an action, it better be the only thing returned, and there can't be parameters in the URL bool actionExists = false; bool functionExists = false; - for (int i=0; i < candidatesMatchingOperations.Count; i++) + for (int i = 0; i < candidatesMatchingOperations.Count; i++) { if (candidatesMatchingOperations[i].IsFunction()) { @@ -215,7 +215,7 @@ internal static bool ResolveOperationFromList(string identifier, IList p return true; } - if (foundActionsWhenLookingForFunctions.Count > 0) + if (foundActionsWhenLookingForFunctions != null && foundActionsWhenLookingForFunctions.Count > 0) { throw ExceptionUtil.CreateBadRequestError(ODataErrorStrings.RequestUriProcessor_SegmentDoesNotSupportKeyPredicates(identifier)); } @@ -223,7 +223,7 @@ internal static bool ResolveOperationFromList(string identifier, IList p // If more than one overload matches, try to select based on optional parameters if (candidatesMatchingOperations.Count > 1) { - candidatesMatchingOperations = candidatesMatchingOperations.FindBestOverloadBasedOnParameters(parameterNames); + candidatesMatchingOperations = candidatesMatchingOperations.FindBestOverloadBasedOnParameterCount(parameterNames.Count); } if (candidatesMatchingOperations.Count > 1) diff --git a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs index bea716e3fa..db7ec9006a 100644 --- a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs @@ -100,14 +100,23 @@ public virtual IEdmNavigationSource ResolveNavigationSource(IEdmModel model, str } var result = container.Elements.OfType() - .Where(source => string.Equals(identifier, source.Name, StringComparison.OrdinalIgnoreCase)).ToList(); + .Where(source => string.Equals(identifier, source.Name, StringComparison.OrdinalIgnoreCase)); + + IEdmNavigationSource resolvedNavigationSource = default(IEdmNavigationSource); - if (result.Count > 1) + foreach (var candidate in result) { - throw new ODataException(Strings.UriParserMetadata_MultipleMatchingNavigationSourcesFound(identifier)); + if (resolvedNavigationSource == default(IEdmNavigationSource)) + { + resolvedNavigationSource = candidate; + } + else + { + throw new ODataException(Strings.UriParserMetadata_MultipleMatchingNavigationSourcesFound(identifier)); + } } - return result.SingleOrDefault(); + return resolvedNavigationSource; } /// @@ -125,15 +134,23 @@ public virtual IEdmProperty ResolveProperty(IEdmStructuredType type, string prop } var result = type.Properties() - .Where(_ => string.Equals(propertyName, _.Name, StringComparison.OrdinalIgnoreCase)) - .ToList(); + .Where(_ => string.Equals(propertyName, _.Name, StringComparison.OrdinalIgnoreCase)); + + IEdmProperty resolvedProperty = default(IEdmProperty); - if (result.Count > 1) + foreach(var candidate in result) { - throw new ODataException(Strings.UriParserMetadata_MultipleMatchingPropertiesFound(propertyName, type.FullTypeName())); + if (resolvedProperty == default(IEdmProperty)) + { + resolvedProperty = candidate; + } + else + { + throw new ODataException(Strings.UriParserMetadata_MultipleMatchingPropertiesFound(propertyName, type.FullTypeName())); + } } - return result.SingleOrDefault(); + return resolvedProperty; } /// @@ -151,6 +168,11 @@ public virtual IEdmTerm ResolveTerm(IEdmModel model, string termName) } IList results = FindAcrossModels(model, termName, /*caseInsensitive*/ true); + + if (results == null) + { + return null; + } if (results.Count > 1) { @@ -175,6 +197,12 @@ public virtual IEdmSchemaType ResolveType(IEdmModel model, string typeName) } IList results = FindAcrossModels(model, typeName, /*caseInsensitive*/ true); + + if (results == null) + { + return null; + } + if (results.Count > 1) { throw new ODataException(Strings.UriParserMetadata_MultipleMatchingTypesFound(typeName)); @@ -314,22 +342,20 @@ public virtual IDictionary ResolveOpera /// The resolved key list. public virtual IEnumerable> ResolveKeys(IEdmEntityType type, IList positionalValues, Func convertFunc) { - var keyProperties = type.Key().ToList(); - // Throw an error if key size from url doesn't match that from model. // Other derived ODataUriResolver intended for alternative key resolution, such as the built in AlternateKeysODataUriResolver, // should override this ResolveKeys method. - if (keyProperties.Count != positionalValues.Count) + if (type.Key().Count() != positionalValues.Count) { throw ExceptionUtil.CreateBadRequestError(Strings.BadRequest_KeyCountMismatch(type.FullName())); } var keyPairList = new List>(positionalValues.Count); - - for (int i = 0; i < keyProperties.Count; i++) + + int i = 0; + foreach (IEdmProperty keyProperty in type.Key()) { - string valueText = positionalValues[i]; - IEdmProperty keyProperty = keyProperties[i]; + string valueText = positionalValues[i++]; object convertedValue = convertFunc(keyProperty.Type, valueText); if (convertedValue == null) { @@ -352,17 +378,16 @@ public virtual IEnumerable> ResolveKeys(IEdmEntityT public virtual IEnumerable> ResolveKeys(IEdmEntityType type, IDictionary namedValues, Func convertFunc) { var convertedPairs = new Dictionary(StringComparer.Ordinal); - var keyProperties = type.Key().ToList(); // Throw an error if key size from url doesn't match that from model. // Other derived ODataUriResolver intended for alternative key resolution, such as the built in AlternateKeysODataUriResolver, // should override this ResolveKeys method. - if (keyProperties.Count != namedValues.Count) + if (type.Key().Count() != namedValues.Count) { throw ExceptionUtil.CreateBadRequestError(Strings.BadRequest_KeyCountMismatch(type.FullName())); } - foreach (IEdmStructuralProperty property in keyProperties) + foreach (IEdmStructuralProperty property in type.Key()) { string valueText; @@ -370,17 +395,27 @@ public virtual IEnumerable> ResolveKeys(IEdmEntityT { if (EnableCaseInsensitive) { - var list = namedValues.Keys.Where(key => string.Equals(property.Name, key, StringComparison.OrdinalIgnoreCase)).ToList(); - if (list.Count > 1) + var list = namedValues.Keys.Where(key => string.Equals(property.Name, key, StringComparison.OrdinalIgnoreCase)); + + string caseInsensitiveKey = string.Empty; + int count = 0; + foreach (var key in list) { - throw new ODataException(Strings.UriParserMetadata_MultipleMatchingKeysFound(property.Name)); + if (count >= 1) + { + throw new ODataException(Strings.UriParserMetadata_MultipleMatchingKeysFound(property.Name)); + } + + caseInsensitiveKey = key; + count++; } - else if (list.Count == 0) + + if (count == 0) { throw ExceptionUtil.CreateSyntaxError(); } - valueText = namedValues[list.Single()]; + valueText = namedValues[caseInsensitiveKey]; } else { @@ -409,24 +444,28 @@ public virtual IEnumerable> ResolveKeys(IEdmEntityT internal static IEdmOperationParameter ResolveOperationParameterNameCaseInsensitive(IEdmOperation operation, string identifier) { // first look for a case-sensitive match - var list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.Ordinal)).ToList(); - if (list.Count == 0) + var list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.Ordinal)); + if (!list.Any()) { // if no case sensitive, try case-insensitive - list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.OrdinalIgnoreCase)).ToList(); + list = operation.Parameters.Where(parameter => string.Equals(identifier, parameter.Name, StringComparison.OrdinalIgnoreCase)); } - if (list.Count > 1) - { - throw new ODataException(Strings.UriParserMetadata_MultipleMatchingParametersFound(identifier)); - } + IEdmOperationParameter resolvedOperationParameter = null; - if (list.Count == 1) + foreach(var parameter in list) { - return list.Single(); + if (resolvedOperationParameter == null) + { + resolvedOperationParameter = parameter; + } + else + { + throw new ODataException(Strings.UriParserMetadata_MultipleMatchingParametersFound(identifier)); + } } - return null; + return resolvedOperationParameter; } internal static ODataUriResolver GetUriResolver(IServiceProvider container) @@ -441,7 +480,7 @@ internal static ODataUriResolver GetUriResolver(IServiceProvider container) private static IList FindAcrossModels(IEdmModel model, String qualifiedName, bool caseInsensitive) where T : IEdmSchemaElement { - IList results = new List(); + IList results = null; FindSchemaElements(model, qualifiedName, caseInsensitive, ref results); foreach (IEdmModel reference in model.ReferencedModels) @@ -452,7 +491,7 @@ private static IList FindAcrossModels(IEdmModel model, String qualifiedNam return results; } - private static IList FindSchemaElements(IEdmModel model, string qualifiedName, bool caseInsensitive, ref IList results) where T : IEdmSchemaElement + private static void FindSchemaElements(IEdmModel model, string qualifiedName, bool caseInsensitive, ref IList results) where T : IEdmSchemaElement { foreach (var schema in model.SchemaElements) { @@ -460,12 +499,15 @@ private static IList FindSchemaElements(IEdmModel model, string qualifiedN { if (schema is T) { + if (results == null) + { + results = new List(); + } + results.Add((T)schema); } } } - - return results; } } } From 68b195551f9337ec7a10001fe9e8975a9597b157 Mon Sep 17 00:00:00 2001 From: KanishManuja-MS <41647051+KanishManuja-MS@users.noreply.github.com> Date: Thu, 23 Jan 2020 13:03:13 -0800 Subject: [PATCH 9/9] Address PR changes --- .../Binders/SelectPathSegmentTokenBinder.cs | 4 +- .../Parsers/FunctionOverloadResolver.cs | 25 ++++++++++++ .../UriParser/Resolver/ODataUriResolver.cs | 10 +++-- .../ExtensionMethods/ExtensionMethodTests.cs | 38 +++++++++++++++++++ 4 files changed, 71 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs b/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs index b54ebd806f..bd86a8c371 100644 --- a/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs +++ b/src/Microsoft.OData.Core/UriParser/Binders/SelectPathSegmentTokenBinder.cs @@ -178,13 +178,13 @@ internal static bool TryBindAsOperation(PathSegmentToken pathToken, IEdmModel mo } // Only filter if there is more than one and its needed. - if (possibleFunctions.Count() > 1) + if (possibleFunctions.CountGreaterThan(1)) { possibleFunctions = possibleFunctions.FilterBoundOperationsWithSameTypeHierarchyToTypeClosestToBindingType(entityType); } // If more than one overload matches, try to select based on optional parameters - if (possibleFunctions.Count() > 1 && parameterNames.Count > 0) + if (possibleFunctions.CountGreaterThan(1) && parameterNames.Count > 0) { possibleFunctions = possibleFunctions.FindBestOverloadBasedOnParameterCount(parameterNames.Count); } diff --git a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs index 1a75c7e6b4..6d6e718230 100644 --- a/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Parsers/FunctionOverloadResolver.cs @@ -190,6 +190,11 @@ internal static bool ResolveOperationFromList(string identifier, IList p { actionExists = true; } + + if (functionExists && actionExists) + { + break; + } } if (actionExists) @@ -244,5 +249,25 @@ internal static bool HasAny(this IEnumerable enumerable) where T : class return false; } + + internal static bool CountGreaterThan(this IEnumerable enumerable, int count) where T : class + { + int currentCount = 0; + foreach (T element in enumerable) + { + if (element == null) + { + continue; + } + + currentCount++; + if (currentCount > count) + { + return true; + } + } + + return false; + } } } \ No newline at end of file diff --git a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs index db7ec9006a..9fd6831b98 100644 --- a/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs +++ b/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs @@ -345,7 +345,8 @@ public virtual IEnumerable> ResolveKeys(IEdmEntityT // Throw an error if key size from url doesn't match that from model. // Other derived ODataUriResolver intended for alternative key resolution, such as the built in AlternateKeysODataUriResolver, // should override this ResolveKeys method. - if (type.Key().Count() != positionalValues.Count) + IEnumerable keys = type.Key(); + if (keys.Count() != positionalValues.Count) { throw ExceptionUtil.CreateBadRequestError(Strings.BadRequest_KeyCountMismatch(type.FullName())); } @@ -353,7 +354,7 @@ public virtual IEnumerable> ResolveKeys(IEdmEntityT var keyPairList = new List>(positionalValues.Count); int i = 0; - foreach (IEdmProperty keyProperty in type.Key()) + foreach (IEdmProperty keyProperty in keys) { string valueText = positionalValues[i++]; object convertedValue = convertFunc(keyProperty.Type, valueText); @@ -382,12 +383,13 @@ public virtual IEnumerable> ResolveKeys(IEdmEntityT // Throw an error if key size from url doesn't match that from model. // Other derived ODataUriResolver intended for alternative key resolution, such as the built in AlternateKeysODataUriResolver, // should override this ResolveKeys method. - if (type.Key().Count() != namedValues.Count) + IEnumerable keys = type.Key(); + if (keys.Count() != namedValues.Count) { throw ExceptionUtil.CreateBadRequestError(Strings.BadRequest_KeyCountMismatch(type.FullName())); } - foreach (IEdmStructuralProperty property in type.Key()) + foreach (IEdmStructuralProperty property in keys) { string valueText; diff --git a/test/FunctionalTests/Microsoft.OData.Edm.Tests/ExtensionMethods/ExtensionMethodTests.cs b/test/FunctionalTests/Microsoft.OData.Edm.Tests/ExtensionMethods/ExtensionMethodTests.cs index d20d8d26b0..389bca4adf 100644 --- a/test/FunctionalTests/Microsoft.OData.Edm.Tests/ExtensionMethods/ExtensionMethodTests.cs +++ b/test/FunctionalTests/Microsoft.OData.Edm.Tests/ExtensionMethods/ExtensionMethodTests.cs @@ -800,6 +800,17 @@ public void FindDeclaredEntitySetWithNonExistingEntitySetName() Assert.Null(TestModel.Instance.Model.FindDeclaredEntitySet(entitySetName)); } + [Fact] + public void TestHasAnyParity() + { + IList list = new List(); + for(int i=1; i <10; i++) + { + Assert.Equal(HasAnyCheap(list), HasAnyExpensive(list)); + list.Add(new EdmEntityType("NS", "f" + i, TestModel.Instance.EntitySet.EntityType())); + } + } + [Fact] public void FindDeclaredEntitySetWithSingletonName() { @@ -1080,5 +1091,32 @@ public IEdmType Definition get { return type; } } } + + internal static bool HasAnyExpensive(IEnumerable enumerable) where T : class + { + IList list = enumerable as IList; + if (list != null) + { + return list.Count > 0; + } + + T[] array = enumerable as T[]; + if (array != null) + { + return array.Length > 0; + } + + return enumerable.FirstOrDefault() != null; + } + + internal static bool HasAnyCheap(IEnumerable enumerable) where T : class + { + if (enumerable != null && enumerable.GetEnumerator().MoveNext()) + { + return true; + } + + return false; + } } }