Code First: One-to-one FK APIs should introduce shadow properties

[anpete]

Given this:

modelBuilder
    .Entity<Person2>(
        e => e.OneToOne(p => p.Address, a => a.Resident)
            .ForeignKey(typeof(Address2), "PersonId"));

Where "PersonId" has not been defined, we currently throw ModelItemNotFound; should we just add the shadow key instead?

Should we have an overload of ForeignKey like:

ForeignKey<TDependent>(string)

[AndriySvyryd]

We shouldn't add new shadow properties if the property referenced by name is not found as this might be a user-error that otherwise would be hard to notice.

We could add an overload to explicitly specify a shadow FK, something like

ForeignKey(Type, bool, params string[])

[anpete]

Not sure I agree in this case. Seems like it would smooth the usage of a common case.

Any opinions @rowanmiller, @divega?

[rowanmiller]

I'm torn on this one. I think having an FK in the class is a 'pit of success' because we know that shadow state FKs (the equivalent of IAs in the old stack) don't work well in detached scenarios. My gut feel is to not introduce shadow state stuff by convention for these reasons... but I could be convinced otherwise.

[divega]

Actually many of the things that were hard about IAs don't apply to FKs in shadow-state.

[anpete]

@rowanmiller Yeah, not sure. My initial expectation was that the prop would just be introduced.

[ajcvickers]

@rowanmiller We introduce shadow properties by convention if the app code doesn't specify a property and we don't find one by convention. I don't think we should change this. However, this is not that case. In this case the app code has specified a property which may or may not be in shadow state. If we don't find that property in the model then should we throw or introduce the property in shadow state? Introducing the property is only useful where you want the property in shadow state and you want it to have a specific name, either because the name we chose is not what you want or because we found some other property by convention and you don't want us to use it. To me this is a relatively corner case--or it should be, if our conventions are good. If it is what you want, then a previous call to explicitly put the property in shadow state is not hard to do. Also, I think there is some reasonable chance that you are trying to specify a property not in shadow state, or even a different property in shadow state. So throwing has some value and I don't think we should introduce a shadow state property in this case. But I don't feel strongly one way or the other.

[divega]

I would add that one reason to want to use an API like this to specify the name is because you are planning to refer to the FK property in state manipulation code later and you don't want to have to guess how the convention will name it. I personally believe less friction is better and I don't believe it would be harmful if we just synthetize a shadow state property with the name passed to use in this API, but on the other hand I don't think it is a super high priority to do it.

[AndriySvyryd]

We could allow this, but record a warning that would be surfaced during model validation, see #1205

[ajcvickers]

@AndriySvyryd I think we decided that it would introduce the shadow state property. If it is being used correctly I don't think it is a warning. But the fact we did it should be logged.

[smitpatel]

Implemented in #3576