Fix to #32972 - The database default created by Migrations for primitive collections mapped to JSON is invalid #33039
Conversation
|
yes, we already do this for JSON InitializeJsonColumn. Only need to do this for references - JSON collections can never be required |
…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
There was a problem hiding this comment.
I know this implementation is in line with how things are currently done, but I'm wondering whether implementing the default value in the differ (as opposed to the migrations SQL generator) as the right place... Doesn't this mean that we're setting the default value even when simply a table is created with a JSON collection (as opposed to a collection column being added to an existing table)? In that case having a default value doesn't seem appropriate...
In other words, should this all happen in the AddColumn case in SqlMigrationsGenerator instead?
| @@ -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 | |||
There was a problem hiding this comment.
Should this logic move into GetDefaultValue instead, which is the code responsible for determining default values? It would have to accept the column as opposed to just the CLR type, of course.
There was a problem hiding this comment.
This code looks messy because it's conflating the default value that should be used when a new row is added with the default value that should be used when a new required column is added or an existing column is made required.
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