Skip to content

fix(service-analytics): resolve a user (and tree) dimension's display label through its reference, as lookup already does - #17470

Merged
os-sales merged 4 commits into
mainfrom
claude/issue-16390-user-dimension-display-name
Sep 10, 2026
Merged

os-sales merged 4 commits into
mainfrom
claude/issue-16390-user-dimension-display-name

Conversation

@claude

@claude claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Fixes #16390

Clause-②: no — no symbol is published, no error code is minted, and no authorable key is added. DatasetDimensionSchema (and all of packages/spec) is read, never written. What changes is a RETURNED VALUE: a user dimension's cell was the stored user id and is now the referenced user's display name. Re-tested against the actual diff after writing it: git diff over packages/services/service-analytics/src adds no line containing export.

What this changes

packages/spec declares one reference class — REFERENCE_VALUE_TYPES = lookup, master_detail, user, tree, "value points at another record … a record-id string in stored form" — and service-analytics already treats it as one class where it annotates measure result types (measure-result-type.ts imports that very set). The label resolver, one file away, hand-wrote a two-member subset of it, so a single dataset query answered one axis with a name and the other with a raw id for two fields that differ in one word:

Field.user({ label: 'Person' })            -> { type: 'user',   reference: 'sys_user' }
Field.lookup('sys_business_unit', { … })   -> { type: 'lookup', reference: 'sys_business_unit' }

packages/services/service-analytics/src/dimension-labels.ts now asks spec's referenceTargetOf — the declared SINGLE arbiter of "what does this reference field point at" — at all three sites that classified a dimension: the display pass in resolveDimensionLabels, plus both classification points of the #3680 order-label hook (isLabelBearing, and the label fetch beside it). The private LOOKUP_TYPES set is gone rather than extended by two literals, because a hand-written subset of a declared class is the defect, not the two members it happened to be missing.

Two consequences that a LOOKUP_TYPES.add('user') patch would not have had:

  • A user field authored without reference resolves too. sys_user is a constant of the type, which referenceTargetOf materializes. That shape reaches production — packages/objectql/src/query-expression-conformance.test.ts captures one — and the old meta.reference && test dropped it silently.
  • The #3602 read scope reaches the new members. Turning a user id into a name IS a read of sys_user. It travels the same LabelScopeResolver path every other member travels: the referenced object's own RLS is resolved and ANDed into the id lookup, and an unresolvable scope still fails closed to the raw id rather than fetching unscoped. This had to land WITH the widening, not after it — a resolution that read sys_user unscoped would leak "who is who" to a caller who cannot read sys_user.

Route: (a), as ruled. packages/spec is untouched

Route (b) — a displayField key on the strict DatasetDimensionSchema — is not taken and was not needed: nothing about (a) required a spec type to widen, so the prohibition never fired. Nor is a name denormalised into the result table. DatasetDimensionSchema.type is ['string','number','date','boolean','lookup'] — it has no user member at all — which is the structural reason the fix belongs at the resolution site: there is no author-facing declaration to fix it with.

Zone-3 re-verification — the card's repro is STILL LIVE on today's main

The card was filed 2026-09-06 and #16778 landed in this package today, so the premises were re-measured rather than carried across, at base 9788f1e91:

  • The resolution site has NOT moved: dimension-labels.ts still holds it, and its last touch is #17061, before this card.
  • Field.user() still returns { type: 'user', reference: 'sys_user' } and Field.lookup(...) still returns { type: 'lookup', reference: … } — asserted by the new pin itself, on every run, from the real builders.
  • Declaring the dimension type: 'lookup' still does not change the result: outside the date branch the resolver reads only the OBJECT field's type. Pinned both ways (a user axis declared lookup and one declared string resolve identically).
  • The new pin, written BEFORE the fix and run against unmodified main: 7 failed / 5 passed. The defect reproduces.

Verification

Base fd62a66b7, reported at HEAD 44918e39d.

Testspnpm --filter @objectstack/service-analytics test: 106 files / 2284 tests passed. typecheck: clean, and tsc --noEmit --listFiles | grep -c confirms the new test file is one of the files that program compiles (1), so the typecheck really covers it.

Ablation (C5). The fix is committed first; the mutation restores the pre-fix classification verbatim inside referenceLabelTarget (the two-member subset plus the meta.reference test). On-disk proof, not an editor exit code — the deleted text goes 1 to 0 and the injected marker 0 to 1, and the blob hash moves off the HEAD blob. Resolution path: the pin imports the subject relatively inside its own package, so it reads source, not dist (pnpm check:test-source-alias passes, and the spec dependency it does read from dist is unchanged by the mutation).

HEAD_BLOB=352a94e60a00890fab5cf374bb7020d2e18c72f6
deleted-text occurrences : 1 -> 0
injected-text occurrences: 0 -> 1
mutated blob: c1ece3783868ecd962650165dd60c566f4ca3073
Test Files  2 failed (2)
     Tests  10 failed | 33 passed (43)

Direction was predicted before running and is written into the test header: ordinary, no inversion, no count movement — red exactly where a user or tree axis is load-bearing, green for every lookup, master_detail, select, unresolved-id, no-display-field and fail-closed case. master_detail was already inside the old subset, so it is a control here, not a casualty. Predicted 7 red in the new file + 3 in the sibling; measured exactly that. The 7 also match the pre-fix run above, so the ablation reproduces the original defect rather than some third state.

Restoration proven BY STATE, not by exit code:

restored blob: 352a94e60a00890fab5cf374bb7020d2e18c72f6
HEAD blob    : 352a94e60a00890fab5cf374bb7020d2e18c72f6
git diff HEAD -- target, bytes of output: 0 (0 == clean)

Gates. Derived from the actual diff (node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack), all 60 run, exit codes captured before any pipe, reconciled with --ran: 60 derived, 57 run green, 3 NOT MEASURED, 0 unrun. The three are check:dual-build-cjs-loads, check:lean-entry-closure and check:type-check-debt — each exits 3, PREREQUISITE NOT MET, because each reads built output for the whole workspace. Those are CI's repo-scale build, not a per-card local run; they are recorded as NOT MEASURED, not as passes. One caveat on the derivation itself: it was taken at 44918e39d, by which time origin/main had moved one derivation-source script (scripts/measure-test-shard-timings.mjs); the family it feeds (report-test-timings.mjs --self-test) is in the list above and ran green.

Lint — a declared narrowing, with its three pieces of evidence. pnpm lint (= eslint . --no-inline-config over the repo root) is CI's run. Locally this diff was linted file-by-file at HEAD 44918e39d: --format json reports 3 files, 0 errors, 0 warnings (the changeset markdown is not a lint target — eslint answers "File ignored because no matching configuration was supplied" for it, so 3 is the diff's whole lint surface). The narrowing excludes nothing, because this repo runs one eslint.config.mjs which never enables type-aware linting for ANY file — no parserOptions.project, no typed @typescript-eslint rules, stated and positively controlled in the config's own header above QUERY_OPTIONS_TEST_GLOBS. With no cross-file type program, an edit confined to three files cannot move the verdict on a file it did not touch.

Acceptance notes

C3 — the symmetry pin. packages/services/service-analytics/src/__tests__/dataset-reference-dimension-labels.test.ts runs ONE AnalyticsService.queryDataset over one dataset holding a Field.lookup() axis and a Field.user() axis with an avg measure, and asserts a display name in BOTH cells of the same row. queryDataset is exactly what POST /api/v1/analytics/dataset/query dispatches to (rest-server.ts calls svc.queryDataset and does nothing to the rows); the route itself lives in packages/rest, outside this card's declared file surface, so it is not re-tested here.

C4 — the negative control, stated. An unresolvable user degrades to the raw id and the query still answers. Three shapes are pinned: an orphaned or RLS-hidden user id (the row keeps the id; a sibling row in the same result still resolves), a sys_user carrying no display field at all (every id stays raw), and a sys_user unknown to the engine (same). A fourth, the fail-closed one, is pinned separately: when the read-scope provider throws for sys_user, nothing is fetched and the id renders. Nothing throws, nothing blanks, and no id-labelled chart turns into an error — this is the pre-existing contract for an unresolved lookup id, now reached by the same code path.

Triage's stated acceptance condition, and one correction to it. The triage comment asked that dimension-labels.test.ts:167's expect(scopeCalls).toBe(0) be converted into a positive four-member assertion. Read on the tree, that assertion is about a select dimension, which resolves from field metadata and reads no other object — it stays correct after the widening and was kept, with its now-stale trailing comment rewritten. The positive counterpart it asked for was ADDED beside it instead: #16390 — a user and a tree dimension resolve the REFERENCED object scope, same as lookup asserts the resolver is consulted for crm_account, sys_user and task in order, and that each scope reaches fetchRecordLabels.

Out of scope, noted, not filed. Two further two-member spellings of the same class survive in this package, both outside this card's declared file surface:

  • plugin.tsrelationshipResolver admits only lookup / master_detail when mapping a dataset include to a joined table. Reachable through Dataset.include, not through the dimension path this card fixes, and a user join has a fixed target so it is a different question than the label read. Carrier: the next PR that touches dataset joins in service-analytics.
  • dataset-executor.ts (order-hook prose) and analytics-service.ts (a doc comment) describe the sort behaviour as select or lookup/master_detail. The BEHAVIOUR is already corrected by this PR — both read createOrderLabelResolver, which now covers the class, so a person axis sorts by name from this commit on. Only the two comments are now understated. Carrier: the same next PR; comment-only drift, filed nowhere.

Neither is a filed card: both are stale prose or an untested adjacent path, i.e. observations, not reproducible defects, contract breaches, or metadata traps.

Docs drift check — answered

Re-derived on the tree the bot actually read, not a worktree cut from an older main: merge commit 46332726694e99851d1e794aed90ee80f4ab3cf1 (head 44918e39da into base c4d1759fa3), fetched and checked out per the comment's own recipe, then node scripts/docs-audit/affected-docs.mjs --json c4d1759fa3c73b68cd16df8bf8562c4f86268dc6. Same numbers as the advisory: 27 docs (21 hand-written + 6 release-owned), 7 anchors, 9 package-mention fallback rows.

Why all 21 rows arrived, in one sentence. Every one of the 27 rows is anchored on the same single item — master_detail (literal, a string literal in LOOKUP_TYPES). This diff DELETES the LOOKUP_TYPES set, whose members were the literals 'lookup' and 'master_detail', so every page that spells the field type master_detail anywhere was selected. That is the anchor doing its job precisely; it is not a wrong row. But it also means the listing carries no signal about which page states the RULE this diff changed, so each page was read against the change rather than dismissed as a class.

Per-page verdict — the 21 hand-written pages. One is falsified. The other 20 are not, and they divide into three groups by what their master_detail occurrence actually says:

  • FALSIFIED — content/docs/data-modeling/analytics.mdx:209-213. The falsifying sentence: "a select dimension returns its option label (not the stored value), a lookup / master_detail dimension returns the related record's display name (not the FK id), and a date dimension … returns a human bucket label". This is the page that restates exactly the rule this PR widened. After this change a user and a tree dimension also return the related record's display name, so the two-member enumeration is now wrong by omission — and wrong in the direction that matters, because a reader asking "does a person axis render a name?" reads this list and concludes it does not. Reported below, ⛔ not edited.
  • Not falsified — field-type vocabulary (11 pages). data-modeling/field-types.mdx, field-type-decision-tree.mdx, fields.mdx, relationships.mdx, index.mdx, validation-rules.mdx, getting-started/common-patterns.mdx, quick-reference.mdx, deployment/cli.mdx, deployment/troubleshooting.mdx, protocol/objectql/types.mdx. Each names master_detail as a field type — cascade delete, parent-child modelling, Field.masterDetail(), delete behaviour. None makes a claim about analytics, dimensions or what a grouped value renders as. This diff changes no field type and no delete behaviour.
  • Not falsified — expand / reference-class pages, already stating all four members (5 pages). api/data-api.mdx:24, automation/hook-bodies.mdx:103, data-modeling/queries.mdx:328, protocol/objectql/query-syntax.mdx:739 and :803, protocol/objectui/widget-contract.mdx:162. These describe expand / the relational field family and already enumerate lookup / master_detail / user / tree (two of them naming REFERENCE_VALUE_TYPES outright). They agree with this change rather than being falsified by it — they document the same class this diff stops splitting.
  • Not falsified — error and permission pages (4). api/error-catalog.mdx, protocol/kernel/error-handling.mdx, permissions/authorization.mdx, protocol/objectql/schema.mdx. The first three describe the master-access gate and required-field validation for a master_detail; this diff mints no error code, changes no status and touches no admission path. On the fourth, schema.mdx:298 (| reference | string | lookup, master_detail | Target object for relationships. |) understates which types carry reference — but it understated it identically before this diff, since referenceTargetOf and REFERENCE_VALUE_TYPES predate it. Pre-existing, not caused here, and reported as an observation rather than a finding.

The half the bot states it can never do — hand-checked emitter-blind set. The advisory says a page that states a rule by its INPUTS shares no identifier with the emitter, so an emitter-only diff cannot list it on this or any run. So the analytics / dataset / dimension / chart / report / dashboard surface was swept by hand for pages that describe what a dimension axis renders WITHOUT naming referenceTargetOf, REFERENCE_VALUE_TYPES, dimension-labels, createOrderLabelResolver or master_detail. Two sweeps over all of content/docs outside releases/: one on output phrasings (display name, display label, FK id, raw id, option label, stored value, presentation-ready, resolved server-side), one on rendering verbs near dimension / axis (shows the, renders, returns the, reads the, id instead, name instead). It found a second falsified page the bot did not and could not list:

  • FALSIFIED, and absent from the advisory's 21 — content/docs/ui/dashboards.mdx:146-150. The falsifying sentence: "A sortBy naming a select or lookup dimension orders by the display label the rows render (the option label / the related record's name), not the stored value or foreign-key id". This states the #3680 sort-key rule, which this diff widened through createOrderLabelResolver.isLabelBearing: a sortBy naming a user or tree dimension now orders by the display label too. The page names none of this change's identifiers — not master_detail, not OrderLabelResolver — which is exactly why no anchor could reach it. Reported below, ⛔ not edited. (Its omission of master_detail is separately pre-existing and not caused here.)

Also read and cleared in that sweep, so the next reader need not redo them: references/ui/dataset.mdx:57 and references/data/analytics.mdx:129 (generated reference tables mirroring DatasetDimensionSchema.type / the Cube dimension shape — packages/spec is untouched, so neither moved); references/ui/report.mdx, references/ui/chart.mdx, references/api/analytics.mdx, references/api/sortability.mdx, capabilities/analytics.mdx (name dimensions but state nothing about the rendered value); and ui/reports.mdx:157-159, which explicitly delegates the ordering rule to data-modeling/analytics.mdx and says it "does not repeat" it — so it carries no independent claim to falsify.

The 6 release-owned pages: read, and left alone. releases/implementation-status.mdx, releases/v12.mdx, releases/v17/17-0.mdx, releases/v17/17-3.mdx, releases/v17/17-4.mdx, releases/v9.mdx. ⛔ Nothing under content/docs/releases/** was edited. Two of them do state this rule — v9.mdx:114-115 ("a select dimension shows its option label … a lookup/master_detail dimension shows the …") and 17-0.mdx:1385 ("selections sort by display label for select/lookup dimensions") — and neither is falsified, for a reason rather than by exemption: a release page is a dated record of what a given release shipped, and both sentences remain true of v9 and 17.0 respectively. A later widening does not retro-falsify the note that describes the narrower behaviour at its own release. If that reading is wrong, it is a report, not an edit.

Two findings handed to the domain:services seat, ⛔ not edited here. content/docs/** is domain:devx, so neither page is touched in this PR:

  1. content/docs/data-modeling/analytics.mdx:211-212 — "a lookup / master_detail dimension returns the related record's display name (not the FK id)" should name all four members of the reference class.
  2. content/docs/ui/dashboards.mdx:146-147 — "A sortBy naming a select or lookup dimension orders by the display label" should name the reference class, not lookup alone.

Neither is a generated block, so the generated-surface exception does not apply.


Generated by Claude Code


Generated by Claude Code

@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-analytics, touching 7 documentable anchor(s).

21 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json c4d1759fa3c73b68cd16df8bf8562c4f86268dc6.

6 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json c4d1759fa3c73b68cd16df8bf8562c4f86268dc6packageMentionDocs.

Which tree this was computed on

This run read content/docs from 46332726694e99851d1e794aed90ee80f4ab3cf1 — the merge of head 44918e39daec2d24b6390cc52299303065a93088 into base c4d1759fa3c73b68cd16df8bf8562c4f86268dc6, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 46332726694e99851d1e794aed90ee80f4ab3cf1 && git checkout 46332726694e99851d1e794aed90ee80f4ab3cf1
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin c4d1759fa3c73b68cd16df8bf8562c4f86268dc6 44918e39daec2d24b6390cc52299303065a93088 && git checkout -B drift-repro c4d1759fa3c73b68cd16df8bf8562c4f86268dc6 && git merge --no-ff 44918e39daec2d24b6390cc52299303065a93088

node scripts/docs-audit/affected-docs.mjs --json c4d1759fa3c73b68cd16df8bf8562c4f86268dc6

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs c4d1759fa3c73b68cd16df8bf8562c4f86268dc6 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-sales
os-sales marked this pull request as ready for review September 10, 2026 16:47
@os-sales
os-sales enabled auto-merge September 10, 2026 16:47
@os-sales
os-sales added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit 113050e Sep 10, 2026
41 checks passed
@os-sales
os-sales deleted the claude/issue-16390-user-dimension-display-name branch September 10, 2026 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

analytics: 数据集维度只解析 lookup 字段,user 字段维度返回原始用户 id —— 以人为轴的图表/报表标签是一串 id

2 participants