Skip to content

Commit 20d69fa

Browse files
maumarmaumar
committed
Fix to #32972 - The database default created by Migrations for primitive collections mapped to JSON is invalid
Problem was that when adding new required column to an existing table we always need to provide default value (to fill the values for rows already in the table). For collection of primitives that get map to json we should be setting the value to empty JSON collection, i.e. '[]' but we were not doing that. Fixes #32972
1 parent 6081e8c commit 20d69fa

File tree

2 files changed

+338
-1
lines changed

2 files changed

+338
-1
lines changed

‎src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1243,7 +1243,13 @@ private void Initialize(
12431243

12441244
if (!column.TryGetDefaultValue(out var defaultValue))
12451245
{
1246-
defaultValue = null;
1246+
// for non-nullable collections of primitives that are mapped to JSON we set a default value corresponding to empty JSON collection
1247+
defaultValue = !inline
1248+
&& column is { IsNullable: false, StoreTypeMapping: { ElementTypeMapping: not null, Converter: ValueConverter columnValueConverter } }
1249+
&& columnValueConverter.GetType() is Type { IsGenericType: true } columnValueConverterType
1250+
&& columnValueConverterType.GetGenericTypeDefinition() == typeof(CollectionToJsonStringConverter<>)
1251+
? "[]"
1252+
: null;
12471253
}
12481254

12491255
columnOperation.ColumnType = column.StoreType;

‎test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs‎

Lines changed: 331 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10770,6 +10770,337 @@ CONSTRAINT [PK_HistoryTable] PRIMARY KEY ([Id])
1077010770
""");
1077110771
}
1077210772

10773+
[ConditionalFact]
10774+
public virtual async Task Add_required_primitve_collection_to_existing_table()
10775+
{
10776+
await Test(
10777+
builder => builder.Entity(
10778+
"Customer", e =>
10779+
{
10780+
e.Property<int>("Id").ValueGeneratedOnAdd();
10781+
e.HasKey("Id");
10782+
e.Property<string>("Name");
10783+
e.ToTable("Customers");
10784+
}),
10785+
builder => builder.Entity(
10786+
"Customer", e =>
10787+
{
10788+
e.Property<int>("Id").ValueGeneratedOnAdd();
10789+
e.HasKey("Id");
10790+
e.Property<string>("Name");
10791+
e.Property<List<int>>("Numbers").IsRequired();
10792+
e.ToTable("Customers");
10793+
}),
10794+
model =>
10795+
{
10796+
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
10797+
10798+
Assert.Collection(
10799+
customersTable.Columns,
10800+
c => Assert.Equal("Id", c.Name),
10801+
c => Assert.Equal("Name", c.Name),
10802+
c => Assert.Equal("Numbers", c.Name));
10803+
Assert.Same(
10804+
customersTable.Columns.Single(c => c.Name == "Id"),
10805+
Assert.Single(customersTable.PrimaryKey!.Columns));
10806+
});
10807+
10808+
AssertSql(
10809+
"""
10810+
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NOT NULL DEFAULT N'[]';
10811+
""");
10812+
}
10813+
10814+
[ConditionalFact]
10815+
public virtual async Task Add_required_primitve_collection_with_custom_default_value_to_existing_table()
10816+
{
10817+
await Test(
10818+
builder => builder.Entity(
10819+
"Customer", e =>
10820+
{
10821+
e.Property<int>("Id").ValueGeneratedOnAdd();
10822+
e.HasKey("Id");
10823+
e.Property<string>("Name");
10824+
e.ToTable("Customers");
10825+
}),
10826+
builder => builder.Entity(
10827+
"Customer", e =>
10828+
{
10829+
e.Property<int>("Id").ValueGeneratedOnAdd();
10830+
e.HasKey("Id");
10831+
e.Property<string>("Name");
10832+
e.Property<List<int>>("Numbers").IsRequired().HasDefaultValue(new List<int> { 1, 2, 3 });
10833+
e.ToTable("Customers");
10834+
}),
10835+
model =>
10836+
{
10837+
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
10838+
10839+
Assert.Collection(
10840+
customersTable.Columns,
10841+
c => Assert.Equal("Id", c.Name),
10842+
c => Assert.Equal("Name", c.Name),
10843+
c => Assert.Equal("Numbers", c.Name));
10844+
Assert.Same(
10845+
customersTable.Columns.Single(c => c.Name == "Id"),
10846+
Assert.Single(customersTable.PrimaryKey!.Columns));
10847+
});
10848+
10849+
AssertSql(
10850+
"""
10851+
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NOT NULL DEFAULT N'[1,2,3]';
10852+
""");
10853+
}
10854+
10855+
[ConditionalFact]
10856+
public virtual async Task Add_required_primitve_collection_with_custom_default_value_sql_to_existing_table()
10857+
{
10858+
await Test(
10859+
builder => builder.Entity(
10860+
"Customer", e =>
10861+
{
10862+
e.Property<int>("Id").ValueGeneratedOnAdd();
10863+
e.HasKey("Id");
10864+
e.Property<string>("Name");
10865+
e.ToTable("Customers");
10866+
}),
10867+
builder => builder.Entity(
10868+
"Customer", e =>
10869+
{
10870+
e.Property<int>("Id").ValueGeneratedOnAdd();
10871+
e.HasKey("Id");
10872+
e.Property<string>("Name");
10873+
e.Property<List<int>>("Numbers").IsRequired().HasDefaultValueSql("N'[3, 2, 1]'");
10874+
e.ToTable("Customers");
10875+
}),
10876+
model =>
10877+
{
10878+
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
10879+
10880+
Assert.Collection(
10881+
customersTable.Columns,
10882+
c => Assert.Equal("Id", c.Name),
10883+
c => Assert.Equal("Name", c.Name),
10884+
c => Assert.Equal("Numbers", c.Name));
10885+
Assert.Same(
10886+
customersTable.Columns.Single(c => c.Name == "Id"),
10887+
Assert.Single(customersTable.PrimaryKey!.Columns));
10888+
});
10889+
10890+
AssertSql(
10891+
"""
10892+
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NOT NULL DEFAULT (N'[3, 2, 1]');
10893+
""");
10894+
}
10895+
10896+
[ConditionalFact(Skip = "issue #33038")]
10897+
public virtual async Task Add_required_primitve_collection_with_custom_converter_to_existing_table()
10898+
{
10899+
await Test(
10900+
builder => builder.Entity(
10901+
"Customer", e =>
10902+
{
10903+
e.Property<int>("Id").ValueGeneratedOnAdd();
10904+
e.HasKey("Id");
10905+
e.Property<string>("Name");
10906+
e.ToTable("Customers");
10907+
}),
10908+
builder => builder.Entity(
10909+
"Customer", e =>
10910+
{
10911+
e.Property<int>("Id").ValueGeneratedOnAdd();
10912+
e.HasKey("Id");
10913+
e.Property<string>("Name");
10914+
e.Property<List<int>>("Numbers").HasConversion(new ValueConverter<List<int>, string>(
10915+
convertToProviderExpression: x => x != null && x.Count > 0 ? "some numbers" : "nothing",
10916+
convertFromProviderExpression: x => x == "nothing" ? new List<int> { } : new List<int> { 7, 8, 9 }))
10917+
.IsRequired();
10918+
e.ToTable("Customers");
10919+
}),
10920+
model =>
10921+
{
10922+
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
10923+
10924+
Assert.Collection(
10925+
customersTable.Columns,
10926+
c => Assert.Equal("Id", c.Name),
10927+
c => Assert.Equal("Name", c.Name),
10928+
c => Assert.Equal("Numbers", c.Name));
10929+
Assert.Same(
10930+
customersTable.Columns.Single(c => c.Name == "Id"),
10931+
Assert.Single(customersTable.PrimaryKey!.Columns));
10932+
});
10933+
10934+
AssertSql(
10935+
"""
10936+
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NOT NULL DEFAULT N'nothing';
10937+
""");
10938+
}
10939+
10940+
[ConditionalFact]
10941+
public virtual async Task Add_required_primitve_collection_with_custom_converter_and_custom_default_value_to_existing_table()
10942+
{
10943+
await Test(
10944+
builder => builder.Entity(
10945+
"Customer", e =>
10946+
{
10947+
e.Property<int>("Id").ValueGeneratedOnAdd();
10948+
e.HasKey("Id");
10949+
e.Property<string>("Name");
10950+
e.ToTable("Customers");
10951+
}),
10952+
builder => builder.Entity(
10953+
"Customer", e =>
10954+
{
10955+
e.Property<int>("Id").ValueGeneratedOnAdd();
10956+
e.HasKey("Id");
10957+
e.Property<string>("Name");
10958+
e.Property<List<int>>("Numbers").HasConversion(new ValueConverter<List<int>, string>(
10959+
convertToProviderExpression: x => x != null && x.Count > 0 ? "some numbers" : "nothing",
10960+
convertFromProviderExpression: x => x == "nothing" ? new List<int> { } : new List<int> { 7, 8, 9 }))
10961+
.HasDefaultValue(new List<int> { 42 })
10962+
.IsRequired();
10963+
e.ToTable("Customers");
10964+
}),
10965+
model =>
10966+
{
10967+
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
10968+
10969+
Assert.Collection(
10970+
customersTable.Columns,
10971+
c => Assert.Equal("Id", c.Name),
10972+
c => Assert.Equal("Name", c.Name),
10973+
c => Assert.Equal("Numbers", c.Name));
10974+
Assert.Same(
10975+
customersTable.Columns.Single(c => c.Name == "Id"),
10976+
Assert.Single(customersTable.PrimaryKey!.Columns));
10977+
});
10978+
10979+
AssertSql(
10980+
"""
10981+
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NOT NULL DEFAULT N'some numbers';
10982+
""");
10983+
}
10984+
10985+
[ConditionalFact]
10986+
public virtual async Task Add_optional_primitive_collection_to_existing_table()
10987+
{
10988+
await Test(
10989+
builder => builder.Entity(
10990+
"Customer", e =>
10991+
{
10992+
e.Property<int>("Id").ValueGeneratedOnAdd();
10993+
e.HasKey("Id");
10994+
e.Property<string>("Name");
10995+
e.ToTable("Customers");
10996+
}),
10997+
builder => builder.Entity(
10998+
"Customer", e =>
10999+
{
11000+
e.Property<int>("Id").ValueGeneratedOnAdd();
11001+
e.HasKey("Id");
11002+
e.Property<string>("Name");
11003+
e.Property<List<int>>("Numbers");
11004+
e.ToTable("Customers");
11005+
}),
11006+
model =>
11007+
{
11008+
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
11009+
11010+
Assert.Collection(
11011+
customersTable.Columns,
11012+
c => Assert.Equal("Id", c.Name),
11013+
c => Assert.Equal("Name", c.Name),
11014+
c => Assert.Equal("Numbers", c.Name));
11015+
Assert.Same(
11016+
customersTable.Columns.Single(c => c.Name == "Id"),
11017+
Assert.Single(customersTable.PrimaryKey!.Columns));
11018+
});
11019+
11020+
AssertSql(
11021+
"""
11022+
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NULL;
11023+
""");
11024+
}
11025+
11026+
[ConditionalFact]
11027+
public virtual async Task Create_table_with_required_primitive_collection()
11028+
{
11029+
await Test(
11030+
builder => { },
11031+
builder => builder.Entity(
11032+
"Customer", e =>
11033+
{
11034+
e.Property<int>("Id").ValueGeneratedOnAdd();
11035+
e.HasKey("Id");
11036+
e.Property<string>("Name");
11037+
e.Property<List<int>>("Numbers").IsRequired();
11038+
e.ToTable("Customers");
11039+
}),
11040+
model =>
11041+
{
11042+
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
11043+
11044+
Assert.Collection(
11045+
customersTable.Columns,
11046+
c => Assert.Equal("Id", c.Name),
11047+
c => Assert.Equal("Name", c.Name),
11048+
c => Assert.Equal("Numbers", c.Name));
11049+
Assert.Same(
11050+
customersTable.Columns.Single(c => c.Name == "Id"),
11051+
Assert.Single(customersTable.PrimaryKey!.Columns));
11052+
});
11053+
11054+
AssertSql(
11055+
"""
11056+
CREATE TABLE [Customers] (
11057+
[Id] int NOT NULL IDENTITY,
11058+
[Name] nvarchar(max) NULL,
11059+
[Numbers] nvarchar(max) NOT NULL,
11060+
CONSTRAINT [PK_Customers] PRIMARY KEY ([Id])
11061+
);
11062+
""");
11063+
}
11064+
11065+
[ConditionalFact]
11066+
public virtual async Task Create_table_with_optional_primitive_collection()
11067+
{
11068+
await Test(
11069+
builder => { },
11070+
builder => builder.Entity(
11071+
"Customer", e =>
11072+
{
11073+
e.Property<int>("Id").ValueGeneratedOnAdd();
11074+
e.HasKey("Id");
11075+
e.Property<string>("Name");
11076+
e.Property<List<int>>("Numbers");
11077+
e.ToTable("Customers");
11078+
}),
11079+
model =>
11080+
{
11081+
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
11082+
11083+
Assert.Collection(
11084+
customersTable.Columns,
11085+
c => Assert.Equal("Id", c.Name),
11086+
c => Assert.Equal("Name", c.Name),
11087+
c => Assert.Equal("Numbers", c.Name));
11088+
Assert.Same(
11089+
customersTable.Columns.Single(c => c.Name == "Id"),
11090+
Assert.Single(customersTable.PrimaryKey!.Columns));
11091+
});
11092+
11093+
AssertSql(
11094+
"""
11095+
CREATE TABLE [Customers] (
11096+
[Id] int NOT NULL IDENTITY,
11097+
[Name] nvarchar(max) NULL,
11098+
[Numbers] nvarchar(max) NULL,
11099+
CONSTRAINT [PK_Customers] PRIMARY KEY ([Id])
11100+
);
11101+
""");
11102+
}
11103+
1077311104
protected override string NonDefaultCollation
1077411105
=> _nonDefaultCollation ??= GetDatabaseCollation() == "German_PhoneBook_CI_AS"
1077511106
? "French_CI_AS"

0 commit comments

Comments
 (0)