fix: improve scope building performance - #1312
Conversation
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
There was a problem hiding this comment.
Pull request overview
Improves PostgreSQL request-scope statement construction by reducing per-call allocations and reusing prebuilt SQL fragments, while updating call sites and tests to the new API.
Changes:
- Refactors
buildScopeStatementto take connection options + payload strings directly and select from prebuilt SQL strings. - Adds
ScopeConnectionOptions(derived fromTenantConnectionOptions) and removes the priorScopeobject shape from this module’s public surface. - Updates
PgTenantConnection.setScopeand expands test coverage for placeholder ordering (incl. timeout-only case).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/internal/database/postgres/scope.ts | Prebuilds scope SQL variants at module load and refactors scope statement builder to avoid dynamic setter list construction. |
| src/internal/database/postgres/scope.test.ts | Updates tests to new buildScopeStatement signature and adds coverage for timeout-only placeholder behavior. |
| src/internal/database/pg-connection.ts | Updates setScope to call the refactored buildScopeStatement API without constructing an intermediate object. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward performance refactor with no behavior change.
Extended reasoning...
Overview
This PR refactors buildScopeStatement in src/internal/database/postgres/scope.ts to avoid allocating an intermediate options object on every call and to avoid rebuilding the SQL setter array/string each invocation. The four possible SQL text variants (no optional settings, timeout only, search path only, both) are now precomputed as module-level constants, and the function takes positional arguments (including the request-scoped options object directly) instead of spreading fields into a fresh object. The caller in pg-connection.ts is updated to match the new signature, passing this.options directly rather than constructing a new scope object.
Security risks
None identified. This is a pure refactor of internal SQL-building logic; all values are still passed as bound parameters ($1..$11), not interpolated into the SQL text, so there is no injection risk introduced. The set of settings applied (role, jwt claims, headers, method, path, operation, timeout, search path) is unchanged.
Level of scrutiny
Low-to-medium. This touches a function used on every tenant connection scope-setting query, but the change is mechanical: same values, same parameter ordering, same conditional SQL branches, just restructured to avoid redundant allocation/string-building work. I traced the value ordering through all four SQL variants and confirmed placeholder indices ($10/$11) match the values array in each case, and verified the new ScopeConnectionOptions type (a Pick of TenantConnectionOptions) matches how options.user.jwt, options.user.payload.sub, options.method, options.path, and options.operation?.() are used, matching the pre-refactor field access semantics exactly.
Other factors
Test coverage was updated in scope.test.ts to match the new signature and adds a new case (timeout-only, no search path) that wasn't previously covered. No outstanding review comments exist on this PR.
Coverage Report for CI Build 31391575575Coverage increased (+0.1%) to 80.864%Details
Uncovered Changes
Coverage Regressions2232 previously-covered lines in 113 files lost coverage.
Coverage Stats
💛 - Coveralls |
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Scope building allocates an intermediate object and sql is rebuilt each time.
What is the new behavior?
Pass values directly, no copy, and initialize sql module level once.
Additional context
Related to #1309