belongs_to detach: null the FK if nullable, error if not (#3127)

* belongs_to detach: null the FK if nullable, clean error if not

`delete_<field>()` on a belongs_to (`ActiveHasOne::Delete`) was silently ignored on
save — `take()` mapped `Delete` to `None`, so the branch did nothing and the foreign
key was never touched.

Handle it in the belongs_to save action: `clear_parent_key::<Related>()` nulls this
row's FK when the column is nullable, and returns `false` when it is not — in which
case we return a clean `DbErr::Type` ("relation cannot be detached") instead of
attempting an UPDATE that a raw NOT NULL constraint would reject. Composite FKs are
handled by the relation def. Also make `clear_key_on_active_model` a no-op when the
FK column was never set (a freshly-built ActiveModel) rather than erroring, so
`delete_<field>()` on a fresh model just inserts NULL.

Keeps the single `ActiveHasOne` type — nullability drives behavior at runtime rather
than being encoded in the type.

Adds tests for nullable detach, non-nullable clean error, and detach-on-unset no-op.

* Use if-let over single-arm match in clear_key_on_active_model (clippy)

* Support nested writes for duplicate-target belongs_to

When two belongs_to fields share a target entity (e.g. `user_follower.user` +
`user_follower.follower`, both -> `user`), there is no unique `Related<E>` to key
the FK write by, so the save action's `entity_count == 1` guard dropped them
entirely — nested writes silently no-op'd.

Route such relations through the relation-keyed helpers instead: capture their
relation variant at detection time (the explicit `relation_enum`, else the default
inferred the same way the loader/model_ex does) and use `set_parent_key_for` /
`clear_parent_key_for(Relation::X)`. Unique-target belongs_to keep the entity-keyed
path unchanged. Adds `ActiveModelTrait::clear_parent_key_for` (the relation-keyed
counterpart to `clear_parent_key`, mirroring `set_parent_key_for`).

Adds `test_belongs_to_duplicate_target`.

Author: tyt2y3

sea-orm-macros/src/derives/active_model_ex.rs

@@ -1,7 +1,9 @@
 use super::active_model::DeriveActiveModel;
 use super::attributes::compound_attr;
+use super::model_ex::infer_relation_name_from_entity;
 use super::util::{extract_compound_entity, field_not_ignored_compound, is_compound_field};
-use proc_macro2::{Ident, TokenStream};
+use heck::ToUpperCamelCase;
+use proc_macro2::{Ident, Span, TokenStream};
 use quote::{format_ident, quote};
 use std::collections::HashMap;
 use syn::{Attribute, Data, Expr, Fields, LitStr, Type, Visibility};
@@ -98,11 +100,29 @@ pub fn expand_derive_active_model_ex(
                                     {
                                         has_many_self_fields
                                             .push((ident.clone(), relation_enum.clone()));
+                                    } else if compound_attrs.belongs_to.is_some() {
+                                        // A belongs_to sharing its target entity with another
+                                        // relation, disambiguated by an explicit relation_enum:
+                                        // key the FK write by that relation variant.
+                                        let variant = Ident::new(
+                                            &relation_enum.value().to_upper_camel_case(),
+                                            relation_enum.span(),
+                                        );
+                                        belongs_to_fields.push((
+                                            ident.clone(),
+                                            entity_path.to_owned(),
+                                            Some(variant),
+                                        ));
                                     }
                                 } else if *entity_count.get(entity_path).unwrap() == 1 {
                                     // can only Related to another Entity once
                                     if compound_attrs.belongs_to.is_some() {
-                                        belongs_to_fields.push(ident.clone());
+                                        // Unique target: key the FK write by entity.
+                                        belongs_to_fields.push((
+                                            ident.clone(),
+                                            entity_path.to_owned(),
+                                            None,
+                                        ));
                                     } else if compound_attrs.has_one.is_some() {
                                         has_one_fields.push(ident.clone());
                                     } else if compound_attrs.has_many.is_some()
@@ -117,6 +137,20 @@ pub fn expand_derive_active_model_ex(
                                             compound_attrs.via.as_ref().unwrap().value(),
                                         ));
                                     }
+                                } else if compound_attrs.belongs_to.is_some() {
+                                    // A belongs_to sharing its target entity with another
+                                    // relation but without an explicit relation_enum: key the
+                                    // FK write by the inferred (default) relation variant.
+                                    let variant = Ident::new(
+                                        &infer_relation_name_from_entity(entity_path)
+                                            .to_upper_camel_case(),
+                                        Span::call_site(),
+                                    );
+                                    belongs_to_fields.push((
+                                        ident.clone(),
+                                        entity_path.to_owned(),
+                                        Some(variant),
+                                    ));
                                 }
                                 if compound_attrs.self_ref.is_some()
                                     && compound_attrs.via.is_some()
@@ -326,7 +360,7 @@ pub fn expand_derive_active_model_ex(
 }
 
 fn expand_active_model_action(
-    belongs_to: &[Ident],
+    belongs_to: &[(Ident, String, Option<Ident>)],
     belongs_to_self: &[(Ident, LitStr)],
     has_one: &[Ident],
     has_many: &[Ident],
@@ -353,12 +387,35 @@ fn expand_active_model_action(
         quote!()
     };
 
-    for field in belongs_to {
+    for (field, related_entity, relation_variant) in belongs_to {
+        let related_entity: TokenStream = related_entity.parse().unwrap();
+        // Disambiguate the FK write. A relation sharing its target entity with another
+        // is keyed by its relation variant (there is no unique `Related<E>` to key by);
+        // a unique target is keyed by entity.
+        let (set_parent_key, clear_parent_key) = match relation_variant {
+            Some(variant) => (
+                quote!(self.set_parent_key_for(&model, Relation::#variant)?),
+                quote!(self.clear_parent_key_for(Relation::#variant)?),
+            ),
+            None => (
+                quote!(self.set_parent_key(&model)?),
+                quote!(self.clear_parent_key::<#related_entity>()?),
+            ),
+        };
         belongs_to_action.extend(quote! {
+            // Detach: `Delete` on a belongs_to nulls this row's own foreign key.
+            // If the FK is not nullable it cannot be detached — return a clean error
+            // instead of a raw database constraint violation.
+            if self.#field.is_delete() && !#clear_parent_key {
+                return Err(sea_orm::DbErr::Type(format!(
+                    "Relation `{}` cannot be detached: its foreign key is not nullable",
+                    stringify!(#field)
+                )));
+            }
             let #field = if let Some(model) = self.#field.take() {
                 if model.is_update() {
                     // has primary key
-                    self.set_parent_key(&model)?;
+                    #set_parent_key;
                     if model.is_changed() {
                         let model = #box_pin(model.action(action, db))#await_?;
                         Some(model)
@@ -368,7 +425,7 @@ fn expand_active_model_action(
                 } else {
                     // new model
                     let model = #box_pin(model.action(action, db))#await_?;
-                    self.set_parent_key(&model)?;
+                    #set_parent_key;
                     Some(model)
                 }
             } else {

sea-orm-macros/src/derives/model_ex.rs

@@ -730,7 +730,7 @@ fn get_related<'a>(attr: &compound_attr::SeaOrm, ty: &'a str) -> (&'a str, Ident
     (related_entity, relation_enum)
 }
 
-fn infer_relation_name_from_entity(s: &str) -> &str {
+pub(crate) fn infer_relation_name_from_entity(s: &str) -> &str {
     let s = s.trim_end_matches("::Entity");
     if let Some((_, suffix)) = s.rsplit_once("::") {
         return suffix;

sea-orm-sync/src/entity/active_model.rs

@@ -782,6 +782,22 @@ pub trait ActiveModelTrait: Clone + Debug {
         clear_key_on_active_model(&rel_def.from_col, self)
     }
 
+    #[doc(hidden)]
+    /// Clear a specific belongs_to relation, keyed by relation (to disambiguate
+    /// multiple relations to the same entity), if it is optional; return true.
+    fn clear_parent_key_for(
+        &mut self,
+        rel: <Self::Entity as EntityTrait>::Relation,
+    ) -> Result<bool, DbErr> {
+        let rel_def = rel.def();
+
+        if rel_def.is_owner {
+            return Err(DbErr::Type(format!("Relation {rel:?} is not belongs_to")));
+        }
+
+        clear_key_on_active_model(&rel_def.from_col, self)
+    }
+
     #[doc(hidden)]
     fn clear_parent_key_for_self_rev(
         &mut self,

sea-orm-sync/src/query/util.rs

@@ -126,19 +126,13 @@ where
         if !column.def().is_null() {
             return Ok(false);
         }
-        model.set(
-            column,
-            match model.get(column).into_value() {
-                Some(value) => value.as_null(),
-                None => {
-                    return Err(DbErr::AttrNotSet(format!(
-                        "{}.{}",
-                        <ActiveModel::Entity as Default>::default().as_str(),
-                        col_name
-                    )));
-                }
-            },
-        );
+        // Null out the key column so the detach is persisted. If it was never set
+        // (a freshly-built ActiveModel) there is nothing to clear — it will insert as
+        // NULL / stay absent on update — so treat it as already cleared rather than
+        // erroring.
+        if let Some(value) = model.get(column).into_value() {
+            model.set(column, value.as_null());
+        }
     }
 
     Ok(true)

sea-orm-sync/tests/active_model_ex_tests.rs

@@ -775,3 +775,152 @@ fn test_has_one_replace_and_delete() -> Result<(), DbErr> {
 
     Ok(())
 }
+
+#[sea_orm_macros::test]
+fn test_belongs_to_nullable_detach() -> Result<(), DbErr> {
+    use common::bakery_dense::{bakery, cake};
+
+    let ctx = TestContext::new("test_belongs_to_nullable_detach");
+    let db = &ctx.db;
+
+    db.get_schema_builder()
+        .register(bakery::Entity)
+        .register(cake::Entity)
+        .apply(db)?;
+
+    info!("attach a cake to a bakery through the nullable belongs_to");
+    let cake = cake::ActiveModel::builder()
+        .set_name("Cheesecake")
+        .set_price(Decimal::from(10))
+        .set_gluten_free(false)
+        .set_serial(Uuid::nil())
+        .set_bakery(
+            bakery::ActiveModel::builder()
+                .set_name("SeaSide")
+                .set_profit_margin(10.0),
+        )
+        .save(db)?;
+    assert!(cake::Entity::find().one(db)?.unwrap().bakery_id.is_some());
+
+    info!("delete_<field> on a nullable belongs_to nulls the FK and keeps both rows");
+    cake.delete_bakery().save(db)?;
+
+    let reloaded = cake::Entity::find().one(db)?.unwrap();
+    assert!(reloaded.bakery_id.is_none());
+    assert_eq!(bakery::Entity::find().all(db)?.len(), 1);
+
+    ctx.delete();
+
+    Ok(())
+}
+
+#[sea_orm_macros::test]
+fn test_belongs_to_non_null_detach_errors() -> Result<(), DbErr> {
+    use common::blogger::*;
+
+    let ctx = TestContext::new("test_belongs_to_non_null_detach_errors");
+    let db = &ctx.db;
+
+    db.get_schema_builder()
+        .register(user::Entity)
+        .register(post::Entity)
+        .apply(db)?;
+
+    let user = user::ActiveModel::builder()
+        .set_name("Alice")
+        .set_email("alice@sea-ql.org")
+        .save(db)?;
+    let post = post::ActiveModel::builder()
+        .set_title("post 1")
+        .set_author(user)
+        .save(db)?;
+
+    info!("detaching a non-nullable belongs_to (post.author) returns a clean error");
+    let err = post.delete_author().save(db).unwrap_err();
+    assert!(
+        matches!(err, DbErr::Type(_)),
+        "expected a clean DbErr::Type, got {err:?}"
+    );
+
+    // The row is untouched — nothing was deleted or nulled.
+    assert!(post::Entity::find().one(db)?.is_some());
+
+    ctx.delete();
+
+    Ok(())
+}
+
+#[sea_orm_macros::test]
+fn test_detach_unset_belongs_to_is_noop() -> Result<(), DbErr> {
+    use common::bakery_dense::{bakery, cake};
+
+    let ctx = TestContext::new("test_detach_unset_belongs_to_is_noop");
+    let db = &ctx.db;
+
+    db.get_schema_builder()
+        .register(bakery::Entity)
+        .register(cake::Entity)
+        .apply(db)?;
+
+    info!("delete_<field> on a never-set nullable belongs_to is a no-op, not an error");
+    cake::ActiveModel::builder()
+        .set_name("Plain")
+        .set_price(Decimal::from(5))
+        .set_gluten_free(true)
+        .set_serial(Uuid::nil())
+        .delete_bakery()
+        .save(db)?;
+
+    let reloaded = cake::Entity::find().one(db)?.unwrap();
+    assert!(reloaded.bakery_id.is_none());
+
+    ctx.delete();
+
+    Ok(())
+}
+
+#[sea_orm_macros::test]
+fn test_belongs_to_duplicate_target() -> Result<(), DbErr> {
+    use common::blogger::*;
+
+    let ctx = TestContext::new("test_belongs_to_duplicate_target");
+    let db = &ctx.db;
+
+    db.get_schema_builder()
+        .register(user::Entity)
+        .register(user_follower::Entity)
+        .apply(db)?;
+
+    // `user_follower` has two belongs_to fields — `user` and `follower` — both
+    // targeting `user::Entity`. Nested writes on such duplicate-target relations
+    // used to be silently dropped; each now writes its own FK, disambiguated by
+    // relation (`follower` via its `relation_enum`, `user` via the default).
+    let alice = user::ActiveModel::builder()
+        .set_name("Alice")
+        .set_email("alice@sea-ql.org")
+        .save(db)?;
+    let bob = user::ActiveModel::builder()
+        .set_name("Bob")
+        .set_email("bob@sea-ql.org")
+        .save(db)?;
+
+    info!("link the two users through the disambiguated nested belongs_to");
+    let follow = user_follower::ActiveModelEx {
+        user: ActiveHasOne::set(alice),
+        follower: ActiveHasOne::set(bob),
+        ..Default::default()
+    }
+    .insert(db)?;
+
+    // Each belongs_to wrote its own FK (previously a silent no-op).
+    assert_eq!(follow.user_id, 1);
+    assert_eq!(follow.follower_id, 2);
+
+    let row = user_follower::Entity::find().one(db)?.expect("row");
+    assert_eq!(row.user_id, 1);
+    assert_eq!(row.follower_id, 2);
+
+    ctx.delete();
+
+    Ok(())
+}

src/entity/active_model.rs

@@ -790,6 +790,22 @@ pub trait ActiveModelTrait: Clone + Debug {
         clear_key_on_active_model(&rel_def.from_col, self)
     }
 
+    #[doc(hidden)]
+    /// Clear a specific belongs_to relation, keyed by relation (to disambiguate
+    /// multiple relations to the same entity), if it is optional; return true.
+    fn clear_parent_key_for(
+        &mut self,
+        rel: <Self::Entity as EntityTrait>::Relation,
+    ) -> Result<bool, DbErr> {
+        let rel_def = rel.def();
+
+        if rel_def.is_owner {
+            return Err(DbErr::Type(format!("Relation {rel:?} is not belongs_to")));
+        }
+
+        clear_key_on_active_model(&rel_def.from_col, self)
+    }
+
     #[doc(hidden)]
     fn clear_parent_key_for_self_rev(
         &mut self,

src/query/util.rs

@@ -126,19 +126,13 @@ where
         if !column.def().is_null() {
             return Ok(false);
         }
-        model.set(
-            column,
-            match model.get(column).into_value() {
-                Some(value) => value.as_null(),
-                None => {
-                    return Err(DbErr::AttrNotSet(format!(
-                        "{}.{}",
-                        <ActiveModel::Entity as Default>::default().as_str(),
-                        col_name
-                    )));
-                }
-            },
-        );
+        // Null out the key column so the detach is persisted. If it was never set
+        // (a freshly-built ActiveModel) there is nothing to clear — it will insert as
+        // NULL / stay absent on update — so treat it as already cleared rather than
+        // erroring.
+        if let Some(value) = model.get(column).into_value() {
+            model.set(column, value.as_null());
+        }
     }
 
     Ok(true)