Skip to content

Support self referencing type substitution #382

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Support self referencing type substitution #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7e87848
Support substitution of types with self-references.
LuukN2 Oct 24, 2018
52cd033
Support substitution of types with self-references.
LuukN2 Oct 24, 2018
566a133
Added test cases for substitution of types with self-referencing prop…
LuukN2 Oct 24, 2018
bc6b151
Spacing fixes.
LuukN2 Oct 24, 2018
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
9 changes: 9 additions & 0 deletions src/Common.OData.ApiExplorer/AspNet.OData/DefaultModelTypeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

  1. Add a hashable struct, something like VisitedTypeKey
  2. 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();


/// <summary>
/// Initializes a new instance of the <see cref="DefaultModelTypeBuilder"/> class.
Expand Down Expand Up @@ -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 ) )
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

var typeName = structuredTypeRef.FullName();

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

{
properties.Add(new ClassProperty(property, passedType));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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. ;)

Copy link
Author

Choose a reason for hiding this comment

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

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.

continue;
}

passedTypes.Add(structuredTypeRef.ToString(), propertyType);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


if ( structuredTypeRef.IsCollection() )
{
var collectionType = structuredTypeRef.AsCollection();
Expand Down
12 changes: 12 additions & 0 deletions test/Microsoft.AspNet.OData.Versioning.ApiExplorer.Tests/AspNet.OData/Company.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
namespace Microsoft.AspNet.OData
{
public class Company
{
public int CompanyId { get; set; }

public Company ParentCompany { get; set; }

public string Name { get; set; }

}
}
23 changes: 23 additions & 0 deletions ...oft.AspNet.OData.Versioning.ApiExplorer.Tests/AspNet.OData/DefaultModelTypeBuilderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,29 @@ public void type_should_match_edm_with_nested_entity_substitution()
innerType.Should().HaveProperty<string>( nameof( Contact.FirstName ) );
innerType.Should().HaveProperty<string>( nameof( Contact.LastName ) );
}

[Fact]
public void type_should_match_with_self_referencing_property_substitution()
{
// arrange
var modelBuilder = new ODataConventionModelBuilder();
var person = modelBuilder.EntitySet<Company>( "Companies" ).EntityType;

var context = NewContext( modelBuilder.GetEdmModel() );
var originalType = typeof(Company);

//act
var subsitutedType = originalType.SubstituteIfNecessary( context );

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

Choose a reason for hiding this comment

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

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

subsitutedType.Should().HaveProperty<string>( nameof(Company.Name) );
subsitutedType = subsitutedType.GetRuntimeProperties().Single( p => p.Name == nameof( Company.ParentCompany ) ).PropertyType;
subsitutedType.GetRuntimeProperties().Should().HaveCount( 3 );
subsitutedType.Should().HaveProperty<int>( nameof(Company.CompanyId) );
subsitutedType.Should().HaveProperty<string>( nameof(Company.Name) );
}

[Theory]
[MemberData( nameof( SubstitutionData ) )]
Expand Down
37 changes: 16 additions & 21 deletions ...a.Versioning.ApiExplorer.Tests/Microsoft.AspNet.OData.Versioning.ApiExplorer.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,18 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


<PropertyGroup>
<TargetFramework>net452</TargetFramework>
<RootNamespace>Microsoft</RootNamespace>
</PropertyGroup>

<ItemGroup>
<ProjectReference Include="..\..\src\Microsoft.AspNet.OData.Versioning.ApiExplorer\Microsoft.AspNet.OData.Versioning.ApiExplorer.csproj" />
</ItemGroup>

<ItemGroup>
<Reference Include="Microsoft.CSharp" />
<Reference Include="System" />
<Reference Include="System.ComponentModel.DataAnnotations" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="System.Runtime" Version="4.1.0" />
<PackageReference Include="System.Threading.Tasks" Version="4.0.11" />
</ItemGroup>

<PropertyGroup>
<TargetFramework>net452</TargetFramework>
<RootNamespace>Microsoft</RootNamespace>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\..\src\Microsoft.AspNet.OData.Versioning.ApiExplorer\Microsoft.AspNet.OData.Versioning.ApiExplorer.csproj" />
</ItemGroup>
<ItemGroup>
<Reference Include="Microsoft.CSharp" />
<Reference Include="System" />
<Reference Include="System.ComponentModel.DataAnnotations" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="System.Runtime" Version="4.1.0" />
<PackageReference Include="System.Threading.Tasks" Version="4.0.11" />
</ItemGroup>
</Project>
12 changes: 12 additions & 0 deletions test/Microsoft.AspNetCore.OData.Versioning.ApiExplorer.Tests/AspNet.OData/Company.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
namespace Microsoft.AspNet.OData
{
public class Company
{
public int CompanyId { get; set; }

public Company ParentCompany { get; set; }

public string Name { get; set; }

}
}
23 changes: 23 additions & 0 deletions ...AspNetCore.OData.Versioning.ApiExplorer.Tests/AspNet.OData/DefaultModelTypeBuilderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,29 @@ public void type_should_match_edm_with_nested_entity_substitution()
innerType.Should().HaveProperty<int>( nameof( Contact.ContactId ) );
innerType.Should().HaveProperty<string>( nameof( Contact.FirstName ) );
innerType.Should().HaveProperty<string>( nameof( Contact.LastName ) );
}

[Fact]
public void type_should_match_with_self_referencing_property_substitution()
{
// arrange
var modelBuilder = new ODataConventionModelBuilder();
var person = modelBuilder.EntitySet<Company>( "Companies" ).EntityType;

var context = NewContext( modelBuilder.GetEdmModel() );
var originalType = typeof(Company);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


//act
var subsitutedType = originalType.SubstituteIfNecessary( context );

// assert
subsitutedType.GetRuntimeProperties().Should().HaveCount( 3 );
subsitutedType.Should().HaveProperty<int>( nameof(Company.CompanyId) );
subsitutedType.Should().HaveProperty<string>( nameof(Company.Name) );
subsitutedType = subsitutedType.GetRuntimeProperties().Single( p => p.Name == nameof( Company.ParentCompany ) ).PropertyType;
subsitutedType.GetRuntimeProperties().Should().HaveCount( 3 );
subsitutedType.Should().HaveProperty<int>( nameof(Company.CompanyId) );
subsitutedType.Should().HaveProperty<string>( nameof(Company.Name) );
}

[Theory]
Expand Down