-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Fix to #21006 - Support a default value for non-nullable properties #35746
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
Conversation
0ea33cb
to
d55392e
Compare
Only for scalar properties when projecting Json-mapped entity. Only need to change code for Cosmos - relational already works in the desired way after the change to streaming (properties that are not encountered maintain their default value) We still throw exception if JSON contains explicit null where non-nullable scalar is expected. Fixes #21006
This is now ready for review. Tested (and working) scenarios: tested top level and nested, as well as properties requiring converters vs no. what doesn't work: to make it work users need to cast to nullable, otherwise we throw nullable object must have a value. This would be quite tricky to fix - when we project properties directly, we lose the IProperty information, so we can't easily reason with what is being projected (type doesn't provide any hints, it's always nullable - a requirement we check at the start of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR fixes issue #21006 by ensuring that non-nullable scalar properties on JSON‐mapped entities get a proper default value when they are missing during projection. Key changes include new test scenarios for Cosmos, Sqlite, and SQL Server providers, as well as modifications to the Cosmos projection binding logic to correctly handle non-nullable scalars.
Reviewed Changes
File | Description |
---|---|
test/EFCore.Cosmos.FunctionalTests/Query/AdHocCosmosTestHelpers.cs | Introduces a helper to create entities in Cosmos with JSON payloads (note a spelling mistake in one of the error messages). |
test/EFCore.Cosmos.FunctionalTests/Query/AdHocMiscellaneousQueryCosmosTest.cs | Adds tests for projecting entities with missing scalar values and navigations. |
test/EFCore.Sqlite.FunctionalTests/Query/AdHocJsonQuerySqliteTest.cs | Adds insert scripts to cover scenarios with missing and null scalars/navigations. |
test/EFCore.SqlServer.FunctionalTests/Query/AdHocJsonQuerySqlServerTestBase.cs | Updates base test class to validate SQL output for various missing value cases. |
test/EFCore.Relational.Specification.Tests/Query/AdHocJsonQueryRelationalTestBase.cs | Revises model building and overrides tests for missing required values. |
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs | Adjusts the expression visitor logic to account for non-nullable scalars. |
test/EFCore.InMemory.FunctionalTests/InMemoryComplianceTest.cs | Registers the new AdHocJsonQueryTestBase in the in-memory test suite. |
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
test/EFCore.Cosmos.FunctionalTests/Query/AdHocCosmosTestHelpers.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Cosmos.FunctionalTests/Query/AdHocMiscellaneousQueryCosmosTest.cs
Outdated
Show resolved
Hide resolved
…s.cs Co-authored-by: Copilot <[email protected]>
…ryCosmosTest.cs Co-authored-by: Copilot <[email protected]>
Does it work in the sense that navigation remains |
@AndriySvyryd returns null for the missing navigations
|
NestedRequiredReference = new Context21006.JsonEntityNested { DoB = new DateTime(2000, 1, 1), Text = "e1 rr nrr" }, | ||
NestedCollection = new List<Context21006.JsonEntityNested> | ||
{ | ||
new Context21006.JsonEntityNested { DoB = new DateTime(2000, 1, 1), Text = "e1 rr c1" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maumar as a general rule, can we please avoid testing things on DateTimes (as opposed to strings and ints) where we're not specifically testing timestamp stuff? Support for timestamps really does vary across databases (whereas it doesn't for e.g. ints), and this fails on PG without some significant tweaking (since you can't shove an Unspecified DateTime into the default PG timestamp type).
Basically let's default to the basic, unproblematic types (int, string) unless we're specifically wanting to test DateTime behavior.
Only for scalar properties when projecting Json-mapped entity. Only need to change code for Cosmos - relational already works in the desired way after the change to streaming (properties that are not encountered maintain their default value) We still throw exception if JSON contains explicit null where non-nullable scalar is expected.
Fixes #21006