fix(nextly): let read hooks narrow the query, and run them for count - #439
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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 please review this PR |
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
create-nextly-app
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: 4ff53f1ab3
ℹ️ 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".
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b81ebe0de
ℹ️ 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".
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dff5cd6d18
ℹ️ 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".
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 929d062531
ℹ️ 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".
| if (CollectionQueryService.readHooksRunning.getStore()) { | ||
| return params.where; |
There was a problem hiding this comment.
When a collection-specific beforeOperation or beforeRead hook on collection A uses the documented req.nextly.find() API to read collection B, this process-wide boolean is already set, so B returns the caller's raw filter without running B's hooks. Nested list reads previously ran those hooks, and B may rely on them for tenant or soft-delete scoping; A can therefore make decisions using rows B's hook would have withheld. Track the active collection or handler instead of suppressing every nested list read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and I am deliberately leaving this thread unresolved rather than patching it, because the two guard findings together say the design is wrong, not that a line is missing.
Both are real, and confirmed against the code:
- This one (nested reads of another collection): the guard is a process-wide boolean, so a hook on A reading B via
req.nextly.find()suppresses B's hooks. B may rely on them for tenant or soft-delete scoping, so A can then make decisions using rows B would have withheld. That is a worse failure than the recursion it was added to prevent — it is silent under-scoping rather than a loud stack overflow. - The
getEntryone (count → findByID):run(true, ...)does not bypass its callback when a store already exists, so the inverse direction still recurses.
Taken together: a boolean that means "some read's hooks are running" cannot express what is actually needed, which is "this handler is already running, for this collection". Each targeted patch to it has produced the next hole — this is the third round on the same guard, and I have half-applied it twice now (list-but-not-detail, then detail-but-not-the-inverse).
The right shape is a scope carrying the collection and the handler identity, so a nested read of a different collection runs its own hooks normally while re-entry of the same handler is refused. That is a genuine design change with its own tests, not something to bolt on at the end of a long session — and the surrounding fixes on this PR already lack discriminating tests, which is the debt I do not want to add to.
So: recorded as the top item for the next session, alongside the re-diagnosis these findings make necessary anyway. Leaving both threads open so they stay visible rather than marking them addressed when they are not.
If you would rather this PR not ship a guard that under-scopes nested reads, the cleaner option is to drop the reentrancy guard from #439 entirely and land it separately once the scoped version exists — the recursion it prevents is a pre-existing hazard that only became reachable because this PR made countEntries run hooks. Happy to do that instead; it is your call.
| const entryId = await CollectionQueryService.readHooksRunning.run( | ||
| true, | ||
| async () => { |
There was a problem hiding this comment.
When a hook newly started by countEntries() calls ctx.req.nextly.findByID(), getEntry() unconditionally starts another guarded context but still executes the same global or collection-specific hook, which can call findByID() again indefinitely. The fresh evidence beyond the earlier getEntry-to-count report is this inverse count-to-getEntry path: run(true, ...) does not bypass its callback when a store already exists. Check the current store and skip the detail read hooks for a hook-originated read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and I am deliberately leaving this thread unresolved rather than patching it, because the two guard findings together say the design is wrong, not that a line is missing.
Both are real, and confirmed against the code:
- This one (nested reads of another collection): the guard is a process-wide boolean, so a hook on A reading B via
req.nextly.find()suppresses B's hooks. B may rely on them for tenant or soft-delete scoping, so A can then make decisions using rows B would have withheld. That is a worse failure than the recursion it was added to prevent — it is silent under-scoping rather than a loud stack overflow. - The
getEntryone (count → findByID):run(true, ...)does not bypass its callback when a store already exists, so the inverse direction still recurses.
Taken together: a boolean that means "some read's hooks are running" cannot express what is actually needed, which is "this handler is already running, for this collection". Each targeted patch to it has produced the next hole — this is the third round on the same guard, and I have half-applied it twice now (list-but-not-detail, then detail-but-not-the-inverse).
The right shape is a scope carrying the collection and the handler identity, so a nested read of a different collection runs its own hooks normally while re-entry of the same handler is refused. That is a genuine design change with its own tests, not something to bolt on at the end of a long session — and the surrounding fixes on this PR already lack discriminating tests, which is the debt I do not want to add to.
So: recorded as the top item for the next session, alongside the re-diagnosis these findings make necessary anyway. Leaving both threads open so they stay visible rather than marking them addressed when they are not.
If you would rather this PR not ship a guard that under-scopes nested reads, the cleaner option is to drop the reentrancy guard from #439 entirely and land it separately once the scoped version exists — the recursion it prevents is a pre-existing hazard that only became reachable because this PR made countEntries run hooks. Happy to do that instead; it is your call.
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
What
The hooks that precede a where-filtered read now shape the query, and
countEntriesruns them.Closes gaps G and K of
tasks/094-collection-hook-lifecycle-spec.md.The audit that decided the scope
tasks/097-hooks-remaining-gaps-plan.mdrecords it in full. Two findings changed what this PR had to be.1. The read-surface enumeration is now complete. Spec §3.6 enumerated the mutation service only and said the read surface needed the same treatment before it could be trusted.
CollectionQueryServicehas exactly three public methods:listEntriesandgetEntryrun the full chain;countEntriesran nothing. So the coverage rule can finally be stated as a rule:Two omissions against an otherwise uniform convention — not a systemic hole.
2. Gap G is three defects, not one. The task file recorded only "the return is discarded". Tracing
:584-621showed the chain is disconnected from the caller's filter at every stage:beforeOperationwas handedargs: { where: {} }— an empty filter, neverparams.wherebeforeReadwas handedwhereFromHook ?? {}— so for any ordinary read it saw{}beforeRead's return was awaited and dropped; the query was built fromparams.whereA hook cannot narrow a filter it was never shown. Both
whereFromHookand thebeforeReadreturn were dead, while a third mechanism (theCollectionsListQueryfilter seam) was live.The precedence, stated
Each stage sees the previous stage's result. The seam stays last because it is the only one already live, so every app using it is unaffected.
Note the
elsebranch of that seam also had to change: it fell back toparams.where, which would have dropped the hooks' narrowing whenever no plugin filter was registered — the common case.Why G and K ship together
#348 established that a list and its count must narrow identically, or the total describes rows the list correctly withheld — a disclosure, not a rounding error. Fixing G alone would have created exactly that: narrowed rows beside an unnarrowed total.
countEntriesalready mirrorslistEntrieson access, scope, status and locale, with "parity with listEntries" comments throughout — and then omitted hooks. It even declares acontextparameter documented as "Arbitrary data passed to hooks via context", for hooks that never ran.The double-execution hazard
listEntriescallscountEntriesfor its total. Running the chain in both would fire every read hook twice per list request — an audit entry, a rate-limit tick, doubled. The nested call is toldreadHooksAlreadyRan: trueand uses the filter the list already settled on. Covered by a test asserting exactly one run.DRY, applied where the semantics are shared
resolveReadWhereis extracted forlistEntriesandcountEntries, whose chains must stay identical.getEntryis deliberately not folded in: its chain isid-shaped rather thanwhere-shaped, itsbeforeOperation.idseam is already live, and it shares only boilerplate. Forcing all three through one helper would abstract over a difference that matters.Verification
resolveReadWhereand would have passed even if the service diverged. That is the vacuous-test pattern this program keeps finding, and shipping it would have been a worse outcome than not testing.7b0dddf1overhooks+domains/collections: identical, 13 pre-existing, zero new.tsc0 errors.Not in this PR
Gaps I and E were scoped into the same read-path batch and are not here — this became large enough on its own once G turned out to be three defects. They follow next, in one PR. Gaps A/F (write path) are sequenced after the mutation service quietens down; #428/#429/#434 are all sitting on it.