Support self referencing type substitution #382

LuukN2 · 2018-10-24T14:43:47Z

Thanks for the substitution fixes.

However, there was still a small recursion issue when a model references itself on a property. (Or when two models reference each other, etc.)
The program would get stuck in a loop getting the structured type of the properties causing a stackoverflow exception.
Now it keeps track of what types it has already substituted in a dictionary and passes these values instead of recreating the structured type again.

Thanks again for your hard work, your project is awesome!

…erties.

msftclas · 2018-10-24T14:43:58Z

All CLA requirements met.

commonsensesoftware · 2018-10-24T19:57:06Z

This is great! I suspected there were still some edge cases I was missing. This is a really convenient feature to use, but it's not so easy to implement. Let me go through this and we'll get it merged.

commonsensesoftware · 2018-10-24T19:59:37Z

src/Common.OData.ApiExplorer/AspNet.OData/DefaultModelTypeBuilder.cs

+                    continue;
+                }
+
+                passedTypes.Add(structuredTypeRef.ToString(), propertyType);


Let's use FullName() here instead of ToString() (unless you know better). It looks like the implementation of ToString is ToTraceString, which may not be the same. IMO FullName() also gives better semantic meaning to what the code is doing here.

commonsensesoftware · 2018-10-24T20:01:02Z

src/Common.OData.ApiExplorer/AspNet.OData/DefaultModelTypeBuilder.cs

@@ -66,6 +67,14 @@ public Type NewStructuredType( IEdmStructuredType structuredType, Type clrType,
                var propertyType = property.PropertyType;
                var structuredTypeRef = structuralProperty.Type;

+                if ( passedTypes.TryGetValue( structuredTypeRef.ToString(), out var passedType ) )


Let's capture the type name so we don't have to build it two times. Something like:

var typeName = structuredTypeRef.FullName();

This obviously supplanted by the struct key or hash code if you refactor

commonsensesoftware · 2018-10-24T20:10:15Z

src/Common.OData.ApiExplorer/AspNet.OData/DefaultModelTypeBuilder.cs

@@ -30,6 +30,7 @@ public sealed class DefaultModelTypeBuilder : IModelTypeBuilder
        readonly ICollection<Assembly> assemblies;
        readonly ConcurrentDictionary<ApiVersion, ModuleBuilder> modules = new ConcurrentDictionary<ApiVersion, ModuleBuilder>();
        readonly ConcurrentDictionary<ClassSignature, Type> generatedTypes = new ConcurrentDictionary<ClassSignature, Type>();
+        private readonly Dictionary<string, Type> passedTypes = new Dictionary<string, Type>();


nit: let's use visitedTypes as it's more consistent with the pattern being used

commonsensesoftware · 2018-10-24T20:14:14Z

src/Common.OData.ApiExplorer/AspNet.OData/DefaultModelTypeBuilder.cs

@@ -66,6 +67,14 @@ public Type NewStructuredType( IEdmStructuredType structuredType, Type clrType,
                var propertyType = property.PropertyType;
                var structuredTypeRef = structuralProperty.Type;

+                if ( passedTypes.TryGetValue( structuredTypeRef.ToString(), out var passedType ) )
+                {
+                    properties.Add(new ClassProperty(property, passedType));


nit: the spacing is off by my rules, but this should be flagged by code analysis and auto-formatted by the .editorconfig settings. correcting this should be as easy as formatting the document.

Definitely let me know you don't see CA warnings and/or formatting the document doesn't fix all of these. You're not supposed to have to guess or do a lot of work to format things the way I want them to be. ;)

It formats for methods, but I had to manually add the formatting rules for nameof, typeof etc.
I'm using Jetbrains Rider if that means anything.

commonsensesoftware · 2018-10-24T20:15:02Z

...rosoft.AspNet.OData.Versioning.ApiExplorer.Tests/AspNet.OData/DefaultModelTypeBuilderTest.cs

+
+            // assert
+            subsitutedType.GetRuntimeProperties().Should().HaveCount( 3 );
+            subsitutedType.Should().HaveProperty<int>( nameof(Company.CompanyId) );


nit: spacing; format of the document should fix them all

commonsensesoftware · 2018-10-24T20:15:20Z

...ft.AspNetCore.OData.Versioning.ApiExplorer.Tests/AspNet.OData/DefaultModelTypeBuilderTest.cs

+            var person = modelBuilder.EntitySet<Company>( "Companies" ).EntityType;
+
+            var context = NewContext( modelBuilder.GetEdmModel() );
+            var originalType = typeof(Company);


nit: spacing; format of the document should fix them all

commonsensesoftware · 2018-10-24T20:17:26Z

...Data.Versioning.ApiExplorer.Tests/Microsoft.AspNet.OData.Versioning.ApiExplorer.Tests.csproj

@@ -1,23 +1,18 @@
 <Project Sdk="Microsoft.NET.Sdk">


This file doesn't seem to have actually been changed. It appears that only the extra lines have been removed. It's not a huge deal, but there's not much sense in changing a file that doesn't need changing.

commonsensesoftware · 2018-10-24T20:26:28Z

src/Common.OData.ApiExplorer/AspNet.OData/DefaultModelTypeBuilder.cs

@@ -30,6 +30,7 @@ public sealed class DefaultModelTypeBuilder : IModelTypeBuilder
        readonly ICollection<Assembly> assemblies;
        readonly ConcurrentDictionary<ApiVersion, ModuleBuilder> modules = new ConcurrentDictionary<ApiVersion, ModuleBuilder>();
        readonly ConcurrentDictionary<ClassSignature, Type> generatedTypes = new ConcurrentDictionary<ClassSignature, Type>();
+        private readonly Dictionary<string, Type> passedTypes = new Dictionary<string, Type>();


In thinking about this, I'm pretty sure that using string alone for the key is not going to work because there can be a distinctly generated type by API version. This should mean that the key for the corresponding EDM type should be a combination of EDM type and API version; otherwise, it's possible that the wrong generated type will be picked.

For example, imagine that Default.Company is generated for API version 2.0 and then it's requested again for API version 1.0. If the models aren't exactly the same (which they probably aren't), the wrong selection is made.

There's two ways this can be handled:

Add a hashable struct, something like VisitedTypeKey

Use int as the key and add a hashing function

Option 2 might look like:

static int GetVisitedTypeKey( IEdmTypeReference type, ApiVersion apiVersion ) => ( type.FullName().GetHashCode() * 397 ) ^ apiVersion.GetHashCode();

commonsensesoftware · 2018-10-24T20:31:35Z

src/Common.OData.ApiExplorer/AspNet.OData/DefaultModelTypeBuilder.cs

+                    continue;
+                }
+
+                passedTypes.Add(structuredTypeRef.ToString(), propertyType);


Adding the visited type needs to be added toward the end; probably after line 102. The propertyType variable can be mutated after this line, which means the wrong type will be stored in the dictionary.

commonsensesoftware · 2018-10-25T17:52:19Z

Thanks @LuukN2 for your contribution; really - it was useful. I'm going to close this one out because it turns out there's quite a bit more going on than just these rather basic fixes. Your change does address the basic recursion problem, but it completely misses recursively emitting a self-referencing type when it's actually substituted. I realized that this was missing when incorporating your unit tests. I even had to do some research because I had no idea how to actually build a type like that. I also expanded your scenario to also include the property List<Company> Subsidiaries { get; }, which should be substituted as IEnumerable<TDeclaringType> Subsidiaries { get; } when required.

I've incorporated your bits and all the additional required changes into #384 that I already had going. The last commit has all of the changes relevant to this PR. I'll leave #384 open for a bit so you have some time to review and comment if you want.

Thanks again for this submission. I don't think I would have remembered about this use case otherwise.

LuukN2 added 4 commits October 24, 2018 15:31

Support substitution of types with self-references.

7e87848

Support substitution of types with self-references.

52cd033

Added test cases for substitution of types with self-referencing prop…

566a133

…erties.

Spacing fixes.

bc6b151

commonsensesoftware requested changes Oct 24, 2018

View reviewed changes

commonsensesoftware pushed a commit that referenced this pull request Oct 25, 2018

Support self-referencing type substitution. Related to #382

6699d96

commonsensesoftware closed this Oct 25, 2018

commonsensesoftware pushed a commit that referenced this pull request Oct 26, 2018

Support self-referencing type substitution. Related to #382

f97406c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support self referencing type substitution #382

Support self referencing type substitution #382

LuukN2 commented Oct 24, 2018

msftclas commented Oct 24, 2018 •

edited

Loading

commonsensesoftware commented Oct 24, 2018

commonsensesoftware Oct 24, 2018

commonsensesoftware Oct 24, 2018

commonsensesoftware Oct 24, 2018

commonsensesoftware Oct 24, 2018

commonsensesoftware Oct 24, 2018

LuukN2 Oct 25, 2018

commonsensesoftware Oct 24, 2018

commonsensesoftware Oct 24, 2018

commonsensesoftware Oct 24, 2018

commonsensesoftware Oct 24, 2018

commonsensesoftware Oct 24, 2018

commonsensesoftware commented Oct 25, 2018

Support self referencing type substitution #382

Support self referencing type substitution #382

Conversation

LuukN2 commented Oct 24, 2018

msftclas commented Oct 24, 2018 • edited Loading

commonsensesoftware commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

commonsensesoftware commented Oct 25, 2018

msftclas commented Oct 24, 2018 •

edited

Loading