-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Full OldTable on AlterTableOperation, further comment support #16981
Conversation
return; | ||
} | ||
|
||
schema ??= model?.GetDefaultSchema() ?? DefaultSchema; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably generate SCHEMA_NAME()
in the SQL instead of relying on hard-coding dbo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently can't pass function calls to SQL Server stored procedures, need to define a variable instead?
😖
Declared a variable, which forces us to think about variable scope etc. - take a look at the second commit.
On the way found another bug where we weren't generating column comments in migration C# code for CreateTable...
ec2f71b
to
f532505
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(After addressing comments)
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
builder.Append("SET @schema = SCHEMA_NAME()") | ||
.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); | ||
} | ||
schema = "@schema"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a variable at all? Can't we just inline SCHEMA_NAME()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean inline SCHEMA_NAME()
into the stored procedure EXEC call? Apparently that simply isn't supported, only literals and variables (e.g. https://www.sqlservercentral.com/forums/topic/using-functions-as-parameters-in-stored-procedure-calls).
I definitely don't like that extra T-SQL variable, e.g. it forces us to distinguish first GenerateComment call (which declares and sets it) and the others (which don't, since it's also not possible to re-declare or un-declare a variable).
Will hold off on merging this in case you have some better SQL Server trick or something - otherwise let me know and I'll merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Doh, just saw your comment above about this too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any better ideas
Changed AlterTableOperation.OldTable from Annotatable to TableOperation,
and did various fixes throughout to support comment alteration on tables.
Some comment fixes and refactoring.
Fixes #16819
Fixes #16798