Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections;
using System.Text;
using System.Text.Json;
using Microsoft.EntityFrameworkCore.Storage.Internal;

namespace Microsoft.EntityFrameworkCore.Query.Internal;

Expand Down Expand Up @@ -89,8 +90,13 @@ void WriteJsonObject(Utf8JsonWriter writer, IComplexType complexType, object? ob
Check.DebugAssert(jsonPropertyName is not null);
writer.WritePropertyName(jsonPropertyName);

var jsonValueReaderWriter = property.GetJsonValueReaderWriter() ?? property.GetTypeMapping().JsonValueReaderWriter;

var propertyValue = property.GetGetter().GetClrValue(objectValue);
if (propertyValue is null)
if (propertyValue is null
#pragma warning disable EF1001 // Internal EF Core API usage.
&& jsonValueReaderWriter is not IJsonConvertedValueReaderWriter { Converter.ConvertsNulls: true })
#pragma warning restore EF1001 // Internal EF Core API usage.
{
if (!property.IsNullable)
{
Expand All @@ -101,9 +107,8 @@ void WriteJsonObject(Utf8JsonWriter writer, IComplexType complexType, object? ob
}
else
{
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

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.

@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

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.

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.

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

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.

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

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.

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

@JoasE JoasE Mar 31, 2026

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.

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

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3188,6 +3188,56 @@ private Expression CreateReadJsonPropertyValueExpression(
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

Expression nullExpression;
if (converter?.ConvertsNulls == true)
{
var typeMappingExpression = Call(
Convert(
_parentVisitor.Dependencies.LiftableConstantFactory.CreateLiftableConstant(
property,
LiftableConstantExpressionHelpers.BuildMemberAccessLambdaForProperty(property),
property.Name + "Property",
typeof(IPropertyBase)),
typeof(IReadOnlyProperty)),
PropertyGetTypeMappingMethod);

var converterExpression = (Expression)Property(typeMappingExpression, nameof(CoreTypeMapping.Converter));

var converterType = converter.GetType();
var typedConverterType = converterType.GetGenericTypeImplementations(typeof(ValueConverter<,>)).FirstOrDefault();
if (typedConverterType != null)
{
if (converterExpression.Type != converter.GetType())
{
converterExpression = Convert(converterExpression, converter.GetType());
}

nullExpression = Invoke(
Property(
converterExpression,
nameof(ValueConverter<object, object>.ConvertFromProviderTyped)),
Default(converter.ProviderClrType));
}
else
{
nullExpression = Invoke(
Property(
converterExpression,
nameof(ValueConverter.ConvertFromProvider)),
Default(typeof(object)));
}

if (nullExpression.Type != property.ClrType)
{
nullExpression = Convert(nullExpression, property.ClrType);
}
}
else
{
nullExpression = Default(property.ClrType);
}

resultExpression = Condition(
Equal(
Property(
Expand All @@ -3196,7 +3246,7 @@ private Expression CreateReadJsonPropertyValueExpression(
Utf8JsonReaderManagerCurrentReaderField),
Utf8JsonReaderTokenTypeProperty),
Constant(JsonTokenType.Null)),
Default(property.ClrType),
nullExpression,
resultExpression);
}

Expand Down
8 changes: 5 additions & 3 deletions src/EFCore.Relational/Update/ModificationCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
using Microsoft.EntityFrameworkCore.Internal;
using Microsoft.EntityFrameworkCore.Metadata.Internal;
using Microsoft.EntityFrameworkCore.Storage.Internal;
using IColumnMapping = Microsoft.EntityFrameworkCore.Metadata.IColumnMapping;
using ITableMapping = Microsoft.EntityFrameworkCore.Metadata.ITableMapping;

Expand Down Expand Up @@ -969,11 +970,12 @@ private void WriteJsonObject(
#pragma warning disable EF1001 // Internal EF Core API usage.
writer.WritePropertyName(jsonPropertyName);

if (propertyValue is not null)
var jsonValueReaderWriter = property.GetJsonValueReaderWriter() ?? property.GetTypeMapping().JsonValueReaderWriter;
if (propertyValue is not null ||
jsonValueReaderWriter is IJsonConvertedValueReaderWriter { Converter.ConvertsNulls: true })
{
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!);
Comment thread
JoasE marked this conversation as resolved.
Outdated
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,74 @@ public class JsonNestedType

#endregion HasJsonPropertyName

#region Value converter equality null scalar

[ConditionalFact]
public virtual async Task Value_converter_equality_null_scalar()
{
var contextFactory = await InitializeNonSharedTest<Context37983>(
onConfiguring: b => b.ConfigureWarnings(ConfigureWarnings),
onModelCreating: m => m.Entity<Context37983.Entity>().ComplexProperty(e => e.Json, b =>
{
b.ToJson();

b.Property(j => j.IntToString).HasConversion(new Context37983_StringToIntConverter());
}),
seed: context =>
{
context.Set<Context37983.Entity>().Add(new Context37983.Entity
{
Json = new Context37983.JsonComplexType
{
IntToString = null,
}
});

return context.SaveChangesAsync();
});

await using var context = contextFactory.CreateDbContext();

TestSqlLoggerFactory.Clear();

var complexType = new Context37983.JsonComplexType
{
IntToString = null,
};

Assert.Equal(1, await context.Set<Context37983.Entity>().CountAsync(e => e.Json == complexType));
}

protected class Context37983(DbContextOptions options) : DbContext(options)
{
public DbSet<Entity> Entities { get; set; }

public class Entity
{
public int Id { get; set; }
public JsonComplexType Json { get; set; }
}

public class JsonComplexType
{
public int? IntToString { get; set; }
}
}

protected class Context37983_StringToIntConverter : ValueConverter<int?, string>
{
public Context37983_StringToIntConverter()
: base(
v => v == null ? "<null>" : v.ToString(),
v => int.Parse(v))
{
}

public override bool ConvertsNulls => true;
}
Comment thread
JoasE marked this conversation as resolved.

#endregion

protected TestSqlLoggerFactory TestSqlLoggerFactory
=> (TestSqlLoggerFactory)ListLoggerFactory;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Net;
using System.Net.NetworkInformation;
using System.Text.Json;
using Microsoft.EntityFrameworkCore.TestModels.ConcurrencyModel;
Comment thread
JoasE marked this conversation as resolved.
Outdated

// ReSharper disable StaticMemberInGenericType
namespace Microsoft.EntityFrameworkCore;
Expand Down Expand Up @@ -168,38 +169,50 @@ public virtual async Task Can_insert_and_read_back_with_conversions(int[] valueO
{
var entity = new ConvertingEntity();

Add(context, entity);

SetPropertyValues(context, entity, valueOrder[0], -1);

context.Add(entity);
await context.SaveChangesAsync();

id = entity.Id;
}

using (var context = CreateContext())
{
SetPropertyValues(context, await context.Set<ConvertingEntity>().SingleAsync(e => e.Id == id), valueOrder[1], valueOrder[0]);
SetPropertyValues(context, await GetAsync(context, id), valueOrder[1], valueOrder[0]);
await context.SaveChangesAsync();
}

using (var context = CreateContext())
{
SetPropertyValues(context, await context.Set<ConvertingEntity>().SingleAsync(e => e.Id == id), valueOrder[2], valueOrder[1]);
SetPropertyValues(context, await GetAsync(context, id), valueOrder[2], valueOrder[1]);
await context.SaveChangesAsync();
}

using (var context = CreateContext())
{
SetPropertyValues(context, await context.Set<ConvertingEntity>().SingleAsync(e => e.Id == id), valueOrder[3], valueOrder[2]);
SetPropertyValues(context, await GetAsync(context, id), valueOrder[3], valueOrder[2]);
await context.SaveChangesAsync();
}
}

private static void SetPropertyValues(DbContext context, ConvertingEntity entity, int valueIndex, int previousValueIndex)
protected virtual void Add(DbContext context, ConvertingEntity entity)
=> context.Add(entity);

protected virtual Task<ConvertingEntity> GetAsync(DbContext context, Guid id)
=> context.Set<ConvertingEntity>().SingleAsync(e => e.Id == id);

protected virtual PropertyEntry Property(DbContext context, ConvertingEntity entity, IProperty property)
=> context.Entry(entity).Property(property);

protected virtual ITypeBase FindType(DbContext context)
=> context.Model.FindEntityType(
typeof(ConvertingEntity))!;

private void SetPropertyValues(DbContext context, ConvertingEntity entity, int valueIndex, int previousValueIndex)
{
var entry = context.Entry(entity);
foreach (var property in context.Model.FindEntityType(
entity.GetType())!.GetProperties().Where(p => !p.IsPrimaryKey() && !p.IsShadowProperty()))
foreach (var property in FindType(context).GetProperties().Where(p => !p.IsPrimaryKey() && !p.IsShadowProperty()))
{
var testValues = (property.ClrType == typeof(string)
? StringTestValues[property.GetValueConverter()!.ProviderClrType.UnwrapNullableType()]
Expand All @@ -211,7 +224,7 @@ private static void SetPropertyValues(DbContext context, ConvertingEntity entity
testValues[3] = null;
}

var propertyEntry = entry.Property(property);
var propertyEntry = Property(context, entity, property);

if (previousValueIndex >= 0
&& property.FindAnnotation("Relational:DefaultValue") == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -680,6 +680,20 @@ public override async Task Entity_splitting_with_owned_json()
SELECT TOP(2) [m].[Id], [m].[PropertyInMainTable], [o].[PropertyInOtherTable], [m].[Json]
FROM [MyEntity] AS [m]
INNER JOIN [OtherTable] AS [o] ON [m].[Id] = [o].[Id]
""");
}

public override async Task Value_converter_equality_null_scalar()
{
await base.Value_converter_equality_null_scalar();

AssertSql(
"""
@entity_equality_complexType='{"IntToString":"\u003Cnull\u003E"}' (Size = 34)

SELECT COUNT(*)
FROM [Entities] AS [e]
WHERE [e].[Json] = @entity_equality_complexType
""");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2497,9 +2497,9 @@ public override async Task Add_and_update_nested_optional_primitive_collection(b

var parameterSize = value switch
{
true => "1558",
false => "1555",
_ => "1557"
true => "1560",
false => "1557",
_ => "1559"
};

var updateParameter = value switch
Expand Down Expand Up @@ -2531,7 +2531,7 @@ public override async Task Add_and_update_nested_optional_primitive_collection(b
AssertSql(
@"@p0='[{""TestBoolean"":false,""TestBooleanCollection"":[],""TestByte"":0,""TestByteArray"":null,""TestByteCollection"":null,""TestCharacter"":""\u0000"",""TestCharacterCollection"":"
+ characterCollection
+ @",""TestDateOnly"":""0001-01-01"",""TestDateOnlyCollection"":[],""TestDateTime"":""0001-01-01T00:00:00"",""TestDateTimeCollection"":[],""TestDateTimeOffset"":""0001-01-01T00:00:00+00:00"",""TestDateTimeOffsetCollection"":[],""TestDecimal"":0,""TestDecimalCollection"":[],""TestDefaultString"":null,""TestDefaultStringCollection"":[],""TestDouble"":0,""TestDoubleCollection"":[],""TestEnum"":0,""TestEnumCollection"":[],""TestEnumWithIntConverter"":0,""TestEnumWithIntConverterCollection"":[],""TestGuid"":""00000000-0000-0000-0000-000000000000"",""TestGuidCollection"":[],""TestInt16"":0,""TestInt16Collection"":[],""TestInt32"":0,""TestInt32Collection"":[],""TestInt64"":0,""TestInt64Collection"":[],""TestMaxLengthString"":null,""TestMaxLengthStringCollection"":[],""TestNullableEnum"":null,""TestNullableEnumCollection"":[],""TestNullableEnumWithConverterThatHandlesNulls"":null,""TestNullableEnumWithConverterThatHandlesNullsCollection"":[],""TestNullableEnumWithIntConverter"":null,""TestNullableEnumWithIntConverterCollection"":[],""TestNullableInt32"":null,""TestNullableInt32Collection"":[],""TestSignedByte"":0,""TestSignedByteCollection"":[],""TestSingle"":0,""TestSingleCollection"":[],""TestTimeOnly"":""00:00:00.0000000"",""TestTimeOnlyCollection"":[],""TestTimeSpan"":""0:00:00"",""TestTimeSpanCollection"":[],""TestUnsignedInt16"":0,""TestUnsignedInt16Collection"":[],""TestUnsignedInt32"":0,""TestUnsignedInt32Collection"":[],""TestUnsignedInt64"":0,""TestUnsignedInt64Collection"":[]}]' (Nullable = false) (Size = "
+ @",""TestDateOnly"":""0001-01-01"",""TestDateOnlyCollection"":[],""TestDateTime"":""0001-01-01T00:00:00"",""TestDateTimeCollection"":[],""TestDateTimeOffset"":""0001-01-01T00:00:00+00:00"",""TestDateTimeOffsetCollection"":[],""TestDecimal"":0,""TestDecimalCollection"":[],""TestDefaultString"":null,""TestDefaultStringCollection"":[],""TestDouble"":0,""TestDoubleCollection"":[],""TestEnum"":0,""TestEnumCollection"":[],""TestEnumWithIntConverter"":0,""TestEnumWithIntConverterCollection"":[],""TestGuid"":""00000000-0000-0000-0000-000000000000"",""TestGuidCollection"":[],""TestInt16"":0,""TestInt16Collection"":[],""TestInt32"":0,""TestInt32Collection"":[],""TestInt64"":0,""TestInt64Collection"":[],""TestMaxLengthString"":null,""TestMaxLengthStringCollection"":[],""TestNullableEnum"":null,""TestNullableEnumCollection"":[],""TestNullableEnumWithConverterThatHandlesNulls"":""Null"",""TestNullableEnumWithConverterThatHandlesNullsCollection"":[],""TestNullableEnumWithIntConverter"":null,""TestNullableEnumWithIntConverterCollection"":[],""TestNullableInt32"":null,""TestNullableInt32Collection"":[],""TestSignedByte"":0,""TestSignedByteCollection"":[],""TestSingle"":0,""TestSingleCollection"":[],""TestTimeOnly"":""00:00:00.0000000"",""TestTimeOnlyCollection"":[],""TestTimeSpan"":""0:00:00"",""TestTimeSpanCollection"":[],""TestUnsignedInt16"":0,""TestUnsignedInt16Collection"":[],""TestUnsignedInt32"":0,""TestUnsignedInt32Collection"":[],""TestUnsignedInt64"":0,""TestUnsignedInt64Collection"":[]}]' (Nullable = false) (Size = "
+ parameterSize
+ @")
@p1='7624'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2445,9 +2445,9 @@ public override async Task Add_and_update_nested_optional_primitive_collection(b

var parameterSize = value switch
{
true => "1558",
false => "1555",
_ => "1557"
true => "1560",
false => "1557",
_ => "1559"
};

var updateParameterSize = value switch
Expand All @@ -2466,7 +2466,7 @@ public override async Task Add_and_update_nested_optional_primitive_collection(b

var p0 = @"@p0='[{""TestBoolean"":false,""TestBooleanCollection"":[],""TestByte"":0,""TestByteArray"":null,""TestByteCollection"":null,""TestCharacter"":""\u0000"",""TestCharacterCollection"":"
+ characterCollection
+ @",""TestDateOnly"":""0001-01-01"",""TestDateOnlyCollection"":[],""TestDateTime"":""0001-01-01T00:00:00"",""TestDateTimeCollection"":[],""TestDateTimeOffset"":""0001-01-01T00:00:00+00:00"",""TestDateTimeOffsetCollection"":[],""TestDecimal"":0,""TestDecimalCollection"":[],""TestDefaultString"":null,""TestDefaultStringCollection"":[],""TestDouble"":0,""TestDoubleCollection"":[],""TestEnum"":0,""TestEnumCollection"":[],""TestEnumWithIntConverter"":0,""TestEnumWithIntConverterCollection"":[],""TestGuid"":""00000000-0000-0000-0000-000000000000"",""TestGuidCollection"":[],""TestInt16"":0,""TestInt16Collection"":[],""TestInt32"":0,""TestInt32Collection"":[],""TestInt64"":0,""TestInt64Collection"":[],""TestMaxLengthString"":null,""TestMaxLengthStringCollection"":[],""TestNullableEnum"":null,""TestNullableEnumCollection"":[],""TestNullableEnumWithConverterThatHandlesNulls"":null,""TestNullableEnumWithConverterThatHandlesNullsCollection"":[],""TestNullableEnumWithIntConverter"":null,""TestNullableEnumWithIntConverterCollection"":[],""TestNullableInt32"":null,""TestNullableInt32Collection"":[],""TestSignedByte"":0,""TestSignedByteCollection"":[],""TestSingle"":0,""TestSingleCollection"":[],""TestTimeOnly"":""00:00:00.0000000"",""TestTimeOnlyCollection"":[],""TestTimeSpan"":""0:00:00"",""TestTimeSpanCollection"":[],""TestUnsignedInt16"":0,""TestUnsignedInt16Collection"":[],""TestUnsignedInt32"":0,""TestUnsignedInt32Collection"":[],""TestUnsignedInt64"":0,""TestUnsignedInt64Collection"":[]}]' (Nullable = false) (Size = "
+ @",""TestDateOnly"":""0001-01-01"",""TestDateOnlyCollection"":[],""TestDateTime"":""0001-01-01T00:00:00"",""TestDateTimeCollection"":[],""TestDateTimeOffset"":""0001-01-01T00:00:00+00:00"",""TestDateTimeOffsetCollection"":[],""TestDecimal"":0,""TestDecimalCollection"":[],""TestDefaultString"":null,""TestDefaultStringCollection"":[],""TestDouble"":0,""TestDoubleCollection"":[],""TestEnum"":0,""TestEnumCollection"":[],""TestEnumWithIntConverter"":0,""TestEnumWithIntConverterCollection"":[],""TestGuid"":""00000000-0000-0000-0000-000000000000"",""TestGuidCollection"":[],""TestInt16"":0,""TestInt16Collection"":[],""TestInt32"":0,""TestInt32Collection"":[],""TestInt64"":0,""TestInt64Collection"":[],""TestMaxLengthString"":null,""TestMaxLengthStringCollection"":[],""TestNullableEnum"":null,""TestNullableEnumCollection"":[],""TestNullableEnumWithConverterThatHandlesNulls"":""Null"",""TestNullableEnumWithConverterThatHandlesNullsCollection"":[],""TestNullableEnumWithIntConverter"":null,""TestNullableEnumWithIntConverterCollection"":[],""TestNullableInt32"":null,""TestNullableInt32Collection"":[],""TestSignedByte"":0,""TestSignedByteCollection"":[],""TestSingle"":0,""TestSingleCollection"":[],""TestTimeOnly"":""00:00:00.0000000"",""TestTimeOnlyCollection"":[],""TestTimeSpan"":""0:00:00"",""TestTimeSpanCollection"":[],""TestUnsignedInt16"":0,""TestUnsignedInt16Collection"":[],""TestUnsignedInt32"":0,""TestUnsignedInt32Collection"":[],""TestUnsignedInt64"":0,""TestUnsignedInt64Collection"":[]}]' (Nullable = false) (Size = "
+ parameterSize
+ @")";

Expand Down
Loading
Loading