-
Notifications
You must be signed in to change notification settings - Fork 351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cache container elements in ODataUriResolver model elements cache #2623
Changes from all commits
5d3119d
5c30ad1
0abc32a
54c9e19
aaf46f9
a9f37f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
//--------------------------------------------------------------------- | ||
// <copyright file="NormalizedSchemaElementsCache.cs" company="Microsoft"> | ||
// <copyright file="NormalizedModelElementsCache.cs" company="Microsoft"> | ||
// Copyright (C) Microsoft Corporation. All rights reserved. See License.txt in the project root for license information. | ||
// </copyright> | ||
//--------------------------------------------------------------------- | ||
|
@@ -12,30 +12,34 @@ | |
namespace Microsoft.OData.Edm | ||
{ | ||
/// <summary> | ||
/// Cache used to store schema elements using case-normalized names | ||
/// Cache used to store model elements using case-normalized names | ||
/// to speed up case-insensitive model lookups. The cache is populated | ||
/// up front so that if an item is not found in the cache, we can assume | ||
/// it doesn't exist in the model. For this reason, it's important that | ||
/// the model be immutable. | ||
/// </summary> | ||
internal sealed class NormalizedSchemaElementsCache | ||
internal sealed class NormalizedModelElementsCache | ||
{ | ||
// We create different caches for different types of schema elements because all current usage request schema elements | ||
// of specific types. If we were to use a single dictionary <string, ISchemaElement> we would need | ||
// to do additional work (and allocations) during lookups to filter the results to the susbset that matches the request type. | ||
// to do additional work (and allocations) during lookups to filter the results to the subset that matches the request type. | ||
private readonly Dictionary<string, List<IEdmSchemaType>> schemaTypesCache = new Dictionary<string, List<IEdmSchemaType>>(StringComparer.OrdinalIgnoreCase); | ||
private readonly Dictionary<string, List<IEdmOperation>> operationsCache = new Dictionary<string, List<IEdmOperation>>(StringComparer.OrdinalIgnoreCase); | ||
private readonly Dictionary<string, List<IEdmTerm>> termsCache = new Dictionary<string, List<IEdmTerm>>(StringComparer.OrdinalIgnoreCase); | ||
private readonly Dictionary<string, List<IEdmNavigationSource>> navigationSourcesCache = new Dictionary<string, List<IEdmNavigationSource>>(StringComparer.OrdinalIgnoreCase); | ||
private readonly Dictionary<string, List<IEdmOperationImport>> operationImportsCache = new Dictionary<string, List<IEdmOperationImport>>(StringComparer.OrdinalIgnoreCase); | ||
|
||
/// <summary> | ||
/// Builds a case-insensitive cache of schema elements from | ||
/// the specified <paramref name="model"/>. | ||
/// </summary> | ||
/// <param name="model">The model whose schema elements to cache. This model should be immutable. See <see cref="ExtensionMethods.MarkAsImmutable(IEdmModel)"/>.</param> | ||
public NormalizedSchemaElementsCache(IEdmModel model) | ||
public NormalizedModelElementsCache(IEdmModel model) | ||
{ | ||
Debug.Assert(model != null); | ||
|
||
PopulateContainerElements(model); | ||
|
||
PopulateSchemaElements(model); | ||
|
||
foreach (IEdmModel referencedModel in model.ReferencedModels) | ||
|
@@ -89,26 +93,76 @@ public List<IEdmTerm> FindTerms(string qualifiedName) | |
return null; | ||
} | ||
|
||
/// <summary> | ||
/// Find all navigation sources that match the <paramref name="name"/>. | ||
/// </summary> | ||
/// <param name="name">The case-insensitive name to match.</param> | ||
/// <returns>A list of matching navigation sources, or null if no navigation source matches the name.</returns> | ||
public List<IEdmNavigationSource> FindNavigationSources(string name) | ||
habbes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
if (navigationSourcesCache.TryGetValue(name, out List<IEdmNavigationSource> results)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the name the simple identifier of the navigation source? not the qualified name? How about searching the navigation source in the referenced model? with the same name but a different namespace? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, is it only on Top-level model entity container? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's the simple identifier of the navigation source. And yes, it's only searching the top-level model entity container. The reason I implemented it this way is because I wanted to retain the same behaviour as the existing implementation which performs the search as follows: IEdmEntityContainer container = model.EntityContainer;
if (container == null)
{
return null;
}
var result = container.Elements.OfType<IEdmNavigationSource>()
.Where(source => string.Equals(identifier, source.Name, StringComparison.OrdinalIgnoreCase)); As you can see the existing implementation only searches the top-level model's container and it only compares the If I change the behaviour of the cache to include referenced models or fully qualified identifier, then there would be an inconsistency between the cache and non-cache code paths. On the other hand, if the expected behaviour is that the URI resolver should search referenced models and/or fully qualified name, then the current implementation is a bug. And I think that should be a separate discussion since that change of behaviour could be observable to the customer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The doc comment of the
However, the existing implementation does not consider the qualified name in its search: IEdmEntityContainer container = model.EntityContainer;
if (container == null)
{
return Enumerable.Empty<IEdmOperationImport>();
}
return container.Elements.OfType<IEdmOperationImport>()
.Where(source => string.Equals(identifier, source.Name, StringComparison.OrdinalIgnoreCase)); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are specifying the navigation source identifier and not searching in referenced models, is there a scenario where whatever that gets returned is a list and not just a single navigation source? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like can we have several navigation sources with the same identifier in the same model? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're searching a single container, I don't think that's possible. Maybe if we were searching in multiple containers that could be possible, but that would be arguably a new feature, extending the existing behaviour, which is beyond the scope of this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, because we are dealing with a case insensitive scenario, we can have multiple navigation sources in the same container whose names only differ by case. That's why we return a list. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we have navigation sources in the same container whose names only differ by case? or we can access a navigation source using different cases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The edm model is case-sensitive by default, so it is possible to have navigation sources which only differ by case because they are different names as far as the Since this cache is case-insensitive, for the same key we might have multiple matching entries, that's why were store the values in lists. |
||
{ | ||
return results; | ||
} | ||
|
||
return null; | ||
} | ||
|
||
/// <summary> | ||
/// Find all operation imports that match the <paramref name="name"/>. | ||
/// </summary> | ||
/// <param name="name">The case-insensitive name to match.</param> | ||
/// <returns>A list of matching operation imports, or null if no operation import matches the name.</returns> | ||
public List<IEdmOperationImport> FindOperationImports(string name) | ||
{ | ||
if (operationImportsCache.TryGetValue(name, out List<IEdmOperationImport> results)) | ||
habbes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
return results; | ||
} | ||
|
||
return null; | ||
} | ||
|
||
private void PopulateSchemaElements(IEdmModel model) | ||
{ | ||
foreach (IEdmSchemaElement element in model.SchemaElements) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can the model be null here or the check is done by the caller? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is the responsibility of the caller. In this class I only placed a That said, I checked the public methods in the public There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added null argument checks to the public methods of ODataUriResolver. |
||
{ | ||
if (element is IEdmSchemaType schemaType) | ||
{ | ||
AddElementToCache(schemaType, schemaTypesCache); | ||
AddSchemaElementToCache(schemaType, schemaTypesCache); | ||
} | ||
else if (element is IEdmOperation operation) | ||
{ | ||
AddElementToCache(operation, operationsCache); | ||
AddSchemaElementToCache(operation, operationsCache); | ||
} | ||
else if (element is IEdmTerm term) | ||
{ | ||
AddElementToCache(term, termsCache); | ||
AddSchemaElementToCache(term, termsCache); | ||
} | ||
} | ||
} | ||
|
||
private void PopulateContainerElements(IEdmModel model) | ||
{ | ||
if (model.EntityContainer is null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can the model be null here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the responsibility of the caller to ensure that the model is not null here. Since this is an internal class, I placed a |
||
{ | ||
return; | ||
} | ||
|
||
foreach (IEdmEntityContainerElement element in model.EntityContainer.Elements) | ||
{ | ||
if (element is IEdmOperationImport operationImport) | ||
{ | ||
AddContainerElementToCache(operationImport, operationImportsCache); | ||
} | ||
else if (element is IEdmNavigationSource navigationSource) | ||
{ | ||
AddContainerElementToCache(navigationSource, navigationSourcesCache); | ||
} | ||
} | ||
} | ||
|
||
private static void AddElementToCache<T>(T element, Dictionary<string, List<T>> cache) where T : IEdmSchemaElement | ||
private static void AddSchemaElementToCache<T>(T element, Dictionary<string, List<T>> cache) where T : IEdmSchemaElement | ||
{ | ||
List<T> results; | ||
string normalizedKey = element.FullName(); | ||
|
@@ -120,5 +174,18 @@ private static void AddElementToCache<T>(T element, Dictionary<string, List<T>> | |
|
||
results.Add(element); | ||
} | ||
|
||
private static void AddContainerElementToCache<T>(T element, Dictionary<string, List<T>> cache) where T : IEdmNamedElement | ||
{ | ||
List<T> results; | ||
string normalizedKey = element.Name; | ||
if (!cache.TryGetValue(normalizedKey, out results)) | ||
{ | ||
results = new List<T>(); | ||
cache[normalizedKey] = results; | ||
} | ||
|
||
results.Add(element); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,6 @@ | |
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Linq; | ||
using Microsoft.OData.Edm; | ||
using Microsoft.OData.Edm.Vocabularies; | ||
|
@@ -85,12 +84,36 @@ public virtual void PromoteBinaryOperandTypes( | |
/// <returns>The resolved navigation source.</returns> | ||
public virtual IEdmNavigationSource ResolveNavigationSource(IEdmModel model, string identifier) | ||
{ | ||
ExceptionUtils.CheckArgumentNotNull(model, nameof(model)); | ||
ExceptionUtils.CheckArgumentNotNull(identifier, nameof(identifier)); | ||
|
||
IEdmNavigationSource navSource = model.FindDeclaredNavigationSource(identifier); | ||
if (navSource != null || !EnableCaseInsensitive) | ||
{ | ||
return navSource; | ||
} | ||
|
||
if (model.IsImmutable()) | ||
{ | ||
NormalizedModelElementsCache cache = GetNormalizedModelElementsCache(model); | ||
IList<IEdmNavigationSource> cachedResults = cache.FindNavigationSources(identifier); | ||
|
||
if (cachedResults != null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the reason why it's preferrable to return an empty collection rather than null from a function that has a collection return type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I agree that it's generally preferrable, return an empty collection for the cache would introduce more complexity to the code than this single null-check that's not exposed to the user. Most of the null checks against the result occur in code paths where we already have an if statement checking whether the collection count is 0, so they didn't result in that much uglier code in my opinion. To return empty collections efficiently, I would have to create a static empty list for each element type that the cache supports: static readonly List<IEdmSchemaType> emptySchemaTypesList = new List<IEdmSchemaType>();
static readonly List<IEdmOperation> emptyOperationsList = new List<...>;
static readonly List<IEdmTerm> emptyTermsList = ...;
static readonly List<IEdmNavigationSource> emptyNavigationSourcesList = ...;
static readonly List<IEdmOperationImport> emptyOperationImportsList = ...;
// then in the find methods:
if (navigationSources.TryGetValue(...))
{
return results;
}
return emptyNavigationSourcesList; The runtime overhead is not that bad since these collections are allocated only once, but it didn't feel worthwhile since the cache is an internal implementation detail used in a very limited scope. That said, I don't hold this opinion strongly. If you still believe that I should return an empty collection despite the explanation above, I can go ahead and make the change. |
||
{ | ||
if (cachedResults.Count == 1) | ||
{ | ||
return cachedResults[0]; | ||
} | ||
|
||
if (cachedResults.Count > 1) | ||
{ | ||
throw new ODataException(Strings.UriParserMetadata_MultipleMatchingNavigationSourcesFound(identifier)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why throw an exception if cachedResults are more than one and you said this is a possibility? or what does this mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This cache-based code path replicates the existing behaviour of the Notice that this method returns a single |
||
} | ||
} | ||
|
||
return null; | ||
} | ||
|
||
IEdmEntityContainer container = model.EntityContainer; | ||
if (container == null) | ||
{ | ||
|
@@ -125,6 +148,9 @@ public virtual IEdmNavigationSource ResolveNavigationSource(IEdmModel model, str | |
/// <returns>The resolved <see cref="IEdmProperty"/></returns> | ||
public virtual IEdmProperty ResolveProperty(IEdmStructuredType type, string propertyName) | ||
{ | ||
ExceptionUtils.CheckArgumentNotNull(type, nameof(type)); | ||
ExceptionUtils.CheckArgumentNotNull(propertyName, nameof(propertyName)); | ||
|
||
IEdmProperty property = type.FindProperty(propertyName); | ||
if (property != null || !EnableCaseInsensitive) | ||
{ | ||
|
@@ -159,6 +185,9 @@ public virtual IEdmProperty ResolveProperty(IEdmStructuredType type, string prop | |
/// <returns>Resolved term.</returns> | ||
public virtual IEdmTerm ResolveTerm(IEdmModel model, string termName) | ||
{ | ||
ExceptionUtils.CheckArgumentNotNull(model, nameof(model)); | ||
ExceptionUtils.CheckArgumentNotNull(termName, nameof(termName)); | ||
|
||
IEdmTerm term = model.FindTerm(termName); | ||
if (term != null || !EnableCaseInsensitive) | ||
{ | ||
|
@@ -188,6 +217,9 @@ public virtual IEdmTerm ResolveTerm(IEdmModel model, string termName) | |
/// <returns>Resolved type.</returns> | ||
public virtual IEdmSchemaType ResolveType(IEdmModel model, string typeName) | ||
{ | ||
ExceptionUtils.CheckArgumentNotNull(model, nameof(model)); | ||
ExceptionUtils.CheckArgumentNotNull(typeName, nameof(typeName)); | ||
|
||
IEdmSchemaType type = model.FindType(typeName); | ||
if (type != null || !EnableCaseInsensitive) | ||
{ | ||
|
@@ -218,6 +250,10 @@ public virtual IEdmSchemaType ResolveType(IEdmModel model, string typeName) | |
/// <returns>Resolved operation list.</returns> | ||
public virtual IEnumerable<IEdmOperation> ResolveBoundOperations(IEdmModel model, string identifier, IEdmType bindingType) | ||
{ | ||
ExceptionUtils.CheckArgumentNotNull(model, nameof(model)); | ||
ExceptionUtils.CheckArgumentNotNull(identifier, nameof(identifier)); | ||
ExceptionUtils.CheckArgumentNotNull(bindingType, nameof(bindingType)); | ||
|
||
IEnumerable<IEdmOperation> results = model.FindBoundOperations(identifier, bindingType); | ||
if (results.Any() || !EnableCaseInsensitive) | ||
{ | ||
|
@@ -251,6 +287,9 @@ public virtual IEnumerable<IEdmOperation> ResolveBoundOperations(IEdmModel model | |
/// <returns>Resolved operation list.</returns> | ||
public virtual IEnumerable<IEdmOperation> ResolveUnboundOperations(IEdmModel model, string identifier) | ||
{ | ||
ExceptionUtils.CheckArgumentNotNull(model, nameof(model)); | ||
ExceptionUtils.CheckArgumentNotNull(identifier, nameof(identifier)); | ||
|
||
IEnumerable<IEdmOperation> results = model.FindOperations(identifier); | ||
if (results.Any() || !EnableCaseInsensitive) | ||
{ | ||
|
@@ -284,12 +323,28 @@ public virtual IEnumerable<IEdmOperation> ResolveUnboundOperations(IEdmModel mod | |
/// <returns>All operation imports that can be found by the specified name, returns an empty enumerable if no operation import exists.</returns> | ||
public virtual IEnumerable<IEdmOperationImport> ResolveOperationImports(IEdmModel model, string identifier) | ||
{ | ||
ExceptionUtils.CheckArgumentNotNull(model, nameof(model)); | ||
ExceptionUtils.CheckArgumentNotNull(identifier, nameof(identifier)); | ||
|
||
IEnumerable<IEdmOperationImport> results = model.FindDeclaredOperationImports(identifier); | ||
if (results.Any() || !EnableCaseInsensitive) | ||
{ | ||
return results; | ||
} | ||
|
||
if (model.IsImmutable()) | ||
{ | ||
NormalizedModelElementsCache cache = GetNormalizedModelElementsCache(model); | ||
IEnumerable<IEdmOperationImport> cachedResults = cache.FindOperationImports(identifier); | ||
|
||
if (cachedResults != null) | ||
{ | ||
return cachedResults; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why don't you do the checks for multiple operations here like you doing with navigation sources? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because this cache-based code path aims to replicate the existing behaviour of the Unlike the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how do you check for the params? or when two operations with the same name are returned but with different params how do you know which is which? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know exactly. That is outside the scope of the cache, and possibly resolved elsewhere. The cache was only concerned in replicating the already existing behaviour in a more efficient manner. Here's a snippet of the existing implementation (this code path will still be used when the model is not immutable): return container.Elements.OfType<IEdmOperationImport>()
.Where(source => string.Equals(identifier, source.Name, StringComparison.OrdinalIgnoreCase)); It would be a bug if the cache returned something different from the non-cached version. |
||
} | ||
|
||
return Enumerable.Empty<IEdmOperationImport>(); | ||
} | ||
|
||
IEdmEntityContainer container = model.EntityContainer; | ||
if (container == null) | ||
{ | ||
|
@@ -308,6 +363,9 @@ public virtual IEnumerable<IEdmOperationImport> ResolveOperationImports(IEdmMode | |
/// <returns>A dictionary containing resolved parameters.</returns> | ||
public virtual IDictionary<IEdmOperationParameter, SingleValueNode> ResolveOperationParameters(IEdmOperation operation, IDictionary<string, SingleValueNode> input) | ||
{ | ||
ExceptionUtils.CheckArgumentNotNull(operation, nameof(operation)); | ||
ExceptionUtils.CheckArgumentNotNull(input, nameof(input)); | ||
|
||
Dictionary<IEdmOperationParameter, SingleValueNode> result = new Dictionary<IEdmOperationParameter, SingleValueNode>(EqualityComparer<IEdmOperationParameter>.Default); | ||
foreach (var item in input) | ||
{ | ||
|
@@ -342,6 +400,10 @@ public virtual IDictionary<IEdmOperationParameter, SingleValueNode> ResolveOpera | |
/// <returns>The resolved key list.</returns> | ||
public virtual IEnumerable<KeyValuePair<string, object>> ResolveKeys(IEdmEntityType type, IList<string> positionalValues, Func<IEdmTypeReference, string, object> convertFunc) | ||
{ | ||
ExceptionUtils.CheckArgumentNotNull(type, nameof(type)); | ||
ExceptionUtils.CheckArgumentNotNull(positionalValues, nameof(positionalValues)); | ||
ExceptionUtils.CheckArgumentNotNull(convertFunc, nameof(convertFunc)); | ||
|
||
// 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. | ||
|
@@ -378,6 +440,10 @@ public virtual IEnumerable<KeyValuePair<string, object>> ResolveKeys(IEdmEntityT | |
/// <returns>The resolved key list.</returns> | ||
public virtual IEnumerable<KeyValuePair<string, object>> ResolveKeys(IEdmEntityType type, IDictionary<string, string> namedValues, Func<IEdmTypeReference, string, object> convertFunc) | ||
{ | ||
ExceptionUtils.CheckArgumentNotNull(type, nameof(type)); | ||
ExceptionUtils.CheckArgumentNotNull(namedValues, nameof(namedValues)); | ||
ExceptionUtils.CheckArgumentNotNull(convertFunc, nameof(convertFunc)); | ||
|
||
if (!TryResolveKeys(type, namedValues, convertFunc, out IEnumerable<KeyValuePair<string, object>> resolvedKeys)) | ||
{ | ||
throw ExceptionUtil.CreateBadRequestError(Strings.BadRequest_KeyMismatch(type.FullName())); | ||
|
@@ -507,11 +573,11 @@ internal static ODataUriResolver GetUriResolver(IServiceProvider container) | |
private static IReadOnlyList<T> FindSchemaElements<T>( | ||
IEdmModel model, | ||
string identifier, | ||
Func<NormalizedSchemaElementsCache, string, List<T>> cacheLookupFunc) where T : IEdmSchemaElement | ||
Func<NormalizedModelElementsCache, string, List<T>> cacheLookupFunc) where T : IEdmSchemaElement | ||
{ | ||
if (model.IsImmutable()) | ||
{ | ||
NormalizedSchemaElementsCache cache = GetNormalizedSchemaElementsCache(model); | ||
NormalizedModelElementsCache cache = GetNormalizedModelElementsCache(model); | ||
return cacheLookupFunc(cache, identifier); | ||
} | ||
|
||
|
@@ -545,11 +611,9 @@ private static void FindSchemaElementsInModel<T>(IEdmModel model, string qualifi | |
} | ||
} | ||
|
||
private static NormalizedSchemaElementsCache GetNormalizedSchemaElementsCache(IEdmModel model) | ||
private static NormalizedModelElementsCache GetNormalizedModelElementsCache(IEdmModel model) | ||
{ | ||
Debug.Assert(model != null); | ||
|
||
NormalizedSchemaElementsCache cache = model.GetAnnotationValue<NormalizedSchemaElementsCache>(model); | ||
NormalizedModelElementsCache cache = model.GetAnnotationValue<NormalizedModelElementsCache>(model); | ||
if (cache == null) | ||
{ | ||
// There's a chance 2 or more threads can reach here concurrently | ||
|
@@ -559,7 +623,7 @@ private static NormalizedSchemaElementsCache GetNormalizedSchemaElementsCache(IE | |
// We can avoid this waste by providing a method that user can call manually to build | ||
// the cache before any request is made. But I did not want to add a new method to the public API. | ||
// We revisit this if it turns out to be a problem in practice. | ||
cache = new NormalizedSchemaElementsCache(model); | ||
cache = new NormalizedModelElementsCache(model); | ||
model.SetAnnotationValue(model, cache); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we do the cache in the 'EdmModel'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by cache in the
EdmModel
? Does theEdmModel
have a cache?Also, in this case we're dealing with the
IEdmModel
interface, we can't guarantee whether it's anEdmModel
, aCsdlSemanticsModel
or some other implementation.I've checked around other users in the codebase and in WebAPI, and any time some metadata is added to the model, it's added as an "annotation". In this case this cache will also be an annotation that's bound to the model. It's not a static cache.
Have I answered your question?