Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing string property to required results in unexpected default value in migration #25899

Open
lauxjpn opened this issue Sep 7, 2021 · 5 comments

Comments

@lauxjpn
Copy link
Contributor

lauxjpn commented Sep 7, 2021

When introducing a new required (NOT NULL) string property to an entity, no default value is being setup implicitly (expected). If you want an empty string as a default value for the property, you have to set it up manually in the model.

However, when changing an existing non-required (NULL) string property to a required one, without specifying an explicit default value, MigrationsModelDiffer.GetDefaultValue() will suddenly introduce an empty string as a default value for that property, that was not requested by the user.

I understand, that it can be helpful to users that NULL values in string-based columns get automatically changed to empty strings, which is probably what most users want in those cases.

But shouldn't this be implemented as part of a one-time UPDATE statement, instead of as an implicit and permanent structure change?

Both operations are irreversible in regards to existing data anyway (destructive change), but the UPDATE statement would at least not lead to an unexpected permanent structure change.

An alternative would be two instead of one alter column statements, where the first one is the current implementation, and the second one immediately removes the empty string default value again. That should have the same effect as an UPDATE statement (though it would not help us with our particular Pomelo issue; see below).


We came across this issue while fixing PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#1476, because MySQL < 8.0.16 does not support default values for longtext columns. In the past, we just ignored those default values silently, but we want to issue a warning in the future, if users setup an unsupported default value.

After implementing a warning for those scenarios, the MigrationsTestBase.Alter_column_make_required() test suddenly failed unexpectedly with the newly introduced warning, because of the implicit empty string default value that EF Core generates for the string column.


Tested with EF Core 6 Preview 5.

@ajcvickers
Copy link
Member

/cc @bricelam

@lauxjpn
Copy link
Contributor Author

lauxjpn commented Sep 7, 2021

Or solution is to remove the empty string default value from the AlterColumnOperation and to add an UpdateDataOperation.
See PomeloFoundation/Pomelo.EntityFrameworkCore.MySql@042ee70

@bricelam
Copy link
Contributor

bricelam commented Sep 7, 2021

Related to #21765

@lauxjpn
Copy link
Contributor Author

lauxjpn commented Sep 7, 2021

@bricelam Oops, good point, we should of course emit the UpdateDataOperation before the AlterColumnOperation 😄

@zbarnett
Copy link

I believe I ran into a consequence of this behavior when attempting to squash migrations. After deleting all the migrations along with the snapshot, the new initial migration does not contain default values for required columns that previously had default values set as they had transitioned from not required to required.

Currently going through and explicitly setting those default values in the context file so the schema will be identical after the squash, but would be nice if those default values hadn't been added in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants