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 72.72727% with 63 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cot/src/db/migrations.rs	0.00%	42 Missing ⚠️
cot-cli/src/migration_generator.rs	88.88%	21 Missing ⚠️

Flag	Coverage Δ
rust	`89.54% <72.72%> (-0.12%)`	⬇️

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

Files with missing lines	Coverage Δ
cot-cli/src/migration_generator.rs	`88.81% <88.88%> (+1.13%)`	⬆️
cot/src/db/migrations.rs	`78.17% <0.00%> (-4.66%)`	⬇️

🚀 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.

cot-cli/src/migration_generator.rs

seqre · 2026-02-12T13:27:25Z

cot-cli/src/migration_generator.rs

+                    DynOperation::AddField { .. } => unreachable!(
+                        "AddField operation shouldn't be a dependency of CreateModel \
+                    because it doesn't create a new model"
+                    ),
+                    DynOperation::RemoveField { .. } => unreachable!(
+                        "RemoveField operation shouldn't be a dependency of CreateModel \
+                    because it doesn't create a new model"
+                    ),
+                    DynOperation::RemoveModel { .. } => unreachable!(
+                        "RemoveModel operation shouldn't be a dependency of CreateModel \
+                    because it doesn't create a new model"
+                    ),
+                    DynOperation::AlterField { .. } => unreachable!(
+                        "AlterField operation shouldn't be a dependency of CreateModel \
+                    because it doesn't create a new model"
+                    ),


I think we should just make a blanket default response, DynOperation will grow over time and it'd be annoying to add more entries to such match statements. The same would apply for lines 1174-1184. What do you think @m4tx?

Or should we just redo DynOperation so that all of those actions are just trait methods that we just call here? lol

This reverts commit 3d80f57.

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

cot-cli/src/migration_generator.rs

-        #[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

cot/src/db/migrations.rs

+            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

cot/src/db/migrations.rs

+            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

cot-cli/src/migration_generator.rs

+                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

cot/src/db/migrations.rs

        RemoveModelBuilder::new()
    }

+    // TODO: docs


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

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

cot/src/db/migrations.rs

    }
 }

+// 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.

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

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

kumarUjjawal and others added 5 commits February 16, 2026 20:06

feat: Add AlterField Operation to Migration Generator

ae1a31b

added some more tests

d269a87

added missing docs

ef8841a

Marked DynOperation as #[non_exhaustive]

a8fc093

Revert "added missing docs"

dac062f

This reverts commit 3d80f57.

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

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!

seqre Feb 12, 2026

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add `AlterField` Operation to Migration Generator

codecov bot commented Jul 4, 2025 •

edited

Loading