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

Default value constraint re-added when using .HasComment() #24272

Closed
kaspertygesen opened this issue Feb 25, 2021 · 4 comments · Fixed by #24274 or #24305
Closed

Default value constraint re-added when using .HasComment() #24272

kaspertygesen opened this issue Feb 25, 2021 · 4 comments · Fixed by #24274 or #24305
Assignees
Labels
area-migrations closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression Servicing-approved type-bug
Milestone

Comments

@kaspertygesen
Copy link

If I use .HasComment on a property which also has a default value, then the migration tries to ADD the constraint even if its already there.

modelBuilder.HasSequence<int>("TransactionId", "dbo")
   .StartsAt(100000);

modelBuilder.Entity<TransactionEntity>()
   .Property(o => o.TransactionId)
   .HasDefaultValueSql("NEXT VALUE FOR TransactionId");

modelBuilder.Entity<TransactionEntity>()
   .Property(_ => _.TransactionId)
   .HasComment("The id of the transaction. IMPORTANT: TransactionId must only be set to values returned by the sequence \"TransactionId\".");

The migration looks like this:

migrationBuilder.AlterColumn<int>(
    name: "TransactionId",
    table: "Transaction",
    type: "int",
    nullable: false,
    defaultValueSql: "NEXT VALUE FOR TransactionId",
    comment: "The id of the transaction. IMPORTANT: TransactionId must only be set to values returned by the sequence \"TransactionId\".",
    oldClrType: typeof(int),
    oldType: "int",
    oldDefaultValueSql: "NEXT VALUE FOR TransactionId",
    oldComment: "The id of the transaction IMPORTANT: TransactionId must only be set to values returned by the sequence \"TransactionId\".");

As the defaultValueSql and oldDefaultValueSql are identical I wouldn't expect a ALTER TABLE in my migration script.
But the migration script looks like this:

BEGIN TRANSACTION;
GO

ALTER TABLE [Transaction] ADD DEFAULT (NEXT VALUE FOR TransactionId) FOR [TransactionId];
DECLARE @defaultSchema AS sysname;
SET @defaultSchema = SCHEMA_NAME();
DECLARE @description AS sql_variant;
EXEC sp_dropextendedproperty 'MS_Description', 'SCHEMA', @defaultSchema, 'TABLE', N'Transaction', 'COLUMN', N'TransactionId';
SET @description = N'The id of the transaction. IMPORTANT: TransactionId must only be set to values returned by the sequence "TransactionId".';
EXEC sp_addextendedproperty 'MS_Description', @description, 'SCHEMA', @defaultSchema, 'TABLE', N'Transaction', 'COLUMN', N'TransactionId';
GO

INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
VALUES (N'20210225103305_Doc2', N'5.0.3');
GO

COMMIT;
GO

This results in the following error on on runtime:

Column already has a DEFAULT bound to it.
Could not create constraint or index. See previous errors.

EF CLI 5.0.3 (dotnet CLI)
Microsoft.EntityFrameworkCore 5.0.3
Microsoft.EntityFrameworkCore.SqlServer 5.0.3
MSSQL

@roji roji added the type-bug label Feb 25, 2021
@roji roji self-assigned this Feb 25, 2021
@roji roji added this to the 6.0.0 milestone Feb 25, 2021
@roji
Copy link
Member

roji commented Feb 25, 2021

Thanks for reporting this - I can reproduce it and will look into it.

@ajcvickers
Copy link
Member

/cc @bricelam to take a look.

@bricelam
Copy link
Contributor

bricelam commented Mar 1, 2021

This is a regression from 3.1 where we always dropped the default constraint every alter.

@bricelam bricelam removed this from the 6.0.0 milestone Mar 1, 2021
roji added a commit that referenced this issue Mar 1, 2021
roji added a commit that referenced this issue Mar 1, 2021
roji added a commit that referenced this issue Mar 1, 2021
@roji roji reopened this Mar 1, 2021
@roji
Copy link
Member

roji commented Mar 3, 2021

5.0.x patch proposal in #24305

@AndriySvyryd AndriySvyryd added this to the 5.0.5 milestone Mar 5, 2021
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Mar 5, 2021
roji added a commit that referenced this issue Mar 10, 2021
roji added a commit that referenced this issue Mar 10, 2021
roji added a commit that referenced this issue Mar 10, 2021
@roji roji closed this as completed Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migrations closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression Servicing-approved type-bug
Projects
None yet
5 participants