diff --git a/cot-cli/src/migration_generator.rs b/cot-cli/src/migration_generator.rs index 7dc40dcf..acbbce09 100644 --- a/cot-cli/src/migration_generator.rs +++ b/cot-cli/src/migration_generator.rs @@ -716,9 +716,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 { @@ -728,20 +728,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] @@ -1009,24 +1004,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 {}", @@ -1063,6 +1056,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") + } } } @@ -1161,6 +1159,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() @@ -1317,6 +1327,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. @@ -1371,6 +1387,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, .. } => { @@ -2053,4 +2085,241 @@ 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}" + ); + } + + #[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" + ); + } } diff --git a/cot/src/db/migrations.rs b/cot/src/db/migrations.rs index 67659494..68fc7e12 100644 --- a/cot/src/db/migrations.rs +++ b/cot/src/db/migrations.rs @@ -494,6 +494,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: _, @@ -560,6 +571,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 { @@ -606,6 +629,11 @@ enum OperationInner { table_name: Identifier, fields: &'static [Field], }, + AlterField { + table_name: Identifier, + old_field: Field, + new_field: Field, + }, } /// A field in a model. @@ -1536,6 +1564,58 @@ 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 altering a field in a model. +#[must_use] +#[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, + } + } + + /// 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), + }) + } +} + /// A trait for defining a migration. /// /// # Cot CLI Usage