Skip to content
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

Support Instance annotations using annotation container #1259

Open
wants to merge 3 commits into
base: release-8.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions src/Microsoft.AspNetCore.OData/Deltas/DeltaOfT.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using System.Runtime.Serialization;
using Microsoft.AspNetCore.OData.Abstracts;
using Microsoft.AspNetCore.OData.Common;
using Microsoft.OData.ModelBuilder;

namespace Microsoft.AspNetCore.OData.Deltas
{
Expand Down Expand Up @@ -47,6 +48,9 @@ private static readonly ConcurrentDictionary<Type, Dictionary<string, PropertyAc
private HashSet<string> _changedDynamicProperties;
private IDictionary<string, object> _dynamicDictionaryCache;

private PropertyInfo _instanceAnnotationContainerPropertyInfo;
private IODataInstanceAnnotationContainer _instanceAnnotationContainer;

/// <summary>
/// Initializes a new instance of <see cref="Delta{T}"/>.
/// </summary>
Expand Down Expand Up @@ -150,6 +154,20 @@ public override bool TrySetPropertyValue(string name, object value)
throw Error.ArgumentNull(nameof(name));
}

if (IsInstanceAnnotation(name, out PropertyInfo annotationContainerPropertyInfo))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned about the performance impact of this check. It would be good to run some profiling to verify whether this call adds significant overhead. Here's why I'm concerned:

  • The method IsInstanceAnnotation is called each time we set a property value. Potentially this means we call it for each property at least once for each PUT/PATCH request that uses Delta<T>.
  • The IsInstanceAnnotation method scans all the properties. So if TrySetPropertyValue is called for each property, then we have an O(n^2) operation which could be costly if the entity has a lot of properties
  • We call IsInstanceAnnotation even if the _instanceAnnotationsContainer has already been found. I think we only expect one instance annotations container property in the entity class. If there are multiple, I think we should only select the first one. So we could add a condition if (_instanceAnnotationContainer == null && IsInstanceAnnotation(...)).
  • This approach will be most expensive when the class doesn't have an annotations container. Meaning the people who don't use the feature pay biggest price (should measure the cost to see whether this would actually be an issue in practice)

When adding new opt-in features, I think we should consider as much as possible ways to minimize or eliminate the cost for people who don't use the feature. Maybe we can consider computing only once whether this type has an instance annotations container and cache it somewhere where it's fast to lookup. Since Delta<T> is used during deserialization, is there a flag we can set during deserialization when we detect instance annotations? And use that flag to determine whether the contain exists? Not sure if that's a feasible approach.

{
IODataInstanceAnnotationContainer annotationContainer = value as IODataInstanceAnnotationContainer;
if (value != null && annotationContainer == null)
{
return false;
}

annotationContainerPropertyInfo.SetValue(_instance, annotationContainer);
_instanceAnnotationContainer = annotationContainer;
_instanceAnnotationContainerPropertyInfo = annotationContainerPropertyInfo;
return true;
}

if (_dynamicDictionaryPropertyinfo != null)
{
// Dynamic property can have the same name as the dynamic property dictionary.
Expand Down Expand Up @@ -374,6 +392,8 @@ public void CopyChangedValues(T original)

CopyChangedDynamicValues(original);

CopyInstanceAnnotations(original);

// For nested resources.
foreach (string nestedResourceName in _deltaNestedResources.Keys)
{
Expand Down Expand Up @@ -685,6 +705,44 @@ private static bool IsIgnoredProperty(bool isTypeDataContract, PropertyInfo prop
return propertyInfo.GetCustomAttributes(typeof(IgnoreDataMemberAttribute), inherit: true).Any();
}

private void CopyInstanceAnnotations(T targetEntity)
{
if (_instanceAnnotationContainerPropertyInfo == null)
{
return;
}

IODataInstanceAnnotationContainer sourceContainer =
_instanceAnnotationContainerPropertyInfo.GetValue(_instance) as IODataInstanceAnnotationContainer;
if (sourceContainer == null)
{
return;
}

IODataInstanceAnnotationContainer desContainer =
_instanceAnnotationContainerPropertyInfo.GetValue(targetEntity) as IODataInstanceAnnotationContainer;
if (desContainer == null)
{
_instanceAnnotationContainerPropertyInfo.SetValue(targetEntity, sourceContainer);
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the source container gets modified after this, (e.g. new items added to it), the changes will also affect the destination container since they'll be references to the same instance. Is this expected behaviour? Is it something we should worry about? Or is it a non-issue?

}

foreach (var item in sourceContainer.InstanceAnnotations)
{
foreach (var annotation in item.Value)
{
if (item.Key == null || item.Key == string.Empty)
{
desContainer.AddResourceAnnotation(annotation.Key, annotation.Value);
}
else
{
desContainer.AddPropertyAnnotation(item.Key, annotation.Key, annotation.Value);
}
}
}
}

// Copy changed dynamic properties and leave the unchanged dynamic properties
private void CopyChangedDynamicValues(T targetEntity)
{
Expand Down Expand Up @@ -842,5 +900,24 @@ private bool TrySetNestedResourceInternal(string name, object deltaNestedResourc

return true;
}

private bool IsInstanceAnnotation(string name, out PropertyInfo propertyInfo)
{
propertyInfo = null;
if (!_allProperties.TryGetValue(name, out PropertyAccessor<T> propertyAccessor))
{
return false;
}

propertyInfo = propertyAccessor.Property;

if (propertyInfo.PropertyType == typeof(ODataInstanceAnnotationContainer) ||
typeof(IODataInstanceAnnotationContainer).IsAssignableFrom(propertyInfo.PropertyType))
{
return true;
}

return false;
}
}
}
28 changes: 28 additions & 0 deletions src/Microsoft.AspNetCore.OData/Edm/EdmModelAnnotationExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,34 @@ public static PropertyInfo GetDynamicPropertyDictionary(this IEdmModel edmModel,
return null;
}

/// <summary>
/// Gets the instance annotation container property info.
/// </summary>
/// <param name="edmModel">The Edm model.</param>
/// <param name="edmType">The Edm type.</param>
/// <returns>The instance annotation container property info.</returns>
public static PropertyInfo GetInstanceAnnotationsContainer(this IEdmModel edmModel, IEdmStructuredType edmType)
{
if (edmModel == null)
{
throw Error.ArgumentNull(nameof(edmModel));
}

if (edmType == null)
{
throw Error.ArgumentNull(nameof(edmType));
}

InstanceAnnotationContainerAnnotation annotation =
edmModel.GetAnnotationValue<InstanceAnnotationContainerAnnotation>(edmType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we can figure out way to avoid this pattern because this has become a constant perf problem in OData. It's also a usability issue because customers wouldn't know which annotations need to be registered for which feature and we don't have good docs for it. I will investigate possible alternatives and share findings with the team.

if (annotation != null)
{
return annotation.PropertyInfo;
}

return null;
}

/// <summary>
/// Gets the model name.
/// </summary>
Expand Down
69 changes: 61 additions & 8 deletions src/Microsoft.AspNetCore.OData/Edm/EdmModelExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,15 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Security.AccessControl;
using Microsoft.AspNetCore.OData.Formatter.Deserialization;
using Microsoft.AspNetCore.OData.Formatter.Wrapper;
using Microsoft.AspNetCore.OData.Routing.Template;
using Microsoft.OData;
using Microsoft.OData.Edm;
using Microsoft.OData.Edm.Csdl;
using Microsoft.OData.Edm.Validation;
using Microsoft.OData.Edm.Vocabularies;
using Microsoft.OData.UriParser;

namespace Microsoft.AspNetCore.OData.Edm
Expand Down Expand Up @@ -61,32 +65,36 @@ public static IEdmCollectionTypeReference ResolveResourceSetType(this IEdmModel
/// Resolve the type reference from the type name of <see cref="ODataResourceBase"/>
/// </summary>
/// <param name="model">The Edm model.</param>
/// <param name="resource">The given resource.</param>
/// <param name="resourceWrapper">The given resource wrapper.</param>
/// <returns>The resolved type.</returns>
public static IEdmStructuredTypeReference ResolveResourceType(this IEdmModel model, ODataResourceBase resource)
public static IEdmStructuredTypeReference ResolveResourceType(this IEdmModel model, ODataResourceWrapper resourceWrapper)
{
if (model == null)
{
throw Error.ArgumentNull(nameof(model));
}

if (resource == null)
if (resourceWrapper == null)
{
throw Error.ArgumentNull(nameof(resource));
throw Error.ArgumentNull(nameof(resourceWrapper));
}

string typeName = resourceWrapper.IsResourceValue ?
resourceWrapper.ResourceValue.TypeName :
resourceWrapper.Resource.TypeName;

IEdmStructuredTypeReference resourceType;
if (string.IsNullOrEmpty(resource.TypeName) ||
string.Equals(resource.TypeName, "Edm.Untyped", StringComparison.OrdinalIgnoreCase))
if (string.IsNullOrEmpty(typeName) ||
string.Equals(typeName, "Edm.Untyped", StringComparison.OrdinalIgnoreCase))
{
resourceType = EdmUntypedStructuredTypeReference.NullableTypeReference;
}
else
{
IEdmStructuredType actualType = model.FindType(resource.TypeName) as IEdmStructuredType;
IEdmStructuredType actualType = model.FindType(typeName) as IEdmStructuredType;
if (actualType == null)
{
throw new ODataException(Error.Format(SRResources.ResourceTypeNotInModel, resource.TypeName));
throw new ODataException(Error.Format(SRResources.ResourceTypeNotInModel, typeName));
}

if (actualType is IEdmEntityType actualEntityType)
Expand All @@ -102,6 +110,51 @@ public static IEdmStructuredTypeReference ResolveResourceType(this IEdmModel mod
return resourceType;
}

/// <summary>
/// Resolve the term using the annotation identifier.
/// </summary>
/// <param name="model">The Edm model.</param>
/// <param name="annotationIdentifier"> It consists of the namespace or alias of the schema that defines the term, followed by a dot (.),
/// followed by the name of the term, optionally followed by a hash (#) and a qualifier.</param>
/// <returns>The resolved term or null if not found.</returns>
public static IEdmTerm ResolveTerm(this IEdmModel model, string annotationIdentifier)
{
if (model == null)
{
throw Error.ArgumentNull(nameof(model));
}

string[] identifier = annotationIdentifier.Split('#');

IEdmTerm term = model.FindTerm(identifier[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it sucks that we have to perform a new string allocation just so we can lookup a term using a substring. It would be great if model.FindTerm would support ReadOnlySpan<char> so we can avoid string allocation in such cases. We have an open issue for that: OData/odata.net#2801

if (term != null)
{
return term;
}

// TODO: Let's support namespace alias when we get requirement and ODL publics 'ReplaceAlias' extension method.
// identifier = model.ReplaceAlias(identifier);

string termName = identifier[0];
var terms = model.SchemaElements.OfType<IEdmTerm>()
.Where(e => string.Equals(termName, e.FullName(), StringComparison.OrdinalIgnoreCase));

foreach (var refModels in model.ReferencedModels)
{
var refedTerms = refModels.SchemaElements.OfType<IEdmTerm>()
.Where(e => string.Equals(termName, e.FullName(), StringComparison.OrdinalIgnoreCase));

terms = terms.Concat(refedTerms);
}
Comment on lines +135 to +148
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the kind of thing that should be implemented in EdmLib and provided as single method call. It is also expensive to scan all the referenced models if we have no term. Is this meant to support case-insensitive lookups? Should we also do case-insensitive lookups or is that something that the customer enables via configuration?

In ODL we created a cache for case-insensitive lookups of model elements (including IEdmTerm) for use with the case-insensitive ODataUriResolver. We could consider refactoring the cache and using it in more scenario. It was designed to avoid the cost of scanning the schemas frequently, which was causing a lot of CPU usage in workloads:
OData/odata.net#2610


if (terms.Count() > 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

terms.Count() will scan the whole collection. If we want to find duplicates, maybe we should use terms.Take(2).Count().

{
throw new ODataException(Error.Format(SRResources.AmbiguousTypeNameFound, termName));
}

return terms.SingleOrDefault();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single ensures that the returned item is unique, but we have already checked for duplicates in the block above. I think it's redundant to have both checks.

}

/// <summary>
/// Get all property names for the given structured type.
/// </summary>
Expand Down
5 changes: 5 additions & 0 deletions src/Microsoft.AspNetCore.OData/Edm/EdmUntypedHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,13 @@ namespace Microsoft.AspNetCore.OData.Edm
{
internal class EdmUntypedHelpers
{
// Collection(Edm.Untyped) for resource set
public readonly static EdmCollectionTypeReference NullableUntypedCollectionReference
= new EdmCollectionTypeReference(
new EdmCollectionType(EdmUntypedStructuredTypeReference.NullableTypeReference));

// Collection(Edm.Untyped) for collection of (Primitive, enum)
public readonly static EdmCollectionTypeReference NullablePrimitiveUntypedCollectionReference
= new EdmCollectionTypeReference(new EdmCollectionType(EdmCoreModel.Instance.GetUntyped()));
}
}
38 changes: 38 additions & 0 deletions src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ namespace Microsoft.AspNetCore.OData.Extensions
/// </summary>
public static class HttpRequestExtensions
{
private static readonly string ODataInstanceAnnotationContainerKey = "odataInstanceAnnotation_14802D58-69EF-4B28-9BDC-963D3648F06A";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would const be better here since the value is known at compile-time? (I assume we'll never change this)

Suggested change
private static readonly string ODataInstanceAnnotationContainerKey = "odataInstanceAnnotation_14802D58-69EF-4B28-9BDC-963D3648F06A";
private const string ODataInstanceAnnotationContainerKey = "odataInstanceAnnotation_14802D58-69EF-4B28-9BDC-963D3648F06A";


/// <summary>
/// Returns the <see cref="IODataFeature"/> from the DI container.
/// </summary>
Expand Down Expand Up @@ -86,6 +88,42 @@ public static IEdmModel GetModel(this HttpRequest request)
return request.ODataFeature().Model;
}

/// <summary>
/// Set the top-level instance annotations for the request.
/// </summary>
/// <param name="request">The <see cref="HttpRequest"/> instance to extend.</param>
/// <param name="instanceAnnotations">The instance annotations</param>
public static HttpRequest SetInstanceAnnotations(this HttpRequest request, IDictionary<string, object> instanceAnnotations)
{
IODataFeature odataFeature = request.ODataFeature();

// The last wins.
odataFeature.RoutingConventionsStore[ODataInstanceAnnotationContainerKey] = instanceAnnotations;

return request;
}

/// <summary>
/// Get the top-level instance annotations for the request.
/// </summary>
/// <param name="request">The <see cref="HttpRequest"/> instance to extend.</param>
/// <returns>null or top-level instance annotations.</returns>
public static IDictionary<string, object> GetInstanceAnnotations(this HttpRequest request)
{
if (request == null)
{
return null;
}

IODataFeature odataFeature = request.ODataFeature();
if (!odataFeature.RoutingConventionsStore.TryGetValue(ODataInstanceAnnotationContainerKey, out object annotations))
{
return null;
}

return annotations as IDictionary<string, object>;
}

/// <summary>
/// Gets the <see cref="TimeZoneInfo"/> setting.
/// </summary>
Expand Down
Loading