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.
  • Loading branch information
@maumar
maumar committed Feb 10, 2024
1 parent 36634d7 commit 1e56b22
Show file tree
Hide file tree
Showing 4 changed files with 473 additions and 1 deletion.
12 changes: 11 additions & 1 deletion src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ namespace Microsoft.EntityFrameworkCore.Migrations.Internal;
/// </summary>
public class MigrationsModelDiffer : IMigrationsModelDiffer
{
private static readonly bool UseOldBehavior32972 =
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue32972", out var enabled32972) && enabled32972;

private static readonly Type[] DropOperationTypes =
{
typeof(DropIndexOperation),
Expand Down Expand Up @@ -1190,7 +1193,14 @@ private void Initialize(

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 = !UseOldBehavior32972
&& !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.ColumnType = column.StoreType;
Expand Down
267 changes: 267 additions & 0 deletions test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1874,6 +1874,273 @@ await Test(
""");
}

[ConditionalFact]
public virtual Task Add_required_primitve_collection_to_existing_table()
=> 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));
});

[ConditionalFact]
public virtual Task Add_required_primitve_collection_with_custom_default_value_to_existing_table()
=> 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));
});

[ConditionalFact]
public abstract Task Add_required_primitve_collection_with_custom_default_value_sql_to_existing_table();

protected virtual Task Add_required_primitve_collection_with_custom_default_value_sql_to_existing_table_core(string defaultValueSql)
=> 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(defaultValueSql);
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));
});

[ConditionalFact(Skip = "issue #33038")]
public virtual Task Add_required_primitve_collection_with_custom_converter_to_existing_table()
=> 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));
});

[ConditionalFact]
public virtual Task Add_required_primitve_collection_with_custom_converter_and_custom_default_value_to_existing_table()
=> 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));
});

[ConditionalFact]
public virtual Task Add_optional_primitive_collection_to_existing_table()
=> 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));
});

[ConditionalFact]
public virtual Task Create_table_with_required_primitive_collection()
=> 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));
});

[ConditionalFact]
public virtual Task Create_table_with_optional_primitive_collection()
=> 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));
});

protected class Person
{
public int Id { get; set; }
Expand Down
Loading

0 comments on commit 1e56b22

Please sign in to comment.