belongs_to detach: null the FK if nullable, clean error if not#3127
Merged
Conversation
`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.
Member
|
I think a runtime error is much worse than an operation that is impossible to express. A foreign key is just a nullable ID column. |
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`.
Member
Author
|
I think we can have so I guess we can continue to experiment on #3118 with something like: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detaching a
belongs_torelation via the generateddelete_<field>()builder (ActiveHasOne::Delete) was silently ignored on save:take()mapsDeletetoNone, so the belongs_to action did nothing and the FK was never touched. Socake.delete_bakery().save(db)was a no-op.This is the deliberately-minimal fix, keeping the single
ActiveHasOne<E>type — nullability drives behavior at runtime, rather than being pushed into the type parameter (which is the direction #3118 explored; closing that in favor of this).New Behavior
delete_<field>()on abelongs_tonow:DbErr::Type("relation cannot be detached") instead of attempting an UPDATE that the database's NOT NULL constraint would reject with a raw driver error.AttrNotSet.has_onedetach (delete/orphan the child) is unchanged.