Uh oh!
There was an error while loading. Please reload this page.
Do not carry a named default constraint onto the temporal history table - #38875
Open
HuzaifaChaudary wants to merge 1 commit into
Open
Do not carry a named default constraint onto the temporal history table#38875HuzaifaChaudary wants to merge 1 commit into
HuzaifaChaudary wants to merge 1 commit into
Conversation
the history column operation is a copy of the current one and the copy brings every annotation with it, including the default constraint name. so migrating away from a named default emitted a drop against the history table for a constraint that only ever existed on the current one and sql server failed with error 3728 dropping the name lets the existing lookup path run instead, which checks sys.columns first and does nothing when there is no default there the identity annotations right above are removed for the same reason
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Add-column history copies still propagate named default constraints and can generate duplicate constraint names.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Comment on lines
+3700
to
+3701
| RemoveDefaultConstraintNameAnnotation(alterHistoryTableColumn); | ||
| RemoveDefaultConstraintNameAnnotation(alterHistoryTableColumn.OldColumn); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#38836
problem
removing a named default constraint from a property on a temporal table generates a drop against the history table as well, and applying the migration fails:
the constraint only ever existed on the current table.
cause
when an
AlterColumnOperationruns against a temporal table the generator makes a copy of it for the history table.CopyColumnOperationcopies every annotation from the source, so the copy carriesRelational:DefaultConstraintNamewith the current table's constraint name on it.DropDefaultConstraintthen has a name to work with and emits the direct form:which fails, because that constraint is not there.
fix
drop the annotation from the history copy, the same way the identity annotations directly above it are already dropped and for the same reason: it describes something that belongs to the current table only.
with no name to use,
DropDefaultConstraintfalls through to its other branch, which looks the default up insys.columnsfirst and does nothing when the column has none. so the history column still gets altered, it just no longer tries to drop a constraint that was never created.tests
AlterColumnOperation_default_constraint_name_not_propagated_to_history_tableinSqlServerMigrationsSqlGeneratorTest, sitting next to the existingAddColumnOperation_identity_not_propagated_to_history_tablesince it is the same shape of problem.it is a generator test, so it needs no sql server. on
mainit fails withand passes with the change.
whole
SqlServerMigrationsSqlGeneratorTestclass: 132 passed, 0 failed.built with the repo's own sdk via
restore.sh, net11.0 on macos arm64.disclaimer: this contribution was prepared with the assistance of an ai agent. i reproduced the generated sql against
mainfirst, reviewed the change, and ran the generator test class locally before opening it.