fix(nextly): decide what happens to a junction by its table, not by its field - #1814
Conversation
…ts field Round 4 of review found three cases the field-keyed rules got wrong, and two more share their cause: a table, not a field, is what the database holds. junctionLifecycle now compares the tables the old and new definitions name. A table no longer named is dropped; a newly named one is created; a name reused for another relation is dropped and created again - CREATE TABLE IF NOT EXISTS would keep the old tags_id column for a field storing authors_id, which the round-3 keep-by-name guard caused - with drops first so the name is free. A field kept by name whose table's name changed (its junctionTable edited) now carries the table and its links, as a field rename does; a field retargeted to another collection gets a new table instead of pointing at none. Referential actions are not part of the comparison: rebuilding for them would lose links. A junction may not move off a table another field still stores its links in (a definition saved before shared junctions were refused), nor onto a table that exists: JUNCTION_TABLE_IN_USE, by name. The ownership rule now asks the name each field resolves to, through the schema service's own resolver, from both Builder save paths - an author-named table spelling another field's generated name collided unseen.
…nd one reserved-name list Create and update each lower-cased the field names, dropped the same five reserved fields from a list of their own (update had two), and validated; with the junction-ownership check added to both, the two copies crossed the clone threshold. userFieldsOf does the four steps once, in the same order, over one RESERVED_FIELD_NAMES.
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
@nextlyhq/eslint-plugin
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b06c21dabc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
…e-decided-by-table
…name in the database Two more corners of the same rename, both from review. A definition saved before shared junctions were refused can name one table from two fields. Giving each of them a table of its own asks this save to rename one table twice: the first statement succeeds, the second meets a table that is no longer there, and the migration stops half-applied while the registry records both new names. Refused by name, before any statement. The generator can only see the tables this collection's own fields name, so a junction left behind by an older migration is invisible to it and a rename would meet that table at apply time - in production, where a migration can no longer be refused. It publishes the moves it would make; the save boundary, which can ask the database, refuses one whose destination already exists.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: beb8e980bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const newByName = new Map(newFields.map(f => [f.name, f])); | ||
| const carries = oldFields.filter(usesJunctionTable).flatMap(field => { | ||
| const next = newByName.get(field.name); | ||
| if (!next || !this.sameRelationTarget(field, next)) return []; |
There was a problem hiding this comment.
When a many-to-many field is changed to a non-relationship type while its now-inert options retain the same target/cardinality and its junctionTable is changed or cleared, sameRelationTarget still passes because it does not inspect type. The new field is absent from after, but this branch nevertheless carries the old junction to the newly resolved name, so the migration leaves an orphan table and its links instead of dropping it. Include usesJunctionTable(next) in this guard before treating the edit as a carry.
AGENTS.md reference: AGENTS.md:L302-L305
Useful? React with 👍 / 👎.
1 participant
What
The follow-up to #1796, which merged with three review findings open. All three were real, and they share one cause: the lifecycle was keyed on the FIELD, and what the database holds is a TABLE.
Ledger:
task:junction-lifecycle-decided-by-table, researchresearch:junction-table-lifecycle-prior-art.Threads this closes, all on #1796's head
914574bcf:3991752095(Codex, P2) — ownership was checked on the author-namedjunctionTableonly, so a field naming a table exactly what another field's GENERATED name is passed validation, and the DDL then created one table for two fields.3991752107(Codex, P2) — with a junction shared by two fields in a definition saved before sharing was refused, renaming one field renamed the shared table away from the surviving field: it pointed at a table that was gone, and the renamed field inherited every link.3991758326(CodeRabbit, Major) — a regression feat(nextly): a many-to-many field's junction table follows the field through its life #1796's round 3 introduced. Its keep-the-table-by-name guard also kept a table whose name a field of ANOTHER relation now uses, andCREATE TABLE IF NOT EXISTSleft the oldtags_idcolumn standing for a field that storesauthors_id.Two gaps older than #1796 fall out of the same repair: changing a field's
junctionTableemitted nothing (the field then pointed at a table that does not exist), and pointing a field at another collection emitted nothing (same).How
junctionLifecyclecompares the junction TABLES the old and new definitions name (junctionTables, keyed by resolved name, each with its shape and the fields naming it):DROP TABLE IF EXISTS;sameJunctionColumnscompares the link columns and their references) → dropped and created again. Drops run first, so the name is free before anything is created under it;junctionTableedited — carries the table and its links (junctionCarries), through the samerenameJunctionStatementsas before;onDeletechanged would lose its links. That gap isfinding:referential-action-edits-emit-no-ddl, unchanged here.Refusals, by name, before any statement is written (
refuseJunctionMoves,JUNCTION_TABLE_IN_USE): a junction may not move off a table another field still stores its links in, nor onto a table that already exists.Ownership (
validateJunctionOwnership,JUNCTION_TABLE_SHARED) now asks the name each field RESOLVES to, through the schema service's ownjunctionTableNameFor— the naming the DDL is written with — and both Builder save paths pass that resolver. Those two paths also shared a field prologue (lower-case the names, drop the reserved five, validate); adding the check to both crossed fallow's clone threshold, souserFieldsOfdoes it once over oneRESERVED_FIELD_NAMES.Verification
junction-table-lifecycle.test.ts(69 = 23 × 3 dialects), including: ajunctionTableedit carries the table; a retarget replaces it; a reused name is dropped THEN created, with the create carryingauthors_id; both refusals.junction-ownership-is-exclusive.test.ts(6): the generated-name collision, the named collision, two own tables, generated names, ajunctionTableon a many-to-one (inert), and both save paths refusing through the real resolver.junction-table-lifecycle.integration.test.ts(7, real SQLite, fixture built fromgenerateMigrationSQL): links survive ajunctionTableedit; a reused name is rebuilt —PRAGMA table_infoshowsauthors_id, notags_id, and no rows.userFieldsOffails both save-path tests.b06c21dab(branched frommainc1fdd9bb6, after refactor(nextly): decide access with one mechanism, not two #1659): nextly lint 0, check-types 0, unit 2013/2013 (dynamic-collections + schema + the bare-Error allowlist), integration 414 passed / 46 skipped (dynamic-collections + collections), docs-claims 0,pnpm exec fallow audit --changed-since origin/main --gate new-onlypass, 0 introduced.