Skip to content

Commit

Permalink
Fix to #32972 - The database default created by Migrations for primit…
Browse files Browse the repository at this point in the history 
…ive 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
  • Loading branch information
@maumar
maumar committed Feb 9, 2024
1 parent 6081e8c commit bff6751
Show file tree
Hide file tree
Showing 2 changed files with 343 additions and 6 deletions.
18 changes: 12 additions & 6 deletions src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1243,9 +1243,20 @@ protected virtual IEnumerable<MigrationOperation> Remove(IColumn source, DiffCon

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

columnOperation.DefaultValue = defaultValue
?? (inline || isNullable
? null
: GetDefaultValue(columnOperation.ClrType));
columnOperation.DefaultValueSql = column.DefaultValueSql;
columnOperation.ColumnType = column.StoreType;
columnOperation.MaxLength = column.MaxLength;
columnOperation.Precision = column.Precision;
Expand All @@ -1254,11 +1265,6 @@ protected virtual IEnumerable<MigrationOperation> Remove(IColumn source, DiffCon
columnOperation.IsFixedLength = column.IsFixedLength;
columnOperation.IsRowVersion = column.IsRowVersion;
columnOperation.IsNullable = isNullable;
columnOperation.DefaultValue = defaultValue
?? (inline || isNullable
? null
: GetDefaultValue(columnOperation.ClrType));
columnOperation.DefaultValueSql = column.DefaultValueSql;
columnOperation.ComputedColumnSql = column.ComputedColumnSql;
columnOperation.IsStored = column.IsStored;
columnOperation.Comment = column.Comment;
Expand Down
331 changes: 331 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10770,6 +10770,337 @@ public virtual async Task Temporal_multiop_convert_from_temporal_create_another_
""");
}

[ConditionalFact]
public virtual async Task Add_required_primitve_collection_to_existing_table()
{
await Test(
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.ToTable("Customers");
}),
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.Property<List<int>>("Numbers").IsRequired();
e.ToTable("Customers");
}),
model =>
{
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
Assert.Collection(
customersTable.Columns,
c => Assert.Equal("Id", c.Name),
c => Assert.Equal("Name", c.Name),
c => Assert.Equal("Numbers", c.Name));
Assert.Same(
customersTable.Columns.Single(c => c.Name == "Id"),
Assert.Single(customersTable.PrimaryKey!.Columns));
});

AssertSql(
"""
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NOT NULL DEFAULT N'[]';
""");
}

[ConditionalFact]
public virtual async Task Add_required_primitve_collection_with_custom_default_value_to_existing_table()
{
await Test(
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.ToTable("Customers");
}),
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.Property<List<int>>("Numbers").IsRequired().HasDefaultValue(new List<int> { 1, 2, 3 });
e.ToTable("Customers");
}),
model =>
{
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
Assert.Collection(
customersTable.Columns,
c => Assert.Equal("Id", c.Name),
c => Assert.Equal("Name", c.Name),
c => Assert.Equal("Numbers", c.Name));
Assert.Same(
customersTable.Columns.Single(c => c.Name == "Id"),
Assert.Single(customersTable.PrimaryKey!.Columns));
});

AssertSql(
"""
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NOT NULL DEFAULT N'[1,2,3]';
""");
}

[ConditionalFact]
public virtual async Task Add_required_primitve_collection_with_custom_default_value_sql_to_existing_table()
{
await Test(
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.ToTable("Customers");
}),
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.Property<List<int>>("Numbers").IsRequired().HasDefaultValueSql("N'[3, 2, 1]'");
e.ToTable("Customers");
}),
model =>
{
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
Assert.Collection(
customersTable.Columns,
c => Assert.Equal("Id", c.Name),
c => Assert.Equal("Name", c.Name),
c => Assert.Equal("Numbers", c.Name));
Assert.Same(
customersTable.Columns.Single(c => c.Name == "Id"),
Assert.Single(customersTable.PrimaryKey!.Columns));
});

AssertSql(
"""
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NOT NULL DEFAULT (N'[3, 2, 1]');
""");
}

[ConditionalFact(Skip = "issue #33038")]
public virtual async Task Add_required_primitve_collection_with_custom_converter_to_existing_table()
{
await Test(
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.ToTable("Customers");
}),
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.Property<List<int>>("Numbers").HasConversion(new ValueConverter<List<int>, string>(
convertToProviderExpression: x => x != null && x.Count > 0 ? "some numbers" : "nothing",
convertFromProviderExpression: x => x == "nothing" ? new List<int> { } : new List<int> { 7, 8, 9 }))
.IsRequired();
e.ToTable("Customers");
}),
model =>
{
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
Assert.Collection(
customersTable.Columns,
c => Assert.Equal("Id", c.Name),
c => Assert.Equal("Name", c.Name),
c => Assert.Equal("Numbers", c.Name));
Assert.Same(
customersTable.Columns.Single(c => c.Name == "Id"),
Assert.Single(customersTable.PrimaryKey!.Columns));
});

AssertSql(
"""
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NOT NULL DEFAULT N'nothing';
""");
}

[ConditionalFact]
public virtual async Task Add_required_primitve_collection_with_custom_converter_and_custom_default_value_to_existing_table()
{
await Test(
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.ToTable("Customers");
}),
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.Property<List<int>>("Numbers").HasConversion(new ValueConverter<List<int>, string>(
convertToProviderExpression: x => x != null && x.Count > 0 ? "some numbers" : "nothing",
convertFromProviderExpression: x => x == "nothing" ? new List<int> { } : new List<int> { 7, 8, 9 }))
.HasDefaultValue(new List<int> { 42 })
.IsRequired();
e.ToTable("Customers");
}),
model =>
{
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
Assert.Collection(
customersTable.Columns,
c => Assert.Equal("Id", c.Name),
c => Assert.Equal("Name", c.Name),
c => Assert.Equal("Numbers", c.Name));
Assert.Same(
customersTable.Columns.Single(c => c.Name == "Id"),
Assert.Single(customersTable.PrimaryKey!.Columns));
});

AssertSql(
"""
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NOT NULL DEFAULT N'some numbers';
""");
}

[ConditionalFact]
public virtual async Task Add_optional_primitive_collection_to_existing_table()
{
await Test(
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.ToTable("Customers");
}),
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.Property<List<int>>("Numbers");
e.ToTable("Customers");
}),
model =>
{
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
Assert.Collection(
customersTable.Columns,
c => Assert.Equal("Id", c.Name),
c => Assert.Equal("Name", c.Name),
c => Assert.Equal("Numbers", c.Name));
Assert.Same(
customersTable.Columns.Single(c => c.Name == "Id"),
Assert.Single(customersTable.PrimaryKey!.Columns));
});

AssertSql(
"""
ALTER TABLE [Customers] ADD [Numbers] nvarchar(max) NULL;
""");
}

[ConditionalFact]
public virtual async Task Create_table_with_required_primitive_collection()
{
await Test(
builder => { },
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.Property<List<int>>("Numbers").IsRequired();
e.ToTable("Customers");
}),
model =>
{
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
Assert.Collection(
customersTable.Columns,
c => Assert.Equal("Id", c.Name),
c => Assert.Equal("Name", c.Name),
c => Assert.Equal("Numbers", c.Name));
Assert.Same(
customersTable.Columns.Single(c => c.Name == "Id"),
Assert.Single(customersTable.PrimaryKey!.Columns));
});

AssertSql(
"""
CREATE TABLE [Customers] (
[Id] int NOT NULL IDENTITY,
[Name] nvarchar(max) NULL,
[Numbers] nvarchar(max) NOT NULL,
CONSTRAINT [PK_Customers] PRIMARY KEY ([Id])
);
""");
}

[ConditionalFact]
public virtual async Task Create_table_with_optional_primitive_collection()
{
await Test(
builder => { },
builder => builder.Entity(
"Customer", e =>
{
e.Property<int>("Id").ValueGeneratedOnAdd();
e.HasKey("Id");
e.Property<string>("Name");
e.Property<List<int>>("Numbers");
e.ToTable("Customers");
}),
model =>
{
var customersTable = Assert.Single(model.Tables.Where(t => t.Name == "Customers"));
Assert.Collection(
customersTable.Columns,
c => Assert.Equal("Id", c.Name),
c => Assert.Equal("Name", c.Name),
c => Assert.Equal("Numbers", c.Name));
Assert.Same(
customersTable.Columns.Single(c => c.Name == "Id"),
Assert.Single(customersTable.PrimaryKey!.Columns));
});

AssertSql(
"""
CREATE TABLE [Customers] (
[Id] int NOT NULL IDENTITY,
[Name] nvarchar(max) NULL,
[Numbers] nvarchar(max) NULL,
CONSTRAINT [PK_Customers] PRIMARY KEY ([Id])
);
""");
}

protected override string NonDefaultCollation
=> _nonDefaultCollation ??= GetDatabaseCollation() == "German_PhoneBook_CI_AS"
? "French_CI_AS"
Expand Down

0 comments on commit bff6751

Please sign in to comment.