Skip to content

Commit 47fa51e

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

File tree

10 files changed

+128
-48
lines changed

10 files changed

+128
-48
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/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.Specification.Tests/Query/Associations/AssociationsBulkUpdateTestBase.cs‎

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public virtual Task Update_property_on_projected_association()
6161
ss => ss.Set<RootEntity>().Select(c => c.RequiredRelated),
6262
a => a,
6363
s => s.SetProperty(c => c.String, "foo_updated"),
64-
rowsAffectedCount: 6);
64+
rowsAffectedCount: 7);
6565

6666
[ConditionalFact]
6767
public virtual Task Update_property_on_projected_association_with_OrderBy_Skip()
@@ -79,8 +79,8 @@ public virtual Task Update_multiple_properties_inside_associations_and_on_entity
7979
s => s
8080
.SetProperty(c => c.Name, c => c.Name + "Modified")
8181
.SetProperty(c => c.RequiredRelated.String, c => c.OptionalRelated!.String)
82-
.SetProperty(c => c.OptionalRelated!.String, "foo_updated"),
83-
rowsAffectedCount: 5);
82+
.SetProperty(c => c.OptionalRelated!.RequiredNested.String, "foo_updated"),
83+
rowsAffectedCount: 6);
8484

8585
[ConditionalFact]
8686
public virtual Task Update_multiple_projected_assocations_via_anonymous_type()
@@ -97,7 +97,7 @@ public virtual Task Update_multiple_projected_assocations_via_anonymous_type()
9797
s => s
9898
.SetProperty(c => c.RequiredRelated.String, c => c.OptionalRelated!.String)
9999
.SetProperty(c => c.OptionalRelated!.String, "foo_updated"),
100-
rowsAffectedCount: 5);
100+
rowsAffectedCount: 6);
101101

102102
#endregion Update properties
103103

@@ -124,7 +124,7 @@ public virtual Task Update_association_to_parameter()
124124
ss => ss.Set<RootEntity>(),
125125
c => c,
126126
s => s.SetProperty(x => x.RequiredRelated, newRelated),
127-
rowsAffectedCount: 6);
127+
rowsAffectedCount: 7);
128128
}
129129

130130
[ConditionalFact]
@@ -141,7 +141,7 @@ public virtual Task Update_nested_association_to_parameter()
141141
ss => ss.Set<RootEntity>(),
142142
c => c,
143143
s => s.SetProperty(x => x.RequiredRelated.RequiredNested, newNested),
144-
rowsAffectedCount: 6);
144+
rowsAffectedCount: 7);
145145
}
146146

147147
[ConditionalFact]
@@ -150,15 +150,15 @@ public virtual Task Update_association_to_another_association()
150150
ss => ss.Set<RootEntity>(),
151151
c => c,
152152
s => s.SetProperty(x => x.OptionalRelated, x => x.RequiredRelated),
153-
rowsAffectedCount: 6);
153+
rowsAffectedCount: 7);
154154

155155
[ConditionalFact]
156156
public virtual Task Update_nested_association_to_another_nested_association()
157157
=> AssertUpdate(
158158
ss => ss.Set<RootEntity>(),
159159
c => c,
160160
s => s.SetProperty(x => x.RequiredRelated.OptionalNested, x => x.RequiredRelated.RequiredNested),
161-
rowsAffectedCount: 6);
161+
rowsAffectedCount: 7);
162162

163163
[ConditionalFact]
164164
public virtual Task Update_association_to_inline()
@@ -182,7 +182,7 @@ public virtual Task Update_association_to_inline()
182182
OptionalNested = null,
183183
NestedCollection = []
184184
}),
185-
rowsAffectedCount: 6);
185+
rowsAffectedCount: 7);
186186

187187
[ConditionalFact]
188188
public virtual Task Update_association_to_inline_with_lambda()
@@ -206,7 +206,7 @@ public virtual Task Update_association_to_inline_with_lambda()
206206
OptionalNested = null,
207207
NestedCollection = new List<NestedType>()
208208
}),
209-
rowsAffectedCount: 6);
209+
rowsAffectedCount: 7);
210210

211211
[ConditionalFact]
212212
public virtual Task Update_nested_association_to_inline_with_lambda()
@@ -221,23 +221,23 @@ public virtual Task Update_nested_association_to_inline_with_lambda()
221221
Int = 80,
222222
String = "Updated nested string"
223223
}),
224-
rowsAffectedCount: 6);
224+
rowsAffectedCount: 7);
225225

226226
[ConditionalFact]
227227
public virtual Task Update_association_to_null()
228228
=> AssertUpdate(
229229
ss => ss.Set<RootEntity>(),
230230
c => c,
231231
s => s.SetProperty(x => x.OptionalRelated, (RelatedType?)null),
232-
rowsAffectedCount: 6);
232+
rowsAffectedCount: 7);
233233

234234
[ConditionalFact]
235235
public virtual Task Update_association_to_null_with_lambda()
236236
=> AssertUpdate(
237237
ss => ss.Set<RootEntity>(),
238238
c => c,
239239
s => s.SetProperty(x => x.OptionalRelated, x => null),
240-
rowsAffectedCount: 6);
240+
rowsAffectedCount: 7);
241241

242242
[ConditionalFact]
243243
public virtual Task Update_association_to_null_parameter()
@@ -248,7 +248,7 @@ public virtual Task Update_association_to_null_parameter()
248248
ss => ss.Set<RootEntity>(),
249249
c => c,
250250
s => s.SetProperty(x => x.OptionalRelated, nullRelated),
251-
rowsAffectedCount: 6);
251+
rowsAffectedCount: 7);
252252
}
253253

254254
#endregion Update association
@@ -292,7 +292,7 @@ public virtual Task Update_collection_to_parameter()
292292
ss => ss.Set<RootEntity>(),
293293
c => c,
294294
s => s.SetProperty(x => x.RelatedCollection, collection),
295-
rowsAffectedCount: 6);
295+
rowsAffectedCount: 7);
296296
}
297297

298298
[ConditionalFact]
@@ -318,7 +318,7 @@ public virtual Task Update_nested_collection_to_parameter()
318318
ss => ss.Set<RootEntity>(),
319319
c => c,
320320
s => s.SetProperty(x => x.RequiredRelated.NestedCollection, collection),
321-
rowsAffectedCount: 6);
321+
rowsAffectedCount: 7);
322322
}
323323

324324
[ConditionalFact]
@@ -343,7 +343,17 @@ public virtual Task Update_nested_collection_to_inline_with_lambda()
343343
String = "Updated nested string2"
344344
}
345345
}),
346-
rowsAffectedCount: 6);
346+
rowsAffectedCount: 7);
347+
348+
[ConditionalFact]
349+
public virtual Task Update_collection_referencing_the_original_collection()
350+
=> AssertUpdate(
351+
ss => ss.Set<RootEntity>().Where(e => e.RequiredRelated.NestedCollection.Count >= 2),
352+
c => c,
353+
s => s.SetProperty(
354+
e => e.RequiredRelated.NestedCollection,
355+
e => new List<NestedType> { e.RequiredRelated.NestedCollection[1], e.RequiredRelated.NestedCollection[0]}),
356+
rowsAffectedCount: 7);
347357

348358
[ConditionalFact]
349359
public virtual Task Update_nested_collection_to_another_nested_collection()
@@ -353,7 +363,7 @@ public virtual Task Update_nested_collection_to_another_nested_collection()
353363
s => s.SetProperty(
354364
x => x.RequiredRelated.NestedCollection,
355365
x => x.OptionalRelated!.NestedCollection),
356-
rowsAffectedCount: 6);
366+
rowsAffectedCount: 7);
357367

358368
#endregion Update collection
359369

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

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,39 @@ void SetRelatedValues(RelatedType related)
137137
e.RelatedCollection.Clear();
138138
e.RequiredRelated.NestedCollection.Clear();
139139
e.OptionalRelated!.NestedCollection.Clear();
140+
}),
141+
142+
// Entity with all string properties set to a value with special characters
143+
CreateRootEntity(
144+
id++, description: "With_special_characters", e =>
145+
{
146+
SetRelatedValues(e.RequiredRelated);
147+
148+
if (e.OptionalRelated is not null)
149+
{
150+
SetRelatedValues(e.OptionalRelated);
151+
}
152+
153+
foreach (var related in e.RelatedCollection)
154+
{
155+
SetRelatedValues(related);
156+
}
157+
158+
void SetRelatedValues(RelatedType related)
159+
{
160+
related.Int = 10;
161+
related.String = "{ this may/look:like JSON but it [isn't]: ממש ממש לאéèéè }";
162+
related.RequiredNested.Int = 10;
163+
related.RequiredNested.String = "{ this may/look:like JSON but it [isn't]: ממש ממש לאéèéè }";
164+
related.OptionalNested?.Int = 10;
165+
related.OptionalNested?.String = "{ this may/look:like JSON but it [isn't]: ממש ממש לאéèéè }";
166+
167+
foreach (var nested in related.NestedCollection)
168+
{
169+
nested.Int = 10;
170+
nested.String = "{ this may/look:like JSON but it [isn't]: ממש ממש לאéèéè }";
171+
}
172+
}
140173
})
141174
];
142175

@@ -325,12 +358,12 @@ private static List<RootReferencingEntity> CreateRootReferencingEntities(IEnumer
325358
var id = 1;
326359

327360
rootReferencingEntities.Add(new RootReferencingEntity { Id = id++, Root = null });
328-
// foreach (var rootEntity in rootEntities.Take(2))
329-
// {
330-
// var rootReferencingEntity = new RootReferencingEntity { Id = id++, Root = rootEntity };
331-
// rootEntity.RootReferencingEntity = rootReferencingEntity;
332-
// rootReferencingEntities.Add(rootReferencingEntity);
333-
// }
361+
foreach (var rootEntity in rootEntities.Take(2))
362+
{
363+
var rootReferencingEntity = new RootReferencingEntity { Id = id++, Root = rootEntity };
364+
rootEntity.RootReferencingEntity = rootReferencingEntity;
365+
rootReferencingEntities.Add(rootReferencingEntity);
366+
}
334367

335368
return rootReferencingEntities;
336369
}

0 commit comments

Comments
 (0)