Fix change_column to preserve old column attributes.#1381
Conversation
aidanharan
changed the title
There was a problem hiding this comment.
Pull request overview
Updates the SQLServer adapter’s change_column implementation to better preserve existing column attributes (notably defaults and nullability) when changing a column’s type, aligning behavior with upstream Rails expectations.
Changes:
- Preserve existing default constraints across
change_columntype changes when no new default is provided. - Preserve existing
NOT NULLwhennull:is not explicitly specified. - Update migration tests and add a changelog entry for the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/cases/migration_test_sqlserver.rb | Strengthens assertions around default preservation when changing lock_version type. |
| lib/active_record/connection_adapters/sqlserver/schema_statements.rb | Adjusts change_column to retain prior default/nullability more often during type changes. |
| CHANGELOG.md | Documents the fix in the Unreleased section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if no_constraint_options || (changing_type && default.present?) | ||
| default = quote_default_expression(default, column_object || column_for(table_name, column_name)) | ||
| sql_commands << "ALTER TABLE #{quote_table_name(table_name)} ADD CONSTRAINT #{default_constraint_name(table_name, column_name)} DEFAULT #{default} FOR #{quote_column_name(column_name)}" |
There was a problem hiding this comment.
default.present? will evaluate to false for valid defaults like false and "", so changing a column’s type would still drop those defaults. Consider checking for nil instead (and also preserving column_object.default_function when present) so boolean/empty-string/function defaults are retained across type changes.
| it "not raise exception when column contains default constraint" do | ||
| lock_version_column = Person.columns_hash["lock_version"] | ||
| assert_equal :integer, lock_version_column.type | ||
| assert lock_version_column.default.present? | ||
| assert_equal 0, lock_version_column.default | ||
| assert_nothing_raised { connection.change_column "people", "lock_version", :string } | ||
| Person.reset_column_information | ||
| lock_version_column = Person.columns_hash["lock_version"] | ||
| assert_equal :string, lock_version_column.type | ||
| assert lock_version_column.default.nil? | ||
| assert lock_version_column.default.present? | ||
| assert_equal "0", lock_version_column.default | ||
| assert_nothing_raised { connection.change_column "people", "lock_version", :integer } |
There was a problem hiding this comment.
This test covers preserving an integer default through a type change, but it won’t catch regressions for defaults like false, empty string, or default SQL functions (where present? is false / default_function is used). Adding a couple of assertions for those cases would help ensure change_column truly preserves existing defaults.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
aidanharan
changed the title
1 participant
Fix change_column to preserve old column attributes.
Ref: rails/rails#55837