feat(query): full v5 migration#3040
Open
3scava1i3r wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR migrates the query package to PostGraphile v5, replacing Apollo-based server and plugin wiring with grafast/grafserv, updating historical filtering, ordering, distinct, search, metadata, and subscription behavior, and rewriting the test harness to run against v5 schemas and middleware. ChangesPostGraphile v5 migration
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
packages/query/src/graphql/plugins/QueryDepthLimitPlugin.ts-20-23 (1)
20-23: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winDo not bypass depth checks by operation name alone.
Any client can name a deep operation
IntrospectionQueryand skip this limiter. Only skip verified introspection-only operations, or remove the special case.Also applies to: 44-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/QueryDepthLimitPlugin.ts` around lines 20 - 23, The depth limiter in QueryDepthLimitPlugin currently skips any operation named IntrospectionQuery, which can be spoofed to bypass checks. Update the operation handling in QueryDepthLimitPlugin so the special case only applies to verified introspection-only operations (or remove the name-based bypass entirely), and make the same change in the other matching block referenced by the same plugin logic.packages/query/src/graphql/graphql.module.ts-240-242 (1)
240-242: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn after writing validation errors instead of calling
next(error).These branches already send a GraphQL error response; forwarding the error logs expected client failures as unhandled middleware errors. The batch-limit response should also be
400, not500.Proposed fix
- res.status(400).json({errors: [new GraphQLError(e.message)]}); - return next(e); + res.status(400).json({errors: [new GraphQLError(e.message)]}); + return;- res.status(500).json({errors: [error]}); - return next(error); + res.status(400).json({errors: [error]}); + return;Also applies to: 266-268, 293-295, 307-310
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/graphql.module.ts` around lines 240 - 242, In the GraphQL validation/error handling branches in graphql.module.ts, stop forwarding the error after sending the client response: the catch blocks that call res.status(...).json(...) should immediately return after writing the GraphQL error payload instead of calling next(error). Apply this same change to the related validation branches referenced in the module (including the batch-limit path) so these expected client errors are treated as handled responses, and keep the batch-limit response status as 400 rather than 500.packages/query/src/graphql/graphql.module.ts-43-49 (1)
43-49: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd
Vary: Originwhen reflecting CORS origins.Line 46 echoes the request
Origin, while Line 69 marks successful responses public-cacheable. WithoutVary: Origin, shared caches can replay a response with the wrongAccess-Control-Allow-Origin.Proposed fix
export function corsMiddleware(req: Request, res: Response, next: NextFunction): void { const origin = req.headers?.origin ?? '*'; + res.vary('Origin'); res.setHeader('Access-Control-Allow-Origin', origin);Also applies to: 64-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/graphql.module.ts` around lines 43 - 49, The corsMiddleware in graphql.module.ts reflects the request Origin into Access-Control-Allow-Origin, so add a Vary: Origin header whenever a non-wildcard origin is echoed. Update the corsMiddleware response headers and any successful response path in the same module that marks responses cacheable to include Vary: Origin, so shared caches keep origins separated. Use the existing corsMiddleware and the success-response code path around the GraphQL handler as the places to apply the change.packages/query/src/graphql/plugins/QueryDepthLimitPlugin.ts-30-35 (1)
30-35: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winThrow when computed depth exceeds the limit.
A leaf at
maxDepth + 1returns that value without throwing, and Lines 32-34 cap it back tomaxDepth, allowing an over-limit query through.Proposed fix
- // Return the max of actual depth or maxDepth (whichever is smaller) - // This ensures we show the limit when capped if (maxQueryDepth > maxDepth) { - return maxDepth; + throw new GraphQLError(`Query is too deep. Maximum depth allowed is ${maxDepth}.`); } return maxQueryDepth;case Kind.FIELD: { + const nextDepth = depthSoFar + 1; + if (nextDepth > maxDepth) { + throw new GraphQLError(`Query is too deep. Maximum depth allowed is ${maxDepth}.`, {nodes: [node]}); + } if (!(node as any).selectionSet) { - return depthSoFar + 1; + return nextDepth; } let maxChild = 0; for (const selection of (node as any).selectionSet.selections) { - const child = checkDepth(selection, fragments, depthSoFar + 1, maxDepth); + const child = checkDepth(selection, fragments, nextDepth, maxDepth);Also applies to: 81-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/QueryDepthLimitPlugin.ts` around lines 30 - 35, Update QueryDepthLimitPlugin so computed depth is treated as an error instead of being capped: in the depth calculation/validation path around QueryDepthLimitPlugin and the related logic referenced by the same comment, throw once the actual depth exceeds maxDepth rather than returning maxDepth. Make sure the helper that computes depth no longer masks a maxDepth + 1 result, and apply the same guard in the second affected block so over-limit queries are rejected consistently.packages/query/src/graphql/plugins/QueryDepthLimitPlugin.ts-96-98 (1)
96-98: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard fragment recursion.
Cyclic fragments recurse until stack overflow because this AST walker runs before GraphQL validation. Track visited fragment names while descending.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/QueryDepthLimitPlugin.ts` around lines 96 - 98, The fragment traversal in QueryDepthLimitPlugin’s checkDepth currently follows Kind.FRAGMENT_SPREAD without any cycle protection, so cyclic fragments can recurse indefinitely before validation. Add a visited-fragment tracking set while descending through fragment spreads, and pass it through the recursive checkDepth calls so repeated fragment names are skipped or short-circuited. Make sure the guard is applied in the Kind.FRAGMENT_SPREAD branch and does not affect non-recursive fragment depth checks.packages/query/src/graphql/graphql.module.ts-82-87 (1)
82-87: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDelegate errors after headers are sent.
Line 85 returns without calling
next(err), which leaves Express unable to finish error handling for partially-sent responses.Proposed fix
-export function errorBoundaryMiddleware(err: Error, _req: Request, res: Response, _next: NextFunction): void { +export function errorBoundaryMiddleware(err: Error, _req: Request, res: Response, next: NextFunction): void { logger.error({err}, 'Unhandled middleware error'); if (res.headersSent) { - return; + return next(err); } res.status(500).json({errors: [new GraphQLError(err.message)]}); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/graphql.module.ts` around lines 82 - 87, The errorBoundaryMiddleware currently returns early when res.headersSent is true, which prevents Express from delegating partially-sent response errors to the next error handler. Update errorBoundaryMiddleware to call next(err) in the headersSent branch instead of returning, while keeping the existing logger.error and 500 GraphQLError response path for unsent responses.packages/query/src/graphql/graphql.module.ts-156-158 (1)
156-158: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry the eager schema build as part of
buildSchema.postgraphile(preset as any)is only instance construction; the actual schema failure path isinstance.getSchema()increateServer, so transient introspection errors still bypass the retry loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/graphql.module.ts` around lines 156 - 158, The retry logic currently only wraps `postgraphile(preset as any)`, but the real eager schema failure happens later when `createServer` calls `instance.getSchema()`. Update `buildSchema` in `graphql.module.ts` to perform the eager schema build there as part of the retry path, using the `makeRuntimePreset`/`postgraphile` flow and forcing schema generation so transient introspection errors are retried before server creation.packages/query/src/graphql/graphql.module.ts-222-225 (1)
222-225: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAwait the schema before applying query complexity limits.
getSchemaResult()is typed as aPromise, so this branch treats a pending schema as “no schema” and returns early, which bypasses--query-complexityduring reloads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/graphql.module.ts` around lines 222 - 225, The schema lookup in graphql.module.ts is treating a pending getSchemaResult() as missing schema, which causes query complexity checks to be skipped during reloads. Update the logic around currentPgInstance?.getSchemaResult() so it properly awaits the schema result before deciding whether to continue, and keep the complexity-limit path active even while the live schema is still resolving.packages/query/src/graphql/plugins/historical/PgConnectionFilterBlockHeightPlugin.ts-102-105 (1)
102-105: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftScope
filterBlockHeightto the current filter block.
enterWithis never restored, so one relation filter’sfilterBlockHeightcan affect later sibling filters that also callgetEffectiveBlockHeight(). Pass the override through a per-condition/proxy map or restore the previous store around the current filter application.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/historical/PgConnectionFilterBlockHeightPlugin.ts` around lines 102 - 105, The `filterBlockHeight` override in `PgConnectionFilterBlockHeightPlugin` is leaking across sibling filters because `currentBlockHeight.enterWith` is never restored. Update the `apply` path so the override is scoped only to the current filter condition, either by using a per-condition/proxy map or by saving and restoring the previous async context store around the call that applies the filter; make sure `getEffectiveBlockHeight()` only sees the temporary value during that single filter application.packages/query/src/graphql/plugins/PgAggregatesHistoricalPlugin.ts-105-109 (1)
105-109: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCast aggregate block-height predicates to
bigint.The main historical filter emits
_block_range @> ...::bigint, but aggregate subqueries interpolateblockHeightStepwithout the cast. This can break or diverge from historical filtering when the GraphQL arg is a string-backed value.Proposed fix
- conditions.push(sql.fragment`${tableAlias}._block_range @> ${blockHeightStep}`); + conditions.push(sql.fragment`${tableAlias}._block_range @> ${blockHeightStep}::bigint`); ... - conditions.push(sql.fragment`${tableAlias}._block_range @> ${blockHeightStep}`); + conditions.push(sql.fragment`${tableAlias}._block_range @> ${blockHeightStep}::bigint`);Also applies to: 219-223
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/PgAggregatesHistoricalPlugin.ts` around lines 105 - 109, The aggregate historical filter in PgAggregatesHistoricalPlugin is missing the same bigint cast used by the main `_block_range` predicate, so `blockHeightStep` can behave differently for string-backed GraphQL inputs. Update the `remoteHasBlockRange` branch where `blockHeightStepMap.get($select)` feeds `conditions.push(...)` to cast the interpolated value to bigint, and apply the same change in the other aggregate subquery path noted in the review so filtering stays consistent with the historical query logic.packages/query/src/graphql/plugins/historical/requestContext.ts-10-16 (1)
10-16: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAvoid shipping known sibling-bleed semantics.
The note documents that
enterWithcan leak height to later sibling fields. Because historical height changes query results, this needs scoped storage instead of a process-current async store fallback for field/filter inheritance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/historical/requestContext.ts` around lines 10 - 16, The currentBlockHeight AsyncLocalStorage in requestContext still uses enterWith-style request-wide mutation, which can leak historical height across sibling fields. Update the historical request context flow so field/filter inheritance uses scoped, per-branch storage instead of a shared current async store fallback, and adjust the applyPlan/request context plumbing to read from the branch-specific source rather than the mutable currentBlockHeight.packages/query/src/graphql/plugins/PgConnectionFirstLastClampPlugin.ts-61-68 (1)
61-68: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winTreat
first: nullandlast: nullas omitted.Currently
null !== undefined, so a client can sendfirst: nulland skip the defaultqueryLimitset on Line 75, allowing an unbounded connection query.Proposed fix
const hasFirst = rawFirst !== null && rawFirst !== undefined && typeof rawFirst.eval === 'function' - ? rawFirst.eval() !== undefined + ? rawFirst.eval() !== null && rawFirst.eval() !== undefined : rawFirst !== null && rawFirst !== undefined; const hasLast = rawLast !== null && rawLast !== undefined && typeof rawLast.eval === 'function' - ? rawLast.eval() !== undefined + ? rawLast.eval() !== null && rawLast.eval() !== undefined : rawLast !== null && rawLast !== undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/PgConnectionFirstLastClampPlugin.ts` around lines 61 - 68, In PgConnectionFirstLastClampPlugin, the hasFirst/hasLast checks currently treat null as present because they only test against undefined, so `first: null` or `last: null` bypasses the `queryLimit` fallback. Update the logic in the plugin’s `rawFirst`/`rawLast` handling to treat both null and undefined as omitted, including when values come from `eval()`, so the default clamp is applied whenever the client passes a null value.packages/query/src/graphql/plugins/historical/PgBlockHeightPlugin.ts-233-246 (1)
233-246: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve an explicit
timestampstep here
blockHeight's default applyPlan still writesHEIGHT_DEFAULTintoblockHeightStepMap, so a query that setstimestampand leavesblockHeightunset can lose the historical height in aggregate order subqueries. Only store the default when no explicit step is already present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/historical/PgBlockHeightPlugin.ts` around lines 233 - 246, The current applyPlan in PgBlockHeightPlugin still overwrites blockHeightStepMap with HEIGHT_DEFAULT even when an explicit timestamp step was already set, which can erase the historical height used by aggregate order subqueries. Update makeApplyPlan/applyPlan to check for an existing entry for the same PgSelectStep before calling blockHeightStepMap.set, and only store the default when no explicit step is already present while keeping the current currentBlockHeight.enterWith behavior for non-default resolved heights.packages/query/src/graphql/plugins/historical/PgBlockHeightPlugin.ts-112-124 (1)
112-124: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't use
constructor.nameto decide whetherblockHeightwas provided.getRaw('blockHeight')can return aConstantStepeven for a user-supplied literal, soblockHeight: "123"may fall back to the inherited/default height here. Compare the resolved value againstHEIGHT_DEFAULTinstead, and apply the same logic whereblockHeightStepMapis populated later in the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/historical/PgBlockHeightPlugin.ts` around lines 112 - 124, The block-height resolution logic in resolveEffectiveHeight is incorrectly using constructor.name to detect user-provided blockHeight, which can misclassify literal inputs like blockHeight: "123" as inherited defaults. Update the blockHeight handling to compare the resolved value against HEIGHT_DEFAULT instead of checking for ConstantStep, and apply the same presence check consistently when populating blockHeightStepMap later in PgBlockHeightPlugin so both code paths treat explicit literals the same way.packages/query/src/graphql/plugins/PgSubscriptionPlugin.ts-76-90 (1)
76-90: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftImplement the declared subscription filters.
The field exposes
idandmutation, and the comment says events are filtered, butargsis ignored and every event on the topic is delivered. Apply those filters in the Grafast plan before returning events.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/PgSubscriptionPlugin.ts` around lines 76 - 90, The subscribePlan in PgSubscriptionPlugin is ignoring the declared subscription filters, so every event for the topic is returned. Update the EXPORTABLE subscribePlan to use the args passed into subscribePlan and apply the id and mutation filtering in the Grafast plan before returning listen(...) results, while keeping the existing pgSubscriber lookup and topic setup intact.packages/query/src/graphql/plugins/GetMetadataPlugin.ts-200-201 (1)
200-201: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the indexer polling interval across schema rebuilds.
This runs inside
extendSchema; hot schema reloads or repeated test schema builds can create duplicatefetchFromApiintervals. Store the interval handle and ensure it is started once or cleaned up on schema teardown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/GetMetadataPlugin.ts` around lines 200 - 201, The indexer polling in extendSchema can be started multiple times across schema rebuilds, creating duplicate fetchFromApi intervals. Update GetMetadataPlugin’s extendSchema path to store the interval handle returned by setAsyncInterval and guard against starting it more than once, or add cleanup on schema teardown so repeated hot reloads/tests don’t leak extra polling timers.packages/query/src/graphql/plugins/PgDistinctPlugin.ts-53-57 (1)
53-57: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep
DISTINCT ONaligned withORDER BY.This injects
DISTINCT ON (...)but does not ensure the generatedORDER BYstarts with the same expressions. Connection fields often add default/unique ordering, sodistinctcan produce invalid SQL for non-matching orderings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/PgDistinctPlugin.ts` around lines 53 - 57, The PgDistinctPlugin logic in the DISTINCT ON injection path needs to also enforce matching ORDER BY prefixes so the generated SQL stays valid. Update the code around the selects rewrite in PgDistinctPlugin to ensure the DISTINCT ON expressions are mirrored as the leading ORDER BY expressions, or otherwise adjust the ordering generation so connection defaults and unique ordering cannot conflict with DISTINCT ON.packages/query/src/graphql/plugins/GetMetadataPlugin.ts-245-245 (1)
245-245: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply or remove the
_metadatas(chainId)argument.The schema exposes
chainId, but the resolver ignoresargs.chainIdand returns every metadata table. Filter togetMetadataTableName(args.chainId)when provided, or remove the argument from the field.Also applies to: 273-284
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/GetMetadataPlugin.ts` at line 245, The _metadatas field in GetMetadataPlugin exposes a chainId argument, but the resolver currently ignores it and returns all metadata tables. Update the _metadatas resolver and related schema/type definitions so it either filters by getMetadataTableName(args.chainId) when chainId is provided, or remove the chainId argument entirely if it is not meant to be used; make the same adjustment in the matching _metadatas implementation referenced by the other affected section.packages/query/src/graphql/plugins/PgSubscriptionPlugin.ts-67-100 (1)
67-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDerive the schema instead of falling back to
public.PostGraphile v5 resources may not expose
namespace; this makes both the NOTIFY topic and raw lookup SQL usepublicfor SubQuery schemas likesubquery_1. Reuse thefrom.tschema extraction pattern used inGetMetadataPluginbefore callinghashNameor buildingfromIdent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/PgSubscriptionPlugin.ts` around lines 67 - 100, The subscription topic and raw table lookup in PgSubscriptionPlugin are incorrectly defaulting to public when resource.namespace is missing, which breaks SubQuery schemas. Update the schema resolution in the subscription setup to derive the namespace from the source object using the same from.t extraction pattern used in GetMetadataPlugin, and reuse that resolved schema both for hashName and for building fromIdent. Keep the existing PgSubscriptionPlugin subscribePlan flow and topic generation, but replace the fallback-only namespace logic with derived schema handling.packages/query/src/graphql/plugins/index.ts-23-24 (1)
23-24: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid parsing CLI options while importing the preset.
getYargsOption().argvis evaluated at module load, andpackages/query/src/yargs.tsmarksnameas required. ImportingqueryPresetfrom tests or other modules can therefore parse/exit before the caller has configured argv/env. Defer these reads to schema construction/runtime, or expose a preset factory that accepts feature flags.Also applies to: 96-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/index.ts` around lines 23 - 24, The preset is reading CLI options too early because `getYargsOption().argv` and `aggregateEnabled` are evaluated at module import time in `queryPreset`. Move the `argv` access out of module scope and into schema construction/runtime, or change `queryPreset` into a factory that accepts the needed feature flags so importing the preset does not trigger yargs parsing or exit before callers configure argv/env. Also update the other module-scope `getYargsOption().argv` usage at the referenced location to follow the same lazy pattern.packages/query/src/graphql/plugins/PgDistinctPlugin.ts-108-110 (1)
108-110: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInitialize
_metabefore storingdistinctOn.If
_metais undefined, Line 110 throws and the emptycatchsilently disablesdistinct.Proposed fix
const distinctValues = rawStep.eval(); if (!Array.isArray(distinctValues) || distinctValues.length === 0) return $connection; + ($select as any)._meta ??= {}; ($select as any)._meta.distinctOn = distinctValues;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/PgDistinctPlugin.ts` around lines 108 - 110, The PgDistinctPlugin path that sets `distinctOn` is writing to `($select as any)._meta` without ensuring `_meta` exists, which can throw and be swallowed by the empty catch. In `PgDistinctPlugin` where `rawStep.eval()` is processed, initialize `_meta` on the select object before assigning `distinctOn`, or safely create it inline if missing, so the `distinct` metadata is preserved instead of silently failing.packages/query/src/graphql/plugins/index.ts-61-68 (1)
61-68: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward
_optionstopreviousin both inflection overrides. Callingprevious(resource)drops the expected options argument, which can change the generated field names inallRowsConnectionandallRowsList.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/index.ts` around lines 61 - 68, The inflection overrides in allRowsConnection and allRowsList are not forwarding the _options argument when delegating to previous, which can alter generated field names. Update both functions to pass _options through to previous alongside resource, keeping the existing _metadata special-case behavior intact.packages/query/src/graphql/plugins/PgSearchPlugin.ts-22-43 (1)
22-43: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve the Grafast
argsobject here.{...args, search: sanitized}turns the FieldArgs wrapper into a plain object beforeorigPlanruns, so any downstreamgetRaw/lazy-arg logic can break. Use the v5 argument-transform path or mutatesearchwithout replacingargs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/PgSearchPlugin.ts` around lines 22 - 43, Preserve the Grafast FieldArgs wrapper in PgSearchPlugin’s field.plan logic instead of replacing it with a spread copy. The current `{...args, search: sanitized}` converts lazy v5 args into a plain object, which can break downstream raw-arg handling; update the argument transformation in this plan hook so only `search` is adjusted while the original `args` object is preserved, or use the v5-supported argument-transform mechanism before calling `origPlan`.
🟡 Minor comments (1)
packages/query/src/graphql/plugins/historical/utils.ts-4-9 (1)
4-9: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn
falsewhen block-range support is unknown.
hasBlockRange(undefined)and entities without attributes currently returntrue, which can trigger_block_rangeSQL on targets that were not proven historical.Proposed fix
export function hasBlockRange(entity: any): boolean { - if (!entity) return true; + if (!entity) return false; const codec = entity.attributes ? entity : entity.codec; - if (!codec?.attributes) return true; + if (!codec?.attributes) return false; return '_block_range' in (codec?.attributes ?? {}); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/historical/utils.ts` around lines 4 - 9, The hasBlockRange helper in utils.ts is defaulting to true for undefined inputs and entities/codecs without attributes, which incorrectly treats unknown historical support as present. Update hasBlockRange(entity) so that missing entity, missing codec, or missing attributes all return false, and only return true when the codec.attributes object explicitly contains _block_range.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/query/src/postgraphile.d.ts`:
- Around line 81-89: The GraphileConfig augmentation is currently inside a
script-style declaration file, so `declare global` lacks module scope. Update
`postgraphile.d.ts` by making it an external module with `export {};` before the
`declare global` block, or alternatively move the `GraphileConfig.Preset`
augmentation to a top-level `declare namespace` so the module/global typing is
valid.
---
Major comments:
In `@packages/query/src/graphql/graphql.module.ts`:
- Around line 240-242: In the GraphQL validation/error handling branches in
graphql.module.ts, stop forwarding the error after sending the client response:
the catch blocks that call res.status(...).json(...) should immediately return
after writing the GraphQL error payload instead of calling next(error). Apply
this same change to the related validation branches referenced in the module
(including the batch-limit path) so these expected client errors are treated as
handled responses, and keep the batch-limit response status as 400 rather than
500.
- Around line 43-49: The corsMiddleware in graphql.module.ts reflects the
request Origin into Access-Control-Allow-Origin, so add a Vary: Origin header
whenever a non-wildcard origin is echoed. Update the corsMiddleware response
headers and any successful response path in the same module that marks responses
cacheable to include Vary: Origin, so shared caches keep origins separated. Use
the existing corsMiddleware and the success-response code path around the
GraphQL handler as the places to apply the change.
- Around line 82-87: The errorBoundaryMiddleware currently returns early when
res.headersSent is true, which prevents Express from delegating partially-sent
response errors to the next error handler. Update errorBoundaryMiddleware to
call next(err) in the headersSent branch instead of returning, while keeping the
existing logger.error and 500 GraphQLError response path for unsent responses.
- Around line 156-158: The retry logic currently only wraps `postgraphile(preset
as any)`, but the real eager schema failure happens later when `createServer`
calls `instance.getSchema()`. Update `buildSchema` in `graphql.module.ts` to
perform the eager schema build there as part of the retry path, using the
`makeRuntimePreset`/`postgraphile` flow and forcing schema generation so
transient introspection errors are retried before server creation.
- Around line 222-225: The schema lookup in graphql.module.ts is treating a
pending getSchemaResult() as missing schema, which causes query complexity
checks to be skipped during reloads. Update the logic around
currentPgInstance?.getSchemaResult() so it properly awaits the schema result
before deciding whether to continue, and keep the complexity-limit path active
even while the live schema is still resolving.
In `@packages/query/src/graphql/plugins/GetMetadataPlugin.ts`:
- Around line 200-201: The indexer polling in extendSchema can be started
multiple times across schema rebuilds, creating duplicate fetchFromApi
intervals. Update GetMetadataPlugin’s extendSchema path to store the interval
handle returned by setAsyncInterval and guard against starting it more than
once, or add cleanup on schema teardown so repeated hot reloads/tests don’t leak
extra polling timers.
- Line 245: The _metadatas field in GetMetadataPlugin exposes a chainId
argument, but the resolver currently ignores it and returns all metadata tables.
Update the _metadatas resolver and related schema/type definitions so it either
filters by getMetadataTableName(args.chainId) when chainId is provided, or
remove the chainId argument entirely if it is not meant to be used; make the
same adjustment in the matching _metadatas implementation referenced by the
other affected section.
In `@packages/query/src/graphql/plugins/historical/PgBlockHeightPlugin.ts`:
- Around line 233-246: The current applyPlan in PgBlockHeightPlugin still
overwrites blockHeightStepMap with HEIGHT_DEFAULT even when an explicit
timestamp step was already set, which can erase the historical height used by
aggregate order subqueries. Update makeApplyPlan/applyPlan to check for an
existing entry for the same PgSelectStep before calling blockHeightStepMap.set,
and only store the default when no explicit step is already present while
keeping the current currentBlockHeight.enterWith behavior for non-default
resolved heights.
- Around line 112-124: The block-height resolution logic in
resolveEffectiveHeight is incorrectly using constructor.name to detect
user-provided blockHeight, which can misclassify literal inputs like
blockHeight: "123" as inherited defaults. Update the blockHeight handling to
compare the resolved value against HEIGHT_DEFAULT instead of checking for
ConstantStep, and apply the same presence check consistently when populating
blockHeightStepMap later in PgBlockHeightPlugin so both code paths treat
explicit literals the same way.
In
`@packages/query/src/graphql/plugins/historical/PgConnectionFilterBlockHeightPlugin.ts`:
- Around line 102-105: The `filterBlockHeight` override in
`PgConnectionFilterBlockHeightPlugin` is leaking across sibling filters because
`currentBlockHeight.enterWith` is never restored. Update the `apply` path so the
override is scoped only to the current filter condition, either by using a
per-condition/proxy map or by saving and restoring the previous async context
store around the call that applies the filter; make sure
`getEffectiveBlockHeight()` only sees the temporary value during that single
filter application.
In `@packages/query/src/graphql/plugins/historical/requestContext.ts`:
- Around line 10-16: The currentBlockHeight AsyncLocalStorage in requestContext
still uses enterWith-style request-wide mutation, which can leak historical
height across sibling fields. Update the historical request context flow so
field/filter inheritance uses scoped, per-branch storage instead of a shared
current async store fallback, and adjust the applyPlan/request context plumbing
to read from the branch-specific source rather than the mutable
currentBlockHeight.
In `@packages/query/src/graphql/plugins/index.ts`:
- Around line 23-24: The preset is reading CLI options too early because
`getYargsOption().argv` and `aggregateEnabled` are evaluated at module import
time in `queryPreset`. Move the `argv` access out of module scope and into
schema construction/runtime, or change `queryPreset` into a factory that accepts
the needed feature flags so importing the preset does not trigger yargs parsing
or exit before callers configure argv/env. Also update the other module-scope
`getYargsOption().argv` usage at the referenced location to follow the same lazy
pattern.
- Around line 61-68: The inflection overrides in allRowsConnection and
allRowsList are not forwarding the _options argument when delegating to
previous, which can alter generated field names. Update both functions to pass
_options through to previous alongside resource, keeping the existing _metadata
special-case behavior intact.
In `@packages/query/src/graphql/plugins/PgAggregatesHistoricalPlugin.ts`:
- Around line 105-109: The aggregate historical filter in
PgAggregatesHistoricalPlugin is missing the same bigint cast used by the main
`_block_range` predicate, so `blockHeightStep` can behave differently for
string-backed GraphQL inputs. Update the `remoteHasBlockRange` branch where
`blockHeightStepMap.get($select)` feeds `conditions.push(...)` to cast the
interpolated value to bigint, and apply the same change in the other aggregate
subquery path noted in the review so filtering stays consistent with the
historical query logic.
In `@packages/query/src/graphql/plugins/PgConnectionFirstLastClampPlugin.ts`:
- Around line 61-68: In PgConnectionFirstLastClampPlugin, the hasFirst/hasLast
checks currently treat null as present because they only test against undefined,
so `first: null` or `last: null` bypasses the `queryLimit` fallback. Update the
logic in the plugin’s `rawFirst`/`rawLast` handling to treat both null and
undefined as omitted, including when values come from `eval()`, so the default
clamp is applied whenever the client passes a null value.
In `@packages/query/src/graphql/plugins/PgDistinctPlugin.ts`:
- Around line 53-57: The PgDistinctPlugin logic in the DISTINCT ON injection
path needs to also enforce matching ORDER BY prefixes so the generated SQL stays
valid. Update the code around the selects rewrite in PgDistinctPlugin to ensure
the DISTINCT ON expressions are mirrored as the leading ORDER BY expressions, or
otherwise adjust the ordering generation so connection defaults and unique
ordering cannot conflict with DISTINCT ON.
- Around line 108-110: The PgDistinctPlugin path that sets `distinctOn` is
writing to `($select as any)._meta` without ensuring `_meta` exists, which can
throw and be swallowed by the empty catch. In `PgDistinctPlugin` where
`rawStep.eval()` is processed, initialize `_meta` on the select object before
assigning `distinctOn`, or safely create it inline if missing, so the `distinct`
metadata is preserved instead of silently failing.
In `@packages/query/src/graphql/plugins/PgSearchPlugin.ts`:
- Around line 22-43: Preserve the Grafast FieldArgs wrapper in PgSearchPlugin’s
field.plan logic instead of replacing it with a spread copy. The current
`{...args, search: sanitized}` converts lazy v5 args into a plain object, which
can break downstream raw-arg handling; update the argument transformation in
this plan hook so only `search` is adjusted while the original `args` object is
preserved, or use the v5-supported argument-transform mechanism before calling
`origPlan`.
In `@packages/query/src/graphql/plugins/PgSubscriptionPlugin.ts`:
- Around line 76-90: The subscribePlan in PgSubscriptionPlugin is ignoring the
declared subscription filters, so every event for the topic is returned. Update
the EXPORTABLE subscribePlan to use the args passed into subscribePlan and apply
the id and mutation filtering in the Grafast plan before returning listen(...)
results, while keeping the existing pgSubscriber lookup and topic setup intact.
- Around line 67-100: The subscription topic and raw table lookup in
PgSubscriptionPlugin are incorrectly defaulting to public when
resource.namespace is missing, which breaks SubQuery schemas. Update the schema
resolution in the subscription setup to derive the namespace from the source
object using the same from.t extraction pattern used in GetMetadataPlugin, and
reuse that resolved schema both for hashName and for building fromIdent. Keep
the existing PgSubscriptionPlugin subscribePlan flow and topic generation, but
replace the fallback-only namespace logic with derived schema handling.
In `@packages/query/src/graphql/plugins/QueryDepthLimitPlugin.ts`:
- Around line 20-23: The depth limiter in QueryDepthLimitPlugin currently skips
any operation named IntrospectionQuery, which can be spoofed to bypass checks.
Update the operation handling in QueryDepthLimitPlugin so the special case only
applies to verified introspection-only operations (or remove the name-based
bypass entirely), and make the same change in the other matching block
referenced by the same plugin logic.
- Around line 30-35: Update QueryDepthLimitPlugin so computed depth is treated
as an error instead of being capped: in the depth calculation/validation path
around QueryDepthLimitPlugin and the related logic referenced by the same
comment, throw once the actual depth exceeds maxDepth rather than returning
maxDepth. Make sure the helper that computes depth no longer masks a maxDepth +
1 result, and apply the same guard in the second affected block so over-limit
queries are rejected consistently.
- Around line 96-98: The fragment traversal in QueryDepthLimitPlugin’s
checkDepth currently follows Kind.FRAGMENT_SPREAD without any cycle protection,
so cyclic fragments can recurse indefinitely before validation. Add a
visited-fragment tracking set while descending through fragment spreads, and
pass it through the recursive checkDepth calls so repeated fragment names are
skipped or short-circuited. Make sure the guard is applied in the
Kind.FRAGMENT_SPREAD branch and does not affect non-recursive fragment depth
checks.
---
Minor comments:
In `@packages/query/src/graphql/plugins/historical/utils.ts`:
- Around line 4-9: The hasBlockRange helper in utils.ts is defaulting to true
for undefined inputs and entities/codecs without attributes, which incorrectly
treats unknown historical support as present. Update hasBlockRange(entity) so
that missing entity, missing codec, or missing attributes all return false, and
only return true when the codec.attributes object explicitly contains
_block_range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd032009-23db-4f96-8e55-805007a813e5
📒 Files selected for processing (34)
packages/query/package.jsonpackages/query/src/configure/configure.module.tspackages/query/src/configure/x-postgraphile/debugClient.tspackages/query/src/graphql/graphql.module.tspackages/query/src/graphql/plugins/GetMetadataPlugin.tspackages/query/src/graphql/plugins/PgAggregateSpecsPlugin.tspackages/query/src/graphql/plugins/PgAggregatesHistoricalPlugin.tspackages/query/src/graphql/plugins/PgAggregationPlugin.tspackages/query/src/graphql/plugins/PgBackwardRelationPlugin.tspackages/query/src/graphql/plugins/PgConnectionArgFirstLastBeforeAfter.tspackages/query/src/graphql/plugins/PgConnectionFirstLastClampPlugin.tspackages/query/src/graphql/plugins/PgDistinctPlugin.tspackages/query/src/graphql/plugins/PgOrderByAggregatesPlugin.tspackages/query/src/graphql/plugins/PgOrderByUnique.tspackages/query/src/graphql/plugins/PgRowByVirtualIdPlugin.tspackages/query/src/graphql/plugins/PgSearchPlugin.tspackages/query/src/graphql/plugins/PgSubscriptionPlugin.tspackages/query/src/graphql/plugins/PlaygroundPlugin.tspackages/query/src/graphql/plugins/QueryAliasLimitPlugin.tspackages/query/src/graphql/plugins/QueryComplexityPlugin.tspackages/query/src/graphql/plugins/QueryDepthLimitPlugin.spec.tspackages/query/src/graphql/plugins/QueryDepthLimitPlugin.tspackages/query/src/graphql/plugins/__tests__/testHelpers.tspackages/query/src/graphql/plugins/historical/PgBlockHeightPlugin.tspackages/query/src/graphql/plugins/historical/PgConnectionArgFilterBackwardRelationsPlugin.tspackages/query/src/graphql/plugins/historical/PgConnectionArgFilterForwardRelationsPlugin.tspackages/query/src/graphql/plugins/historical/PgConnectionFilterBlockHeightPlugin.tspackages/query/src/graphql/plugins/historical/index.tspackages/query/src/graphql/plugins/historical/requestContext.tspackages/query/src/graphql/plugins/historical/utils.tspackages/query/src/graphql/plugins/index.tspackages/query/src/graphql/plugins/smartTagsPlugin.tspackages/query/src/postgraphile.d.tspackages/query/src/yargs.ts
💤 Files with no reviewable changes (12)
- packages/query/src/graphql/plugins/smartTagsPlugin.ts
- packages/query/src/graphql/plugins/PgRowByVirtualIdPlugin.ts
- packages/query/src/graphql/plugins/PgBackwardRelationPlugin.ts
- packages/query/src/graphql/plugins/PgAggregateSpecsPlugin.ts
- packages/query/src/graphql/plugins/QueryDepthLimitPlugin.spec.ts
- packages/query/src/graphql/plugins/PgConnectionArgFirstLastBeforeAfter.ts
- packages/query/src/configure/x-postgraphile/debugClient.ts
- packages/query/src/graphql/plugins/PlaygroundPlugin.ts
- packages/query/src/graphql/plugins/PgAggregationPlugin.ts
- packages/query/src/graphql/plugins/historical/PgConnectionArgFilterForwardRelationsPlugin.ts
- packages/query/src/graphql/plugins/historical/PgConnectionArgFilterBackwardRelationsPlugin.ts
- packages/query/src/graphql/plugins/PgOrderByAggregatesPlugin.ts
Comment on lines
+81
to
+89
| declare global { | ||
| namespace GraphileConfig { | ||
| interface Preset { | ||
| pgServices?: readonly any[]; | ||
| grafast?: Record<string, any>; | ||
| grafserv?: Record<string, any>; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the declaration file is treated as an external module.
fd -i '^postgraphile\.d\.ts$' --exec sh -c 'grep -n "declare global\|export {}" "$1"' sh {}Repository: subquery/subql
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/query/src/postgraphile.d.ts"
wc -l "$file"
echo '--- top ---'
sed -n '1,120p' "$file" | cat -n
echo '--- export/import search ---'
grep -nE '^(import|export)\b|export \{\}' "$file" || trueRepository: subquery/subql
Length of output: 3718
Make this declaration file an external module before declare global. declare global needs module scope here; add export {}; or move GraphileConfig to a top-level declare namespace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/query/src/postgraphile.d.ts` around lines 81 - 89, The
GraphileConfig augmentation is currently inside a script-style declaration file,
so `declare global` lacks module scope. Update `postgraphile.d.ts` by making it
an external module with `export {};` before the `declare global` block, or
alternatively move the `GraphileConfig.Preset` augmentation to a top-level
`declare namespace` so the module/global typing is valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/query/src/graphql/plugins/__tests__/PgSmartTagsPlugin.spec.ts (1)
6-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate yargs mock across test files.
This same
jest.mock('../../../yargs', ...)block (with only minor field differences) is repeated inGetMetadataPlugin.spec.tsandGetMetadataPlugin.indexer.spec.ts. Consider extracting a shared mock factory (e.g. amockYargs(overrides)helper intestHelpers.tsor a manual__mocks__/yargs.ts) to reduce duplication and keep default argv fields consistent across suites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/__tests__/PgSmartTagsPlugin.spec.ts` around lines 6 - 21, The yargs mock setup in PgSmartTagsPlugin.spec.ts is duplicated across multiple test suites, so consolidate it into a shared helper or manual mock and reuse it from the affected tests. Extract the common jest.mock('../../../yargs', ...) behavior into a centralized mock factory such as a testHelpers.ts utility or __mocks__/yargs.ts, and let each spec override only the fields it needs while keeping the default argv shape consistent.packages/query/src/graphql/plugins/__tests__/GetMetadataPlugin.spec.ts (1)
168-194: 🎯 Functional Correctness | 🔵 TrivialTest may not exercise the schema-detection logic it claims to verify.
Per
testHelpers.ts(createTestContext→buildTestSchema),pgServicesis scoped to a single schema:makePgService({pool, schemas: [dbSchema]}). SincesecondarySchemais never added topgServices, its_metadatatable/resources are never introspected intopgRegistryat all — meaning the assertion that the primary schema's chain is returned would hold true even ifGetMetadataPlugin's schema-name-detection logic (viapgResource.from) were broken, since the secondary table simply isn't visible to the query. This makes the test largely tautological for its stated purpose ("v5 derives schema name from pgResource.from SQL text").To genuinely test multi-schema resolution,
createTestContext/buildTestSchemawould need to register both schemas inpgServicesso both_metadataresources are present inpgRegistrysimultaneously.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/__tests__/GetMetadataPlugin.spec.ts` around lines 168 - 194, The schema-detection test in GetMetadataPlugin.spec is currently tautological because only the primary schema is registered via createTestContext/buildTestSchema and makePgService, so the secondary _metadata table is never introspected into pgRegistry. Update the test setup so pgServices includes both dbSchema and secondarySchema (using the relevant test helpers) before running runQuery, then keep the assertion in the detects schema name correctly from pgResource case verifying the primary schema’s chain is returned. This ensures the test actually exercises GetMetadataPlugin’s pgResource.from-based schema resolution rather than relying on the secondary schema being invisible.packages/query/src/graphql/plugins/__tests__/PgConnectionFirstLastClampPlugin.spec.ts (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent identifier quoting for schema name.
Other statements in this file and sibling spec files quote the schema identifier (
"${dbSchema}"), but these two use it unquoted. Harmless with the current literal but inconsistent.✏️ Suggested fix
- await pool.query(`CREATE SCHEMA IF NOT EXISTS ${dbSchema}`); + await pool.query(`CREATE SCHEMA IF NOT EXISTS "${dbSchema}"`);- await pool.query(`DROP SCHEMA IF EXISTS ${dbSchema} CASCADE`); + await pool.query(`DROP SCHEMA IF EXISTS "${dbSchema}" CASCADE`);Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/__tests__/PgConnectionFirstLastClampPlugin.spec.ts` at line 30, The schema identifier in the PgConnectionFirstLastClampPlugin spec is being interpolated unquoted in the CREATE SCHEMA statements, which is inconsistent with the rest of the file and sibling specs. Update the schema creation queries in the test setup/teardown around the dbSchema usage to quote the identifier the same way as the other statements, keeping the fix localized to the spec helpers that create and drop the schema.packages/query/src/graphql/plugins/__tests__/PgAggregatesHistoricalPlugin.spec.ts (1)
99-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTautological test provides no real coverage.
This test only throws and catches a hardcoded
Error('Function source unsupported')— it never invokes the plugin. It will always pass, regardless of the actualPgAggregatesHistoricalPluginimplementation, and doesn't validate the described error path.♻️ Suggested fix
- it('error path: "Function source unsupported" is defined', () => { - // The plugin throws 'Function source unsupported' when table.from is a function type. - // In v5, table.from is always a SQL fragment, so this path only triggers with - // exotic phantom types. Test verifies the error message format exists. - expect(() => { - throw new Error('Function source unsupported'); - }).toThrow('Function source unsupported'); - }); + // NOTE: table.from is always a SQL fragment in v5, so the + // "Function source unsupported" branch is currently unreachable via + // integration tests. Remove this placeholder once a concrete + // reproduction (e.g. a function-backed resource) is available, or + // cover it with a targeted unit test that exercises the plugin directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/plugins/__tests__/PgAggregatesHistoricalPlugin.spec.ts` around lines 99 - 106, The test in PgAggregatesHistoricalPlugin.spec is tautological and never exercises the plugin’s real error path. Replace the hardcoded throw in the “Function source unsupported” case by invoking PgAggregatesHistoricalPlugin through its actual entry point and asserting that it throws the same error when table.from is a function-like source, so the spec validates the implementation instead of a synthetic Error instance.packages/query/src/graphql/graphql.test.ts (1)
47-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates the shared
createTestContextschema-building logic.
buildTestSchema/runQueryhere re-implement exactly whatcreateTestContextin./plugins/__tests__/testHelpers.tsalready provides (samemakeSchema/grafastwiring, samepgClientinjection intocontextValue/requestContext). Every sibling plugin spec (PgDistinctPlugin.spec.ts,PgSearchPlugin.spec.ts,PgOrderByUniquePlugin.spec.ts,PgBackwardRelationPlugin.spec.ts) imports and reuses that helper instead of duplicating it. Diverging implementations risk drifting apart as the harness evolves (e.g. a future fix to schema caching or pgClient handling intestHelpers.tswon't propagate here).♻️ Suggested consolidation
-import {makeSchema} from 'postgraphile'; -import {makePgService} from 'postgraphile/@dataplan/pg/adaptors/pg'; -import {grafast} from 'postgraphile/grafast'; -import {Config} from '../configure'; -import {queryPreset} from './plugins'; +import {createTestContext} from './plugins/__tests__/testHelpers'; ... - async function buildTestSchema() { - const preset = { - ...queryPreset, - pgServices: [makePgService({pool, schemas: [dbSchema]})], - gather: { - pgFakeConstraintsAutofixForeignKeyUniqueness: true, - }, - }; - return makeSchema(preset as any); - } - - async function runQuery(query: string) { - const {resolvedPreset, schema} = await buildTestSchema(); - const pgClient = pool; - return grafast({ - resolvedPreset, - schema, - source: query, - contextValue: {pgClient}, - requestContext: {pgClient}, - }); - } + const {buildTestSchema, runQuery} = createTestContext(dbSchema);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query/src/graphql/graphql.test.ts` around lines 47 - 68, `buildTestSchema` and `runQuery` in the GraphQL test duplicate the shared test harness already provided by `createTestContext`; replace this local `makeSchema`/`grafast` setup with the helper from `./plugins/__tests__/testHelpers.ts`, keeping the same `pgClient` wiring via `contextValue` and `requestContext`. Update the test to use the shared context builder the same way the sibling plugin specs do, so schema creation and query execution stay centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/query/src/graphql/plugins/__tests__/PgBackwardRelationPlugin.spec.ts`:
- Around line 231-267: The test in PgBackwardRelationPlugin.spec.ts is asserting
the wrong behavior for the current setup: order_items has no foreign key, so it
cannot verify the “unique subset makes backward relation singular” case. Update
the test to either add a real composite FK and assert the backward relation’s
singular/plural type in the __type introspection, or rewrite the test
name/comment so it only covers schema construction for a composite PK plus
unique subset. Use the existing runQuery and OrderItem introspection path to
validate the actual relation behavior being described.
---
Nitpick comments:
In `@packages/query/src/graphql/graphql.test.ts`:
- Around line 47-68: `buildTestSchema` and `runQuery` in the GraphQL test
duplicate the shared test harness already provided by `createTestContext`;
replace this local `makeSchema`/`grafast` setup with the helper from
`./plugins/__tests__/testHelpers.ts`, keeping the same `pgClient` wiring via
`contextValue` and `requestContext`. Update the test to use the shared context
builder the same way the sibling plugin specs do, so schema creation and query
execution stay centralized and consistent.
In `@packages/query/src/graphql/plugins/__tests__/GetMetadataPlugin.spec.ts`:
- Around line 168-194: The schema-detection test in GetMetadataPlugin.spec is
currently tautological because only the primary schema is registered via
createTestContext/buildTestSchema and makePgService, so the secondary _metadata
table is never introspected into pgRegistry. Update the test setup so pgServices
includes both dbSchema and secondarySchema (using the relevant test helpers)
before running runQuery, then keep the assertion in the detects schema name
correctly from pgResource case verifying the primary schema’s chain is returned.
This ensures the test actually exercises GetMetadataPlugin’s
pgResource.from-based schema resolution rather than relying on the secondary
schema being invisible.
In
`@packages/query/src/graphql/plugins/__tests__/PgAggregatesHistoricalPlugin.spec.ts`:
- Around line 99-106: The test in PgAggregatesHistoricalPlugin.spec is
tautological and never exercises the plugin’s real error path. Replace the
hardcoded throw in the “Function source unsupported” case by invoking
PgAggregatesHistoricalPlugin through its actual entry point and asserting that
it throws the same error when table.from is a function-like source, so the spec
validates the implementation instead of a synthetic Error instance.
In
`@packages/query/src/graphql/plugins/__tests__/PgConnectionFirstLastClampPlugin.spec.ts`:
- Line 30: The schema identifier in the PgConnectionFirstLastClampPlugin spec is
being interpolated unquoted in the CREATE SCHEMA statements, which is
inconsistent with the rest of the file and sibling specs. Update the schema
creation queries in the test setup/teardown around the dbSchema usage to quote
the identifier the same way as the other statements, keeping the fix localized
to the spec helpers that create and drop the schema.
In `@packages/query/src/graphql/plugins/__tests__/PgSmartTagsPlugin.spec.ts`:
- Around line 6-21: The yargs mock setup in PgSmartTagsPlugin.spec.ts is
duplicated across multiple test suites, so consolidate it into a shared helper
or manual mock and reuse it from the affected tests. Extract the common
jest.mock('../../../yargs', ...) behavior into a centralized mock factory such
as a testHelpers.ts utility or __mocks__/yargs.ts, and let each spec override
only the fields it needs while keeping the default argv shape consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5464493e-6b4c-4bd1-a20d-c34ebfef0e9d
📒 Files selected for processing (24)
packages/query/src/graphql/__tests__/benchmark.test.tspackages/query/src/graphql/__tests__/integration.test.tspackages/query/src/graphql/__tests__/middleware.spec.tspackages/query/src/graphql/graphql.historical.test.tspackages/query/src/graphql/graphql.test.tspackages/query/src/graphql/limit.test.tspackages/query/src/graphql/plugins/__tests__/GetMetadataPlugin.indexer.spec.tspackages/query/src/graphql/plugins/__tests__/GetMetadataPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgAggregatesHistoricalPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgAggregationPlugin.aggregateOff.spec.tspackages/query/src/graphql/plugins/__tests__/PgAggregationPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgBackwardRelationPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgBlockHeightPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgConnectionFilterBlockHeightPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgConnectionFirstLastClampPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgDistinctPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgOrderByUniquePlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgSearchPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgSmartTagsPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/PgSubscriptionPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/QueryAliasLimitPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/QueryComplexityPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/QueryDepthLimitPlugin.spec.tspackages/query/src/graphql/plugins/__tests__/graphql.module.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/query/src/graphql/plugins/tests/graphql.module.spec.ts
Comment on lines
+231
to
+267
| it('treats composite FK with unique on subset as singular (v5 behavior)', async () => { | ||
| // v5's logic: if ANY unique constraint's columns are a subset of | ||
| // the FK columns, the relation is considered unique. | ||
| // This is correct: if one FK column is unique, each parent row | ||
| // maps to ≤1 child row. | ||
| await pool.query(` | ||
| CREATE TABLE IF NOT EXISTS "${dbSchema}".order_items ( | ||
| order_id text NOT NULL, | ||
| product_id text NOT NULL, | ||
| quantity integer NOT NULL, | ||
| CONSTRAINT order_items_pkey PRIMARY KEY (order_id, product_id), | ||
| CONSTRAINT order_items_product_id_key UNIQUE (product_id) | ||
| ) | ||
| `); | ||
|
|
||
| // Rebuild schema with new table | ||
| const result = await runQuery(` | ||
| { | ||
| __type(name: "OrderItem") { | ||
| fields { | ||
| name | ||
| type { | ||
| name | ||
| kind | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `); | ||
|
|
||
| expect(result.errors).toBeUndefined(); | ||
| // No backward relations on OrderItem since it has no FKs pointing to other tables | ||
| // This just validates schema builds with composite PK + unique subset | ||
| expect(result.data?.__type?.fields?.length).toBeGreaterThan(0); | ||
|
|
||
| await pool.query(`DROP TABLE IF EXISTS "${dbSchema}".order_items`); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test docstring doesn't match what's actually verified.
The comment states v5 treats a composite FK as singular when a unique constraint's columns are a subset of the FK's columns, but order_items here has no FK referencing another table at all — the test only confirms the schema builds and OrderItem has fields, not that any backward relation is singular. This gives false confidence that the "unique subset → singular" claim is covered.
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 235-243: Avoid SQL injection
Context: pool.query(CREATE TABLE IF NOT EXISTS "${dbSchema}".order_items ( order_id text NOT NULL, product_id text NOT NULL, quantity integer NOT NULL, CONSTRAINT order_items_pkey PRIMARY KEY (order_id, product_id), CONSTRAINT order_items_product_id_key UNIQUE (product_id) ))
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-typescript)
[error] 265-265: Avoid SQL injection
Context: pool.query(DROP TABLE IF EXISTS "${dbSchema}".order_items)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-typescript)
🪛 OpenGrep (1.23.0)
[ERROR] 236-244: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
[ERROR] 266-266: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/query/src/graphql/plugins/__tests__/PgBackwardRelationPlugin.spec.ts`
around lines 231 - 267, The test in PgBackwardRelationPlugin.spec.ts is
asserting the wrong behavior for the current setup: order_items has no foreign
key, so it cannot verify the “unique subset makes backward relation singular”
case. Update the test to either add a real composite FK and assert the backward
relation’s singular/plural type in the __type introspection, or rewrite the test
name/comment so it only covers schema construction for a composite PK plus
unique subset. Use the existing runQuery and OrderItem introspection path to
validate the actual relation behavior being described.
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
1 participant
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.
Description
Summary
This PR migrates the
@subql/queryservice from Postgraphile v4 to Postgraphile v5 (crystal). The migration replaces the entire HTTP/GraphQL serving layer, plugin system, and dependency tree while preserving all existing functionality which include historical queries, subscriptions, aggregate ordering, search sanitization, query limits, and more.Key changes:
builder.hook()to v5GraphileConfig.Pluginwithschema.hooks@subql/x-postgraphile-core,@subql/x-graphile-build-pg) with upstream v5res.writeHeadpatch), error boundary (4-arg Express handler)_block_range @>injection (v4 bug fixed)Fixes #1982
Why
@subql/x-postgraphile-core,@subql/x-graphile-build-pg,subql-graphile-engine) that existed only to patch v4 bugs.Type of change
Please delete options that are not relevant.
Checklist
Architectural Changes (v4 → v5)
execute()builder.hook()+addArgDataGeneratorGraphileConfig.Pluginwithschema.hooksgetPostGraphileBuilder(pool, schemas, opts).buildSchema()postgraphile(preset)LISTEN/NOTIFY+schemaListener()monkey-patching Apollo internalsgrafserv.watch: true→ v5 built-inwatchSchema()WebSocketServer+graphql-ws+PgPubSubgrafserv.addTo(app, httpServer, true)handles nativelysmartTagsPlugin.ts(imperative hooks)schema.pgSmartTags(declarative config in preset)PlaygroundPlugin.ts(custom HTML)@pgSubscriptiondirective +PgPubSubenginelisten()step withsubscribePlanpgQuerycallbacks + SQL injection viaaddArgDataGeneratorWeakMapbridgePgOrderByAggregatesPluginwith inline blockHeight logicPgAggregatesPreset+PgAggregatesHistoricalPluginwithblockHeightStepMapbridgepostgraphile-plugin-connection-filternatively +PgConnectionFilterBlockHeightPluginProxy (134 lines)debugClient.tsmonkey-patching PG clientpreset.grafast = {explain: true}PgRowByVirtualIdPlugin.ts(code)pgSmartTags(config)cors()middleware (auto)corsMiddleware(manual Express handler, restores OPTIONS preflight)CacheControlExtension(auto)cacheControlMiddleware(patchesres.writeHead, passes through v4 behavior)errorBoundaryMiddleware(4-arg Express handler, catches middleware throws)Dependency Changes
Removed
postgraphile@^4.13.0@subql/x-postgraphile-core— custom fork, no longer needed@subql/x-graphile-build-pg— custom fork, no longer neededgraphile-build,graphile-utils— replaced by v5 subpath importspg-sql2— now internal to postgraphileapollo-server-express,apollo-server-core,apollo-server-plugin-basegraphql-ws,ws@graphile/pg-pubsub@subql/x-postgraphile-coretypespatch-package(was unused)Added
postgraphile@^5.0.3grafast@^1.0.2(plan executor)graphile-build@^5.0.0(peer dep for community presets)Replaced
@graphile-contrib/pg-simplify-inflector@graphile/simplify-inflection@graphile/pg-aggregatesPgAggregatesPresetpostgraphile-plugin-connection-filter^3.0.0Custom Plugin Rewrites
All 13 custom plugins were migrated from v4 to v5 format. None lost functionality:
builder.hook+addArgDataGeneratorGraphQLObjectType_fields_field+applyPlan+ ALSPgConnectionArgFirstLastBeforeAfter(added args + clamped)builder.hook+newWithHooksregisterEnumType+applyPlanbuilder.hook+addArgDataGeneratorGraphQLObjectType_fields_fieldplan wrapperbuilder.hookwrappingresolve().eval()+ try/catchmakeExtendSchemaPlugin+@pgSubscriptionextendSchema+ Grafastlisten()stepmakeExtendSchemaPlugin+ resolversextendSchema+ Grafast plansPgOrderByAggregatesPluginblockHeightStepMapbridgeschema.pgSmartTagsDeleted (not needed)
PgRowByVirtualIdPlugin— replaced by v5 nativetableByRowId+pgSmartTagsPlaygroundPlugin— replaced by grafserv built-in GraphiQLPgBackwardRelationPlugin— v5 upstream handles unique FK detectionPgConnectionArgFirstLastBeforeAfter— v5 adds pagination args nativelyPgAggregateSpecsPlugin— upstreamPgAggregatesPresethandles specsPgAggregationPlugin— upstreamPgAggregatesPresethandles aggregationPgOrderByAggregatesPlugin— replaced byPgAggregatesHistoricalPluginDeleted directory
packages/query/src/configure/x-postgraphile/— v4-specific debug/explain clientNew Files Created
plugins/PgAggregatesHistoricalPlugin.ts— Aggregate orderBy with blockHeight in subqueriesplugins/PgConnectionFirstLastClampPlugin.ts— Clamps first/last to query limitplugins/historical/PgConnectionFilterBlockHeightPlugin.ts— Injects blockHeight into relation filter EXISTS subqueries via Proxyplugins/historical/requestContext.ts— AsyncLocalStorage for blockHeight propagationplugins/__tests__/testHelpers.ts— SharedcreateTestContext(dbSchema)for all plugin testspostgraphile.d.ts— Type declarations for v5 subpath exports (workaround formoduleResolution: "node")docs/architecture-v5.md— Architecture documentationgraphql/__tests__/integration.test.ts— Full HTTP integration test (15 tests: 10 HTTP + 5 SQL snapshot). Starts real Express + grafserv, tests CORS, Cache-Control, error boundary, historical blockHeight, nested relations, default MAXgraphql/__tests__/middleware.spec.ts— Unit tests for limit/complexity/depth/alias middleware (35 tests, restored from v4 pattern)Other Changes
packages/query/package.json— Removed all v4 deps, added v5 deps, removed unusedpatch-packagepackages/query/src/yargs.ts— Added--order-by-nulls-lastflag for PgOrderByUniquepackages/query/src/configure/configure.module.ts— RemoveddebugPgClientimport, removed old explain wiringpackages/query/src/graphql/graphql.module.ts— Complete rewrite from ApolloServer + WebSocket + schemaListener to grafserv + Express middleware. Restores 3 features not natively provided by grafserv:corsMiddleware(OPTIONS preflight + permissive headers),cacheControlMiddleware(Cache-Control header pass-through viares.writeHeadpatch),errorBoundaryMiddleware(4-arg Express error handler catching middleware throws)Summary by CodeRabbit
ORDER BYviaorder-by-nulls-last.first/lastto the configuredquery-limit.