Add IODataTypeMapper by xuzhg · Pull Request #342 · OData/AspNetCoreOData

xuzhg · 2021-10-21T23:34:00Z

In the existing implementation, customers can't customize the OData CLR type and Edm type mapping.

This PR introduces IODataTypeMapper and its default implementation DefaultODataTypeMapper.

The developer can create his own type of mapper to override it.

corranrogue9 · 2021-10-22T04:40:38Z

+        /// <summary>
+        /// Creates a static instance for the Default type mapper.
+        /// </summary>
+        internal static DefaultODataTypeMapper Default = new DefaultODataTypeMapper();


It would probably be good to mark this readonly. Also, I think usually these singletons are properties, but I guess it's fine since this is internal? #WontFix

Thanks. I keep it unchanged.

corranrogue9 · 2021-10-22T04:49:48Z

+        /// The default mapping between Edm primitive type and Clr primitive type.
+        /// Primitive types are cross Edm models.
+        /// </summary>
+        private static ConcurrentDictionary<Type, IEdmPrimitiveTypeReference> ClrPrimitiveTypes


I'm coming from pretty old .NET versions as this point (4.6 and earlier) so a lot might have changed, but I recall there being lots of overhead to using ConcurrentDictionary compared to Dictionary. I see that we want to be thread-safe, particularly because these are static, but they are only ever mutated during the static constructor. According to this article, we can trust the CLR to only call the static constructor once, meaning that modifications to these dictionaries will not be happening concurrently. Further, because the thread-safety document for Dictionary indicates that reads are thread-safe on instances, I believe it would be best for performance reasons to use Dictionary here instead of ConcurrentDictionary. #Resolved

Changed to IDictionary<T,T>. Thanks.

corranrogue9 · 2021-10-22T05:01:26Z

+                    edmModel
+                    .SchemaElements
+                    .OfType<IEdmType>()
+                    .Select(edmType => new { EdmType = edmType, Annotation = edmModel.GetAnnotationValue<ClrTypeAnnotation>(edmType) })


I'm not sure if this is "nit" or not, but I think the Where should come before the Select. Doing so, we don't lose track of edmType at all (so we maintain functionality from what I can tell), we prevent the allocation (and therefore garbage collection as well) of these anonymous types, and we prevent the extra Select later that also requires dereferencing the EdmType property. #Pending

where clause needs the "Annotation" calculation from Select clause. So, i think it could be infeasible to switch them. And These codes are from existing codes. I don't want to add more perf in a feature request PR. If it's ok, we can improve them in the next Perf improvement PR?

corranrogue9 · 2021-10-22T05:03:43Z

+                    .Select(edmType => new { EdmType = edmType, Annotation = edmModel.GetAnnotationValue<ClrTypeAnnotation>(edmType) })
+                    .Where(tuple => tuple.Annotation != null && tuple.Annotation.ClrType == clrType)
+                    .Select(tuple => tuple.EdmType)
+                    .SingleOrDefault();


Do we have a reasonable expectation that there can never be more than one element found? I ask because SingleOrDefault will still throw (rather than return null) if there is more than one element found. #WontFix

Each EdmType has 0 or 1 'ClrTypeAnnotation' associated.
So, here only returns 'null' or the found returnType.

corranrogue9 · 2021-10-22T05:11:53Z

+            string typeName = edmSchemaType.FullName();
+            IEnumerable<Type> matchingTypes = GetMatchingTypes(typeName, assembliesResolver);
+
+            if (matchingTypes.Count() > 1)


I'm seeing this type of thing a bit. We might be able to reasonably assume that there are few elements in the sequence, but I don't know why we should take that risk. I suggest we have an internal extension (since we are already going to be enumerating here for Count) like:

public static bool AtLeast<T>(this IEnumerable<T> source, int count) { using (var enumerator = source.GetEnumerator()) { for (int i = 0; i < count; ++i) { if (!enumerator.MoveNext()) { return false; } } return true; } }

Where it can act like Any for cases greater than 0 (e.g. AtLeast(1) is equivalent to Any, and here we could say AtLeast(2))

I intend to do something like this for the issue I created last week, so if you want to keep this for now, I can go ahead and incorporate that change when I have it. What do you think? #WontFix

Thanks. I don't like the current implementation. And I'd like to change it in your PR together.

corranrogue9 · 2021-10-22T05:13:28Z

+
+            if (matchingTypes.Count() > 1)
+            {
+                throw Error.Argument("edmTypeReference", SRResources.MultipleMatchingClrTypesForEdmType,


I think this should be an InvalidOperationException rather than ArgumentException #Resolved

corranrogue9 · 2021-10-22T05:15:45Z

+
+        private TypeCacheItem GetOrCreateCacheItem(IEdmModel model)
+        {
+            if (!_cache.TryGetValue(model, out TypeCacheItem map))


Since _cache is ConcurrentDictionary, can't we use the GetOrAdd method? Is there some overhead I'm not aware of that we are trying to avoid? #Resolved

corranrogue9 · 2021-10-22T05:21:51Z

+
+        private static Type ExtractGenericInterface(Type queryType, Type interfaceType)
+        {
+            Func<Type, bool> matchesInterface = t => t.IsGenericType && t.GetGenericTypeDefinition() == interfaceType;


I don't have a proposed fix, but I do know what is written here has potential memory issues because of some maths concepts I don't fully understand about closures (probably @chrisspre knows) and the fact that interfaceType is a parameter. What happens in .NET is that a new Func instance needs to be created for every interfaceType that this method is called with, and if this is a long-lived object, that expands memory a bunch because those Funcs never go out of scope.

If this is the only way we can achieve this functionality, that's fine, but if you can figure out a different way to write this, it would almost certainly be preferable. #WontFix

Thanks for your points.
I'd like to keep them unchanged now since that's the existing codes. We'd make a better way in the next perf PR.

corranrogue9 · 2021-10-22T22:30:09Z

+        /// <summary>
+        /// <see cref="Type"/> to <see cref="IEdmTypeReference"/>.
+        /// </summary>
+        public ConcurrentDictionary<Type, IEdmTypeReference> ClrToEdmTypeCache = new ConcurrentDictionary<Type, IEdmTypeReference>();


This should almost certainly be readonly or a property right? #Resolved

changed to property.

corranrogue9 · 2021-10-22T22:32:08Z

+            Assert.True(primitiveTypeReference.IsNullable);
+        }
+
+        //[Theory]


Why is this commented out? #Resolved

IsNonstandardEdmPrimitive is an extension method on IEdmModel. So, the test case moved to "EdmClrTypeMapExtensionsTests".

Forgot to clean it up.

corranrogue9

I'm approving on the basis of the design that the cache itself will certainly be beneficial in its own right, but I do think that the other performance feedback I gave would be good to incorporate as well.

xuzhg requested review from corranrogue9, gathogojr, habbes and mikepizzo October 21, 2021 23:34

corranrogue9 reviewed Oct 22, 2021

View reviewed changes

corranrogue9 approved these changes Oct 22, 2021

View reviewed changes

corranrogue9 self-requested a review October 27, 2021 00:54

xuzhg added 2 commits November 1, 2021 09:25

Add IODataTypeMapper

92edec5

Resolve the comments

9df1b4c

xuzhg force-pushed the TypeMapping branch from 477f30c to 9df1b4c Compare November 1, 2021 17:34

xuzhg merged commit b30a9b3 into master Nov 1, 2021

renovate Bot mentioned this pull request Nov 10, 2021

chore(*): Update dependency Microsoft.AspNetCore.OData to v8.0.4 Liu233w/acm-statistics#2295

Merged

1 task

This was referenced Apr 29, 2026

Bump Microsoft.AspNetCore.OData from 7.5.1 to 8.3.1 pipeline-foundation/aspnet-api-versioning#8

Open

Bump Microsoft.AspNetCore.OData from 7.5.8 to 8.3.1 peteraritchie/aspnet-api-versioning#7

Open

Uh oh!

Conversation

xuzhg commented Oct 21, 2021

Uh oh!

corranrogue9 Oct 22, 2021 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranrogue9 Oct 22, 2021 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranrogue9 Oct 22, 2021 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranrogue9 Oct 22, 2021 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranrogue9 Oct 22, 2021 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranrogue9 Oct 22, 2021 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranrogue9 Oct 22, 2021 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranrogue9 Oct 22, 2021 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranrogue9 Oct 22, 2021 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranrogue9 Oct 22, 2021 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranrogue9 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

corranrogue9 Oct 22, 2021 •

edited by xuzhg

Loading

corranrogue9 Oct 22, 2021 •

edited by xuzhg

Loading

corranrogue9 Oct 22, 2021 •

edited by xuzhg

Loading

corranrogue9 Oct 22, 2021 •

edited by xuzhg

Loading

corranrogue9 Oct 22, 2021 •

edited by xuzhg

Loading

corranrogue9 Oct 22, 2021 •

edited by xuzhg

Loading

corranrogue9 Oct 22, 2021 •

edited by xuzhg

Loading

corranrogue9 Oct 22, 2021 •

edited by xuzhg

Loading

corranrogue9 Oct 22, 2021 •

edited by xuzhg

Loading

corranrogue9 Oct 22, 2021 •

edited by xuzhg

Loading