refactor(providers): hoist assertObjectPathShape into object-kinds - #985
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cevheri
left a comment
There was a problem hiding this comment.
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-2099to 2070-2074,mysql.ts:1973-1976to 1948-1951,oracle.ts:1545-1546to 1521-1522,sqlite.ts:1189-1192to 1175-1178,mongodb.ts:800-801to 794-795,
oracle.ts:1551-1560to 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.
00a2deb to
952914d
Compare
|
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
The ~140 under Nit 1, taken further than asked. Redis no longer calls Nit 2, deliberately not taken here. Deriving The follow-up is filed: #1013, for the couchbase, embedded-LibreDB and druid copies. Re-validated on the rebased tree: |
|
thanks and welcome Batuhan |
|
also realy like it your personal github projects, congrats |
|
Thank you for the warm welcome and the thorough review process. |
) 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>
What changed
One exported
assertObjectPathShapeinsrc/lib/db/object-kinds.ts, besidecontainerDepth, 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 takespec(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 anenginedescriptor, so the odd MongoDB caller — which had already resolvedspecviafindKindat both call sites — simply passes it.specis required: every caller resolves the kind before the call (findKind,requireSourceKindorrequireEditableKind), 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— notspec.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:
objectPath()collapses a parentless trigger onto the container-level address (standing ruling 5f), so the error names both shapes joined with " or ";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
ObjectPathShapeEnginedescriptor (code,label,attachedSegment), and every thrown error is byte-identical to what each provider printed before. MySQL's privateobjectPathShapesis subsumed into the shared shapes computation and removed.Ripples the citation tests and review caught
src/lib/api/object-route.tsare re-pinned to the moved lines (its own test went red until they were).connection-fingerprint.ts, five test files,BACKLOG.md,AGENT_ANALYST_DESIGN.mdandagent/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.Validation
bun run test— 562 files, 18542 tests: 18532 pass, 10 skip (the skips are the zip-binary/CLI ones, absent in this environment)bun run typecheck— cleanbun run knip— clean (exit 0)bun run format— no fixes to applybun 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.info— 100.00%, 59142/59142 lines, so the shared function's branches are all exercised by the existing suiteThe remaining three engines carrying the same logic inlined (couchbase, embedded LibreDB, druid) are tracked in #1013.
Closes #978.