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

The future of optional dependents sharing table with principal #23564

Closed
smitpatel opened this issue Dec 2, 2020 · 8 comments · Fixed by #23635
Closed

The future of optional dependents sharing table with principal #23564

smitpatel opened this issue Dec 2, 2020 · 8 comments · Fixed by #23635
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@smitpatel
Copy link
Member

The optional dependents which are sharing table with principal has few technicalities around how they are retrieved and materialized.
There are mainly 2 stages where such determination needs to be made.

  • Sql generation so we do get data from server
  • Materialization of entity to create actual instances

The basic set of conditions for optional dependents right now is

  • Find non-pk required properties, if all of them have value then we get data from server and materialize instance. We need value for all required properties else creating instance would throw.
  • If first condition fails. we look for non-pk optional properties. if any of them have value we get data from server and materialize instance. At least one non-null value would indicate that instance exists.
  • If second condition fails then we join with other dependents of this table and find if nested dependents exist. If they exist then the current entity does exist. We get this data from server, but currently materialization does not have any info about other related entities so it ends up materializing them.

Issues:

  • Complicated SQL because use of Union for nested dependent
  • Incorrect results - Cannot determine nested dependent to materialize

Examples

Now SQL side,
#23230 - we need to integrate conditions for existence of dependent when accessing any property which is sharing table with any principal.

So major issue right now in SQL/correct SQL we need for conditional property access & materialization w.r.t nested dependent when dependent does not have at least one non-PK, optional/required property.

Proposals:

  • We stop checking nested dependents. We utilize first 2 conditions only else we consider it always true.

    • What this means is that optional dependents act like required dependents if there is no differentiating property.
    • This matches what we have in current behavior so no breaking change apart from people who don't want this behavior and already broken.
    • This simplify the SQL.
    • This solves all the materialization issues we have without involving bigger design challenge.
    • We can verify further than behavior is consistent and generate a warning to let users know.
  • We guide people have at least one non-nullable property as a discriminator like for existence of dependent.

    • In this kind of scenario first condition will always be hit.
    • The behavior is very easy to understand.
    • We can simplify the conditions to use just one property rather than all properties.
    • This would be breaking change as it requires schema change for people. This would be only needed if they want the dependent to be null and it does not have any property in it. They are already broken right now since we materialize it.

SQL property access would be written as "existence check ? property access : null" (potentially expanded in check AND property == value")
In first proposal, the existence check will be possible (since no longer joins needed).
In second proposal, the check would be much simpler.

@AndriySvyryd
Copy link
Member

I am in favor of the first proposal as it's less breaking. We should throw in model validation for any nested types on optional dependents that don't have required non-shared properties and gather feedback.

@ajcvickers
Copy link
Member

Agreed. First proposal seems good. Thanks for writing all this down, Smit.

@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Dec 9, 2020
smitpatel added a commit that referenced this issue Dec 9, 2020
An optional dependent considered present
If all non-nullable non-PK properties have a non-null value, or
If all non-nullable non-PK properties are shared with principal then at least one nullable non shared property has a non-null value
If there are no non-shared properties then it is always present and act like a required dependent

Resolves #23564
Resolves #21488
Resolves #23198
smitpatel added a commit that referenced this issue Dec 9, 2020
An optional dependent considered present
If all non-nullable non-PK properties have a non-null value, or
If all non-nullable non-PK properties are shared with principal then at least one nullable non shared property has a non-null value
If there are no non-shared properties then it is always present and act like a required dependent

Resolves #23564
Resolves #21488
Resolves #23198
@smitpatel
Copy link
Member Author

Next set of decisions

  • We will keep the current set of conditions for materialization identification
    -> All required properties must have a value.
    -> If all non principal shared columns are nullable (that means none of them is in first set and hence identifying), then one of them should have value.
  • For a dependent which has nested dependent and no identifying required column, we throw error since it can cause data loss.
  • For a dependent which does not have a nested dependent we will just apply above conditions. This means that it may not materialize instance when all properties are nullable and contains null value even though saved to database, it is being tracked by Warn/throw when saving an optional dependent with all null properties when table splitting #24558

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Apr 1, 2021

Also warn when there's no nested dependent and no identifying required column.

@smitpatel
Copy link
Member Author

@AndriySvyryd - Does #23229 (comment) covers everything in validation?

smitpatel added a commit that referenced this issue Apr 1, 2021
An optional dependent considered present
If all non-nullable non-PK properties have a non-null value, or
If all non-nullable non-PK properties are shared with principal then at least one nullable non shared property has a non-null value
If there are no non-shared properties then it is always present and act like a required dependent

Resolves #23564
smitpatel added a commit that referenced this issue Apr 2, 2021
An optional dependent considered present
If all non-nullable non-PK properties have a non-null value, or
If all non-nullable non-PK properties are shared with principal then at least one nullable non shared property has a non-null value
If there are no non-shared properties then it is always present and act like a required dependent

Resolves #23564
@alvorteg
Copy link

alvorteg commented Apr 14, 2021

@smitpatel @ajcvickers Are you planning a release with this fix before 6.0?

@cjblomqvist
Copy link

Sorry for adding this late in the process (thanks smitpatel for refering me here!). A third proposal would be to automatically add a "hidden" column/field which separates a existing dependent with all values set to null from a non-existing dependent.

Pros (that I can see):

  • Simplified user experience
  • The user doesn't need to pollute it's .NET (e.g. C#) code with "implementation detail"-level (in an EF Core sense/level) stuff.

Cons:

  • Adds an unexpected column/field in the data store - can be a surprise for the user (on the other hand, that might already be done in other cases? E.g. PKs?)
  • EF Core implementation complexity

@smitpatel
Copy link
Member Author

@cjblomqvist - Because this feature was already shipped in past without having automatically added column, we cannot add a column automatically now. A user who has created database with EF Core, without making any change to their model will have their query failed because EF Core decided to change the schema altogether. We went with warning/error mechanism so user will know they need to update the model, generate migration and update their database.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants