Skip to content

refactor(providers): hoist assertObjectPathShape into object-kinds - #985

Merged
cevheri merged 1 commit into
libredb:mainfrom
KodYazicam:refactor/hoist-assert-object-path-shape
Sep 20, 2026
Merged

cevheri merged 1 commit into
libredb:mainfrom
KodYazicam:refactor/hoist-assert-object-path-shape

Conversation

@KodYazicam

@KodYazicam KodYazicam commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

What changed

One exported assertObjectPathShape in src/lib/db/object-kinds.ts, beside containerDepth, imported by all nine provider files. grep -rn "function assertObjectPathShape" src/ finds exactly one, and no provider test changed its expectations.

The signature order I picked, and why

The issue asked to choose between (capabilities, kind, path) and MongoDB's (capabilities, path, kind). Measured in the tree, the split was different: seven of the nine copies take spec (PostgreSQL, MySQL, Oracle, SQLite, libSQL, Cassandra — and MySQL's second form), and only ClickHouse, Redis and MongoDB do not. I kept the majority's (capabilities, spec, kind, path) order and added an engine descriptor, so the odd MongoDB caller — which had already resolved spec via findKind at both call sites — simply passes it.

spec is required: every caller resolves the kind before the call (findKind, requireSourceKind or requireEditableKind), so this function is not the kind-existence check — refusing an undeclared kind stays the caller's job, in the caller's own words. kind — not spec.id — stays in the message because that is what seven of the nine copies printed.

The copies had drifted in more than the signature

The interesting part turned out to be bigger than the parameter order. Three behaviours, not one:

  • attached required (PostgreSQL, SQLite, libSQL, Cassandra): an attached kind must be addressed through its parent, so the error names one shape;
  • attached optional (MySQL, Oracle): objectPath() collapses a parentless trigger onto the container-level address (standing ruling 5f), so the error names both shapes joined with " or ";
  • no attached kinds (ClickHouse, Redis, MongoDB): the shape is always the declared levels plus the name.

The messages also differ per engine — "A PostgreSQL" vs "An Oracle", each stamped with its own engine code. Rather than quietly picking one behaviour, the hoisted function takes an ObjectPathShapeEngine descriptor (code, label, attachedSegment), and every thrown error is byte-identical to what each provider printed before. MySQL's private objectPathShapes is subsumed into the shared shapes computation and removed.

Ripples the citation tests and review caught

  • The docblock line citations in src/lib/api/object-route.ts are re-pinned to the moved lines (its own test went red until they were).
  • Per the review, the same class was fixed everywhere the nine removals and two import lines moved it: connection-fingerprint.ts, five test files, BACKLOG.md, AGENT_ANALYST_DESIGN.md and agent/tools.ts — each located by anchor content (what the cited lines hold on main, found in this tree) rather than by arithmetic, which also caught the +2 shift above each removed block.
  • Redis no longer re-looks the kind up inside the assert; it passes the spec each site had already resolved.

Validation

  • bun run test562 files, 18542 tests: 18532 pass, 10 skip (the skips are the zip-binary/CLI ones, absent in this environment)
  • bun run typecheck — clean
  • bun run knip — clean (exit 0)
  • bun run format — no fixes to apply
  • bun run lint — 0 errors (the repo's pre-existing warnings are untouched)
  • bun tests/run-tests.ts --coverage --merge-into=coverage/lcov.info && node scripts/check-coverage.mjs coverage/lcov.info100.00%, 59142/59142 lines, so the shared function's branches are all exercised by the existing suite

The remaining three engines carrying the same logic inlined (couchbase, embedded LibreDB, druid) are tracked in #1013.

Closes #978.

@codecov

codecov Bot commented Sep 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cevheri cevheri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the substance rather than the diff shape: I hoisted-vs-original compared all nine copies, then mutation-tested the descriptor. Flipping attachedSegment turns Postgres, MySQL, Oracle, SQLite, libSQL and Cassandra red, and corrupting label turns eight of the nine red, so the policy and the sentence are both pinned by the existing suite. spec.id to kind on Oracle and Cassandra is safe: both specs come from an exact-id findKind/requireSourceKind. ClickHouse, Redis and MongoDB declare no attachedTo, so passing spec there is inert today. The behaviour claim holds.

One change before merge. Removing 6 to 25 lines from nine files moves every line citation below the removed block. You re-pinned the three in object-route.ts because its citation test went red; the same class is now wrong in unguarded places. Measured as "correct on main, off by exactly this PR's per-file delta":

  • src/lib/db/connection-fingerprint.ts:67,69,70,79: postgres.ts:2095-2099 to 2070-2074, mysql.ts:1973-1976 to 1948-1951, oracle.ts:1545-1546 to 1521-1522, sqlite.ts:1189-1192 to 1175-1178, mongodb.ts:800-801 to 794-795,
    oracle.ts:1551-1560 to 1527-1536
  • 12 in tests: unit/lib/db/connection-fingerprint.test.ts:63,171,226,227, api/db/objects/edit-apply.test.ts:91, components/monitoring/PerformanceTab.test.tsx:197,237,238, components/monitoring/QueriesTab.test.tsx:202,
    integration/db/oracle-provider.test.ts:2873, integration/db/postgres-provider.test.ts:3996
  • 7 in docs/BACKLOG.md: lines 328, 331, 350, 381 (two), 384, 1133

The roughly 140 others are under docs/superpowers/works/; those record the tree as it was, so leave them.

Two nits, neither blocking. Redis calls findKind again at all four sites where requireSourceKind/requireEditableKind already returned the spec, which is the one thing the new docblock says callers do not do. And each provider now carries a second engine identity: Redis has REDIS_ENGINE { displayName, type } and PATH_SHAPE_ENGINE { code, label: "A Redis" } side by side, so label can drift from displayName; deriving it from the existing descriptor plus an article would close that.

Separately, and not for this PR: couchbase/objects.ts:459, embedded/libredb.ts:980 and druid/objects.ts:595 still hold copies of the same function, so #978's class is not fully closed. Worth a follow-up issue.

Nine provider-local copies of the path-shape assert become one exported
definition beside containerDepth, imported everywhere. The copies had
drifted further than their signatures: MySQL and Oracle admit the bare
shape for an attached kind while the SQL engines require the attached
segment, so the hoisted function takes an engine descriptor carrying the
provider code, the message subject ("A PostgreSQL", "An Oracle") and
that one policy, which keeps every thrown error byte-identical.

The majority's (capabilities, spec, kind, path) order is kept and the odd
MongoDB caller, which had already resolved the spec at both call sites,
now passes it. Every caller resolves the kind before this call, so spec
is required and the function is not the kind-existence check. mysql's
private objectPathShapes is subsumed and removed.

Every line citation the nine removals and two import lines moved is
re-pinned across object-route.ts, connection-fingerprint.ts,
AGENT_ANALYST_DESIGN.md, agent/tools.ts, BACKLOG.md and five test files,
located by anchor content rather than by arithmetic.

Closes libredb#978.
@KodYazicam

Copy link
Copy Markdown
Contributor Author

Thank you for the depth of the review — the mutation testing of the descriptor is exactly the check I could not run on my own framing.

The blocking change is done. Every citation the nine removals and the two import lines moved is re-pinned, located by anchor content rather than by arithmetic: a script resolves each citation's anchor (the content at the cited lines on main), finds that content in this tree, and rewrites the citation. The six in connection-fingerprint.ts land exactly on the values you gave (2070-2074, 1948-1951, 1521-1522, 1175-1178, 794-795, 1527-1536), as do the twelve in the tests and the seven in BACKLOG.md. Two classes beyond the enumeration, same mechanism:

  • the two import lines each provider gained shift every citation above the removed block by +2, so docs/AGENT_ANALYST_DESIGN.md, src/lib/agent/tools.ts and four more BACKLOG rows were stale in the other direction;
  • after rebasing onto current main (postgres.ts gained lines from fix(postgres): list objects on an engine whose pg_class has no reltuples #993 since the review), values are computed against the tree as it is now, which is why object-route.ts's own three needed a second pass after the rebase.

The ~140 under docs/superpowers/works/ are left untouched as you said, and one pre-existing mismatch is left alone deliberately: object-kinds.ts's own citation of mongodb.ts:170 points at a blank line on main (supportsInlineRowEdit: false lives at mongodb.ts:631 there), so it was already rotted before this PR and is not this PR's delta.

Nit 1, taken further than asked. Redis no longer calls findKind inside the assert at any of the four sites: describeObject binds the spec it was already checking for, readObjectSource and buildObjectEdit pass the spec they had already resolved, and the fourth site captures the requireEditableKind return it was discarding. With every caller now resolving the kind before the call, spec is required in the signature and the docblock no longer claims anything a caller does not do — the function is explicitly not the kind-existence check, and refusing an undeclared kind stays the caller's job.

Nit 2, deliberately not taken here. Deriving label from the existing displayName needs an article rule whose only irregular case in the set is "An Oracle" ("A libSQL" and "A SQLite" are consonant-initial), so the rule would be one line with one exception. I would rather keep the labels as literals the mutation-tested suite pins than add that rule in the same change as the citation sweep, but happy to do it as a follow-up if you prefer the derivation.

The follow-up is filed: #1013, for the couchbase, embedded-LibreDB and druid copies.

Re-validated on the rebased tree: bun run test — 562 files, 18532 tests, 10 env skips; bun run typecheck, bun run knip, bun run format, bun run lint clean; coverage gate 100.00% (59142/59142).

@cevheri

cevheri commented Sep 20, 2026

Copy link
Copy Markdown
Member

thanks and welcome Batuhan

@cevheri
cevheri merged commit eb58f73 into libredb:main Sep 20, 2026
24 checks passed
@cevheri

cevheri commented Sep 20, 2026

Copy link
Copy Markdown
Member

also realy like it your personal github projects, congrats

@KodYazicam

Copy link
Copy Markdown
Contributor Author

Thank you for the warm welcome and the thorough review process.
I really appreciate you looking at my profile and projects, it's highly motivating.

cevheri added a commit that referenced this pull request Sep 20, 2026
)

Merging main into this branch resolved the `object-route.ts` conflict by
deleting the line number from eight of the nine citations in the edit-affordance
docblock and leaving the ninth stale, plus a stray blank line inside the block.
`tests/unit/lib/api/object-route-edit.test.ts` is the guard for exactly that and
went red on the merge commit, before this branch's own second commit existed.

Numbers recomputed from each anchor at this tree, not carried over: #985 moved
the provider sites this docblock points at.

Co-authored-by: Lazzaro Davide <211658270+DevvoLazza@users.noreply.github.com>
Co-authored-by: Dharshini_RS <231443604+Dharshini-RS03@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hoist assertObjectPathShape: it is defined nine times across the providers

2 participants