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

Enable optional dependents when using table splitting #9005

Closed
AndriySvyryd opened this issue Jun 28, 2017 · 37 comments
Closed

Enable optional dependents when using table splitting #9005

AndriySvyryd opened this issue Jun 28, 2017 · 37 comments
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. punted-for-2.0 type-enhancement
Milestone

Comments

@AndriySvyryd
Copy link
Member

One way of doing this could be mapping the dependent FK to non-PK columns

@julielerman
Copy link

subscribed! :)

@divega
Copy link
Contributor

divega commented Jul 10, 2017

Another way we could consider addressing this is by storing a sentinel value indicating if the object exists in a separate Boolean column. E.g. think Address_IsSet. I wonder if this already works as a workaround with the help of global query filters.

@JackHerring
Copy link

What is the status of this? As I think it is kind of a show stopper

@AndriySvyryd
Copy link
Member Author

@JackHerring This is on the list of things we plan to do, but we don't have a specific version/date for when it would be available.

@JackHerring
Copy link

@AndriySvyryd will owned types be released in 2.0.0 if it is not fully implemented?

@AndriySvyryd
Copy link
Member Author

@JackHerring Yes. You can map optional owned types to a different table.

@JackHerring
Copy link

@AndriySvyryd how do I go about this when using TPH?

@AndriySvyryd
Copy link
Member Author

@JackHerring Owned types don't support inheritance, so I assume that you mean that the owner is using TPH, which doesn't affect configuration:

modelBuilder.Entity<Owner>().OwnsOne(p => p.Owned).ToTable("Owned");

@jrote1
Copy link

jrote1 commented Sep 14, 2017

Is there any progress on the resolution of this issue?

@ajcvickers
Copy link
Member

@jrote1 This issue is in the Backlog milestone. This means that it is not going to happen for the 2.1 release. We will re-assess the backlog following the 2.1 release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources.

@AndriySvyryd
Copy link
Member Author

@jrote1 You can vote for features by using the +1 reaction (👍) on the original comment

@space-alien
Copy link

I would really like to see this. The non-nullability of an owned type seriously restricts the usefulness of this feature.

@julielerman
Copy link

I have a 1/2 written blog post on a great pattern to use until this is supported (something that @anpete and @divega helped me work out). It was getting long and I completely forgot to go finish it up! Will try to get to it over the weekend.

@space-alien
Copy link

Sounds great Julie, look forward to reading it :)

@ajcvickers
Copy link
Member

Note from triage: remember to add API for configuring optional owned types, whether relational/table splitting or not, when implementing this. See #10818

@AndriySvyryd
Copy link
Member Author

AndriySvyryd commented Feb 13, 2018

We should also handle Added and Deleted dependents for Unchanged principals as updates in the database.
Also a way to implement this for dependents that have at least one required property is to use that property as the marker. Also @divega idea would work with this by creating a required shadow property with a client-side default value.
If all non-key properties are nullable we would return a null only if all values are null, this is inefficient, so we should warn and provide the above workaround.

@AndriySvyryd
Copy link
Member Author

Attention all subscribers: Could you provide a simple description of the scenario that this feature would enable?

  1. Show sample entity types and corresponding table structure.
  2. Would you need it primarily for owned types (allows the same CLR type to be mapped to multiple entity types) or for plain table splitting (allows the dependent type to be in a hierarchy)?
  3. Currently OwnsOne maps the owned type to the same table as the principal (table splitting) by default, but you can map it to a separate table. If we introduce OwnsOneOptional would the expected default would also be to use table splitting?

@niwrA
Copy link

niwrA commented Apr 13, 2018

Probably the most important point here would be that you want to separate the decision for how your entity is designed from your database design. A separate table or not for, say, a primary address, should be a decision that is transparent from that you want this to appear as a separate object on your entity.

In that sense, an owned type is basically a shortcut notation for a 1-to-[0..1] relation, and the fact that the owned object is rendered as null / none (f#) the most clear way of expressing that it is currently 1-to-[0], rather than 1-to-1 with no properties set, so that you might need to add properties to indicate whether it is set or not.

As I seem to recall hearing on a podcast (.NET Rocks probably) that you are investigating using Code-First EF modelling for MongoDb and other DocumentDb type databases. In those systems, a child json object will simply be or not be there in the document, and that will result in the behavior as people have expressed to want to see here. And then you want consistent behavior whether you use a sql or documentdb type datastore backing.

Similarly, you may want to optimise your database by having more data per table or less in terms of partitioning etc. This should not have any influence on your Code First modeling, preferably.

I currently always design and implement my repositories at least partly in both SQL(Lite) and MongoDb and use them for state objects that are injected into business objects that only define through interfaces what state (and repository) functionality they need, so neither layer knows about the other and there are no dependencies on either side. The best way to validate this is by being able to DI either repository implementation into the business logic and everything still works and works well. This means preferably going through the root entity (aggregate) by default and then working down from there, which suits me well because I follow Domain Driven Design principles (they also work for MicroServices).

In that sense, as I interact with injected state objects, the fact that EFCore would always give back a state object for the owned entity but MongoDb wouldn't is a specific scenario for me personally, especially as with 2.0, EFCore is starting to reach a level of quality that makes me want to use it over almost anything else out there. ;) (so good job everyone)

@AndriySvyryd AndriySvyryd removed their assignment Mar 28, 2019
@julielerman
Copy link

you did it?

@AndriySvyryd
Copy link
Member Author

@julielerman Yes, all dependents are now optional. (Shipping in preview 4)

@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 Mar 28, 2019
@altmann
Copy link

altmann commented Mar 28, 2019

@AndriySvyryd you are our hero. Thanks a lot.

@pauldbentley
Copy link

@julielerman Yes, all dependents are now optional. (Shipping in preview 4)

Does this mean there is no way to make a dependent mandatory like 2.2?

@smitpatel
Copy link
Member

@pauldbentley - That is true. See #12100

@julielerman
Copy link

@pauldbentley That is something I would recommend defining in the logic of your classes. But it does mean that ef can't double check that for you prior to executing relevant SQL.

@clawrenceks
Copy link

@pauldbentley @smitpatel @julielerman I am hitting this issue due to the breaking change. My classes are designed to ensure that this isn't an issue at the domain layer, but in my opinion this has been a poorly designed change. The configuration builder still allows you to add IsRequired(), which of course has no effect at all on the generated migration, leaving you with a database with the potential for consistency issues.

@marcwittke
Copy link

all dependents are now optional

this only applies to the table split configuration, right?

@smitpatel
Copy link
Member

@marcwittke - It applies to everything as there is no way to enforce dependents being required in EF Core. So All dependents are optional in all scenarios.

@marcwittke
Copy link

But without table splitting I have a strange behavior: I create an instance with an owned type entity that is null, but when I load it back from the database it is not null any more, but an owned type instance with all values being null.

@smitpatel
Copy link
Member

@marcwittke - Please file a new issue with runnable repro code which shows the issue you are seeing.

@morinow
Copy link

morinow commented Mar 24, 2022

EFCore 3.1.6
We stumbled upon an issue where the behavior "entity is NULL when all properties are NULL" was rather confusing.
In our case we had a split entity, which has collection navigation properties. When we loaded the main entity, included the split entity and it's collection navigation properties, the split entity was returned as NULL even though the collections on the entity contained data, because all of it's "flat" properties were NULL. We ofc expected to receive the entity with it's collections filled.

It's probably not so obvious how one would solve this issue, as it kind of breaks the current concept. E.g. what would happen if the included nested collections are empty? Is the split entity then dropped completely (NULL)?
Nevertheless it was quite a confusing encounter.

EDIT:
Typo

@AndriySvyryd
Copy link
Member Author

@morinow In 6.0 we made a breaking change to throw in this scenario.
The workaround is to mark the split entity as required, but this requires 5.0:

ob.Navigation(o => o.ShippingAddress)
            .IsRequired();

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. punted-for-2.0 type-enhancement
Projects
None yet
Development

No branches or pull requests