refactor: unify adapter application into execute(query, target) - #754
Merged
Conversation
The adapter's `QUERY` generic named the backend object it mutates (e.g. a
TypeORM `SelectQueryBuilder`), not the rapiq `IQuery` AST — so `withQuery()`
read as attaching a rapiq query when it attached the target. Fold the whole
apply sequence into one entry point and rename the concept to `target`.
Before:
adapter.withQuery(queryBuilder);
query.accept(new QueryVisitor(adapter));
const out = adapter.execute();
After:
const out = adapter.execute(query, queryBuilder);
- @rapiq/sql: split `IAdapter` into `ISubAdapter` (per-parameter contract:
`setTarget`/`execute`/`clear`) and `IRootAdapter<TARGET, OUTPUT>` whose
`execute(query, target?, options?)` walks the query and returns the result.
`Adapter.execute(query)` returns `SqlFragments`; `build()` is removed.
- @rapiq/typeorm: `TypeormAdapter.execute(query, queryBuilder)` walks and
applies in a single call, returning the applied pagination.
- The `QUERY` type parameter is renamed `TARGET`; the internal `withQuery`
setter becomes `setTarget`.
- Add `ExecuteOptions { clear?: boolean (default true), visitor? }`; execute
now clears accumulated state by default (re-runnable) and forwards
`visitor` options through `QueryVisitor` to the sub-visitors.
Docs and tests updated to the new entry point.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR refactors ChangesTarget-based execute refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TypeormAdapter
participant QueryVisitor
participant SelectQueryBuilder
Caller->>TypeormAdapter: execute(query, options)
TypeormAdapter->>TypeormAdapter: clear state when options.clear
TypeormAdapter->>QueryVisitor: visit query with visitor options
QueryVisitor->>SelectQueryBuilder: apply fields, filters, pagination, relations, and sort
TypeormAdapter-->>Caller: return pagination
Possibly related PRs
🚥 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 |
The migrated clear test asserted idempotency (execute twice, same query -> equal), which cannot catch a broken sort/pagination/relations clear() because those sub-adapters overwrite/dedupe and reproduce the same output on a re-walk. Add a test that a second execute() dropping those parameters returns empty orderBy/limit/offset/relations, restoring the guarantee the pre-refactor 'reset on clear' test provided. Also assert column doubling in the clear:false accumulation test.
tada5hi
force-pushed
the
refactor-adapter-execute-api
branch
from
July 8, 2026 16:27
96fe2ab to
5ba7838
Compare
The package READMEs (published to npm) and the root README still showed the old withQuery() + external QueryVisitor walk + no-arg execute()/build() flow, whose code samples no longer compile. Update them (and the living .agents docs architecture.md, references/typeorm.md, migration-notes.md) to the single execute(query, target) entry point. Historical .agents/plans/* records are left as-is.
The adapter accumulates per-call state, so it is not a long-lived singleton: the shareable unit is the options object, and the instance is constructed per request (like the SelectQueryBuilder it is handed). Note this in the typeorm/sql integration guides and package READMEs to preempt the reuse-vs-single-use question.
…ute()
execute(query, target?, options?) forced a target param that @rapiq/sql
ignored entirely (it produces fragments). Move the write target into the
constructor options behind a shared BaseAdapterOptions<TARGET> { target? }
that both AdapterOptions (sql) and TypeormAdapterOptions extend, so execute is
now the uniform execute(query, options?) on both adapters.
- target is bound once at construction and fanned out to the sub-adapters;
execute() no longer takes or re-binds it.
- target stays optional: @rapiq/sql ignores it, @rapiq/typeorm keeps its
no-target -> postgres-preset fallback.
- per-request construction remains the model; share the config object and
spread it with the request's builder: new TypeormAdapter({ ...config, target }).
Tests, integration docs and READMEs updated to the constructor-bound target.
The TARGET generic threaded through the whole adapter layer, but it was dead in
@rapiq/sql (which never touches a target — it emits fragments) and only earned
its keep typing TypeORM's builder. It also lived in every interface purely
because the old setTarget() signature mentioned it.
Stance B: model the write target as a TypeORM-only concern.
- @rapiq/sql: no target, no setTarget, no TARGET generic, no BaseAdapterOptions.
ISubAdapter { execute; clear } (non-generic), IRootAdapter<OUTPUT>,
execute(query, options?), AdapterOptions = DialectOptions & { rootAlias? }.
The base classes are pure accumulation machinery.
- @rapiq/typeorm: each sub-adapter declares its own
protected target?: SelectQueryBuilder<any> (fixed type, no generic) set from a
constructor param; the root threads options.target into the five sub-adapter
constructors. TypeormAdapterOptions { target?, relations? }. No generic.
The SelectQueryBuilder<User> entity-narrowing was ceremony; fixed <any> keeps
full builder-API type-checking inside TypeORM (incl. relations.ts expressionMap
navigation). Docs/agent guides updated; acceptance spec drops the generic arg.
Now that the write target is a TypeORM-local concept (@rapiq/sql has none),
name it for what it is. TypeormAdapterOptions.target -> queryBuilder, and the
per-adapter this.target field + onJoin's third parameter follow. Usage becomes
new TypeormAdapter({ queryBuilder, relations }).execute(query).
Tests, integration docs and READMEs updated to the shorthand form.
…e/skip PaginationAdapter.execute() only applied take/skip behind if(this.limit)/ if(this.offset) guards, so a second execute() (default clear:true) whose query drops pagination left the previous run's take/skip on the builder — wrong LIMIT/OFFSET, contradicting the clear-is-re-runnable contract that sort/filters already honor. Apply take/skip unconditionally (falsy -> undefined = no clause), matching SortAdapter. Add a re-run regression test. Also: align PaginationAdapter's constructor signature with the other four sub-adapters, and drop the stale 'target bound at construction' JSDoc on the @rapiq/sql Adapter.execute() (sql has no target).
'an adapter instance is re-runnable' overpromised: clear resets the adapter's
own accumulated state, but the bound queryBuilder is mutated in place (joins and
the selected projection do not roll back). Reword to point at per-query
construction and reserve { clear: false } for accumulating onto one builder.
An adapter constructed without a builder was permanently a silent no-op (every sub-adapter guarded on the builder's presence). Now that the builder can only be bound via the constructor, make it required: the option loses its '?', the sub-adapters drop their undefined guards, and resolveQueryDialect no longer models the unbound case.
![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
🤖 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/typeorm/src/adapter/filters.ts`:
- Around line 59-63: In execute(), guard the result of getQueryAndParameters()
before calling queryBuilder.where(): when no active filters produce an empty SQL
string, skip where() or return undefined so cleared reruns do not emit invalid
SQL.
🪄 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: 75068b1c-1bcf-49d0-9302-9154d5edbb71
📒 Files selected for processing (12)
packages/docs/integrations/typeorm.mdpackages/sql/src/adapter/module.tspackages/typeorm/src/adapter/fields.tspackages/typeorm/src/adapter/filters.tspackages/typeorm/src/adapter/module.tspackages/typeorm/src/adapter/pagination.tspackages/typeorm/src/adapter/relations.tspackages/typeorm/src/adapter/sort.tspackages/typeorm/src/adapter/types.tspackages/typeorm/src/dialect.tspackages/typeorm/test/unit/adapter/filters.spec.tspackages/typeorm/test/unit/adapter/module.spec.ts
💤 Files with no reviewable changes (1)
- packages/typeorm/test/unit/adapter/filters.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/typeorm/src/adapter/types.ts
- packages/docs/integrations/typeorm.md
- packages/typeorm/src/adapter/module.ts
- packages/sql/src/adapter/module.ts
typeorm's where() clears expressionMap.wheres before adding a
condition and skips falsy ones, so the adapter's unconditional
where('', []) call is what resets a stale WHERE on re-run — and it
emits no dangling clause. Guarding it on non-empty sql (as suggested
in review) would leak the previous run's WHERE.
queryBuilder is required at construction now, so the only remaining pg-fallback case is an unknown connection type.
This was referenced Jul 14, 2026
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
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.
Motivation
The SQL/TypeORM adapters carried a
QUERYgeneric and awithQuery(query)method — but that "query" was not the rapiqIQueryAST. It was the backend object the adapter mutates (a TypeORMSelectQueryBuilder). So the usage site read confusingly, with "query" meaning two opposite things two lines apart:On top of the naming collision, applying a query was a three-step ritual with invisible ordering constraints, and
QueryVisitorleaked into every usage site.Change
Fold the whole apply sequence into a single entry point; the backend object becomes a TypeORM-local constructor option:
@rapiq/sqlIAdaptersplit intoISubAdapter(per-parameter contract:execute/clear) andIRootAdapter<OUTPUT>.IRootAdapter.execute(query, options?)walks the query (constructs theQueryVisitorinternally — no runtime cycle, the visitor's adapter imports are type-only) and returns the result.QueryVisitoris an implementation detail now;options.visitorremains as the escape hatch.Adapter.execute(query)returnsSqlFragments;build()is removed (it was the only way to read fragments before;executenow returns them).QUERY/TARGETgeneric is dropped entirely — plain SQL has no backend object to mutate; it emits fragments. The oldAdapter<QUERY extends Record<string, any>>was dead weight.QueryVisitorforwards its options to the sub-visitors (previously stored but never forwarded).@rapiq/typeormSelectQueryBuilderis bound at construction:new TypeormAdapter({ queryBuilder }).queryBuilderis required — previously an adapter without a builder compiled fine and everyexecute()was a silent no-op; the sub-adapters lost all theirif (!this.queryBuilder)guards along with it.TypeormAdapter.execute(query)walks + applies in one call, returning the applied pagination (e.g. for the responsemetablock).take/skipinstead of leaking the previous run's values.clearsemantics are scoped to single-use-per-builder (builder-side mutations can't be undone byclear()).Both
ExecuteOptions { clear?: boolean (default true), visitor? }.executeclears accumulated state by default (self-contained calls);{ clear: false }accumulates across calls (apply several queries onto one target).visitorforwards to theQueryVisitor/sub-visitors.adapter.withQuery(target)queryBuilderin constructor options; SQL: no target concept)query.accept(new QueryVisitor(adapter))+adapter.execute()adapter.execute(query, options?)Adapter.build()Adapter.execute(query)returnsSqlFragmentsIAdapter<QUERY>ISubAdapter(execute/clear) /IRootAdapter<OUTPUT>QUERYon adapters/sub-adaptersnew TypeormAdapter()(builder optional, attached later)new TypeormAdapter({ queryBuilder })(required)resolveQueryDialect(query?)resolveQueryDialect(query)(unbound case no longer exists)Committed as
refactor:(notfeat!/BREAKING CHANGE:) so release-please does not auto-bump a major — the version decision is intentionally left to the maintainer.Tests & docs
@rapiq/sql69/69,@rapiq/typeorm45/45 passing; both build clean; lint clean.cleardefault (idempotent re-run) and{ clear: false }accumulation.execute(new Query({ ... }))on a constructor-bound adapter (full path) or the public sub-adapters (adapter.filters, …) for isolated cases; the "no bound builder falls back to pg" spec is gone with the scenario it covered.packages/docs(sql, typeorm, quick-start, landing-page snippet) and READMEs.Notes / follow-ups
RootAliasFn+toRootAliasFn/isRootAliasFn(@rapiq/sql) are exported but unused anywhere in the monorepo — they look like dead code worth removing in a follow-up.Summary by CodeRabbit
execute(query, options)now drives query application for both SQL fragments and TypeORM, with consistent per-call clearing behavior.queryBuilderat construction and returns applied pagination fromexecute(query).execute(query)pattern.