fix(nextly): migrate:fresh drops the tables it actually found - #1736
Conversation
|
Warning Review limit reachedNext included review available in 37 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 selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds search-path-aware PostgreSQL table discovery, keeps MySQL discovery database-scoped, and narrows migration helper functions to the ChangesMigration SQL contracts
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change makes the fresh-migration reset discover exactly the tables that an unqualified drop would resolve through the database search path, closing a gap where tables outside the default schema could survive a reset. No unresolved correctness or data-loss concerns remain, and the accompanying tests cover both PostgreSQL and MySQL scoping. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 73145e7672
ℹ️ 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".
| SELECT tablename | ||
| FROM pg_tables | ||
| WHERE schemaname = 'public' | ||
| WHERE schemaname = current_schema() |
There was a problem hiding this comment.
When search_path is tenant, public and a Nextly table exists only in public—for example, after prepending a tenant schema to an established database—an unqualified DROP TABLE "name" resolves to that fallback table, but this predicate excludes it because current_schema() returns only tenant. Consequently migrate:fresh leaves reachable tables and their data behind, potentially creating shadowing replacements during migration; use relation visibility/search-path resolution rather than assuming the first schema is the only drop target. The added test merely checks for the current_schema() token and therefore does not exercise this separating case.
AGENTS.md reference: AGENTS.md:L206-L213
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and the counter-example is the one that breaks my reasoning: the
DROP walks the whole path, so current_schema() — only its first entry — is
NARROWER than the drop. A Nextly table sitting in public under a
tenant, public path is reachable by the drop and was invisible to my predicate.
I had argued "dropping less is recoverable, dropping more is not", which is true
in general and beside the point here: the safe set is not smaller-than-the-drop,
it is EQUAL to the drop. Anything smaller leaves tables the reset was supposed to
remove.
Discovery now asks visibility directly, which is that question and nothing else:
SELECT c.relname AS tablename
FROM pg_class c
JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE c.relkind IN ('r', 'p')
AND n.nspname NOT IN ('pg_catalog', 'information_schema')
AND pg_table_is_visible(c.oid)pg_table_is_visible is true for the one relation of a given name the path
resolves to and false for the ones it shadows — exactly what the unqualified DROP
will reach. System schemas are excluded by name because pg_catalog is on every
path implicitly, so visibility alone would hand it to a DROP.
Break-verified three ways, each failing the same test alone: back to 'public',
back to current_schema(), and visibility without the system-schema exclusion.
The middle one is the version you rejected, so the test now discriminates against
it too.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| await discoverTables(adapter as any, "postgresql"); |
There was a problem hiding this comment.
This new call, and the repeated MySQL call below it, introduce both as any and an eslint-disable solely to make the fake adapter fit DrizzleAdapter. Define the tested dependency as the required executeQuery surface or provide a genuinely typed test double instead; repository conventions explicitly reject both suppression mechanisms.
AGENTS.md reference: AGENTS.md:L280-L281
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed, and removing the cast proved your point in a way I did not expect.
I narrowed the three helpers to the surface they actually use — executeQuery
and nothing else — and exported it as SqlRunner, so the double is typed against
the real thing:
export type SqlRunner = Pick<DrizzleAdapter, "executeQuery">;With the casts gone the file stopped compiling, because the hand-written fake did
NOT match: it declared executeQuery: (sql: string) => Promise<unknown> while the
real one is generic and takes params. The cast had been hiding a double that was
already wrong. The fake is now type FakeAdapter = SqlRunner, so a change to that
surface breaks it instead of slipping past.
as any and eslint-disable are both now at zero in the file — including the
three pre-existing uses in the FK tests, which the same narrowing made
unnecessary.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5efd96dce7
ℹ️ 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".
| query = ` | ||
| SELECT tablename | ||
| FROM pg_tables | ||
| WHERE schemaname = 'public' | ||
| ORDER BY tablename | ||
| SELECT c.relname AS tablename | ||
| FROM pg_class c | ||
| JOIN pg_namespace n ON n.oid = c.relnamespace | ||
| WHERE c.relkind IN ('r', 'p') | ||
| AND n.nspname NOT IN ('pg_catalog', 'information_schema') | ||
| AND pg_table_is_visible(c.oid) |
There was a problem hiding this comment.
This PostgreSQL branch introduces a catalog query as a raw string and executes it through executeQuery, but database access in packages/nextly product code is required to use Drizzle rather than raw SQL strings. Build the query with Drizzle's sql API and execute it through the adapter's Drizzle instance.
AGENTS.md reference: AGENTS.md:L274-L276
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Both fixed, and they turned out to be one change: composing the query in Drizzle
is what made the shared predicate reachable.
Drizzle. discoverTables now takes Pick<DrizzleAdapter, "queryStatement">
and composes all three dialect branches as sql fragments —
adapter.queryStatement being the Drizzle-native path document-lock-repository
already uses. No raw string reaches the driver from this function.
The canonical predicate. The PostgreSQL branch now interpolates
PG_CLASS_IS_THE_RELATION_THE_WRITES_HIT rather than my own
pg_table_is_visible(c.oid). You were right that the two are one question with
two spellings; the one I wrote could be corrected apart from the schema reads,
and then a destructive command and the introspection would disagree about which
table they mean. pg_class is aliased t so the shared fragment drops in
unchanged.
The test now renders the statement through PgDialect().sqlToQuery() and
asserts on the text the server would receive, rather than inspecting the fragment
object — a fragment built from the right pieces in the wrong order would satisfy
any assertion made against the pieces.
Break-verified three ways, each failing that test alone:
| mutation | why it matters |
|---|---|
back to a 'public' predicate |
the shipped defect |
quote_ident removed from the SHARED predicate |
proves the test now catches drift at the source, which is the whole reason to reuse it |
| system schemas no longer excluded | pg_catalog is on every path implicitly, so visibility alone would hand it to a DROP |
The middle row is the one I care about: before this change, breaking the shared
predicate could not fail this command's test, because this command had its own.
Gates: nextly check-types 0 · lint 0 · vitest 0 (11,892 tests) ·
fallow audit pass, every *_introduced 0 · changeset status 0.
| JOIN pg_namespace n ON n.oid = c.relnamespace | ||
| WHERE c.relkind IN ('r', 'p') | ||
| AND n.nspname NOT IN ('pg_catalog', 'information_schema') | ||
| AND pg_table_is_visible(c.oid) |
There was a problem hiding this comment.
This independently reimplements the same relation-resolution question already centralized as PG_CLASS_IS_THE_RELATION_THE_WRITES_HIT in domains/schema/pipeline/pg-visible-relation.ts and used by schema introspection. A later correction to either implementation can make this destructive command discover a different relation from the schema write paths, so derive this condition from the existing canonical predicate instead of maintaining pg_table_is_visible in parallel.
AGENTS.md reference: AGENTS.md:L288-L291
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd971ab94e
ℹ️ 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".
| expect(seen[0]).toContain("to_regclass"); | ||
| expect(seen[0]).toContain("quote_ident"); |
There was a problem hiding this comment.
Fresh evidence after the earlier thread: the revised test still only checks that the rendered SQL contains to_regclass and quote_ident; it remains green if the predicate is inverted with <> or combined using OR, either of which can enumerate and drop the wrong relations. Because this is the only regression test for the destructive search-path behavior, use a PostgreSQL fixture with tables in tenant and public and assert the actual rows returned so the test distinguishes correct resolution from these plausible broken implementations.
AGENTS.md reference: AGENTS.md:L218-L223
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and it is the rule I quoted at someone else this morning: a
necessary-but-insufficient property returns green from the broken implementation
too, carrying the authority of having been checked.
toContain("to_regclass") survives <> and it survives OR. On a command that
DROPS what it enumerates, that is not a regression test.
#1745 adds the database-backed one, with a fixture built so each plausible wrong
answer shows up as a different failing row:
| table | where | expected | which broken version it catches |
|---|---|---|---|
shadowed |
tenant and public | returned once | a query returning every copy rather than the resolved one |
public_only |
public, path is tenant, public |
returned | current_schema() — the version I wrote first |
hidden_only |
schema off the path | not returned | an inverted or OR-ed predicate |
pg_class |
pg_catalog | not returned | visibility without the system-schema exclusion |
Session-pinned to one pg.Client for the reason you raised on the sibling probe,
with a SHOW search_path case first so a reset session fails as a fixture
problem rather than as a defect in the predicate.
Still unexecuted here — no PostgreSQL on this machine — so the CI Postgres leg is
the verdict. The sibling probe uses this same harness and has since passed on all
three dialects.
1 participant
The destructive site from
finding:pg-schema-pinned-in-seven-places, now that#1729 has released the file.
The bug
migrate:freshempties the database and rebuilds it. Two steps:SELECT tablename FROM pg_tables WHERE schemaname = 'public'DROP TABLE IF EXISTS "name" CASCADE, with no schema on itStep 2 resolves through the
search_path. Step 1 named one schema. ReadingdropTableis what showed this is worse than the finding recorded — the twosteps disagree in both directions at once.
On a
search_pathoftenant, public:tenant. Step 1 cannot see them, so they survivethe reset —
migrate:freshsilently does not do its job.publicis listed, and handed to step 2.Why
current_schema()and notto_regclass#1721 fixed a different question. Those reads asked "does this NAME resolve?",
which
to_regclassanswers by walking the whole path. This one asks "whichtables are MINE to destroy?", and the answer is where an unqualified
CREATElands —
current_schema().That is also exactly where the unqualified
DROPwill go, so discovery anddestruction now ask the same question. Same principle as #1721, different tool,
because it is a different question.
Deliberately not the whole search path. Dropping less than intended leaves a
table behind; dropping more destroys data this command was never pointed at, and
only one of those is recoverable.
No shared predicate, no new export
#1732 was closed on a correct Codex P1 for exporting a raw-SQL API to reach
callers like this one. This needs neither: it is a single query with a shape of
its own, written in place.
Evidence
discoverTablesis now exported for the same reasondisableForeignKeyChecksalready is — what it asks the database decides what this command destroys, and
that deserves a test that does not have to drive the whole command to reach it.
Three assertions, each break-verified to fail alone:
schemaname = 'public'(the shipped defect)DATABASE()That last one is the control: "does not say
public" is also satisfied by aquery scoping to nothing, which on this command would enumerate every table
the role can see.
Gates
nextly
check-types0 ·lint0 ·vitest0 (11,892 tests) ·fallow auditpass, every
*_introduced0 over 3 files ·changeset status0.Summary by CodeRabbit