From ae1a31bdcbf7881372f604f9b3e086c5b9351317 Mon Sep 17 00:00:00 2001 From: Kumar Ujjawal Date: Fri, 4 Jul 2025 23:20:30 +0530 Subject: [PATCH 1/5] 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 4fc8a6a3..bf04015f 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 51dc1976..2b17fd86 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 d269a876a93e8ce9cee9489d15375d1fa1cc6d6d Mon Sep 17 00:00:00 2001 From: Kumar Ujjawal Date: Mon, 28 Jul 2025 23:07:12 +0530 Subject: [PATCH 2/5] 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 bf04015f..c224521e 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 ef8841a69376c4c4b3ad435367ee5303e28e9fc8 Mon Sep 17 00:00:00 2001 From: Kumar Ujjawal Date: Fri, 29 Aug 2025 00:17:20 +0530 Subject: [PATCH 3/5] 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 2b17fd86..b33be324 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 a8fc093f16b7a5271503e5fc1902fc6ebb4eb827 Mon Sep 17 00:00:00 2001 From: Kumar Ujjawal Date: Mon, 16 Feb 2026 13:14:29 +0530 Subject: [PATCH 4/5] 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 c224521e..8d4028d5 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 dac062f6c1d7663566c9ca9c1169788dc585c708 Mon Sep 17 00:00:00 2001 From: Marek 'seqre' Grzelak Date: Mon, 16 Feb 2026 19:16:08 +0100 Subject: [PATCH 5/5] 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 b33be324..88ea9738 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)