Fix run value converters that convert nulls for JSON columns by JoasE · Pull Request #37984 · dotnet/efcore

JoasE · 2026-03-24T15:07:06Z

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

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

JoasE

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

JoasE · 2026-03-24T15:08:48Z

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

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

+                var converter = property.GetTypeMapping().Converter;


This is copied over from above for CreateGetValueExpression

roji · 2026-03-26T08:58:45Z

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

Copilot

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.

test/EFCore.Specification.Tests/ValueConvertersEndToEndTestBase.cs

test/EFCore.Relational.Specification.Tests/Query/AdHocJsonQueryRelationalTestBase.cs

AndriySvyryd · 2026-03-27T00:08:00Z

src/EFCore.Relational/Query/Internal/RelationalJsonUtilities.cs

-                    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!);


propertyValue can be null here

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

Then you should change the signature of ToJson to allow nulls

@AndriySvyryd That's a good point! I had some reservations about that but didn't really give it much thought. I've looked at it more now and FWIG JsonValueReaderWriter currently doesn't allow null input by design, and I'm not sure changing that would make sense and/or would require a different setup all together.

If I change JsonValueReaderWriter to allow null argument for object value, I can't just pass it to ToJsonTyped. I would have to make ToJsonTyped value argument nullable aswell, but I don't think that would be desired. AFAIK The typed JsonValueReaderWriter<T> should generally not have to worry about null values. I've considered having JsonConvertedValueReaderWriter inherit from non-typed JsonValueReaderWriter to solve this. Then I could add a null check and write in the JsonValueReaderWriter<T>.ToJson implementation and also the JsonConvertedValueReaderWriter.ToJson implementation. However, materialization works with the typed JsonValueReaderWriter<T>.FromJsonTyped, so the JsonConvertedValueReaderWriter wouldn't be usable in materialization. I think that if we want to make ToJson accept null and do the null write for the value, the only option that makes sense would be unsealing JsonValueReaderWriter<T>.ToJson so that I can override it in JsonConvertedValueReaderWriter. However, I'm not sure that is desirable either.

Some workarounds could be:

Adding a method to IJsonConvertedValueReaderWriter that allows null input, then doing the ConvertsNulls check and null write there

Leveraging IJsonConvertedValueReaderWriter.ValueConverter and ICompositeJsonValueReaderWriter.InnerReaderWriter to circumvent JsonConvertedValueReaderWriter.ToJson. Invoking the ValueConverter manually and checking if it converted to null (seems cumbersome)

Accepting the null suppression for this specific instance

Side note that I ran into this when working on:
https://github.com/dotnet/efcore/pull/38024/changes#diff-62e5c8b19c66714cf0310ceff8fb90312c039fb64dc9e7af31241e8956bb412dR103-R116

I think this exposed the reason I find this suppression smelly: it's relying on implementation details. You could add JsonValueReaderWriter.HandlesNulls, make the value nullable and throw if it's null and HandlesNulls is false

@AndriySvyryd That makes a lot of sense! I've added JsonValueReaderWriter.HandlesNulls

I'm wondering now how this should (not) affect FromJson

I think we can have FromJson stay unaffected, but would recommend renaming to HandlesNullWrites
Otherwhise a similar signature change for FromJson would be needed, but materialization uses FromJsonTyped

src/EFCore.Relational/Update/ModificationCommand.cs

…et#37983

Fix run value converters that convert nulls for JSON columns

adcc266

During (de)serialization, check if converter converts nulls and run for null values if so Closes: dotnet#37983

github-actions bot added the community-contribution label Mar 24, 2026

JoasE commented Mar 24, 2026

View reviewed changes

JoasE mentioned this pull request Mar 24, 2026

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

Open

Add tests

8399c8d

This comment was marked as resolved.

Sign in to view

JoasE added 2 commits March 24, 2026 16:49

Fix RelationalJsonUtilities

fe913b6

Fix tests

b6cdfc5

JoasE marked this pull request as ready for review March 26, 2026 08:14

JoasE requested a review from a team as a code owner March 26, 2026 08:14

roji assigned AndriySvyryd Mar 26, 2026

AndriySvyryd requested a review from Copilot March 26, 2026 23:42

Copilot started reviewing on behalf of AndriySvyryd March 26, 2026 23:43 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

test/EFCore.Specification.Tests/ValueConvertersEndToEndTestBase.cs Outdated Show resolved Hide resolved

test/EFCore.Relational.Specification.Tests/Query/AdHocJsonQueryRelationalTestBase.cs Show resolved Hide resolved

AndriySvyryd reviewed Mar 27, 2026

View reviewed changes

src/EFCore.Relational/Update/ModificationCommand.cs Outdated Show resolved Hide resolved

Clean

4fcee53

JoasE requested a review from AndriySvyryd March 27, 2026 11:17

JoasE added 6 commits March 30, 2026 11:30

Workaround 1

84dae2f

Merge branch 'main' of https://github.com/dotnet/efcore into fix/dotn…

7a2e95b

…et#37983

Add HandlesNulls to JsonValueReaderWriter

becc2ce

Clean

c7fada1

Rename to HandlesNullWrites

ab4749b

Clean

1b592cf

Conversation

JoasE commented Mar 24, 2026

Uh oh!

JoasE left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

roji commented Mar 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoasE Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JoasE left a comment •

edited

Loading

JoasE Mar 31, 2026 •

edited

Loading