🐛 object is not a valid navigation

[smitpatel]

Fixes #3727
Blocked object being discovered as entity type. Entity type with clr type System.Object is invalid entity type (due to absence of key) which is verified in model validator. Since System.Object is base type for any clr type, we set this entity type as base type for all other entities. Therefore we need to block it from being discovered in first place.
Also modified exception message in order to give information on what is the base type.

[divega]

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?

[smitpatel]

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. 😄

[ajcvickers]

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

[divega]

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.

[smitpatel]

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)

[divega]

@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 😄

[ajcvickers]

@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.

[AndriySvyryd]

[ajcvickers]

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,

[AndriySvyryd]

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