Support Instance annotations using annotation container #1259

xuzhg · 2024-06-10T22:48:32Z

Support instance annotation

OData model builder has the interface

If a customer can modify his model (C# class), he can use the model-based instance annotation container provided in OData model builder.

public class EntityTypeWithAnnotation
{
   public int Id { get; set; }
   public IODataInstanceAnnotationContainer InstanceAnnotations { get; set; }
 }

This PR is to:

Enable Instance annotation deserialization
When a request payload contains instance annotation for resource and property, we can read it and convert to the key/value pairs and save them into instance annotation container.
Enable Instance annotation serialization
When a serialize an object which contains instance annotation in the container, we can convert to ODataInstanceAnnotation and save them into InstanceAnnotation collection of ODataResourceBase or ODataPropertyInfo.
We support to read property without value but with instance annotation
We support to query the entity set which contains instance annotation.
for example: ~/customers/1/name
the payload could be:

{
    "@context":"$metadata#Customers",
     "@my.instanceAnnotation":"Smithe" 
    "value": [
    {
        ....
    },
    ....
  ]
}

We support to query the property directly which contains instance annotation.
for example: ~/customers/1/name
the payload could be:

{
    "@context":"$metadata#Customers(1)/Name",
     "@my.instanceAnnotation":"Smithe" 
    "value":"Smith",
}

Enable Instance annotation deserialization Enable property without value

habbes · 2024-07-19T05:11:16Z

src/Microsoft.AspNetCore.OData/Deltas/DeltaOfT.cs

@@ -150,6 +154,20 @@ public override bool TrySetPropertyValue(string name, object value)
                throw Error.ArgumentNull(nameof(name));
            }

+            if (IsInstanceAnnotation(name, out PropertyInfo annotationContainerPropertyInfo))


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.

habbes · 2024-07-19T05:20:02Z

src/Microsoft.AspNetCore.OData/Deltas/DeltaOfT.cs

+            if (desContainer == null)
+            {
+                _instanceAnnotationContainerPropertyInfo.SetValue(targetEntity, sourceContainer);
+                return;


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?

habbes · 2024-07-19T06:08:09Z

src/Microsoft.AspNetCore.OData/Edm/EdmModelExtensions.cs

+                throw Error.ArgumentNull(nameof(model));
+            }
+
+            string[] identifier = annotationIdentifier.Split('#');


Since we only use identifier[0], maybe we should use Substring instead of Split so that we allocate at most one new string.

It could also be faster:

See benchmark:
https://sharpbench.dev/#s:gqF2AaFj2gJpLy8gdmlzaXQgaHR0cHM6Ly9iZW5jaG1hcmtkb3RuZXQub3JnLyBmb3IgbW9yZSBpbmZvIG9uIEJlbmNobWFya0RvdE5ldAoKCnVzaW5nIEJlbmNobWFya0RvdE5ldC5BdHRyaWJ1dGVzOwp1c2luZyBCZW5jaG1hcmtEb3ROZXQuRGlhZ25vc2VyczsKCltNZW1vcnlEaWFnbm9zZXJdCnB1YmxpYyBjbGFzcyBCZW5jaG1hcmtzCnsKICAgIHByaXZhdGUgc3RyaW5nIGFubm90YXRpb24gPSAic29tZS5uYW1lLnNwYWNlLmlkZW50aWZpZXIjcXVhbGlmaWVyIjsKCiAgICBbQmVuY2htYXJrXQogICAgcHVibGljIHN0cmluZyBHZXRJZGVudGlmaWVyV2l0aFNwbGl0KCkKICAgIHsKICAgICAgICB2YXIgaWRlbnRpZmllciA9IGFubm90YXRpb24uU3BsaXQoJyMnKTsKICAgICAgICByZXR1cm4gaWRlbnRpZmllclswXTsKICAgIH0KCiAgICBbQmVuY2htYXJrXQogICAgcHVibGljIHN0cmluZyBHZXRJZGVudGlmaWVyV2l0aFN1YnN0cmluZygpCiAgICB7CiAgICAgICAgdmFyIGluZGV4ID0gYW5ub3RhdGlvbi5JbmRleE9mKCcjJyk7CiAgICAgICAgdmFyIGlkZW50aWZpZXIgPSBhbm5vdGF0aW9uLlN1YnN0cmluZygwLCBpbmRleCk7CiAgICAgICAgcmV0dXJuIGlkZW50aWZpZXI7CiAgICB9Cn0KCgo=

habbes · 2024-07-19T06:10:45Z

src/Microsoft.AspNetCore.OData/Edm/EdmModelExtensions.cs

+
+            string[] identifier = annotationIdentifier.Split('#');
+
+            IEdmTerm term = model.FindTerm(identifier[0]);


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

habbes · 2024-07-19T06:15:42Z

src/Microsoft.AspNetCore.OData/Edm/EdmModelExtensions.cs

+                terms = terms.Concat(refedTerms);
+            }
+
+            if (terms.Count() > 1)


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

habbes · 2024-07-19T06:17:56Z

src/Microsoft.AspNetCore.OData/Edm/EdmModelExtensions.cs

+                throw new ODataException(Error.Format(SRResources.AmbiguousTypeNameFound, termName));
+            }
+
+            return terms.SingleOrDefault();


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.

habbes · 2024-07-19T06:35:27Z

src/Microsoft.AspNetCore.OData/Edm/EdmModelExtensions.cs

+            // 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);
+            }


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

habbes · 2024-07-19T06:38:30Z

src/Microsoft.AspNetCore.OData/Formatter/Deserialization/DeserializationHelper.cs

@@ -276,28 +277,30 @@ internal static void SetProperty(object resource, string propertyName, object va
        internal static object ConvertValue(object oDataValue, ref IEdmTypeReference propertyType, IODataDeserializerProvider deserializerProvider,
            ODataDeserializerContext readContext, out EdmTypeKind typeKind)
        {
-            if (oDataValue == null)
+            if (oDataValue == null || oDataValue is ODataNullValue)


Why didn't we check ODataNullValue before? Was this a bug?

habbes · 2024-07-19T06:42:12Z

src/Microsoft.AspNetCore.OData/Edm/EdmModelAnnotationExtensions.cs

+            }
+
+            InstanceAnnotationContainerAnnotation annotation =
+                edmModel.GetAnnotationValue<InstanceAnnotationContainerAnnotation>(edmType);


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.

habbes · 2024-07-19T06:56:28Z

src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs

@@ -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";


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";

habbes · 2024-07-19T07:00:58Z

src/Microsoft.AspNetCore.OData/Formatter/Deserialization/DeserializationHelper.cs

+            // 05/01/2024: new scenario, if we specify an instance annotation using the collection value as:
+            // ""[email protected]"":[""Skyline"",7,""Beaver""],
+            // ODL generates 'ODataCollectionValue' without providing the 'TypeName' on ODataCollectionValue.
+            // So the above assumption could not be correct again.


If the above assumption is no longer correct due to the changes introduced, then we should also update the above comment to reflect the new behaviour.

habbes · 2024-07-19T07:27:58Z

src/Microsoft.AspNetCore.OData/Formatter/Deserialization/ODataDeserializerContext.cs

@@ -128,5 +132,78 @@ internal ODataDeserializerContext CloneWithoutType()
                TimeZone = this.TimeZone
            };
        }
+
+        private CachedItem _cached = new CachedItem();


Is this cache only intended for use with the instance annotations container? If so, then maybe it would be better to give it a more specific name. Also, maybe it should moved up where the other private fields are located.

habbes · 2024-07-19T12:25:57Z