Skip to content

Fix run value converters that convert nulls for JSON columns#37984

Open
JoasE wants to merge 5 commits intodotnet:mainfrom
JoasE:fix/#37983
Open

Fix run value converters that convert nulls for JSON columns#37984
JoasE wants to merge 5 commits intodotnet:mainfrom
JoasE:fix/#37983

Conversation

@JoasE
Copy link
Copy Markdown
Contributor

@JoasE JoasE commented Mar 24, 2026

During (de)serialization, check if converter converts nulls and run for null values if so

Closes: #37983

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

During (de)serialization, check if converter converts nulls and run for null values if so

Closes: dotnet#37983
Copy link
Copy Markdown
Contributor Author

@JoasE JoasE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately my github copilot has reached it's token limit, so I can't request review's anymore until next week. If you assign copilot I will fix the issues, or I'll wait till next week

resultExpression = Convert(resultExpression, property.ClrType);
}

var converter = property.GetTypeMapping().Converter;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is copied over from above for CreateGetValueExpression

@JoasE

This comment was marked as resolved.

@JoasE JoasE marked this pull request as ready for review March 26, 2026 08:14
@JoasE JoasE requested a review from a team as a code owner March 26, 2026 08:14
@roji
Copy link
Copy Markdown
Member

roji commented Mar 26, 2026

@AndriySvyryd assigning to you for reviewing but let me know if you want me to do it.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 null values during JSON update serialization (ModificationCommand, RelationalJsonUtilities).
  • Apply value converter logic for JSON null tokens 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.

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!);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

propertyValue can be null here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s actually intentional here, since this change allows propertyValue to be null if the converter has indicated it can handle null values.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

propertyValue can be null here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@JoasE JoasE requested a review from AndriySvyryd March 27, 2026 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Json field with value converter when converter converts nulls isn't converted

4 participants