Skip to content

Commit 28a95fc

Browse files
maumarmaumar
authored
Fix to #32972 - The database default created by Migrations for primitive collections mapped to JSON is invalid (#33039)
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 d079c71 commit 28a95fc

File tree

2 files changed

+343
-6
lines changed

2 files changed

+343
-6
lines changed

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

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1243,9 +1243,20 @@ 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

1255+
columnOperation.DefaultValue = defaultValue
1256+
?? (inline || isNullable
1257+
? null
1258+
: GetDefaultValue(columnOperation.ClrType));
1259+
columnOperation.DefaultValueSql = column.DefaultValueSql;
12491260
columnOperation.ColumnType = column.StoreType;
12501261
columnOperation.MaxLength = column.MaxLength;
12511262
columnOperation.Precision = column.Precision;
@@ -1254,11 +1265,6 @@ private void Initialize(
12541265
columnOperation.IsFixedLength = column.IsFixedLength;
12551266
columnOperation.IsRowVersion = column.IsRowVersion;
12561267
columnOperation.IsNullable = isNullable;
1257-
columnOperation.DefaultValue = defaultValue
1258-
?? (inline || isNullable
1259-
? null
1260-
: GetDefaultValue(columnOperation.ClrType));
1261-
columnOperation.DefaultValueSql = column.DefaultValueSql;
12621268
columnOperation.ComputedColumnSql = column.ComputedColumnSql;
12631269
columnOperation.IsStored = column.IsStored;
12641270
columnOperation.Comment = column.Comment;

‎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)