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.

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

Metadata: Configuring class as EntityType & later as owned entity type explicitly should throw #9148

Closed
smitpatel opened this issue Jul 11, 2017 · 12 comments
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@smitpatel
Copy link
Member

public class Blog
{
    public int Id { get; set; }
    public BlogDetails BlogDetails { get; set; }
}

public class BlogDetails
{
    public int Value { get; set; }
}

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    // Configure model
    modelBuilder.Entity<BlogDetails>();
    modelBuilder.Entity<Blog>().OwnsOne(e => e.BlogDetails);
}

In above model BlogDetails has been configured as entity type first and then used as owned entity type. At present it works and creates the own entity type. It should throw exception because it is conflict in Explicit configuration source (which is what our behavior is for most other fluent APIs).
Also if this does not throw (and silently remove entity type and add owned entity type) then any facet configuration done on entity types are lost which may seem confusing to user since user may understand .EntityType<> as a replacement of .ComplexType<> may be.

found while investigating issue #9144

@ajcvickers
Copy link
Member

Since throwing post 2.0 would be a breaking change, and this doesn't meet the bar to go into 2.0, could we instead copy facets over to the owned type or something like that? @AndriySvyryd?

@AndriySvyryd
Copy link
Member

@ajcvickers Wouldn't that be a breaking change too?

@ajcvickers
Copy link
Member

Yes, but maybe an acceptable one. 😸

@smitpatel
Copy link
Member Author

Then it would become similar to ComplexType API but name merged into EntityType.
Also it could give false impression that you can have a CLR type as entity type and owned type both.

@divega
Copy link
Contributor

divega commented Jul 11, 2017

Then it would become similar to ComplexType API but name merged into EntityType...

I am not convinced that going in that direction would be a bad thing in the long term. Besides the decision we made for 2.0 (and the obvious benefits of forking the two APIs at the root so that Intellisense can better reflect the distinct capabilities of owned entity types vs. non-owned entity types) they are conceptually all entity types in EF Core. I am starting to suspect that negating that is detrimental to the usability of the API.

Also it could give false impression that you can have a CLR type as entity type and owned type both.

Feature request? :trollface:

@ajcvickers
Copy link
Member

Discussed in triage and decided to make it throw in the 2.1-preview1 and gather feedback as to whether this is too breaking.

@ctolkien
Copy link

Feature request?

Well... now that you mention it....

I just hit this scenario whereby I have a stand alone Customer entity... but I also have an Orders entity to which I would like to add a Customer property as an owned entity.

Basically when an order is created, I could "snapshot" the customer details at that point in time and save them off on the particular order.

By the sounds of things, this scenario is not supported. It doesn't throw.. but things get into a weird state?

@AndriySvyryd AndriySvyryd removed their assignment Aug 29, 2017
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Aug 29, 2017
@ajcvickers
Copy link
Member

Marking this for re-triage. It turns out that a later change in 2.1 reverted this behavior, so it currently doesn't throw in this case.

@ajcvickers ajcvickers reopened this May 23, 2018
@ajcvickers ajcvickers removed this from the 2.1.0-preview1 milestone May 23, 2018
@ajcvickers ajcvickers removed closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. priority-high labels May 23, 2018
@ajcvickers
Copy link
Member

We will re-visit this in 3.0 since it is too late to do anything for 2.1.

@nphmuller
Copy link

Is there perhaps time in the 2.2 release window to makes this throw explicitely? I've ran into multiple ways for this to fail: During migration creation (#13944) and during compilation after succesfull migration creation (#13945). It's pretty confusing with the current behaviour.

@ajcvickers
Copy link
Member

@nphmuller Unfortunately, 2.2 is locked down now for all but ship-stoppers. Also, this change has the potential to break some incorrectly configured but working applications, so we are targeting 3.0 since changing the behavior here could be a breaking change for those applications.

@nphmuller
Copy link

@ajcvickers Makes sense, especially if it's breaking. Thanks!

AndriySvyryd added a commit that referenced this issue Feb 5, 2019
Move relationship configuration to be chained after WithOwner
Throw when using HasOne/HasMany, Entity or Set for an owned type.

Fixes #12444
Fixes #9148
Fixes #14153
AndriySvyryd added a commit that referenced this issue Feb 6, 2019
Move relationship configuration to be chained after WithOwner
Throw when using HasOne/HasMany, Entity or Set for an owned type.

Fixes #12444
Fixes #9148
Fixes #14153
AndriySvyryd added a commit that referenced this issue Feb 6, 2019
Move relationship configuration to be chained after WithOwner
Throw when using HasOne/HasMany, Entity or Set for an owned type.

Fixes #12444
Fixes #9148
Fixes #14153
AndriySvyryd added a commit that referenced this issue Feb 6, 2019
Move relationship configuration to be chained after WithOwner
Throw when using HasOne/HasMany, Entity or Set for an owned type.

Fixes #12444
Fixes #9148
Fixes #14153
AndriySvyryd added a commit that referenced this issue Feb 6, 2019
Move relationship configuration to be chained after WithOwner
Throw when using HasOne/HasMany, Entity or Set for an owned type.

Fixes #12444
Fixes #9148
Fixes #14153
AndriySvyryd added a commit that referenced this issue Feb 7, 2019
Move relationship configuration to be chained after WithOwner
Throw when using HasOne/HasMany, Entity or Set for an owned type.

Fixes #12444
Fixes #9148
Fixes #14153
@AndriySvyryd AndriySvyryd reopened this Feb 7, 2019
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Feb 7, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview3 Feb 22, 2019
@AndriySvyryd AndriySvyryd removed their assignment Feb 28, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview3, 3.0.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

6 participants