feat!: Add `AlterField` Operation to Migration Generator by kumarUjjawal · Pull Request #368 · cot-rs/cot

kumarUjjawal · 2025-07-04T17:56:38Z

Add `AlterField` Operation to Migration Generator

Overview

This PR introduces support for the AlterField operation in the migration generator.

Related Issues

#205

Reviewer Notes

I don't have full confidence in my implementation if you think this is not what was the intention of the Support "alter field" in automatic migration generator #205 feel free to close this for now.

codecov · 2025-07-04T18:25:01Z

Codecov Report

❌ Patch coverage is 97.01835% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cot-cli/src/migration_generator.rs	97.27%	7 Missing ⚠️
cot/src/db/migrations.rs	96.64%	4 Missing and 2 partials ⚠️

Flag	Coverage Δ
rust	`90.54% <97.01%> (+0.21%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cot/src/db/migrations.rs	`86.13% <96.64%> (+2.68%)`	⬆️
cot-cli/src/migration_generator.rs	`91.37% <97.27%> (+2.27%)`	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kumarUjjawal · 2025-07-07T07:22:45Z

@seqre any feedback for me?

seqre

This operation has many edge cases, and it should be thoroughly tested. Can you please add more tests first?

Think about possible cases, such as: name change (this one can be skipped for now, it's tricky), type change, attributes change, etc. All those should be either handled or visibly err when a given case is unhandled or ambiguous.

It seems that cot-cli/src/migration_generator.rs does not have a test for alter model, would you be so kind and add that too?

kumarUjjawal · 2025-07-09T04:58:34Z

Sure, Thank you!

kumarUjjawal · 2025-08-07T15:02:47Z

@seqre I added more tests can you check if they are what you are expecting and any feedbacks next steps. Thanks!

kumarUjjawal · 2025-08-29T03:11:02Z

@m4tx the error doesn't seem to come from my PR, can you take a look?

seqre

Thank you for your patience. I fixed the PR so it compiles and the tests pass! I left two comments and I'll try to test it myself tonight.

seqre

Hey @kumarUjjawal, it seems you force-pushed your old changes which broke tests and removed the custom operations added recently to master. I've fixed those for you, please remember to git pull before doing any changes!

Regarding the PR, we're getting closer with every iteration! This feature is really important, but it's slightly complicated, that's why I'm pushing for it to be well tested. I appreciate your efforts!

seqre · 2026-02-16T19:09:15Z

-        #[expect(unreachable_code)]
-        print_status_msg(
-            StatusType::Modified,
-            &format!(
-                "Field '{}' from Model '{}'",
-                &migration_field.name, migration_model.model.name
+                &migration_field.name, app_model.model.name
            ),
        );


Please bring this back

seqre · 2026-02-16T19:17:49Z

+            OperationInner::AlterField {
+                table_name,
+                old_field: _,
+                new_field,
+            } => {
+                let query = sea_query::Table::alter()
+                    .table(*table_name)
+                    .modify_column(new_field.as_column_def(database))
+                    .to_owned();
+                database.execute_schema(query).await?;
+            }


Could you please write some tests that cover this part?

seqre · 2026-02-16T19:17:55Z

+            OperationInner::AlterField {
+                table_name,
+                old_field,
+                new_field: _,
+            } => {
+                // To reverse an alteration, set the column back to the old definition
+                let query = sea_query::Table::alter()
+                    .table(*table_name)
+                    .modify_column(old_field.as_column_def(database))
+                    .to_owned();
+                database.execute_schema(query).await?;
+            }


Could you please write some tests that cover this part?

seqre · 2026-02-16T19:19:44Z

+                DynOperation::AlterField {
+                    new_field,
+                    model_ty,
+                    ..
+                } => {
+                    let mut ops = vec![(i, model_ty.clone())];
+                    // Only depend on the new foreign key, not the old one
+                    if let Some(to_type) = foreign_key_for_field(new_field) {
+                        ops.push((i, to_type));
+                    }
+                    ops
+                }


Could you please write some tests that cover this part?

This one interests me, from what I've read this would only apply when changing constraint on the foreign key, but that's a 3-operation situation (remove constraint, modify, add constraint), which I'm not sure would be supported in our current setup.

seqre · 2026-02-16T19:20:45Z

        RemoveModelBuilder::new()
    }

+    // TODO: docs


Could you please write the docs in similar fashion as other items above?

seqre · 2026-02-16T19:20:49Z

    }
 }

+// TODO: docs


Could you please write the docs in similar fashion as other items above?

kumarUjjawal · 2026-02-17T02:57:15Z

Sorry about the git issues, I had a dirty tree and I messed up merge. Thanks for cleaning this.

seqre · 2026-05-04T17:40:10Z

Hey @kumarUjjawal, how's the work going on this PR? Do you need any help from us?

kumarUjjawal · 2026-05-04T18:02:48Z

Hi @seqre Sorry for the delay, I got busy with another project, doesn't seem like I can give time for the next few weeks Can you pick this up from here?

This reverts commit 3d80f57.

- Restore the "Modified" status message after constructing the AlterField operation in make_alter_field_operation, mirroring the Adding/Added pattern used by make_add_field_operation. - Add rustdoc and examples to Operation::alter_field() and AlterFieldBuilder (and its methods), matching the style of remove_field/remove_model. Examples are marked no_run because SQLite does not support ALTER TABLE ... MODIFY COLUMN. - Add a unit test (test_operation_alter_field) and PostgreSQL/MySQL database tests for AlterField forwards/backwards. SQLite is skipped because sea_query's SQLite backend panics on modify_column; the postgres/mysql tests follow the pattern used by other ignored postgres/mysql tests. - Add unit tests covering the new AlterField branch in get_ops_adding_foreign_keys: one where the new field gains a foreign key and one where the new field has no foreign key.

github-actions Bot added C-cli Crate: cot-cli (issues and Pull Requests related to Cot CLI) C-lib Crate: cot (main library crate) labels Jul 4, 2025

seqre force-pushed the alter-field branch from de8cb85 to 3dac2e1 Compare July 5, 2025 11:22

seqre requested changes Jul 7, 2025

View reviewed changes

kumarUjjawal force-pushed the alter-field branch from 3dac2e1 to 76b9835 Compare July 28, 2025 17:39

kumarUjjawal force-pushed the alter-field branch from 1df5eb5 to c97bc84 Compare August 28, 2025 18:48

seqre force-pushed the alter-field branch from fa06660 to b7c5ae5 Compare December 2, 2025 19:49

seqre reviewed Feb 12, 2026

View reviewed changes

Comment thread cot-cli/src/migration_generator.rs

Comment thread cot-cli/src/migration_generator.rs Outdated

kumarUjjawal force-pushed the alter-field branch from 3f80674 to 3d80f57 Compare February 16, 2026 06:49

seqre force-pushed the alter-field branch from 23bd8d7 to dac062f Compare February 16, 2026 19:06

seqre requested changes Feb 16, 2026

View reviewed changes

seqre changed the title ~~feat: Add AlterField Operation to Migration Generator~~ feat!: Add AlterField Operation to Migration Generator Feb 16, 2026

seqre linked an issue Mar 23, 2026 that may be closed by this pull request

Support "alter field" in automatic migration generator #205

Open

kumarUjjawal and others added 7 commits May 21, 2026 14:24

feat: Add AlterField Operation to Migration Generator

34506da

added some more tests

6716e22

added missing docs

2c2e5e2

Marked DynOperation as #[non_exhaustive]

250c8f7

Revert "added missing docs"

e20434a

This reverts commit 3d80f57.

do not handle foreign key constraints yet

77a4d5d

seqre force-pushed the alter-field branch from 0538ee0 to 77a4d5d Compare May 21, 2026 12:24

chore(pre-commit.ci): auto fixes from pre-commit hooks

447b9d1

Uh oh!

Conversation

kumarUjjawal commented Jul 4, 2025

Add AlterField Operation to Migration Generator

Overview

Related Issues

Reviewer Notes

Uh oh!

codecov Bot commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kumarUjjawal commented Jul 7, 2025

Uh oh!

seqre left a comment

Choose a reason for hiding this comment

Uh oh!

kumarUjjawal commented Jul 9, 2025

Uh oh!

kumarUjjawal commented Aug 7, 2025

Uh oh!

kumarUjjawal commented Aug 29, 2025

Uh oh!

seqre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

seqre left a comment

Choose a reason for hiding this comment

Uh oh!

seqre Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

seqre Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

seqre Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

seqre Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

seqre Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

seqre Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

kumarUjjawal commented Feb 17, 2026

Uh oh!

seqre commented May 4, 2026

Uh oh!

kumarUjjawal commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add `AlterField` Operation to Migration Generator

codecov Bot commented Jul 4, 2025 •

edited

Loading