From 34506da97cee0eef5772ca876641a791851d3c8f Mon Sep 17 00:00:00 2001 From: Kumar Ujjawal Date: Fri, 4 Jul 2025 23:20:30 +0530 Subject: [PATCH 1/8] feat: Add `AlterField` Operation to Migration Generator --- cot-cli/src/migration_generator.rs | 208 ++++++++++++++++++++++++----- cot/src/db/migrations.rs | 73 ++++++++++ 2 files changed, 249 insertions(+), 32 deletions(-) diff --git a/cot-cli/src/migration_generator.rs b/cot-cli/src/migration_generator.rs index 4fc8a6a33..bf04015f1 100644 --- a/cot-cli/src/migration_generator.rs +++ b/cot-cli/src/migration_generator.rs @@ -816,9 +816,9 @@ impl MigrationOperationGenerator { #[must_use] fn make_alter_field_operation( - _app_model: &ModelInSource, + app_model: &ModelInSource, app_field: &Field, - migration_model: &ModelInSource, + _migration_model: &ModelInSource, migration_field: &Field, ) -> Option { if app_field == migration_field { @@ -828,20 +828,15 @@ impl MigrationOperationGenerator { StatusType::Modifying, &format!( "Field '{}' from Model '{}'", - &migration_field.name, migration_model.model.name - ), - ); - - todo!(); - - #[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 ), ); + Some(DynOperation::AlterField { + table_name: app_model.model.table_name.clone(), + model_ty: app_model.model.resolved_ty.clone(), + old_field: Box::new(migration_field.clone()), + new_field: Box::new(app_field.clone()), + }) } #[must_use] @@ -1130,24 +1125,22 @@ impl GeneratedMigration { } => { let to_type = match to { DynOperation::CreateModel { model_ty, .. } => model_ty, - 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::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" + ), }; trace!( "Removing foreign keys from {} to {}", @@ -1184,6 +1177,11 @@ impl GeneratedMigration { // RemoveModel doesn't create dependencies, it only removes a model unreachable!("RemoveModel operation should never create cycles") } + DynOperation::AlterField { .. } => { + // AlterField only changes metadata of an existing field, + // so it does not create dependency cycles. + unreachable!("AlterField operation should never create cycles") + } } } @@ -1282,6 +1280,18 @@ impl GeneratedMigration { // RemoveField Doesnt Add Foreign Keys Vec::new() } + 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 + } DynOperation::RemoveModel { .. } => { // RemoveModel Doesnt Add Foreign Keys Vec::new() @@ -1438,6 +1448,12 @@ pub enum DynOperation { model_ty: syn::Type, fields: Vec, }, + AlterField { + table_name: String, + model_ty: syn::Type, + old_field: Box, + new_field: Box, + }, } /// Returns whether given [`Field`] is a foreign key to given type. @@ -1492,6 +1508,22 @@ impl Repr for DynOperation { .build() } } + Self::AlterField { + table_name, + old_field, + new_field, + .. + } => { + let old_field = old_field.repr(); + let new_field = new_field.repr(); + quote! { + ::cot::db::migrations::Operation::alter_field() + .table_name(::cot::db::Identifier::new(#table_name)) + .old_field(#old_field) + .new_field(#new_field) + .build() + } + } Self::RemoveModel { table_name, fields, .. } => { @@ -2210,4 +2242,116 @@ mod tests { panic!("Expected a function item"); } } + + #[test] + fn make_alter_field_operation() { + let migration_model = get_test_model(); + let mut app_model = migration_model.clone(); + + app_model.model.fields[0].ty = parse_quote!(i32); + + let migration_field = &migration_model.model.fields[0]; + let app_field = &app_model.model.fields[0]; + + let operation = MigrationOperationGenerator::make_alter_field_operation( + &app_model, + app_field, + &migration_model, + migration_field, + ); + + match &operation { + Some(DynOperation::AlterField { + table_name, + model_ty, + old_field, + new_field, + }) => { + assert_eq!(table_name, "test_model"); + assert_eq!(model_ty, &parse_quote!(TestModel)); + assert_eq!(old_field.column_name, "field1"); + assert_eq!(old_field.ty, parse_quote!(String)); + assert_eq!(new_field.column_name, "field1"); + assert_eq!(new_field.ty, parse_quote!(i32)); + } + _ => panic!("Expected Some(DynOperation::AlterField)"), + } + } + + #[test] + fn generate_operations_with_altered_field() { + let migration_model = get_test_model(); + let mut app_model = migration_model.clone(); + + app_model.model.fields[0].ty = parse_quote!(i32); + + let app_models = vec![app_model.clone()]; + let migration_models = vec![migration_model.clone()]; + + let (modified_models, operations) = + MigrationGenerator::generate_operations(&app_models, &migration_models); + + assert_eq!(modified_models.len(), 1); + assert!( + operations.iter().any(|op| match op { + DynOperation::AlterField { + old_field, + new_field, + .. + } => old_field.ty == parse_quote!(String) && new_field.ty == parse_quote!(i32), + _ => false, + }), + "Expected an AlterField operation for changed type" + ); + } + + #[test] + fn repr_for_alter_field_operation() { + let op = DynOperation::AlterField { + table_name: "test_table".to_string(), + model_ty: parse_quote!(TestModel), + old_field: Box::new(Field { + name: format_ident!("test_field"), + column_name: "test_field".to_string(), + ty: parse_quote!(String), + auto_value: false, + primary_key: false, + unique: false, + foreign_key: None, + }), + new_field: Box::new(Field { + name: format_ident!("test_field"), + column_name: "test_field".to_string(), + ty: parse_quote!(i32), + auto_value: false, + primary_key: false, + unique: false, + foreign_key: None, + }), + }; + + let tokens = op.repr(); + let tokens_str = tokens.to_string(); + + assert!( + tokens_str.contains("alter_field"), + "Should call alter_field() but got: {tokens_str}" + ); + assert!( + tokens_str.contains("table_name"), + "Should call table_name() but got: {tokens_str}" + ); + assert!( + tokens_str.contains("old_field"), + "Should call old_field() but got: {tokens_str}" + ); + assert!( + tokens_str.contains("new_field"), + "Should call new_field() but got: {tokens_str}" + ); + assert!( + tokens_str.contains("build"), + "Should call build() but got: {tokens_str}" + ); + } } diff --git a/cot/src/db/migrations.rs b/cot/src/db/migrations.rs index 51dc19761..2b17fd869 100644 --- a/cot/src/db/migrations.rs +++ b/cot/src/db/migrations.rs @@ -521,6 +521,17 @@ impl Operation { .to_owned(); database.execute_schema(query).await?; } + 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?; + } OperationInner::RemoveModel { table_name, fields: _, @@ -594,6 +605,18 @@ impl Operation { .to_owned(); database.execute_schema(query).await?; } + 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?; + } OperationInner::RemoveModel { table_name, fields } => { let mut query = sea_query::Table::create().table(*table_name).to_owned(); for field in *fields { @@ -679,9 +702,16 @@ enum OperationInner { table_name: Identifier, fields: &'static [Field], }, +<<<<<<< HEAD Custom { forwards: CustomOperationFn, backwards: Option, +======= + AlterField { + table_name: Identifier, + old_field: Field, + new_field: Field, +>>>>>>> 09f0c87 (feat: Add `AlterField` Operation to Migration Generator) }, } @@ -1613,6 +1643,7 @@ impl RemoveModelBuilder { } } +<<<<<<< HEAD /// A builder for a custom operation. /// /// # Examples @@ -1663,6 +1694,48 @@ impl CustomBuilder { Operation::new(OperationInner::Custom { forwards: self.forwards, backwards: self.backwards, +======= +pub const fn alter_field() -> AlterFieldBuilder { + AlterFieldBuilder::new() +} + +#[derive(Debug, Copy, Clone)] +pub struct AlterFieldBuilder { + table_name: Option, + old_field: Option, + new_field: Option, +} + +impl AlterFieldBuilder { + const fn new() -> Self { + Self { + table_name: None, + old_field: None, + new_field: None, + } + } + + pub const fn table_name(mut self, table_name: Identifier) -> Self { + self.table_name = Some(table_name); + self + } + + pub const fn old_field(mut self, field: Field) -> Self { + self.old_field = Some(field); + self + } + + pub const fn new_field(mut self, field: Field) -> Self { + self.new_field = Some(field); + self + } + + pub const fn build(self) -> Operation { + Operation::new(OperationInner::AlterField { + table_name: unwrap_builder_option!(self, table_name), + old_field: unwrap_builder_option!(self, old_field), + new_field: unwrap_builder_option!(self, new_field), +>>>>>>> 09f0c87 (feat: Add `AlterField` Operation to Migration Generator) }) } } From 6716e22e92a1bfaa12bbe6e81622b9e948d4fa6b Mon Sep 17 00:00:00 2001 From: Kumar Ujjawal Date: Mon, 28 Jul 2025 23:07:12 +0530 Subject: [PATCH 2/8] added some more tests --- cot-cli/src/migration_generator.rs | 125 +++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/cot-cli/src/migration_generator.rs b/cot-cli/src/migration_generator.rs index bf04015f1..c224521ea 100644 --- a/cot-cli/src/migration_generator.rs +++ b/cot-cli/src/migration_generator.rs @@ -2354,4 +2354,129 @@ mod tests { "Should call build() but got: {tokens_str}" ); } + + #[test] + fn make_alter_field_operation_type_change() { + let migration_model = get_test_model(); + let mut app_model = migration_model.clone(); + + app_model.model.fields[0].ty = parse_quote!(i32); + + let migration_field = &migration_model.model.fields[0]; + let app_field = &app_model.model.fields[0]; + + let alter_op = MigrationOperationGenerator::make_alter_field_operation( + &app_model, + app_field, + &migration_model, + migration_field, + ); + + match alter_op { + Some(DynOperation::AlterField { + table_name, + model_ty, + old_field, + new_field, + }) => { + assert_eq!(table_name, "test_model"); + assert_eq!(model_ty, parse_quote!(TestModel)); + // The old field type should be String + assert_eq!(old_field.ty, parse_quote!(String)); + // The new field type should be i32 + assert_eq!(new_field.ty, parse_quote!(i32)); + assert_eq!(old_field.column_name, new_field.column_name); + } + _ => panic!("Expected DynOperation::AlterField for type change"), + } + } + + #[test] + fn make_alter_field_operation_nullable_change() { + let migration_model = get_test_model(); + let mut app_model = migration_model.clone(); + + app_model.model.fields[0].ty = parse_quote!(Option); + + let migration_field = &migration_model.model.fields[0]; + let app_field = &app_model.model.fields[0]; + + let alter_op = MigrationOperationGenerator::make_alter_field_operation( + &app_model, + app_field, + &migration_model, + migration_field, + ); + + match alter_op { + Some(DynOperation::AlterField { + table_name, + model_ty, + old_field, + new_field, + }) => { + assert_eq!(table_name, "test_model"); + assert_eq!(model_ty, parse_quote!(TestModel)); + // Old field type is String, new is Option + assert_eq!(old_field.ty, parse_quote!(String)); + assert_eq!(new_field.ty, parse_quote!(Option)); + assert_eq!(old_field.column_name, new_field.column_name); + } + _ => panic!("Expected DynOperation::AlterField for nullability change"), + } + } + + #[test] + fn make_alter_field_operation_primary_key_change() { + let migration_model = get_test_model(); + let mut app_model = migration_model.clone(); + + app_model.model.fields[0].primary_key = true; + + let migration_field = &migration_model.model.fields[0]; + let app_field = &app_model.model.fields[0]; + + let alter_op = MigrationOperationGenerator::make_alter_field_operation( + &app_model, + app_field, + &migration_model, + migration_field, + ); + + match alter_op { + Some(DynOperation::AlterField { + table_name, + model_ty, + old_field, + new_field, + }) => { + assert_eq!(table_name, "test_model"); + assert_eq!(model_ty, parse_quote!(TestModel)); + assert_ne!(old_field.primary_key, new_field.primary_key); + assert!(new_field.primary_key); + } + _ => panic!("Expected DynOperation::AlterField for primary_key change"), + } + } + + #[test] + fn make_alter_field_operation_no_change_returns_none() { + let migration_model = get_test_model(); + let app_model = migration_model.clone(); + + let migration_field = &migration_model.model.fields[0]; + let app_field = &app_model.model.fields[0]; + + let alter_op = MigrationOperationGenerator::make_alter_field_operation( + &app_model, + app_field, + &migration_model, + migration_field, + ); + + assert!( + alter_op.is_none(), + "No operation should be produced for identical fields" + ); + } } From 2c2e5e21fd8b23cc3bbdbb7fc05f1f3aeebab0ab Mon Sep 17 00:00:00 2001 From: Kumar Ujjawal Date: Fri, 29 Aug 2025 00:17:20 +0530 Subject: [PATCH 3/8] added missing docs --- cot/src/db/migrations.rs | 234 ++------------------------------------- 1 file changed, 12 insertions(+), 222 deletions(-) diff --git a/cot/src/db/migrations.rs b/cot/src/db/migrations.rs index 2b17fd869..b33be3245 100644 --- a/cot/src/db/migrations.rs +++ b/cot/src/db/migrations.rs @@ -4,9 +4,7 @@ mod sorter; use std::fmt; use std::fmt::{Debug, Formatter}; -use std::future::Future; -pub use cot_macros::migration_op; use sea_query::{ColumnDef, StringLen}; use thiserror::Error; use tracing::{Level, info}; @@ -22,13 +20,9 @@ pub enum MigrationEngineError { /// An error occurred while determining the correct order of migrations. #[error("error while determining the correct order of migrations")] MigrationSortError(#[from] MigrationSorterError), - /// A custom error occurred during a migration. - #[error("error running migration: {0}")] - Custom(String), } -/// A migration engine responsible for managing and applying database -/// migrations. +/// A migration engine that can run migrations. /// /// # Examples /// @@ -139,7 +133,7 @@ impl MigrationEngine { /// /// # Errors /// - /// Returns an error if any of the migrations fail to apply, or if there is + /// Throws an error if any of the migrations fail to apply, or if there is /// an error while interacting with the database, or if there is an /// error while marking a migration as applied. /// @@ -430,32 +424,11 @@ impl Operation { RemoveModelBuilder::new() } - /// Returns a builder for a custom operation. - /// - /// # Examples - /// - /// ``` - /// use cot::db::Result; - /// use cot::db::migrations::{MigrationContext, Operation, migration_op}; - /// - /// #[migration_op] - /// async fn forwards(ctx: MigrationContext<'_>) -> Result<()> { - /// // do something - /// Ok(()) - /// } - /// - /// const OPERATION: Operation = Operation::custom(forwards).build(); - /// ``` - #[must_use] - pub const fn custom(forwards: CustomOperationFn) -> CustomBuilder { - CustomBuilder::new(forwards) - } - /// Runs the operation forwards. /// /// # Errors /// - /// Returns an error if the operation fails to apply. + /// Throws an error if the operation fails to apply. /// /// # Examples /// @@ -539,13 +512,6 @@ impl Operation { let query = sea_query::Table::drop().table(*table_name).to_owned(); database.execute_schema(query).await?; } - OperationInner::Custom { - forwards, - backwards: _, - } => { - let context = MigrationContext::new(database); - forwards(context).await?; - } } Ok(()) } @@ -555,7 +521,7 @@ impl Operation { /// /// # Errors /// - /// Returns an error if the operation fails to apply. + /// Throws an error if the operation fails to apply. /// /// # Examples /// @@ -635,50 +601,11 @@ impl Operation { } database.execute_schema(query).await?; } - OperationInner::Custom { - forwards: _, - backwards, - } => { - if let Some(backwards) = backwards { - let context = MigrationContext::new(database); - backwards(context).await?; - } else { - return Err(crate::db::DatabaseError::MigrationError( - MigrationEngineError::Custom("Backwards migration not implemented".into()), - )); - } - } } Ok(()) } } -/// A context for a custom migration operation. -/// -/// This structure provides access to the database and other information that -/// might be needed during a migration. -#[derive(Debug)] -#[non_exhaustive] -pub struct MigrationContext<'a> { - /// The database connection to run the migration against. - pub db: &'a Database, -} - -impl<'a> MigrationContext<'a> { - fn new(db: &'a Database) -> Self { - Self { db } - } -} - -/// A type alias for a custom migration operation function. -/// -/// Typically, you should use the [`migration_op`] attribute macro to define -/// functions of this type. -pub type CustomOperationFn = - for<'a> fn( - MigrationContext<'a>, - ) -> std::pin::Pin> + Send + 'a>>; - #[derive(Debug, Copy, Clone)] enum OperationInner { /// Create a new model with the given fields. @@ -702,16 +629,10 @@ enum OperationInner { table_name: Identifier, fields: &'static [Field], }, -<<<<<<< HEAD - Custom { - forwards: CustomOperationFn, - backwards: Option, -======= AlterField { table_name: Identifier, old_field: Field, new_field: Field, ->>>>>>> 09f0c87 (feat: Add `AlterField` Operation to Migration Generator) }, } @@ -766,12 +687,6 @@ impl Field { /// Marks the field as a foreign key to the given model and field. /// - /// # Panics - /// - /// This function will panic if `on_delete` or `on_update` is set to - /// [`SetNone`](ForeignKeyOnDeletePolicy::SetNone) and the field is not - /// nullable. - /// /// # Cot CLI Usage /// /// Typically, you shouldn't need to use this directly. Instead, in most @@ -1643,62 +1558,13 @@ impl RemoveModelBuilder { } } -<<<<<<< HEAD -/// A builder for a custom operation. -/// -/// # Examples -/// -/// ``` -/// use cot::db::Result; -/// use cot::db::migrations::{MigrationContext, Operation, migration_op}; -/// -/// #[migration_op] -/// async fn forwards(ctx: MigrationContext<'_>) -> Result<()> { -/// // do something -/// Ok(()) -/// } -/// -/// #[migration_op] -/// async fn backwards(ctx: MigrationContext<'_>) -> Result<()> { -/// // undo something -/// Ok(()) -/// } -/// -/// const OPERATION: Operation = Operation::custom(forwards).backwards(backwards).build(); -/// ``` -#[derive(Debug, Copy, Clone)] -pub struct CustomBuilder { - forwards: CustomOperationFn, - backwards: Option, -} - -impl CustomBuilder { - #[must_use] - const fn new(forwards: CustomOperationFn) -> Self { - Self { - forwards, - backwards: None, - } - } - - /// Sets the backwards operation. - #[must_use] - pub const fn backwards(mut self, backwards: CustomOperationFn) -> Self { - self.backwards = Some(backwards); - self - } - - /// Builds the operation. - #[must_use] - pub const fn build(self) -> Operation { - Operation::new(OperationInner::Custom { - forwards: self.forwards, - backwards: self.backwards, -======= +/// Returns a builder for an operation that alters a field in a model. pub const fn alter_field() -> AlterFieldBuilder { AlterFieldBuilder::new() } +/// A builder for altering a field in a model. +#[must_use] #[derive(Debug, Copy, Clone)] pub struct AlterFieldBuilder { table_name: Option, @@ -1715,27 +1581,31 @@ impl AlterFieldBuilder { } } + /// Sets the name of the table to alter the field in. pub const fn table_name(mut self, table_name: Identifier) -> Self { self.table_name = Some(table_name); self } + /// Sets the old field definition. pub const fn old_field(mut self, field: Field) -> Self { self.old_field = Some(field); self } + /// Sets the new field definition. pub const fn new_field(mut self, field: Field) -> Self { self.new_field = Some(field); self } + /// Builds the operation. + #[must_use] pub const fn build(self) -> Operation { Operation::new(OperationInner::AlterField { table_name: unwrap_builder_option!(self, table_name), old_field: unwrap_builder_option!(self, old_field), new_field: unwrap_builder_option!(self, new_field), ->>>>>>> 09f0c87 (feat: Add `AlterField` Operation to Migration Generator) }) } } @@ -2218,86 +2088,6 @@ mod tests { } } - #[cot::test] - #[cfg_attr( - miri, - ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" - )] - async fn operation_custom() { - // test only on SQLite because we are using raw SQL - let test_db = TestDatabase::new_sqlite().await.unwrap(); - - #[migration_op] - async fn forwards(ctx: MigrationContext<'_>) -> Result<()> { - ctx.db - .raw("CREATE TABLE custom_test (id INTEGER PRIMARY KEY)") - .await?; - Ok(()) - } - - let operation = Operation::custom(forwards).build(); - operation.forwards(&test_db.database()).await.unwrap(); - - let result = test_db.database().raw("SELECT * FROM custom_test").await; - assert!(result.is_ok()); - } - - #[cot::test] - #[cfg_attr( - miri, - ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" - )] - async fn operation_custom_backwards() { - // test only on SQLite because we are using raw SQL - let test_db = TestDatabase::new_sqlite().await.unwrap(); - - #[migration_op] - async fn forwards(_ctx: MigrationContext<'_>) -> Result<()> { - panic!("this should not be called"); - } - - #[migration_op] - async fn backwards(ctx: MigrationContext<'_>) -> Result<()> { - ctx.db.raw("DROP TABLE custom_test_back").await?; - Ok(()) - } - - test_db - .database() - .raw("CREATE TABLE custom_test_back (id INTEGER PRIMARY KEY)") - .await - .unwrap(); - - let operation = Operation::custom(forwards).backwards(backwards).build(); - operation.backwards(&test_db.database()).await.unwrap(); - - let result = test_db - .database() - .raw("SELECT * FROM custom_test_back") - .await; - assert!(result.is_err()); - } - - #[cot::test] - #[cfg_attr( - miri, - ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" - )] - async fn operation_custom_backwards_not_implemented() { - // test only on SQLite because we are using raw SQL - let test_db = TestDatabase::new_sqlite().await.unwrap(); - - #[migration_op] - async fn forwards(_ctx: MigrationContext<'_>) -> Result<()> { - Ok(()) - } - - let operation = Operation::custom(forwards).build(); - let result = operation.backwards(&test_db.database()).await; - - assert!(result.is_err()); - } - #[test] fn field_new() { let field = Field::new(Identifier::new("id"), ColumnType::Integer) From 250c8f7ecfee477217221eb7e1838bc5f6f1b655 Mon Sep 17 00:00:00 2001 From: Kumar Ujjawal Date: Mon, 16 Feb 2026 13:14:29 +0530 Subject: [PATCH 4/8] Marked DynOperation as #[non_exhaustive] --- cot-cli/src/migration_generator.rs | 41 ++++++------------------------ 1 file changed, 8 insertions(+), 33 deletions(-) diff --git a/cot-cli/src/migration_generator.rs b/cot-cli/src/migration_generator.rs index c224521ea..8d4028d55 100644 --- a/cot-cli/src/migration_generator.rs +++ b/cot-cli/src/migration_generator.rs @@ -1125,22 +1125,9 @@ impl GeneratedMigration { } => { let to_type = match to { DynOperation::CreateModel { model_ty, .. } => model_ty, - 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" - ), + _ => { + unreachable!("Only CreateModel can be a dependency target for CreateModel") + } }; trace!( "Removing foreign keys from {} to {}", @@ -1164,23 +1151,10 @@ impl GeneratedMigration { result } - DynOperation::AddField { .. } => { - // AddField only links two already existing models together, so - // removing it shouldn't ever affect whether a graph is cyclic - unreachable!("AddField operation should never create cycles") - } - DynOperation::RemoveField { .. } => { - // RemoveField doesn't create dependencies, it only removes a field - unreachable!("RemoveField operation should never create cycles") - } - DynOperation::RemoveModel { .. } => { - // RemoveModel doesn't create dependencies, it only removes a model - unreachable!("RemoveModel operation should never create cycles") - } - DynOperation::AlterField { .. } => { - // AlterField only changes metadata of an existing field, - // so it does not create dependency cycles. - unreachable!("AlterField operation should never create cycles") + _ => { + // Only CreateModel can create dependency cycles; all other ops + // change existing schema without introducing new FK dependencies. + unreachable!("Only CreateModel operation can create cycles") } } } @@ -1424,6 +1398,7 @@ impl Repr for DynDependency { /// runtime and is using codegen types. /// /// This is used to generate migration files. +#[non_exhaustive] #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum DynOperation { CreateModel { From e20434ae988c07e0d35251a3ff1b8a853812f4b9 Mon Sep 17 00:00:00 2001 From: Marek 'seqre' Grzelak Date: Mon, 16 Feb 2026 19:16:08 +0100 Subject: [PATCH 5/8] Revert "added missing docs" This reverts commit 3d80f57b6c1392a494d5681eeaee2e8f7efb00a6. --- cot/src/db/migrations.rs | 235 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 225 insertions(+), 10 deletions(-) diff --git a/cot/src/db/migrations.rs b/cot/src/db/migrations.rs index b33be3245..88ea97384 100644 --- a/cot/src/db/migrations.rs +++ b/cot/src/db/migrations.rs @@ -4,7 +4,9 @@ mod sorter; use std::fmt; use std::fmt::{Debug, Formatter}; +use std::future::Future; +pub use cot_macros::migration_op; use sea_query::{ColumnDef, StringLen}; use thiserror::Error; use tracing::{Level, info}; @@ -20,9 +22,13 @@ pub enum MigrationEngineError { /// An error occurred while determining the correct order of migrations. #[error("error while determining the correct order of migrations")] MigrationSortError(#[from] MigrationSorterError), + /// A custom error occurred during a migration. + #[error("error running migration: {0}")] + Custom(String), } -/// A migration engine that can run migrations. +/// A migration engine responsible for managing and applying database +/// migrations. /// /// # Examples /// @@ -133,7 +139,7 @@ impl MigrationEngine { /// /// # Errors /// - /// Throws an error if any of the migrations fail to apply, or if there is + /// Returns an error if any of the migrations fail to apply, or if there is /// an error while interacting with the database, or if there is an /// error while marking a migration as applied. /// @@ -424,11 +430,37 @@ impl Operation { RemoveModelBuilder::new() } + // TODO: docs + pub const fn alter_field() -> AlterFieldBuilder { + AlterFieldBuilder::new() + } + + /// Returns a builder for a custom operation. + /// + /// # Examples + /// + /// ``` + /// use cot::db::Result; + /// use cot::db::migrations::{MigrationContext, Operation, migration_op}; + /// + /// #[migration_op] + /// async fn forwards(ctx: MigrationContext<'_>) -> Result<()> { + /// // do something + /// Ok(()) + /// } + /// + /// const OPERATION: Operation = Operation::custom(forwards).build(); + /// ``` + #[must_use] + pub const fn custom(forwards: CustomOperationFn) -> CustomBuilder { + CustomBuilder::new(forwards) + } + /// Runs the operation forwards. /// /// # Errors /// - /// Throws an error if the operation fails to apply. + /// Returns an error if the operation fails to apply. /// /// # Examples /// @@ -512,6 +544,13 @@ impl Operation { let query = sea_query::Table::drop().table(*table_name).to_owned(); database.execute_schema(query).await?; } + OperationInner::Custom { + forwards, + backwards: _, + } => { + let context = MigrationContext::new(database); + forwards(context).await?; + } } Ok(()) } @@ -521,7 +560,7 @@ impl Operation { /// /// # Errors /// - /// Throws an error if the operation fails to apply. + /// Returns an error if the operation fails to apply. /// /// # Examples /// @@ -601,11 +640,50 @@ impl Operation { } database.execute_schema(query).await?; } + OperationInner::Custom { + forwards: _, + backwards, + } => { + if let Some(backwards) = backwards { + let context = MigrationContext::new(database); + backwards(context).await?; + } else { + return Err(crate::db::DatabaseError::MigrationError( + MigrationEngineError::Custom("Backwards migration not implemented".into()), + )); + } + } } Ok(()) } } +/// A context for a custom migration operation. +/// +/// This structure provides access to the database and other information that +/// might be needed during a migration. +#[derive(Debug)] +#[non_exhaustive] +pub struct MigrationContext<'a> { + /// The database connection to run the migration against. + pub db: &'a Database, +} + +impl<'a> MigrationContext<'a> { + fn new(db: &'a Database) -> Self { + Self { db } + } +} + +/// A type alias for a custom migration operation function. +/// +/// Typically, you should use the [`migration_op`] attribute macro to define +/// functions of this type. +pub type CustomOperationFn = + for<'a> fn( + MigrationContext<'a>, + ) -> std::pin::Pin> + Send + 'a>>; + #[derive(Debug, Copy, Clone)] enum OperationInner { /// Create a new model with the given fields. @@ -634,6 +712,10 @@ enum OperationInner { old_field: Field, new_field: Field, }, + Custom { + forwards: CustomOperationFn, + backwards: Option, + }, } /// A field in a model. @@ -687,6 +769,12 @@ impl Field { /// Marks the field as a foreign key to the given model and field. /// + /// # Panics + /// + /// This function will panic if `on_delete` or `on_update` is set to + /// [`SetNone`](ForeignKeyOnDeletePolicy::SetNone) and the field is not + /// nullable. + /// /// # Cot CLI Usage /// /// Typically, you shouldn't need to use this directly. Instead, in most @@ -1558,13 +1646,61 @@ impl RemoveModelBuilder { } } -/// Returns a builder for an operation that alters a field in a model. -pub const fn alter_field() -> AlterFieldBuilder { - AlterFieldBuilder::new() +/// A builder for a custom operation. +/// +/// # Examples +/// +/// ``` +/// use cot::db::Result; +/// use cot::db::migrations::{MigrationContext, Operation, migration_op}; +/// +/// #[migration_op] +/// async fn forwards(ctx: MigrationContext<'_>) -> Result<()> { +/// // do something +/// Ok(()) +/// } +/// +/// #[migration_op] +/// async fn backwards(ctx: MigrationContext<'_>) -> Result<()> { +/// // undo something +/// Ok(()) +/// } +/// +/// const OPERATION: Operation = Operation::custom(forwards).backwards(backwards).build(); +/// ``` +#[derive(Debug, Copy, Clone)] +pub struct CustomBuilder { + forwards: CustomOperationFn, + backwards: Option, +} + +impl CustomBuilder { + #[must_use] + const fn new(forwards: CustomOperationFn) -> Self { + Self { + forwards, + backwards: None, + } + } + + /// Sets the backwards operation. + #[must_use] + pub const fn backwards(mut self, backwards: CustomOperationFn) -> Self { + self.backwards = Some(backwards); + self + } + + /// Builds the operation. + #[must_use] + pub const fn build(self) -> Operation { + Operation::new(OperationInner::Custom { + forwards: self.forwards, + backwards: self.backwards, + }) + } } -/// A builder for altering a field in a model. -#[must_use] +// TODO: docs #[derive(Debug, Copy, Clone)] pub struct AlterFieldBuilder { table_name: Option, @@ -1600,7 +1736,6 @@ impl AlterFieldBuilder { } /// Builds the operation. - #[must_use] pub const fn build(self) -> Operation { Operation::new(OperationInner::AlterField { table_name: unwrap_builder_option!(self, table_name), @@ -2088,6 +2223,86 @@ mod tests { } } + #[cot::test] + #[cfg_attr( + miri, + ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" + )] + async fn operation_custom() { + // test only on SQLite because we are using raw SQL + let test_db = TestDatabase::new_sqlite().await.unwrap(); + + #[migration_op] + async fn forwards(ctx: MigrationContext<'_>) -> Result<()> { + ctx.db + .raw("CREATE TABLE custom_test (id INTEGER PRIMARY KEY)") + .await?; + Ok(()) + } + + let operation = Operation::custom(forwards).build(); + operation.forwards(&test_db.database()).await.unwrap(); + + let result = test_db.database().raw("SELECT * FROM custom_test").await; + assert!(result.is_ok()); + } + + #[cot::test] + #[cfg_attr( + miri, + ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" + )] + async fn operation_custom_backwards() { + // test only on SQLite because we are using raw SQL + let test_db = TestDatabase::new_sqlite().await.unwrap(); + + #[migration_op] + async fn forwards(_ctx: MigrationContext<'_>) -> Result<()> { + panic!("this should not be called"); + } + + #[migration_op] + async fn backwards(ctx: MigrationContext<'_>) -> Result<()> { + ctx.db.raw("DROP TABLE custom_test_back").await?; + Ok(()) + } + + test_db + .database() + .raw("CREATE TABLE custom_test_back (id INTEGER PRIMARY KEY)") + .await + .unwrap(); + + let operation = Operation::custom(forwards).backwards(backwards).build(); + operation.backwards(&test_db.database()).await.unwrap(); + + let result = test_db + .database() + .raw("SELECT * FROM custom_test_back") + .await; + assert!(result.is_err()); + } + + #[cot::test] + #[cfg_attr( + miri, + ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" + )] + async fn operation_custom_backwards_not_implemented() { + // test only on SQLite because we are using raw SQL + let test_db = TestDatabase::new_sqlite().await.unwrap(); + + #[migration_op] + async fn forwards(_ctx: MigrationContext<'_>) -> Result<()> { + Ok(()) + } + + let operation = Operation::custom(forwards).build(); + let result = operation.backwards(&test_db.database()).await; + + assert!(result.is_err()); + } + #[test] fn field_new() { let field = Field::new(Identifier::new("id"), ColumnType::Integer) From e09990505f3d3d9c160e7b9aae65ca2ec7d4388d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:08:27 +0000 Subject: [PATCH 6/8] feat(migrations): address review feedback on AlterField - 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. --- cot-cli/src/migration_generator.rs | 105 ++++++++- cot/src/db/migrations.rs | 341 ++++++++++++++++++++++++++++- 2 files changed, 439 insertions(+), 7 deletions(-) diff --git a/cot-cli/src/migration_generator.rs b/cot-cli/src/migration_generator.rs index 8d4028d55..1401da304 100644 --- a/cot-cli/src/migration_generator.rs +++ b/cot-cli/src/migration_generator.rs @@ -831,12 +831,23 @@ impl MigrationOperationGenerator { &migration_field.name, app_model.model.name ), ); - Some(DynOperation::AlterField { + + let op = DynOperation::AlterField { table_name: app_model.model.table_name.clone(), model_ty: app_model.model.resolved_ty.clone(), old_field: Box::new(migration_field.clone()), new_field: Box::new(app_field.clone()), - }) + }; + + print_status_msg( + StatusType::Modified, + &format!( + "Field '{}' from Model '{}'", + &migration_field.name, app_model.model.name + ), + ); + + Some(op) } #[must_use] @@ -1862,6 +1873,96 @@ mod tests { })); } + #[test] + fn get_foreign_key_dependencies_with_alter_field() { + // AlterField where the new field gains a foreign key should produce a + // dependency on the referenced model. The old field's foreign key + // (if any) is intentionally not tracked here. A `CreateModel` for the + // altered table is included so the altered model is not flagged as an + // external dependency by `get_foreign_key_dependencies`. + let operations = vec![ + DynOperation::CreateModel { + table_name: "table1".to_string(), + model_ty: parse_quote!(Table1), + fields: vec![], + }, + DynOperation::AlterField { + table_name: "table1".to_string(), + model_ty: parse_quote!(Table1), + old_field: Box::new(Field { + name: format_ident!("field1"), + column_name: "field1".to_string(), + ty: parse_quote!(i32), + auto_value: false, + primary_key: false, + unique: false, + foreign_key: None, + }), + new_field: Box::new(Field { + name: format_ident!("field1"), + column_name: "field1".to_string(), + ty: parse_quote!(ForeignKey), + auto_value: false, + primary_key: false, + unique: false, + foreign_key: Some(ForeignKeySpec { + to_model: parse_quote!(crate::Table2), + }), + }), + }, + ]; + + let external_dependencies = GeneratedMigration::get_foreign_key_dependencies(&operations); + assert_eq!(external_dependencies.len(), 1); + assert_eq!( + external_dependencies[0], + DynDependency::Model { + model_type: parse_quote!(crate::Table2), + } + ); + } + + #[test] + fn get_foreign_key_dependencies_alter_field_no_new_foreign_key() { + // If the new field has no foreign key, AlterField should produce no + // external dependencies (when the altered table itself is created in + // the same migration), even when the old field had one. + let operations = vec![ + DynOperation::CreateModel { + table_name: "table1".to_string(), + model_ty: parse_quote!(Table1), + fields: vec![], + }, + DynOperation::AlterField { + table_name: "table1".to_string(), + model_ty: parse_quote!(Table1), + old_field: Box::new(Field { + name: format_ident!("field1"), + column_name: "field1".to_string(), + ty: parse_quote!(ForeignKey), + auto_value: false, + primary_key: false, + unique: false, + foreign_key: Some(ForeignKeySpec { + to_model: parse_quote!(crate::Table2), + }), + }), + new_field: Box::new(Field { + name: format_ident!("field1"), + column_name: "field1".to_string(), + ty: parse_quote!(i32), + auto_value: false, + primary_key: false, + unique: false, + foreign_key: None, + }), + }, + ]; + + let external_dependencies = GeneratedMigration::get_foreign_key_dependencies(&operations); + assert!(external_dependencies.is_empty()); + } + fn get_test_model() -> ModelInSource { ModelInSource { model_item: parse_quote! { diff --git a/cot/src/db/migrations.rs b/cot/src/db/migrations.rs index 88ea97384..c20cd7fa7 100644 --- a/cot/src/db/migrations.rs +++ b/cot/src/db/migrations.rs @@ -430,7 +430,56 @@ impl Operation { RemoveModelBuilder::new() } - // TODO: docs + /// Returns a builder for an operation that alters an existing field on a + /// model. + /// + /// Typically, you shouldn't need to use this directly. Instead, in most + /// cases, this can be automatically generated by the Cot CLI. + /// + /// # Examples + /// + /// Note: `AlterField` is currently only supported on PostgreSQL and + /// MySQL backends; SQLite's `ALTER TABLE` does not support modifying + /// existing columns. + /// + /// ```no_run + /// use cot::db::migrations::{Field, Operation}; + /// use cot::db::{DatabaseField, Identifier}; + /// + /// # #[tokio::main] + /// # async fn main() -> cot::Result<()> { + /// const FIELDS: &[Field] = &[ + /// Field::new(Identifier::new("id"), ::TYPE) + /// .primary_key() + /// .auto(), + /// Field::new(Identifier::new("name"), ::TYPE), + /// ]; + /// + /// // First create the table + /// const CREATE_OPERATION: Operation = Operation::create_model() + /// .table_name(Identifier::new("todoapp__my_model")) + /// .fields(FIELDS) + /// .build(); + /// let database = cot::db::Database::new("postgres://localhost/example").await?; + /// CREATE_OPERATION.forwards(&database).await?; + /// + /// // Then alter the `name` field to allow null values + /// const OPERATION: Operation = Operation::alter_field() + /// .table_name(Identifier::new("todoapp__my_model")) + /// .old_field(Field::new( + /// Identifier::new("name"), + /// ::TYPE, + /// )) + /// .new_field( + /// Field::new(Identifier::new("name"), ::TYPE).null(), + /// ) + /// .build(); + /// + /// OPERATION.forwards(&database).await?; + /// # Ok(()) + /// # } + /// ``` + #[must_use] pub const fn alter_field() -> AlterFieldBuilder { AlterFieldBuilder::new() } @@ -1700,7 +1749,47 @@ impl CustomBuilder { } } -// TODO: docs +/// A builder for altering an existing field on a model. +/// +/// Typically, you shouldn't need to use this directly. Instead, in most +/// cases, this can be automatically generated by the Cot CLI. +/// +/// Note: `AlterField` is currently only supported on PostgreSQL and MySQL +/// backends; SQLite's `ALTER TABLE` does not support modifying existing +/// columns. +/// +/// # Examples +/// +/// ```no_run +/// use cot::db::migrations::{Field, Operation}; +/// use cot::db::{DatabaseField, Identifier}; +/// +/// # #[tokio::main] +/// # async fn main() -> cot::Result<()> { +/// # const CREATE_MODEL_OPERATION: Operation = Operation::create_model() +/// # .table_name(Identifier::new("todoapp__my_model")) +/// # .fields(&[ +/// # Field::new(Identifier::new("id"), ::TYPE) +/// # .primary_key() +/// # .auto(), +/// # Field::new(Identifier::new("name"), ::TYPE), +/// # ]) +/// # .build(); +/// const OPERATION: Operation = Operation::alter_field() +/// .table_name(Identifier::new("todoapp__my_model")) +/// .old_field(Field::new( +/// Identifier::new("name"), +/// ::TYPE, +/// )) +/// .new_field(Field::new(Identifier::new("name"), ::TYPE).null()) +/// .build(); +/// +/// # let database = cot::db::Database::new("postgres://localhost/example").await?; +/// # CREATE_MODEL_OPERATION.forwards(&database).await?; +/// # OPERATION.forwards(&database).await?; +/// # Ok(()) +/// # } +/// ``` #[derive(Debug, Copy, Clone)] pub struct AlterFieldBuilder { table_name: Option, @@ -1708,7 +1797,14 @@ pub struct AlterFieldBuilder { new_field: Option, } +impl Default for AlterFieldBuilder { + fn default() -> Self { + Self::new() + } +} + impl AlterFieldBuilder { + #[must_use] const fn new() -> Self { Self { table_name: None, @@ -1718,24 +1814,98 @@ impl AlterFieldBuilder { } /// Sets the name of the table to alter the field in. + /// + /// # Cot CLI Usage + /// + /// Typically, you shouldn't need to use this directly. Instead, in most + /// cases, this can be automatically generated by the Cot CLI. + /// + /// # Examples + /// + /// ``` + /// use cot::db::migrations::{Field, Operation}; + /// use cot::db::{DatabaseField, Identifier}; + /// + /// const OPERATION: Operation = Operation::alter_field() + /// .table_name(Identifier::new("todoapp__my_model")) + /// .old_field(Field::new( + /// Identifier::new("name"), + /// ::TYPE, + /// )) + /// .new_field(Field::new(Identifier::new("name"), ::TYPE).null()) + /// .build(); + /// ``` + #[must_use] pub const fn table_name(mut self, table_name: Identifier) -> Self { self.table_name = Some(table_name); self } - /// Sets the old field definition. + /// Sets the previous definition of the field that is being altered. + /// + /// This is used to roll the migration back via [`Operation::backwards`]. + /// + /// # Cot CLI Usage + /// + /// Typically, you shouldn't need to use this directly. Instead, in most + /// cases, this can be automatically generated by the Cot CLI. + /// + /// # Examples + /// + /// ``` + /// use cot::db::migrations::{Field, Operation}; + /// use cot::db::{DatabaseField, Identifier}; + /// + /// const OPERATION: Operation = Operation::alter_field() + /// .table_name(Identifier::new("todoapp__my_model")) + /// .old_field(Field::new( + /// Identifier::new("name"), + /// ::TYPE, + /// )) + /// .new_field(Field::new(Identifier::new("name"), ::TYPE).null()) + /// .build(); + /// ``` + #[must_use] pub const fn old_field(mut self, field: Field) -> Self { self.old_field = Some(field); self } - /// Sets the new field definition. + /// Sets the new definition of the field that is being altered. + /// + /// # Cot CLI Usage + /// + /// Typically, you shouldn't need to use this directly. Instead, in most + /// cases, this can be automatically generated by the Cot CLI. + /// + /// # Examples + /// + /// ``` + /// use cot::db::migrations::{Field, Operation}; + /// use cot::db::{DatabaseField, Identifier}; + /// + /// const OPERATION: Operation = Operation::alter_field() + /// .table_name(Identifier::new("todoapp__my_model")) + /// .old_field(Field::new( + /// Identifier::new("name"), + /// ::TYPE, + /// )) + /// .new_field(Field::new(Identifier::new("name"), ::TYPE).null()) + /// .build(); + /// ``` + #[must_use] pub const fn new_field(mut self, field: Field) -> Self { self.new_field = Some(field); self } - /// Builds the operation. + /// Builds the [`Operation`]. + /// + /// # Panics + /// + /// Panics if [`Self::table_name`], [`Self::old_field`] or + /// [`Self::new_field`] have not been called. + #[must_use] pub const fn build(self) -> Operation { Operation::new(OperationInner::AlterField { table_name: unwrap_builder_option!(self, table_name), @@ -2481,6 +2651,167 @@ mod tests { assert!(result.is_ok()); } + #[test] + fn test_operation_alter_field() { + let operation = Operation::alter_field() + .table_name(Identifier::new("testapp__test_model")) + .old_field(Field::new( + Identifier::new("name"), + ::TYPE, + )) + .new_field( + Field::new(Identifier::new("name"), ::TYPE).null(), + ) + .build(); + + if let OperationInner::AlterField { + table_name, + old_field, + new_field, + } = operation.inner + { + assert_eq!(table_name.to_string(), "testapp__test_model"); + assert_eq!(old_field.name.to_string(), "name"); + assert_eq!(old_field.ty, ColumnType::Text); + assert_eq!(new_field.name.to_string(), "name"); + assert!(new_field.null); + } else { + panic!("Expected OperationInner::AlterField"); + } + } + + // Note: AlterField is only tested against PostgreSQL and MySQL because + // SQLite's `sea_query` backend does not support `ALTER TABLE ... MODIFY + // COLUMN` (it panics). These tests follow the pattern used by the + // ignored `*_postgres` / `*_mysql` tests generated by + // `cot_macros::dbtest`. + + async fn run_alter_field_forwards(test_db: &mut TestDatabase) { + const FIELDS: &[Field] = &[ + Field::new(Identifier::new("id"), ::TYPE) + .primary_key() + .auto(), + Field::new(Identifier::new("name"), ::TYPE), + ]; + + let create_operation = Operation::create_model() + .table_name(Identifier::new("testapp__test_model")) + .fields(FIELDS) + .build(); + + create_operation + .forwards(&test_db.database()) + .await + .unwrap(); + + let alter_operation = Operation::alter_field() + .table_name(Identifier::new("testapp__test_model")) + .old_field(Field::new( + Identifier::new("name"), + ::TYPE, + )) + .new_field( + Field::new(Identifier::new("name"), ::TYPE).null(), + ) + .build(); + + let result = alter_operation.forwards(&test_db.database()).await; + assert!(result.is_ok()); + } + + async fn run_alter_field_backwards(test_db: &mut TestDatabase) { + const FIELDS: &[Field] = &[ + Field::new(Identifier::new("id"), ::TYPE) + .primary_key() + .auto(), + Field::new(Identifier::new("name"), ::TYPE), + ]; + + let create_operation = Operation::create_model() + .table_name(Identifier::new("testapp__test_model")) + .fields(FIELDS) + .build(); + + create_operation + .forwards(&test_db.database()) + .await + .unwrap(); + + let alter_operation = Operation::alter_field() + .table_name(Identifier::new("testapp__test_model")) + .old_field(Field::new( + Identifier::new("name"), + ::TYPE, + )) + .new_field( + Field::new(Identifier::new("name"), ::TYPE).null(), + ) + .build(); + + alter_operation + .forwards(&test_db.database()) + .await + .unwrap(); + + let result = alter_operation.backwards(&test_db.database()).await; + assert!(result.is_ok()); + } + + #[ignore = "Tests that use PostgreSQL are ignored by default"] + #[cot::test] + async fn test_alter_field_operation_forwards_postgres() { + let mut database = + TestDatabase::new_postgres(stringify!(test_alter_field_operation_forwards)) + .await + .expect("failed to create PostgreSQL test database"); + run_alter_field_forwards(&mut database).await; + database + .cleanup() + .await + .expect("failed to clean up PostgreSQL test database"); + } + + #[ignore = "Tests that use MySQL are ignored by default"] + #[cot::test] + async fn test_alter_field_operation_forwards_mysql() { + let mut database = TestDatabase::new_mysql(stringify!(test_alter_field_operation_forwards)) + .await + .expect("failed to create MySQL test database"); + run_alter_field_forwards(&mut database).await; + database + .cleanup() + .await + .expect("failed to clean up MySQL test database"); + } + + #[ignore = "Tests that use PostgreSQL are ignored by default"] + #[cot::test] + async fn test_alter_field_operation_backwards_postgres() { + let mut database = + TestDatabase::new_postgres(stringify!(test_alter_field_operation_backwards)) + .await + .expect("failed to create PostgreSQL test database"); + run_alter_field_backwards(&mut database).await; + database + .cleanup() + .await + .expect("failed to clean up PostgreSQL test database"); + } + + #[ignore = "Tests that use MySQL are ignored by default"] + #[cot::test] + async fn test_alter_field_operation_backwards_mysql() { + let mut database = + TestDatabase::new_mysql(stringify!(test_alter_field_operation_backwards)) + .await + .expect("failed to create MySQL test database"); + run_alter_field_backwards(&mut database).await; + database + .cleanup() + .await + .expect("failed to clean up MySQL test database"); + } + #[test] fn test_remove_field_builder_new() { let builder = RemoveFieldBuilder::new(); From 77a4d5d5eb59e763cb98724482ae7ec7dfdb14c6 Mon Sep 17 00:00:00 2001 From: Marek 'seqre' Grzelak Date: Thu, 21 May 2026 14:24:21 +0200 Subject: [PATCH 7/8] do not handle foreign key constraints yet --- cot/src/db/migrations.rs | 54 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/cot/src/db/migrations.rs b/cot/src/db/migrations.rs index c20cd7fa7..9f763c404 100644 --- a/cot/src/db/migrations.rs +++ b/cot/src/db/migrations.rs @@ -577,9 +577,15 @@ impl Operation { } OperationInner::AlterField { table_name, - old_field: _, + old_field, new_field, } => { + if old_field.foreign_key.is_some() || new_field.foreign_key.is_some() { + unimplemented!( + "AlterField with foreign key constraints is not yet supported; \ + use separate RemoveField / AddField operations instead" + ); + } let query = sea_query::Table::alter() .table(*table_name) .modify_column(new_field.as_column_def(database)) @@ -662,8 +668,14 @@ impl Operation { OperationInner::AlterField { table_name, old_field, - new_field: _, + new_field, } => { + if old_field.foreign_key.is_some() || new_field.foreign_key.is_some() { + unimplemented!( + "AlterField with foreign key constraints is not yet supported; \ + use separate RemoveField / AddField operations instead" + ); + } // To reverse an alteration, set the column back to the old definition let query = sea_query::Table::alter() .table(*table_name) @@ -2812,6 +2824,44 @@ mod tests { .expect("failed to clean up MySQL test database"); } + #[tokio::test] + #[should_panic(expected = "AlterField with foreign key constraints is not yet supported")] + async fn test_alter_field_new_fk_forwards_panics() { + let database = Database::new("sqlite::memory:").await.unwrap(); + let operation = Operation::alter_field() + .table_name(Identifier::new("testapp__test_model")) + .old_field(Field::new(Identifier::new("col"), ColumnType::Integer)) + .new_field( + Field::new(Identifier::new("col"), ColumnType::Integer).foreign_key( + Identifier::new("other_table"), + Identifier::new("id"), + ForeignKeyOnDeletePolicy::Cascade, + ForeignKeyOnUpdatePolicy::Cascade, + ), + ) + .build(); + operation.forwards(&database).await.unwrap(); + } + + #[tokio::test] + #[should_panic(expected = "AlterField with foreign key constraints is not yet supported")] + async fn test_alter_field_old_fk_backwards_panics() { + let database = Database::new("sqlite::memory:").await.unwrap(); + let operation = Operation::alter_field() + .table_name(Identifier::new("testapp__test_model")) + .old_field( + Field::new(Identifier::new("col"), ColumnType::Integer).foreign_key( + Identifier::new("other_table"), + Identifier::new("id"), + ForeignKeyOnDeletePolicy::Cascade, + ForeignKeyOnUpdatePolicy::Cascade, + ), + ) + .new_field(Field::new(Identifier::new("col"), ColumnType::Integer)) + .build(); + operation.backwards(&database).await.unwrap(); + } + #[test] fn test_remove_field_builder_new() { let builder = RemoveFieldBuilder::new(); From 447b9d1886225712a473c24bbc3e7b9f574ae1ee Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 21 May 2026 12:25:11 +0000 Subject: [PATCH 8/8] chore(pre-commit.ci): auto fixes from pre-commit hooks --- cot/src/db/migrations.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/cot/src/db/migrations.rs b/cot/src/db/migrations.rs index 9f763c404..dbaefe72c 100644 --- a/cot/src/db/migrations.rs +++ b/cot/src/db/migrations.rs @@ -2671,9 +2671,7 @@ mod tests { Identifier::new("name"), ::TYPE, )) - .new_field( - Field::new(Identifier::new("name"), ::TYPE).null(), - ) + .new_field(Field::new(Identifier::new("name"), ::TYPE).null()) .build(); if let OperationInner::AlterField { @@ -2722,9 +2720,7 @@ mod tests { Identifier::new("name"), ::TYPE, )) - .new_field( - Field::new(Identifier::new("name"), ::TYPE).null(), - ) + .new_field(Field::new(Identifier::new("name"), ::TYPE).null()) .build(); let result = alter_operation.forwards(&test_db.database()).await; @@ -2755,15 +2751,10 @@ mod tests { Identifier::new("name"), ::TYPE, )) - .new_field( - Field::new(Identifier::new("name"), ::TYPE).null(), - ) + .new_field(Field::new(Identifier::new("name"), ::TYPE).null()) .build(); - alter_operation - .forwards(&test_db.database()) - .await - .unwrap(); + alter_operation.forwards(&test_db.database()).await.unwrap(); let result = alter_operation.backwards(&test_db.database()).await; assert!(result.is_ok());