Skip to content

Commit 1bd5d72

Browse files
rojiroji
committed
Address review comments
1 parent 606e906 commit 1bd5d72

File tree

14 files changed

+287
-59
lines changed

14 files changed

+287
-59
lines changed

‎src/EFCore.Relational/Properties/RelationalStrings.Designer.cs‎

Lines changed: 9 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/EFCore.Relational/Properties/RelationalStrings.resx‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1075,8 +1075,11 @@
10751075
<data name="ParameterNotObjectArray" xml:space="preserve">
10761076
<value>The value provided for parameter '{parameter}' cannot be used because it isn't assignable to type 'object[]'.</value>
10771077
</data>
1078-
<data name="JsonPartialUpdateNotSupportedByProvider" xml:space="preserve">
1079-
<value>The provider in use does not support partial updates within JSON columns.</value>
1078+
<data name="JsonPartialExecuteUpdateNotSupportedByProvider" xml:space="preserve">
1079+
<value>The provider in use does not support partial updates with ExecuteUpdate within JSON columns.</value>
1080+
</data>
1081+
<data name="JsonExecuteUpdateNotSupportedWithOwnedEntities" xml:space="preserve">
1082+
<value>ExecuteUpdate over JSON columns is not supported when the column is mapped as an owned entity. Map the column as a complex type instead.</value>
10801083
</data>
10811084
<data name="PendingAmbientTransaction" xml:space="preserve">
10821085
<value>This connection was used with an ambient transaction. The original ambient transaction needs to be completed before this connection can be used outside of it.</value>

‎src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.ExecuteUpdate.cs‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,11 @@ jsonScalar.Path is []
198198
bool TryProcessStructuralJsonSetter(JsonQueryExpression jsonQuery)
199199
{
200200
var jsonColumn = jsonQuery.JsonColumn;
201-
var complexType = (IComplexType)jsonQuery.StructuralType;
201+
202+
if (jsonQuery.StructuralType is not IComplexType complexType)
203+
{
204+
throw new InvalidOperationException(RelationalStrings.JsonExecuteUpdateNotSupportedWithOwnedEntities);
205+
}
202206

203207
Check.DebugAssert(jsonColumn.TypeMapping is not null);
204208

@@ -261,7 +265,7 @@ bool TryProcessStructuralJsonSetter(JsonQueryExpression jsonQuery)
261265
// See #30768 for stopping producing empty Json{Scalar,Query}Expressions.
262266
// Otherwise, convert the JsonQueryExpression to a JsonScalarExpression, which is our current representation for a complex
263267
// JSON in the SQL tree (as opposed to in the shaper) - see #36392.
264-
SqlExpression ProcessJsonQuery(JsonQueryExpression jsonQuery)
268+
static SqlExpression ProcessJsonQuery(JsonQueryExpression jsonQuery)
265269
=> jsonQuery.Path is []
266270
? jsonQuery.JsonColumn
267271
: new JsonScalarExpression(
@@ -309,20 +313,14 @@ bool TryProcessColumn(ColumnExpression column)
309313

310314
case IComplexProperty { ComplexType: var complexType } complexProperty:
311315
{
312-
// TODO: Make this better with #36646
316+
// Find the container column in the relational model to get its type mapping
317+
// Note that we assume exactly one column with the given name mapped to the entity (despite entity splitting).
318+
// See #36647 and #36646 about improving this.
313319
var containerColumnName = complexType.GetContainerColumnName();
314-
var containerColumnCandidates = complexType.ContainingEntityType.GetTableMappings()
320+
targetColumnModel = complexType.ContainingEntityType.GetTableMappings()
315321
.SelectMany(m => m.Table.Columns)
316322
.Where(c => c.Name == containerColumnName)
317-
.ToList();
318-
319-
targetColumnModel = containerColumnCandidates switch
320-
{
321-
[var c] => c,
322-
[] => throw new UnreachableException($"No container column found in relational model for {complexType.DisplayName()}"),
323-
_ => throw new InvalidOperationException(
324-
RelationalStrings.MultipleColumnsWithSameJsonContainerName(complexType.ContainingEntityType.DisplayName(), containerColumnName))
325-
};
323+
.Single();
326324

327325
break;
328326
}
@@ -755,7 +753,7 @@ protected virtual bool IsValidSelectExpressionForExecuteUpdate(
755753
/// </param>
756754
/// <param name="value">The JSON value to be set, ready for use as-is in <see cref="QuerySqlGenerator" />.</param>
757755
protected virtual ColumnValueSetter GenerateJsonPartialUpdateSetter(Expression target, SqlExpression value)
758-
=> throw new InvalidOperationException(RelationalStrings.JsonPartialUpdateNotSupportedByProvider);
756+
=> throw new InvalidOperationException(RelationalStrings.JsonPartialExecuteUpdateNotSupportedByProvider);
759757

760758
private static T? ParameterValueExtractor<T>(
761759
QueryContext context,

‎src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -548,7 +548,7 @@ protected override ColumnValueSetter GenerateJsonPartialUpdateSetter(Expression
548548
_ => throw new UnreachableException(),
549549
};
550550

551-
if (_sqlServerSingletonOptions.SupportsJsonType)
551+
if (jsonColumn.TypeMapping?.StoreType is "json")
552552
{
553553
// Generate a SQL Server 2025 modify method invocation(https://learn.microsoft.com/sql/t-sql/data-types/json-data-type#modify-method)
554554
// UPDATE ... SET [x].modify('$.a.b', 'foo')

‎test/EFCore.Relational.Specification.Tests/Query/Associations/ComplexJson/ComplexJsonBulkUpdateRelationalTestBase.cs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,24 @@ public ComplexJsonBulkUpdateRelationalTestBase(TFixture fixture, ITestOutputHelp
1515
fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
1616
}
1717

18+
// #36678 - ExecuteDelete on complex type
1819
public override Task Delete_required_association()
1920
=> AssertTranslationFailedWithDetails(RelationalStrings.ExecuteDeleteOnNonEntityType, base.Delete_required_association);
2021

22+
// #36678 - ExecuteDelete on complex type
2123
public override Task Delete_optional_association()
2224
=> Assert.ThrowsAsync<InvalidOperationException>(base.Delete_optional_association);
2325

24-
// TODO: #36336
26+
// #36336
2527
public override Task Update_property_on_projected_association_with_OrderBy_Skip()
2628
=> AssertTranslationFailedWithDetails(
2729
RelationalStrings.ExecuteUpdateSubqueryNotSupportedOverComplexTypes("RootEntity.RequiredRelated#RelatedType"),
2830
base.Update_property_on_projected_association_with_OrderBy_Skip);
2931

32+
// #36679: non-constant inline array/list translation
33+
public override Task Update_collection_referencing_the_original_collection()
34+
=> Assert.ThrowsAsync<InvalidOperationException>(base.Update_collection_referencing_the_original_collection);
35+
3036
protected void AssertSql(params string[] expected)
3137
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
3238

‎test/EFCore.Relational.Specification.Tests/Query/Associations/ComplexJson/ComplexJsonCollectionRelationalTestBase.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ public ComplexJsonCollectionRelationalTestBase(TFixture fixture, ITestOutputHelp
1717

1818
public override async Task Distinct_projected(QueryTrackingBehavior queryTrackingBehavior)
1919
{
20-
// #36421
20+
// #36421 - support projecting out complex JSON types after Distinct
2121
var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => base.Distinct_projected(queryTrackingBehavior));
2222

2323
Assert.Equal(RelationalStrings.InsufficientInformationToIdentifyElementOfCollectionJoin, exception.Message);

‎test/EFCore.Relational.Specification.Tests/Query/Associations/ComplexTableSplitting/ComplexTableSplittingBulkUpdateRelationalTestBase.cs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,15 @@ public ComplexTableSplittingBulkUpdateRelationalTestBase(TFixture fixture, ITest
1515
fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
1616
}
1717

18+
// #36678 - ExecuteDelete on complex type
1819
public override Task Delete_required_association()
1920
=> AssertTranslationFailedWithDetails(RelationalStrings.ExecuteDeleteOnNonEntityType, base.Delete_required_association);
2021

22+
// #36678 - ExecuteDelete on complex type
2123
public override Task Delete_optional_association()
2224
=> Assert.ThrowsAsync<InvalidOperationException>(base.Delete_optional_association);
2325

24-
// TODO: #36336
26+
// #36336
2527
public override Task Update_property_on_projected_association_with_OrderBy_Skip()
2628
=> AssertTranslationFailedWithDetails(
2729
RelationalStrings.ExecuteUpdateSubqueryNotSupportedOverComplexTypes("RootEntity.RequiredRelated#RelatedType"),
@@ -53,6 +55,14 @@ public override async Task Update_nested_collection_to_inline_with_lambda()
5355
AssertExecuteUpdateSql();
5456
}
5557

58+
// Collections are not supported with table splitting, only JSON
59+
public override async Task Update_collection_referencing_the_original_collection()
60+
{
61+
await Assert.ThrowsAsync<InvalidOperationException>(base.Update_collection_referencing_the_original_collection);
62+
63+
AssertExecuteUpdateSql();
64+
}
65+
5666
// Collections are not supported with table splitting, only JSON
5767
public override async Task Update_nested_collection_to_another_nested_collection()
5868
{

‎test/EFCore.Relational.Specification.Tests/Query/Associations/OwnedJson/OwnedJsonCollectionRelationalTestBase.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using Microsoft.EntityFrameworkCore.Query.Associations.OwnedNavigations;
5+
using Xunit.Sdk;
56

67
namespace Microsoft.EntityFrameworkCore.Query.Associations.OwnedJson;
78

‎test/EFCore.Specification.Tests/Query/Associations/AssociationsBulkUpdateTestBase.cs‎

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,14 @@ public virtual Task Update_property_inside_association()
4747
s => s.SetProperty(c => c.RequiredRelated.String, "foo_updated"),
4848
rowsAffectedCount: 4);
4949

50+
[ConditionalFact]
51+
public virtual Task Update_property_inside_association_with_special_chars()
52+
=> AssertUpdate(
53+
ss => ss.Set<RootEntity>().Where(c => c.RequiredRelated.String == "{ this may/look:like JSON but it [isn't]: ממש ממש לאéèéè }"),
54+
e => e,
55+
s => s.SetProperty(c => c.RequiredRelated.String, c => "{ Some other/JSON:like text though it [isn't]: ממש ממש לאéèéè }"),
56+
rowsAffectedCount: 1);
57+
5058
[ConditionalFact]
5159
public virtual Task Update_property_inside_nested()
5260
=> AssertUpdate(
@@ -61,7 +69,7 @@ public virtual Task Update_property_on_projected_association()
6169
ss => ss.Set<RootEntity>().Select(c => c.RequiredRelated),
6270
a => a,
6371
s => s.SetProperty(c => c.String, "foo_updated"),
64-
rowsAffectedCount: 6);
72+
rowsAffectedCount: 7);
6573

6674
[ConditionalFact]
6775
public virtual Task Update_property_on_projected_association_with_OrderBy_Skip()
@@ -79,8 +87,8 @@ public virtual Task Update_multiple_properties_inside_associations_and_on_entity
7987
s => s
8088
.SetProperty(c => c.Name, c => c.Name + "Modified")
8189
.SetProperty(c => c.RequiredRelated.String, c => c.OptionalRelated!.String)
82-
.SetProperty(c => c.OptionalRelated!.String, "foo_updated"),
83-
rowsAffectedCount: 5);
90+
.SetProperty(c => c.OptionalRelated!.RequiredNested.String, "foo_updated"),
91+
rowsAffectedCount: 6);
8492

8593
[ConditionalFact]
8694
public virtual Task Update_multiple_projected_assocations_via_anonymous_type()
@@ -97,7 +105,7 @@ public virtual Task Update_multiple_projected_assocations_via_anonymous_type()
97105
s => s
98106
.SetProperty(c => c.RequiredRelated.String, c => c.OptionalRelated!.String)
99107
.SetProperty(c => c.OptionalRelated!.String, "foo_updated"),
100-
rowsAffectedCount: 5);
108+
rowsAffectedCount: 6);
101109

102110
#endregion Update properties
103111

@@ -124,7 +132,7 @@ public virtual Task Update_association_to_parameter()
124132
ss => ss.Set<RootEntity>(),
125133
c => c,
126134
s => s.SetProperty(x => x.RequiredRelated, newRelated),
127-
rowsAffectedCount: 6);
135+
rowsAffectedCount: 7);
128136
}
129137

130138
[ConditionalFact]
@@ -141,7 +149,7 @@ public virtual Task Update_nested_association_to_parameter()
141149
ss => ss.Set<RootEntity>(),
142150
c => c,
143151
s => s.SetProperty(x => x.RequiredRelated.RequiredNested, newNested),
144-
rowsAffectedCount: 6);
152+
rowsAffectedCount: 7);
145153
}
146154

147155
[ConditionalFact]
@@ -150,15 +158,15 @@ public virtual Task Update_association_to_another_association()
150158
ss => ss.Set<RootEntity>(),
151159
c => c,
152160
s => s.SetProperty(x => x.OptionalRelated, x => x.RequiredRelated),
153-
rowsAffectedCount: 6);
161+
rowsAffectedCount: 7);
154162

155163
[ConditionalFact]
156164
public virtual Task Update_nested_association_to_another_nested_association()
157165
=> AssertUpdate(
158166
ss => ss.Set<RootEntity>(),
159167
c => c,
160168
s => s.SetProperty(x => x.RequiredRelated.OptionalNested, x => x.RequiredRelated.RequiredNested),
161-
rowsAffectedCount: 6);
169+
rowsAffectedCount: 7);
162170

163171
[ConditionalFact]
164172
public virtual Task Update_association_to_inline()
@@ -182,7 +190,7 @@ public virtual Task Update_association_to_inline()
182190
OptionalNested = null,
183191
NestedCollection = []
184192
}),
185-
rowsAffectedCount: 6);
193+
rowsAffectedCount: 7);
186194

187195
[ConditionalFact]
188196
public virtual Task Update_association_to_inline_with_lambda()
@@ -206,7 +214,7 @@ public virtual Task Update_association_to_inline_with_lambda()
206214
OptionalNested = null,
207215
NestedCollection = new List<NestedType>()
208216
}),
209-
rowsAffectedCount: 6);
217+
rowsAffectedCount: 7);
210218

211219
[ConditionalFact]
212220
public virtual Task Update_nested_association_to_inline_with_lambda()
@@ -221,23 +229,23 @@ public virtual Task Update_nested_association_to_inline_with_lambda()
221229
Int = 80,
222230
String = "Updated nested string"
223231
}),
224-
rowsAffectedCount: 6);
232+
rowsAffectedCount: 7);
225233

226234
[ConditionalFact]
227235
public virtual Task Update_association_to_null()
228236
=> AssertUpdate(
229237
ss => ss.Set<RootEntity>(),
230238
c => c,
231239
s => s.SetProperty(x => x.OptionalRelated, (RelatedType?)null),
232-
rowsAffectedCount: 6);
240+
rowsAffectedCount: 7);
233241

234242
[ConditionalFact]
235243
public virtual Task Update_association_to_null_with_lambda()
236244
=> AssertUpdate(
237245
ss => ss.Set<RootEntity>(),
238246
c => c,
239247
s => s.SetProperty(x => x.OptionalRelated, x => null),
240-
rowsAffectedCount: 6);
248+
rowsAffectedCount: 7);
241249

242250
[ConditionalFact]
243251
public virtual Task Update_association_to_null_parameter()
@@ -248,7 +256,7 @@ public virtual Task Update_association_to_null_parameter()
248256
ss => ss.Set<RootEntity>(),
249257
c => c,
250258
s => s.SetProperty(x => x.OptionalRelated, nullRelated),
251-
rowsAffectedCount: 6);
259+
rowsAffectedCount: 7);
252260
}
253261

254262
#endregion Update association
@@ -292,7 +300,7 @@ public virtual Task Update_collection_to_parameter()
292300
ss => ss.Set<RootEntity>(),
293301
c => c,
294302
s => s.SetProperty(x => x.RelatedCollection, collection),
295-
rowsAffectedCount: 6);
303+
rowsAffectedCount: 7);
296304
}
297305

298306
[ConditionalFact]
@@ -318,7 +326,7 @@ public virtual Task Update_nested_collection_to_parameter()
318326
ss => ss.Set<RootEntity>(),
319327
c => c,
320328
s => s.SetProperty(x => x.RequiredRelated.NestedCollection, collection),
321-
rowsAffectedCount: 6);
329+
rowsAffectedCount: 7);
322330
}
323331

324332
[ConditionalFact]
@@ -343,7 +351,17 @@ public virtual Task Update_nested_collection_to_inline_with_lambda()
343351
String = "Updated nested string2"
344352
}
345353
}),
346-
rowsAffectedCount: 6);
354+
rowsAffectedCount: 7);
355+
356+
[ConditionalFact]
357+
public virtual Task Update_collection_referencing_the_original_collection()
358+
=> AssertUpdate(
359+
ss => ss.Set<RootEntity>().Where(e => e.RequiredRelated.NestedCollection.Count >= 2),
360+
c => c,
361+
s => s.SetProperty(
362+
e => e.RequiredRelated.NestedCollection,
363+
e => new List<NestedType> { e.RequiredRelated.NestedCollection[1], e.RequiredRelated.NestedCollection[0]}),
364+
rowsAffectedCount: 7);
347365

348366
[ConditionalFact]
349367
public virtual Task Update_nested_collection_to_another_nested_collection()
@@ -353,7 +371,7 @@ public virtual Task Update_nested_collection_to_another_nested_collection()
353371
s => s.SetProperty(
354372
x => x.RequiredRelated.NestedCollection,
355373
x => x.OptionalRelated!.NestedCollection),
356-
rowsAffectedCount: 6);
374+
rowsAffectedCount: 7);
357375

358376
#endregion Update collection
359377

0 commit comments

Comments
 (0)