Avoid adding then dropping FK constraints#12528
Conversation
greg0ire
force-pushed
the
no-add-drop
branch
3 times, most recently
from
a8a6b1d to
59fb1d6
Compare
| }, | ||
| )->create(); | ||
| // @phpstan-ignore class.notFound (Using unreleased Schema::edit() API) | ||
| } catch (InvalidSchemaModification) { |
There was a problem hiding this comment.
@morozov I think this might be one place where the "you know what you added" (or in this case, removed) breaks down: here the user might be the one that removed the table, thanks to the postGenerateSchemaTable event above.
There was a problem hiding this comment.
I think if it breaks down here, that points to an ORM flaw rather than a gap in the editor API design. We could add the needed methods to the editor, but in this specific case the ORM could call $schema->hasTable() and check whether the table is still there.
More generally, this feels like a smell similar to the one being fixed: the ORM defines a schema with a table, the user drops it, and then the ORM has to recalibrate and figure out what the user did. Maybe the FK constraints should be added to the effective/final schema, rather than to the draft before the user's edits?
One more note on catching InvalidSchemaModification: it's a logic exception — essentially "you're doing something wrong" — so it isn't meant to be caught and handled.
There was a problem hiding this comment.
Not sure how I missed that method 😓 … thanks!
As for the flaw, the event I mention is fired after each table is added:
Lines 479 to 483 in 883c76a
So maybe I'm breaking it right now by deferring the addition of the foreign key like I did 🙈
greg0ire
requested review from
SenseException,
derrabus and
morozov
and removed request for
SenseException,
derrabus and
morozov
greg0ire
force-pushed
the
no-add-drop
branch
3 times, most recently
from
b02937c to
2730faf
Compare
Previously, when processing entity relationships, SchemaTool would: 1. Add FK constraints as they were encountered 2. If a conflict was detected (same columns, different target), drop the previously added FK 3. Blacklist the composite key to prevent adding any FK for those columns This add-then-drop pattern is not great, and will make things even worse when migrating to the TableEditor API, because it will require known the exact auto-generated FK name, which in turn would force copying the DBAL algorithm for generating FK names. Solution: Implement a two-pass approach: 1. Collection Phase: Gather all FK metadata without adding to tables - Detect conflicts between FKs (same local columns, different targets) - Mark conflicting composite keys as blacklisted - Store FK metadata for non-conflicting cases 2. Application Phase: Add only non-conflicting FKs to their tables - Skip any composite keys that were blacklisted due to conflicts
| self::equalTo('ALTER TABLE cms_users ADD CONSTRAINT FK_3AF03EC5A832C1C9 FOREIGN KEY (email_id) REFERENCES cms_emails (id)'), | ||
| // DBAL 4.5 | ||
| self::equalTo('ALTER TABLE cms_users ADD FOREIGN KEY (email_id) REFERENCES cms_emails (id)'), | ||
| )); |
There was a problem hiding this comment.
The DBAL will generate the name in this case.
@morozov it looks like the name is not generated at all now: there is no name at all. Am I doing something wrong?
2 participants
Previously, when processing entity relationships, SchemaTool would:
This add-then-drop pattern is not great, and will make things even worse when migrating to the TableEditor API, because it will require known the exact auto-generated FK name, which in turn would force copying the DBAL algorithm for generating FK names.
Solution:
Implement a two-pass approach: