Fix run value converters that convert nulls for JSON columns#37984
Fix run value converters that convert nulls for JSON columns#37984JoasE wants to merge 5 commits intodotnet:mainfrom
Conversation
During (de)serialization, check if converter converts nulls and run for null values if so Closes: dotnet#37983
| resultExpression = Convert(resultExpression, property.ClrType); | ||
| } | ||
|
|
||
| var converter = property.GetTypeMapping().Converter; |
There was a problem hiding this comment.
This is copied over from above for CreateGetValueExpression
This comment was marked as resolved.
This comment was marked as resolved.
|
@AndriySvyryd assigning to you for reviewing but let me know if you want me to do it. |
There was a problem hiding this comment.
Pull request overview
Fixes JSON column (de)serialization so value converters that opt into null conversion (ConvertsNulls == true) are applied even when the JSON token/value is null, addressing incorrect null persistence/materialization behavior.
Changes:
- Apply JSON value converter logic for
nullvalues during JSON update serialization (ModificationCommand,RelationalJsonUtilities). - Apply value converter logic for JSON
nulltokens during query materialization (RelationalShapedQueryCompilingExpressionVisitor). - Add/adjust relational and provider-specific tests to validate null-converting converters in JSON scenarios.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/EFCore.Relational/Update/ModificationCommand.cs | Serialize JSON properties via JsonValueReaderWriter even when the value is null, if the converter converts nulls. |
| src/EFCore.Relational/Query/Internal/RelationalJsonUtilities.cs | Same as above for JSON parameter/constant serialization utilities. |
| src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs | When JSON token is null, invoke converter (when ConvertsNulls) instead of returning default CLR value. |
| test/EFCore.Relational.Specification.Tests/Query/AdHocJsonQueryRelationalTestBase.cs | Adds a regression test exercising JSON equality with a converter that converts nulls. |
| test/EFCore.SqlServer.FunctionalTests/Query/AdHocJsonQuerySqlServerTestBase.cs | Adds SQL assertion for the new regression scenario. |
| test/EFCore.Specification.Tests/ValueConvertersEndToEndTestBase.cs | Refactors base test to allow provider-specific JSON embedding (new overridable hooks). |
| test/EFCore.SqlServer.FunctionalTests/ValueConvertersEndToEndSqlServerJsonTest.cs | New SQL Server functional test running the end-to-end converter suite with the entity stored as JSON. |
| test/EFCore.SqlServer.FunctionalTests/Update/JsonUpdateSqlServerTest.cs | Updates expected JSON payload/parameter sizing for null-converting converter output. |
| test/EFCore.SqlServer.FunctionalTests/Update/JsonUpdateJsonTypeSqlServerTest.cs | Same as above for json column type. |
| test/EFCore.Sqlite.FunctionalTests/Update/JsonUpdateSqliteTest.cs | Updates expected JSON payload/parameter sizing for null-converting converter output. |
test/EFCore.Specification.Tests/ValueConvertersEndToEndTestBase.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Relational.Specification.Tests/Query/AdHocJsonQueryRelationalTestBase.cs
Show resolved
Hide resolved
| var jsonValueReaderWriter = property.GetJsonValueReaderWriter() ?? property.GetTypeMapping().JsonValueReaderWriter; | ||
| Check.DebugAssert(jsonValueReaderWriter is not null, "Missing JsonValueReaderWriter on JSON property"); | ||
| jsonValueReaderWriter.ToJson(writer, propertyValue); | ||
| jsonValueReaderWriter.ToJson(writer, propertyValue!); |
There was a problem hiding this comment.
propertyValue can be null here
There was a problem hiding this comment.
That’s actually intentional here, since this change allows propertyValue to be null if the converter has indicated it can handle null values.
There was a problem hiding this comment.
Then you should change the signature of ToJson to allow nulls
| var jsonValueReaderWriter = property.GetJsonValueReaderWriter() ?? property.GetTypeMapping().JsonValueReaderWriter; | ||
| Check.DebugAssert(jsonValueReaderWriter is not null, "Missing JsonValueReaderWriter on JSON property"); | ||
| jsonValueReaderWriter.ToJson(writer, propertyValue); | ||
| jsonValueReaderWriter.ToJson(writer, propertyValue!); |
There was a problem hiding this comment.
propertyValue can be null here
During (de)serialization, check if converter converts nulls and run for null values if so
Closes: #37983