Skip to content

Commit 78e671c

Browse files
rojiroji
committed
Fix JSON null partial updates (#3855)
Fixes #3850 (cherry picked from commit 5f95083)
1 parent e6d56b4 commit 78e671c

20 files changed

Lines changed: 192 additions & 96 deletions

‎src/EFCore.PG/Query/Internal/NpgsqlQueryableMethodTranslatingExpressionVisitor.cs‎

Lines changed: 118 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ public class NpgsqlQueryableMethodTranslatingExpressionVisitor : RelationalQuery
2020
private readonly RelationalQueryCompilationContext _queryCompilationContext;
2121
private readonly NpgsqlTypeMappingSource _typeMappingSource;
2222
private readonly NpgsqlSqlExpressionFactory _sqlExpressionFactory;
23+
private readonly Version _postgresVersion;
2324
private readonly bool _isRedshift;
2425
private RelationalTypeMapping? _ordinalityTypeMapping;
2526

@@ -55,6 +56,7 @@ public NpgsqlQueryableMethodTranslatingExpressionVisitor(
5556
_queryCompilationContext = queryCompilationContext;
5657
_typeMappingSource = (NpgsqlTypeMappingSource)relationalDependencies.TypeMappingSource;
5758
_sqlExpressionFactory = (NpgsqlSqlExpressionFactory)relationalDependencies.SqlExpressionFactory;
59+
_postgresVersion = npgsqlSingletonOptions.PostgresVersion;
5860
_isRedshift = npgsqlSingletonOptions.UseRedshift;
5961
}
6062

@@ -70,6 +72,7 @@ protected NpgsqlQueryableMethodTranslatingExpressionVisitor(NpgsqlQueryableMetho
7072
_queryCompilationContext = parentVisitor._queryCompilationContext;
7173
_typeMappingSource = parentVisitor._typeMappingSource;
7274
_sqlExpressionFactory = parentVisitor._sqlExpressionFactory;
75+
_postgresVersion = parentVisitor._postgresVersion;
7376
_isRedshift = parentVisitor._isRedshift;
7477
}
7578

@@ -1274,6 +1277,14 @@ protected override bool TrySerializeScalarToJson(
12741277
{
12751278
var jsonTypeMapping = ((ColumnExpression)target.Json).TypeMapping!;
12761279

1280+
if (IsSqlNull(value))
1281+
{
1282+
jsonValue = JsonNull(jsonTypeMapping);
1283+
return true;
1284+
}
1285+
1286+
var coalesceToJsonNull = !SupportsJsonSetLax(jsonTypeMapping) && MayReturnNull(value);
1287+
12771288
if (
12781289
// The base implementation doesn't handle serializing arbitrary SQL expressions to JSON, since that's
12791290
// database-specific. In PostgreSQL we simply do this by wrapping any expression in to_jsonb().
@@ -1286,7 +1297,7 @@ protected override bool TrySerializeScalarToJson(
12861297
switch (value.TypeMapping!.StoreType)
12871298
{
12881299
case "jsonb" or "json":
1289-
jsonValue = value;
1300+
jsonValue = coalesceToJsonNull ? CoalesceToJsonNull(value, jsonTypeMapping) : value;
12901301
return true;
12911302

12921303
case "bytea":
@@ -1319,24 +1330,39 @@ protected override bool TrySerializeScalarToJson(
13191330
jsonScalarValue.Type,
13201331
jsonTypeMapping,
13211332
jsonScalarValue.IsNullable);
1333+
1334+
if (coalesceToJsonNull)
1335+
{
1336+
jsonValue = CoalesceToJsonNull(jsonValue, jsonTypeMapping);
1337+
}
1338+
13221339
return true;
13231340
}
13241341

1325-
jsonValue = _sqlExpressionFactory.Function(
1342+
var valueForJsonScalar = NeedsJsonScalarTypeInference(value) ? CastForJsonScalarTypeInference(value, target) : value;
1343+
var toJson = _sqlExpressionFactory.Function(
13261344
jsonTypeMapping.StoreType switch
13271345
{
13281346
"jsonb" => "to_jsonb",
13291347
"json" => "to_json",
13301348
_ => throw new UnreachableException()
13311349
},
1332-
// Make sure PG interprets constant values correctly by adding explicit typing based on the target property's type mapping.
1350+
// Make sure PG interprets constant/nullable parameter values correctly by adding explicit typing based on the target property's
1351+
// type mapping.
13331352
// Note that we can only be here for scalar properties, for structural types we always already get a jsonb/json value
13341353
// and don't need to add to_jsonb/to_json.
1335-
[value is SqlConstantExpression ? _sqlExpressionFactory.Convert(value, target.Type, target.TypeMapping) : value],
1354+
[valueForJsonScalar],
13361355
nullable: true,
13371356
argumentsPropagateNullability: [true],
13381357
typeof(string),
13391358
jsonTypeMapping);
1359+
1360+
jsonValue = toJson;
1361+
}
1362+
1363+
if (coalesceToJsonNull)
1364+
{
1365+
jsonValue = CoalesceToJsonNull(jsonValue, jsonTypeMapping);
13401366
}
13411367

13421368
return true;
@@ -1362,26 +1388,46 @@ protected override bool TrySerializeScalarToJson(
13621388
_ => throw new UnreachableException(),
13631389
};
13641390

1365-
var jsonSet = _sqlExpressionFactory.Function(
1366-
jsonColumn.TypeMapping?.StoreType switch
1367-
{
1368-
"jsonb" => "jsonb_set",
1369-
"json" => "json_set",
1370-
_ => throw new UnreachableException()
1371-
},
1372-
arguments:
1373-
[
1374-
existingSetterValue ?? jsonColumn,
1375-
// Hack: Rendering of JSONPATH strings happens in value generation. We can have a special expression for modify to hold the
1376-
// IReadOnlyList<PathSegment> (just like Json{Scalar,Query}Expression), but instead we do the slight hack of packaging it
1377-
// as a constant argument; it will be unpacked and handled in SQL generation.
1378-
_sqlExpressionFactory.Constant(path, RelationalTypeMapping.NullMapping),
1379-
value
1380-
],
1381-
nullable: true,
1382-
argumentsPropagateNullability: [true, true, true],
1383-
typeof(string),
1384-
jsonColumn.TypeMapping);
1391+
var jsonTypeMapping = jsonColumn.TypeMapping!;
1392+
var isNullConstant = IsJsonNull(value);
1393+
var isConstant = value is SqlConstantExpression;
1394+
var valueMayBeNull = !isNullConstant && MayReturnNull(value);
1395+
var supportsJsonSetLax = SupportsJsonSetLax(jsonTypeMapping);
1396+
1397+
value = isNullConstant
1398+
? JsonNull(jsonTypeMapping)
1399+
: supportsJsonSetLax || isConstant || !valueMayBeNull
1400+
? value
1401+
: CoalesceToJsonNull(value, jsonTypeMapping);
1402+
1403+
var useJsonSetLax = supportsJsonSetLax && (valueMayBeNull || isNullConstant);
1404+
var jsonSetTarget = existingSetterValue ?? jsonColumn;
1405+
1406+
// Hack: Rendering of JSONPATH strings happens in value generation. We can have a special expression for modify to hold the
1407+
// IReadOnlyList<PathSegment> (just like Json{Scalar,Query}Expression), but instead we do the slight hack of packaging it
1408+
// as a constant argument; it will be unpacked and handled in SQL generation.
1409+
var jsonPath = _sqlExpressionFactory.Constant(path, RelationalTypeMapping.NullMapping);
1410+
1411+
var jsonSet = useJsonSetLax
1412+
? _sqlExpressionFactory.Function(
1413+
"jsonb_set_lax",
1414+
[jsonSetTarget, jsonPath, value],
1415+
nullable: true,
1416+
argumentsPropagateNullability: [true, true, false],
1417+
typeof(string),
1418+
jsonTypeMapping)
1419+
: _sqlExpressionFactory.Function(
1420+
jsonTypeMapping.StoreType switch
1421+
{
1422+
"jsonb" => "jsonb_set",
1423+
"json" => "json_set",
1424+
_ => throw new UnreachableException()
1425+
},
1426+
[jsonSetTarget, jsonPath, value],
1427+
nullable: true,
1428+
argumentsPropagateNullability: [true, true, !isNullConstant],
1429+
typeof(string),
1430+
jsonTypeMapping);
13851431

13861432
if (existingSetterValue is null)
13871433
{
@@ -1394,6 +1440,54 @@ protected override bool TrySerializeScalarToJson(
13941440
}
13951441
}
13961442

1443+
private SqlExpression JsonNull(RelationalTypeMapping jsonTypeMapping)
1444+
=> new SqlUnaryExpression(
1445+
ExpressionType.Convert,
1446+
_sqlExpressionFactory.Constant("null"),
1447+
jsonTypeMapping.ClrType,
1448+
jsonTypeMapping);
1449+
1450+
private SqlExpression CoalesceToJsonNull(SqlExpression value, RelationalTypeMapping jsonTypeMapping)
1451+
=> _sqlExpressionFactory.Coalesce(value, JsonNull(jsonTypeMapping), jsonTypeMapping);
1452+
1453+
private bool SupportsJsonSetLax(RelationalTypeMapping jsonTypeMapping)
1454+
=> jsonTypeMapping.StoreType == "jsonb" && !_isRedshift && _postgresVersion.AtLeast(16);
1455+
1456+
private static bool IsNullConstant(SqlExpression expression)
1457+
=> expression is SqlConstantExpression { Value: null or DBNull };
1458+
1459+
private static bool IsSqlNull(SqlExpression expression)
1460+
=> IsNullConstant(expression)
1461+
|| expression is SqlFragmentExpression { Sql: "NULL" };
1462+
1463+
private static bool IsJsonNull(SqlExpression expression)
1464+
=> IsSqlNull(expression)
1465+
|| expression is SqlFunctionExpression
1466+
{
1467+
Name: "to_jsonb" or "to_json",
1468+
Arguments: [var argument]
1469+
} && IsSqlNull(argument);
1470+
1471+
private static bool NeedsJsonScalarTypeInference(SqlExpression expression)
1472+
=> expression is SqlConstantExpression;
1473+
1474+
private static SqlExpression CastForJsonScalarTypeInference(SqlExpression expression, JsonScalarExpression target)
1475+
=> new SqlUnaryExpression(ExpressionType.Convert, expression, typeof(object), target.TypeMapping ?? expression.TypeMapping);
1476+
1477+
private static bool MayReturnNull(SqlExpression expression)
1478+
=> expression switch
1479+
{
1480+
SqlConstantExpression { Value: null or DBNull } => true,
1481+
SqlFragmentExpression { Sql: "NULL" } => true,
1482+
SqlParameterExpression { IsNullable: true } => true,
1483+
ColumnExpression { IsNullable: true } => true,
1484+
JsonScalarExpression { IsNullable: true } => true,
1485+
PgJsonTraversalExpression => true,
1486+
SqlFunctionExpression { IsNullable: true } => true,
1487+
1488+
_ => false
1489+
};
1490+
13971491
#endregion ExecuteUpdate
13981492

13991493
/// <summary>

‎test/EFCore.PG.FunctionalTests/Query/Associations/ComplexJson/ComplexJsonBulkUpdateNpgsqlTest.cs‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
using Microsoft.EntityFrameworkCore.Query.Associations;
2+
13
namespace Microsoft.EntityFrameworkCore.Query.Associations.ComplexJson;
24

35
public class ComplexJsonBulkUpdateNpgsqlTest(
@@ -47,7 +49,7 @@ public override async Task Update_property_inside_associate()
4749
@p='?'
4850
4951
UPDATE "RootEntity" AS r
50-
SET "RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{String}', to_jsonb(@p))
52+
SET "RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{String}', to_jsonb(@p))
5153
""");
5254
}
5355

@@ -58,7 +60,7 @@ public override async Task Update_property_inside_associate_with_special_chars()
5860
AssertExecuteUpdateSql(
5961
"""
6062
UPDATE "RootEntity" AS r
61-
SET "RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{String}', to_jsonb('{ Some other/JSON:like text though it [isn''t]: ממש ממש לאéèéè }'::text))
63+
SET "RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{String}', to_jsonb('{ Some other/JSON:like text though it [isn''t]: ממש ממש לאéèéè }'::text))
6264
WHERE (r."RequiredAssociate" ->> 'String') = '{ this may/look:like JSON but it [isn''t]: ממש ממש לאéèéè }'
6365
""");
6466
}
@@ -72,7 +74,7 @@ public override async Task Update_property_inside_nested_associate()
7274
@p='?'
7375
7476
UPDATE "RootEntity" AS r
75-
SET "RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{RequiredNestedAssociate,String}', to_jsonb(@p))
77+
SET "RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{RequiredNestedAssociate,String}', to_jsonb(@p))
7678
""");
7779
}
7880

@@ -85,7 +87,7 @@ public override async Task Update_property_on_projected_associate()
8587
@p='?'
8688
8789
UPDATE "RootEntity" AS r
88-
SET "RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{String}', to_jsonb(@p))
90+
SET "RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{String}', to_jsonb(@p))
8991
""");
9092
}
9193

@@ -129,7 +131,7 @@ public override async Task Update_nested_associate_to_parameter()
129131
@complex_type_p='?' (DbType = Object)
130132
131133
UPDATE "RootEntity" AS r
132-
SET "RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{RequiredNestedAssociate}', @complex_type_p)
134+
SET "RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{RequiredNestedAssociate}', @complex_type_p)
133135
""");
134136
}
135137

@@ -256,7 +258,7 @@ public override async Task Update_nested_collection_to_parameter()
256258
@complex_type_p='?' (DbType = Object)
257259
258260
UPDATE "RootEntity" AS r
259-
SET "RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{NestedCollection}', @complex_type_p)
261+
SET "RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{NestedCollection}', @complex_type_p)
260262
""");
261263
}
262264

@@ -278,7 +280,7 @@ public override async Task Update_nested_collection_to_another_nested_collection
278280
AssertExecuteUpdateSql(
279281
"""
280282
UPDATE "RootEntity" AS r
281-
SET "RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{NestedCollection}', r."OptionalAssociate" -> 'NestedCollection')
283+
SET "RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{NestedCollection}', r."OptionalAssociate" -> 'NestedCollection')
282284
WHERE (r."OptionalAssociate") IS NOT NULL
283285
""");
284286
}
@@ -321,7 +323,7 @@ public override async Task Update_primitive_collection_to_parameter()
321323
@ints='?' (DbType = Object)
322324
323325
UPDATE "RootEntity" AS r
324-
SET "RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{Ints}', @ints)
326+
SET "RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{Ints}', @ints)
325327
""");
326328
}
327329

@@ -345,7 +347,7 @@ public override async Task Update_inside_primitive_collection()
345347
@p='?' (DbType = Int32)
346348
347349
UPDATE "RootEntity" AS r
348-
SET "RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{Ints,1}', to_jsonb(@p))
350+
SET "RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{Ints,1}', to_jsonb(@p))
349351
WHERE jsonb_array_length(r."RequiredAssociate" -> 'Ints') >= 2
350352
""");
351353
}
@@ -364,7 +366,7 @@ public override async Task Update_multiple_properties_inside_same_associate()
364366
@p1='?' (DbType = Int32)
365367
366368
UPDATE "RootEntity" AS r
367-
SET "RequiredAssociate" = jsonb_set(jsonb_set(r."RequiredAssociate", '{String}', to_jsonb(@p)), '{Int}', to_jsonb(@p1))
369+
SET "RequiredAssociate" = jsonb_set_lax(jsonb_set_lax(r."RequiredAssociate", '{String}', to_jsonb(@p)), '{Int}', to_jsonb(@p1))
368370
""");
369371
}
370372

@@ -378,8 +380,8 @@ public override async Task Update_multiple_properties_inside_associates_and_on_e
378380
379381
UPDATE "RootEntity" AS r
380382
SET "Name" = r."Name" || 'Modified',
381-
"RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{String}', r."OptionalAssociate" -> 'String'),
382-
"OptionalAssociate" = jsonb_set(r."OptionalAssociate", '{RequiredNestedAssociate,String}', to_jsonb(@p))
383+
"RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{String}', r."OptionalAssociate" -> 'String'),
384+
"OptionalAssociate" = jsonb_set_lax(r."OptionalAssociate", '{RequiredNestedAssociate,String}', to_jsonb(@p))
383385
WHERE (r."OptionalAssociate") IS NOT NULL
384386
""");
385387
}
@@ -393,8 +395,8 @@ public override async Task Update_multiple_projected_associates_via_anonymous_ty
393395
@p='?'
394396
395397
UPDATE "RootEntity" AS r
396-
SET "RequiredAssociate" = jsonb_set(r."RequiredAssociate", '{String}', r."OptionalAssociate" -> 'String'),
397-
"OptionalAssociate" = jsonb_set(r."OptionalAssociate", '{String}', to_jsonb(@p))
398+
SET "RequiredAssociate" = jsonb_set_lax(r."RequiredAssociate", '{String}', r."OptionalAssociate" -> 'String'),
399+
"OptionalAssociate" = jsonb_set_lax(r."OptionalAssociate", '{String}', to_jsonb(@p))
398400
WHERE (r."OptionalAssociate") IS NOT NULL
399401
""");
400402
}

‎test/EFCore.PG.FunctionalTests/Types/Miscellaneous/NpgsqlBoolTypeTest.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public override async Task ExecuteUpdate_within_json_to_parameter()
5858
@Fixture_OtherValue='False'
5959
6060
UPDATE "JsonTypeEntity" AS j
61-
SET "JsonContainer" = jsonb_set(j."JsonContainer", '{Value}', to_jsonb(@Fixture_OtherValue))
61+
SET "JsonContainer" = jsonb_set_lax(j."JsonContainer", '{Value}', to_jsonb(@Fixture_OtherValue))
6262
""");
6363
}
6464

@@ -69,7 +69,7 @@ public override async Task ExecuteUpdate_within_json_to_constant()
6969
AssertSql(
7070
"""
7171
UPDATE "JsonTypeEntity" AS j
72-
SET "JsonContainer" = jsonb_set(j."JsonContainer", '{Value}', to_jsonb(FALSE::boolean))
72+
SET "JsonContainer" = jsonb_set_lax(j."JsonContainer", '{Value}', to_jsonb(FALSE::boolean))
7373
""");
7474
}
7575

@@ -91,7 +91,7 @@ public override async Task ExecuteUpdate_within_json_to_nonjson_column()
9191
AssertSql(
9292
"""
9393
UPDATE "JsonTypeEntity" AS j
94-
SET "JsonContainer" = jsonb_set(j."JsonContainer", '{Value}', to_jsonb(j."OtherValue"))
94+
SET "JsonContainer" = jsonb_set_lax(j."JsonContainer", '{Value}', to_jsonb(j."OtherValue"))
9595
""");
9696
}
9797

‎test/EFCore.PG.FunctionalTests/Types/Miscellaneous/NpgsqlByteArrayTypeTest.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public override async Task ExecuteUpdate_within_json_to_parameter()
5858
@Fixture_OtherValue='0x04050607'
5959
6060
UPDATE "JsonTypeEntity" AS j
61-
SET "JsonContainer" = jsonb_set(j."JsonContainer", '{Value}', to_jsonb(encode(@Fixture_OtherValue, 'base64')))
61+
SET "JsonContainer" = jsonb_set_lax(j."JsonContainer", '{Value}', to_jsonb(encode(@Fixture_OtherValue, 'base64')))
6262
""");
6363
}
6464

@@ -69,7 +69,7 @@ public override async Task ExecuteUpdate_within_json_to_constant()
6969
AssertSql(
7070
"""
7171
UPDATE "JsonTypeEntity" AS j
72-
SET "JsonContainer" = jsonb_set(j."JsonContainer", '{Value}', to_jsonb(encode(BYTEA E'\\x04050607', 'base64')))
72+
SET "JsonContainer" = jsonb_set_lax(j."JsonContainer", '{Value}', to_jsonb(encode(BYTEA E'\\x04050607', 'base64')))
7373
""");
7474
}
7575

@@ -80,7 +80,7 @@ public override async Task ExecuteUpdate_within_json_to_another_json_property()
8080
AssertSql(
8181
"""
8282
UPDATE "JsonTypeEntity" AS j
83-
SET "JsonContainer" = jsonb_set(j."JsonContainer", '{Value}', to_jsonb(encode(decode(j."JsonContainer" ->> 'OtherValue', 'base64'), 'base64')))
83+
SET "JsonContainer" = jsonb_set_lax(j."JsonContainer", '{Value}', to_jsonb(encode(decode(j."JsonContainer" ->> 'OtherValue', 'base64'), 'base64')))
8484
""");
8585
}
8686

@@ -91,7 +91,7 @@ public override async Task ExecuteUpdate_within_json_to_nonjson_column()
9191
AssertSql(
9292
"""
9393
UPDATE "JsonTypeEntity" AS j
94-
SET "JsonContainer" = jsonb_set(j."JsonContainer", '{Value}', to_jsonb(encode(j."OtherValue", 'base64')))
94+
SET "JsonContainer" = jsonb_set_lax(j."JsonContainer", '{Value}', to_jsonb(encode(j."OtherValue", 'base64')))
9595
""");
9696
}
9797

0 commit comments

Comments
 (0)