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.

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

🐛 object is not a valid navigation #3737

Merged
merged 1 commit into from
Nov 13, 2015
Merged

🐛 object is not a valid navigation #3737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/EntityFramework.Core/Metadata/Internal/EntityType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ public virtual Key SetPrimaryKey(
{
if (_baseType != null)
{
throw new InvalidOperationException(CoreStrings.DerivedEntityTypeKey(Name));
throw new InvalidOperationException(CoreStrings.DerivedEntityTypeKey(DisplayName(), _baseType.DisplayName()));
}

if (_primaryKey != null)
Expand Down Expand Up @@ -379,7 +379,7 @@ public virtual Key AddKey([NotNull] IReadOnlyList<Property> properties,

if (_baseType != null)
{
throw new InvalidOperationException(CoreStrings.DerivedEntityTypeKey(Name));
throw new InvalidOperationException(CoreStrings.DerivedEntityTypeKey(DisplayName(), _baseType.DisplayName()));
}

foreach (var property in properties)
Expand Down
54 changes: 27 additions & 27 deletions src/EntityFramework.Core/Properties/CoreStrings.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/EntityFramework.Core/Properties/CoreStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@
<value>The '{factory}' cannot create a value generator for property '{property}' on entity type '{entityType}'. Only integer properties are supported.</value>
</data>
<data name="DerivedEntityTypeKey" xml:space="preserve">
<value>The derived type '{derivedType}' cannot have keys other than those declared on the root type.</value>
<value>The derived type '{derivedType}' cannot have keys other than those declared on the root type '{rootType}'.</value>
</data>
<data name="CircularInheritance" xml:space="preserve">
<value>The entity type '{entityType}' cannot inherit from '{baseEntityType}' because '{baseEntityType}' is a descendent of '{entityType}'.</value>
Expand Down
3 changes: 2 additions & 1 deletion src/Shared/PropertyInfoExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ public static Type FindCandidateNavigationPropertyType(this PropertyInfo propert

if (isPrimitiveProperty(targetType)
|| targetType.GetTypeInfo().IsInterface
|| targetType.GetTypeInfo().IsValueType)
|| targetType.GetTypeInfo().IsValueType
|| targetType == typeof(object))
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 this might be the same thing @divega is saying, but it doesn't seem to me that special casing "object" is the right thing to do here because object is not any more special than any other type that might be the base type for all or some entities in the model,

Copy link
Member

Choose a reason for hiding this comment

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

It is special because it is much less likely to be an entity type than any other base type

{
return null;
}
Expand Down
4 changes: 2 additions & 2 deletions test/EntityFramework.Core.Tests/Metadata/Internal/EntityTypeTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,10 +476,10 @@ public void Adding_keys_throws_when_there_is_a_parent_type()
b.HasBaseType(a);

Assert.Equal(
CoreStrings.DerivedEntityTypeKey(typeof(B).FullName),
CoreStrings.DerivedEntityTypeKey(typeof(B).Name, typeof(A).Name),
Assert.Throws<InvalidOperationException>(() => b.SetPrimaryKey(b.AddProperty("G"))).Message);
Assert.Equal(
CoreStrings.DerivedEntityTypeKey(typeof(B).FullName),
CoreStrings.DerivedEntityTypeKey(typeof(B).Name, typeof(A).Name),
Assert.Throws<InvalidOperationException>(() => b.AddKey(b.AddProperty("E"))).Message);
}

Expand Down
4 changes: 2 additions & 2 deletions test/EntityFramework.Core.Tests/Metadata/Internal/InternalEntityTypeBuilderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,7 @@ public void Key_throws_for_derived_type()
var derivedEntityBuilder = modelBuilder.Entity(typeof(SpecialOrder), ConfigurationSource.Convention);
derivedEntityBuilder.HasBaseType(entityBuilder.Metadata, ConfigurationSource.Convention);

Assert.Equal(CoreStrings.DerivedEntityTypeKey(typeof(SpecialOrder).FullName),
Assert.Equal(CoreStrings.DerivedEntityTypeKey(typeof(SpecialOrder).Name, typeof(Order).Name),
Assert.Throws<InvalidOperationException>(() =>
derivedEntityBuilder.HasKey(new[] { Order.IdProperty.Name, Order.CustomerIdProperty.Name }, ConfigurationSource.DataAnnotation)).Message);
}
Expand Down Expand Up @@ -764,7 +764,7 @@ public void PrimaryKey_throws_for_derived_type()
var derivedEntityBuilder = modelBuilder.Entity(typeof(SpecialOrder), ConfigurationSource.Convention);
derivedEntityBuilder.HasBaseType(entityBuilder.Metadata, ConfigurationSource.Convention);

Assert.Equal(CoreStrings.DerivedEntityTypeKey(typeof(SpecialOrder).FullName),
Assert.Equal(CoreStrings.DerivedEntityTypeKey(typeof(SpecialOrder).Name, typeof(Order).Name),
Assert.Throws<InvalidOperationException>(() =>
derivedEntityBuilder.PrimaryKey(new[] { Order.IdProperty.Name, Order.CustomerIdProperty.Name }, ConfigurationSource.DataAnnotation)).Message);
}
Expand Down
2 changes: 2 additions & 0 deletions ...tityFramework.Core.Tests/Metadata/ModelConventions/RelationshipDiscoveryConventionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,8 @@ private class EntityWithNoValidNavigations
{
public int Id { get; set; }

public object Object { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure, would it make sense to have a test in which:

  1. A CLR entity type derives from a base type that different from System.Object but isn't a valid entity type
  2. There is a property of that base type somewhere in the model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have test to verify

  1. Non-primitive types are discovered as entity type
  2. When base type is present as entity type in the model then base type will be set for derived types
  3. When invalid entity is present in model throw exception
    Above scenario is just combination of all of them and should not be necessary.
    Though I can add test if you want. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see this test too. I'm not understanding how this fix will make the test Diego suggested pass.

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 is important that we understand the behavior in this general case because System.Object should just be just an example of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

General behavior is,
If user have 2 entity types in the model added explicitly/conventionally, and if they are in CLR hierarchy then we will create TPH. We will throw exception if user tries to create key in derived type.
We will throw exception if there is any invalid entity type (entity without PK) regardless of above.

System.Object is kind of stands out because,

  1. We know that it is not valid entity type (unless shadow PK defined but if we want to support that we have to remove this check)
  2. object is base type for every clr type hence it will be set as base type for every other entity type.

This change is not the fix for the user reported exception.
User has property of object type which cannot be mapped by typemapper. Hence user should be notified to exclude it from model since discovering object as entity type throws misleading exception.
For any type other than object, there won't be clr inheritance unless user has defined it using derived class. At which point it is fair to throw above exception notifying user that we have put both entities in same hierarchy by convention (or there is invalid entity type in the model)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ajcvickers I talked to @smitpatel and @AndriySvyryd and right now I understand the reason for this design and I am ok with it. Hopefully I can explain it to you tomorrow before I forget 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

@divega After looking through it all again it seems to me that the only real issue is that the original exception message didn't specify that we had pulled "object" into the model. With an appropriate change to the message (possibly more than what is in this PR, although that is a good start) it seems that is all we need to do. In other words, I don't think we also need to special case object. But I look forward to hearing why special casing object is a better thing to do than just having a better exception message for the general case.


public static OneToManyDependent Static { get; set; }

public OneToManyDependent WriteOnly
Expand Down