Skip to content

refactor: move the relation query layer out of the adapter - #38

Merged
fratzinger merged 2 commits into
mainfrom
refactor/qualify-column
Sep 2, 2026
Merged

fratzinger merged 2 commits into
mainfrom
refactor/qualify-column

Conversation

@fratzinger

Copy link
Copy Markdown
Owner

adapter.ts was 2648 lines, and half of it was relation resolution: 25 methods that call each other constantly and reach outside for only four things — options, app, propertyMap and getPropertyType. That is a clean seam, so it moved. adapter.ts is now 1481 lines.

Pure refactor. No behaviour change, no public API change.

The interface got smaller than what it replaced

Composing a query used to mean three calls in a fixed order with a Map threaded between them:

const { q, query, sortRefs } = this.applyJoins(q, params, { order })
q = this.applyWhere(q, query)
q = this.applySort(q, filters, sortRefs)

Two calls now, order-independent, with normalization and the sort references hidden behind them:

q = relations.applyWhere(q, params.query)
q = relations.applyOrder(q, filters.$sort)

Proof that it is a pure move

All 20 SQL-shape snapshots are unchanged, so the generated SQL is byte-identical. That is what those snapshots were added for — without them, 1100 moved lines could only be backed by "tests are green".

The seam pays for itself

Related services are reached through an injected lookupService instead of the Feathers app:

export type RelationQueryContext = {
  tableName: string
  idField: string
  dialectType: DialectType | undefined
  relations?: Record<string, Relation>
  isOwnColumn: (column: string) => boolean
  getPropertyType: (property: string) => string | undefined
  lookupService: (name: string) => RelatedService | undefined
}

So a multi-hop chain can be exercised with a plain object — no registered services, no app.setup(), no schema, no data. test/relation-query.test.ts drives the module that way, which is how the uniqueness and fallback rules are covered now:

compileOrder({ 'assignment.title': 1 }, { lookupService: () => undefined })
// → group by "assignment"."id"  — without proof of uniqueness, the safe path

The same case needed a dedicated test file with three app variants in #37.

A latent inconsistency fixed on the way

adapter-commons merges params.adapter over the options:

getOptions(params) { return { ...this.options, paginate, ...params.adapter } }

filterQuery honoured that; the relation layer read this.options directly, so a per-call relations or name override was silently ignored there. The context is now built per call, which fixes it by construction. applyWhere takes an optional params for the same reason.

Commits

  1. refactor: extract column qualification into a pure util — col() decided three things at once (an explicit table wins; a declared column gets the service's own table; anything else is left alone because it may already be a qualified ref or a JSON path) and read propertyMap/options.name off the instance, so the rules were only reachable through an adapter. qualifyColumn takes them as parameters, col() stays as the bound convenience for the 21 call sites, and the rules now have in-source tests — precedence, the null "already qualified" case, the empty-string fallback, no double qualification, array mapping, non-string pass-through. Both sides of the split need these rules and neither should own them.
  2. refactor: move the relation query layer behind its own interface — the move itself.

What did not move, and why

  • Upsert (535 lines) reaches into nine adapter members including _create, executeAndReturn and filterQuery. Extracting it means passing the adapter in: a large interface in front of a small implementation, which is the wrong direction. It is already split into named private methods, which is most of the benefit.
  • RelationQuery is not exported from index.ts. It is implementation, not public API; the tests import it by path.

Testing

692 tests pass against sqlite, 815 against postgres, typecheck clean, no new lint warnings. The benchmark and the EXPLAIN cost report run unchanged.

🤖 Generated with Claude Code

Frederik Schmatz and others added 2 commits September 2, 2026 11:05
`col()` decided three things at once — an explicit table wins, a declared
column gets the service's own table, anything else is left alone because it
may already be a qualified ref or a JSON path — and it read `propertyMap`
and `options.name` off the instance, so the rules were only reachable
through an adapter.

`qualifyColumn` takes them as parameters instead. `col()` stays as the
bound convenience the 21 call sites use, and the rules are now covered by
in-source tests: precedence, the `null` "already qualified" case, the
empty-string fallback, no double qualification, array mapping and
non-string pass-through.

Also a prerequisite for moving the relation layer out: both sides need
these rules, and neither should own them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Half of adapter.ts was relation resolution: 25 methods that call each
other constantly and reach outside for only four things — `options`, `app`,
`propertyMap` and `getPropertyType`. It now lives in `RelationQuery` with
those four passed in, and the file drops from 2648 to 1481 lines.

The interface got smaller than what it replaced. Composing a query used to
mean three calls in a fixed order with a Map threaded between them:

  const { q, query, sortRefs } = this.applyJoins(q, params, { order })
  q = this.applyWhere(q, query)
  q = this.applySort(q, filters, sortRefs)

It is now two calls in any order, with normalization and the sort
references hidden:

  q = relations.applyWhere(q, params.query)
  q = relations.applyOrder(q, filters.$sort)

Related services are reached through an injected `lookupService` rather
than the Feathers app, so a multi-hop chain can be exercised without
registering services, running `app.setup()`, or creating a schema —
`test/relation-query.test.ts` drives the module that way, which is how the
uniqueness and fallback rules are covered now.

Building the context per call also fixes a latent inconsistency:
adapter-commons merges `params.adapter` over the options, which
`filterQuery` honoured while the relation layer read `this.options`
directly, silently ignoring a per-call `relations` or `name` override.

Pure move otherwise — all 20 SQL-shape snapshots are unchanged, so the
generated SQL is byte-identical.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
feathers-kysely db5b0fa Commit Preview URL

Branch Preview URL
Sep 02 2026, 09:19 AM

@pkg-pr-new

pkg-pr-new Bot commented Sep 2, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@fratzinger/feathers-kysely@38

commit: db5b0fa

@fratzinger
fratzinger merged commit 852ef37 into main Sep 2, 2026
37 checks passed
@fratzinger
fratzinger deleted the refactor/qualify-column branch September 2, 2026 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant