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.

Sign up for GitHub

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

Jump to bottom

Test: Make sure all owned types are having at least one column other than PK #20336

Closed
smitpatel opened this issue Mar 18, 2020 · 5 comments · Fixed by #20353
Closed

Test: Make sure all owned types are having at least one column other than PK #20336

smitpatel opened this issue Mar 18, 2020 · 5 comments · Fixed by #20353
Assignees
@smitpatel
Labels
area-test closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone
5.0.0

Comments

@smitpatel
Copy link
Member

We see data in the column and we end up calling materializer even when entity type is base.
Happens in our tests since owned type did not have any additional column than PK to identify it's existence.
@smitpatel smitpatel changed the title Query: Include: Only materialize optional dependent if parent is correct Test: Make sure all owned properties are having at least one column other than PK Mar 19, 2020
@smitpatel smitpatel changed the title Test: Make sure all owned properties are having at least one column other than PK Test: Make sure all owned types are having at least one column other than PK Mar 19, 2020
@smitpatel
Copy link
Member Author

A general case would be resolved by #20334
Re-purposed this issue to discuss about owned entity when there are no columns other than PK.
Not sure much value to support them but if we do then they would be required dependents anyway.

@AndriySvyryd
Copy link
Member

Yes, we'll ensure they are required dependents in model validation once we support them. It doesn't matter what the behavior is until then.

@smitpatel
Copy link
Member Author

So we can just update our tests and close this issue?

smitpatel added a commit that referenced this issue Mar 19, 2020
@smitpatel
Query: Do not generate server side joins for optional dependents
bfc029a 
Resolves #18299

Fix involves:
- Adding additional materialization conditions for relational layer when table splitting is used. See #20332 for conditions
- Create EntityProjection using the same table when both types are sharing the table.
- Fix a bug in shaper when null entityType is returned from materialization condition, don't call StartTracking.
- Assign nullability to PK column correctly for optional dependents so we don't project it twice.
- Disabled Sql assertion in OwnedQuerySqlServerTest. Will enable after model update as described in #20336, #20343
smitpatel added a commit that referenced this issue Mar 19, 2020
@smitpatel
Query: Do not generate server side joins for optional dependents
5fe3628 
Fix involves:
- Adding additional materialization conditions for relational layer when table splitting is used. See #20332 for conditions
- Create EntityProjection using the same table when both types are sharing the table.
- Fix a bug in shaper when null entityType is returned from materialization condition, don't call StartTracking.
- Assign nullability to PK column correctly for optional dependents so we don't project it twice.
- Disabled Sql assertion in OwnedQuerySqlServerTest. Will enable after model update as described in #20336, #20343

Resolves #18299
Resolves #20338
Resolves #20332
@smitpatel smitpatel mentioned this issue Mar 19, 2020
Merged
@AndriySvyryd
Copy link
Member

Yes, but add a note on #12100

@smitpatel smitpatel self-assigned this Mar 19, 2020
@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 Mar 19, 2020
@smitpatel smitpatel added this to the 5.0.0 milestone Mar 19, 2020
@smitpatel
Copy link
Member Author

I am adding at least one additional property in each owned type in tests other than PK.

smitpatel added a commit that referenced this issue Mar 19, 2020
@smitpatel
Test: Update Sql assertion in OwnedQuerySqlServerTest
6f5cc15 
- Also add at least one additional property in each owned type. If there no columns other than shared PK then it is required dependent.
- Add a property in owned type which would be sharing column for hierarchy. Currently disabled due to #20334

Resolves #20343
Resolves #20336
Finishes left over test work from #20345
smitpatel added a commit that referenced this issue Mar 19, 2020
@smitpatel
Test: Update Sql assertion in OwnedQuerySqlServerTest
0008b80 
- Also add at least one additional property in each owned type. If there no columns other than shared PK then it is required dependent.
- Add a property in owned type which would be sharing column for hierarchy. Currently disabled due to #20334

Resolves #20343
Resolves #20336
Finishes left over test work from #20345
@smitpatel smitpatel mentioned this issue Mar 19, 2020
Merged
smitpatel added a commit that referenced this issue Mar 20, 2020
@smitpatel
Test: Update Sql assertion in OwnedQuerySqlServerTest
3d6861e 
- Also add at least one additional property in each owned type. If there no columns other than shared PK then it is required dependent.
- Add a property in owned type which would be sharing column for hierarchy. Currently disabled due to #20334

Resolves #20343
Resolves #20336
Finishes left over test work from #20345
@smitpatel smitpatel linked a pull request Mar 20, 2020 that will close this issue
Test: Update Sql assertion in OwnedQuerySqlServerTest #20353
Merged
smitpatel added a commit that referenced this issue Mar 20, 2020
@smitpatel
Query: Do not generate server side joins for optional dependents
0ccab1a 
Fix involves:
- Adding additional materialization conditions for relational layer when table splitting is used. See #20332 for conditions
- Create EntityProjection using the same table when both types are sharing the table.
- Fix a bug in shaper when null entityType is returned from materialization condition, don't call StartTracking.
- Assign nullability to PK column correctly for optional dependents so we don't project it twice.
- Disabled Sql assertion in OwnedQuerySqlServerTest. Will enable after model update as described in #20336, #20343

Resolves #18299
Resolves #20338
Resolves #20332
smitpatel added a commit that referenced this issue Mar 20, 2020
@smitpatel
Test: Update Sql assertion in OwnedQuerySqlServerTest
4ed67db 
- Also add at least one additional property in each owned type. If there no columns other than shared PK then it is required dependent.
- Add a property in owned type which would be sharing column for hierarchy. Currently disabled due to #20334

Resolves #20343
Resolves #20336
Finishes left over test work from #20345
smitpatel added a commit that referenced this issue Mar 20, 2020
@smitpatel
Query: Do not generate server side joins for optional dependents
a2e3531 
Fix involves:
- Adding additional materialization conditions for relational layer when table splitting is used. See #20332 for conditions
- Create EntityProjection using the same table when both types are sharing the table.
- Fix a bug in shaper when null entityType is returned from materialization condition, don't call StartTracking.
- Assign nullability to PK column correctly for optional dependents so we don't project it twice.
- Disabled Sql assertion in OwnedQuerySqlServerTest. Will enable after model update as described in #20336, #20343

Resolves #18299
Resolves #20338
Resolves #20332
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview3 Mar 31, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview3, 5.0.0 Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@smitpatel smitpatel

Labels
area-test closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Milestone
5.0.0
Development

Successfully merging a pull request may close this issue.

Test: Update Sql assertion in OwnedQuerySqlServerTest dotnet/efcore
3 participants
@ajcvickers @smitpatel @AndriySvyryd