Skip to content

Commit 39889b7

Browse files
[release/10.0] Fix 'ordinal -1 is invalid' crash when a nested JSON complex sub-collection grows (#38373)
Fixes #38299 Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
1 parent 5c96234 commit 39889b7

4 files changed

Lines changed: 152 additions & 2 deletions

File tree

src/EFCore/ChangeTracking/Internal/InternalEntryBase.InternalComplexCollectionEntry.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ public partial class InternalEntryBase
1818
internal static readonly bool UseOldBehavior37724 =
1919
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue37724", out var enabled) && enabled;
2020

21+
internal static readonly bool UseOldBehavior38299 =
22+
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue38299", out var enabled) && enabled;
23+
2124
private struct InternalComplexCollectionEntry(InternalEntryBase entry, IComplexProperty complexCollection)
2225
{
2326
private static readonly bool UseOldBehavior37585 =

src/EFCore/ChangeTracking/Internal/InternalEntryBase.cs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -566,9 +566,20 @@ public void SetPropertyModified(
566566

567567
if (recurse)
568568
{
569+
var newElementState = isModified ? EntityState.Modified : EntityState.Unchanged;
569570
foreach (var complexEntry in GetFlattenedComplexEntries())
570571
{
571-
complexEntry.SetEntityState(isModified ? EntityState.Modified : EntityState.Unchanged, modifyProperties: true);
572+
// Added elements represent pending additions with no original ordinal, so forcing them to
573+
// Modified/Unchanged is incorrect and would fail the original ordinal validation. Leave their
574+
// state (computed by change detection) untouched, mirroring the bulk state-change logic in
575+
// InternalComplexCollectionEntry.SetState.
576+
if (!UseOldBehavior38299
577+
&& complexEntry.EntityState is EntityState.Added)
578+
{
579+
continue;
580+
}
581+
582+
complexEntry.SetEntityState(newElementState, modifyProperties: true);
572583
}
573584
}
574585
}

test/EFCore.Relational.Specification.Tests/Update/ComplexCollectionJsonUpdateTestBase.cs

Lines changed: 122 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -592,6 +592,55 @@ public virtual Task Replace_complex_property_mapped_to_json()
592592
}
593593
});
594594

595+
[ConditionalFact]
596+
public virtual Task Grow_nested_sub_collection_in_complex_property_mapped_to_json()
597+
=> TestHelpers.ExecuteWithStrategyInTransactionAsync(
598+
CreateContext,
599+
UseTransaction,
600+
async context =>
601+
{
602+
var widget = await context.Set<WidgetWithDeepJson>().OrderBy(w => w.Id).FirstAsync();
603+
604+
// Replace the entire JSON-mapped complex property with a structure where one nested
605+
// sub-collection (Inner) has grown from one element to two. The sibling sub-collection
606+
// (Others) being present in the type definition is required to trigger the regression.
607+
widget.Deep = new DeepData
608+
{
609+
Mid = new MiddleData
610+
{
611+
Items =
612+
[
613+
new DeepItem
614+
{
615+
Title = "Item1",
616+
Inner =
617+
[
618+
new InnerEntry { Value = "inner-0" },
619+
new InnerEntry { Value = "inner-1" }
620+
],
621+
Others = []
622+
}
623+
]
624+
}
625+
};
626+
627+
ClearLog();
628+
await context.SaveChangesAsync();
629+
},
630+
async context =>
631+
{
632+
using (SuspendRecordingEvents())
633+
{
634+
var widget = await context.Set<WidgetWithDeepJson>().OrderBy(w => w.Id).FirstAsync();
635+
var item = Assert.Single(widget.Deep.Mid.Items);
636+
Assert.Equal("Item1", item.Title);
637+
Assert.Equal(2, item.Inner.Count);
638+
Assert.Equal("inner-0", item.Inner[0].Value);
639+
Assert.Equal("inner-1", item.Inner[1].Value);
640+
Assert.Empty(item.Others);
641+
}
642+
});
643+
595644
protected virtual void UseTransaction(DatabaseFacade facade, IDbContextTransaction transaction)
596645
=> facade.UseTransaction(transaction.GetDbTransaction());
597646

@@ -607,6 +656,7 @@ protected virtual IDisposable SuspendRecordingEvents()
607656
protected class ComplexCollectionJsonContext(DbContextOptions options) : DbContext(options)
608657
{
609658
public DbSet<CompanyWithComplexCollections> Companies { get; set; } = null!;
659+
public DbSet<WidgetWithDeepJson> Widgets { get; set; } = null!;
610660
}
611661

612662
protected class CompanyWithComplexCollections
@@ -645,6 +695,34 @@ protected class Department
645695
public decimal Budget { get; set; }
646696
}
647697

698+
protected class WidgetWithDeepJson
699+
{
700+
public int Id { get; set; }
701+
public required DeepData Deep { get; set; }
702+
}
703+
704+
protected class DeepData
705+
{
706+
public required MiddleData Mid { get; set; }
707+
}
708+
709+
protected class MiddleData
710+
{
711+
public List<DeepItem> Items { get; set; } = [];
712+
}
713+
714+
protected class DeepItem
715+
{
716+
public required string Title { get; set; }
717+
public List<InnerEntry> Inner { get; set; } = [];
718+
public List<InnerEntry> Others { get; set; } = [];
719+
}
720+
721+
protected class InnerEntry
722+
{
723+
public required string Value { get; set; }
724+
}
725+
648726
public abstract class ComplexCollectionJsonUpdateFixtureBase : SharedStoreFixtureBase<DbContext>
649727
{
650728
protected override string StoreName
@@ -660,7 +738,8 @@ protected override Type ContextType
660738
=> typeof(ComplexCollectionJsonContext);
661739

662740
protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
663-
=> modelBuilder.Entity<CompanyWithComplexCollections>(b =>
741+
{
742+
modelBuilder.Entity<CompanyWithComplexCollections>(b =>
664743
{
665744
b.Property(x => x.Id).ValueGeneratedNever();
666745

@@ -686,6 +765,26 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
686765
});
687766
});
688767

768+
modelBuilder.Entity<WidgetWithDeepJson>(b =>
769+
{
770+
b.Property(x => x.Id).ValueGeneratedNever();
771+
772+
b.ComplexProperty(
773+
x => x.Deep, db =>
774+
{
775+
db.ToJson();
776+
db.ComplexProperty(
777+
d => d.Mid, mb =>
778+
mb.ComplexCollection(
779+
m => m.Items, ib =>
780+
{
781+
ib.ComplexCollection(i => i.Inner);
782+
ib.ComplexCollection(i => i.Others);
783+
}));
784+
});
785+
});
786+
}
787+
689788
protected override Task SeedAsync(DbContext context)
690789
{
691790
var company = new CompanyWithComplexCollections
@@ -716,6 +815,28 @@ protected override Task SeedAsync(DbContext context)
716815
};
717816

718817
context.Add(company);
818+
819+
var widget = new WidgetWithDeepJson
820+
{
821+
Id = 1,
822+
Deep = new DeepData
823+
{
824+
Mid = new MiddleData
825+
{
826+
Items =
827+
[
828+
new DeepItem
829+
{
830+
Title = "Item1",
831+
Inner = [new InnerEntry { Value = "inner-0" }],
832+
Others = []
833+
}
834+
]
835+
}
836+
}
837+
};
838+
839+
context.Add(widget);
719840
return context.SaveChangesAsync();
720841
}
721842
}

test/EFCore.Sqlite.FunctionalTests/Update/ComplexCollectionJsonUpdateSqliteTest.cs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,21 @@ public override async Task Replace_complex_property_mapped_to_json()
290290
""");
291291
}
292292

293+
public override async Task Grow_nested_sub_collection_in_complex_property_mapped_to_json()
294+
{
295+
await base.Grow_nested_sub_collection_in_complex_property_mapped_to_json();
296+
297+
AssertSql(
298+
"""
299+
@p0='{"Mid":{"Items":[{"Title":"Item1","Inner":[{"Value":"inner-0"},{"Value":"inner-1"}],"Others":[]}]}}' (Nullable = false) (Size = 99)
300+
@p1='1'
301+
302+
UPDATE "Widgets" SET "Deep" = @p0
303+
WHERE "Id" = @p1
304+
RETURNING 1;
305+
""");
306+
}
307+
293308
public class ComplexCollectionJsonUpdateSqliteFixture : ComplexCollectionJsonUpdateFixtureBase
294309
{
295310
protected override ITestStoreFactory TestStoreFactory

0 commit comments

Comments
 (0)