Skip to content

Commit 113050e

Browse files
claude[bot]claude
andauthored
fix(service-analytics): resolve a user (and tree) dimension's display label through its reference, as lookup already does (#17470)
* wip(service-analytics): resolve every reference-class dimension label through spec's arbiter Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW * changeset: reference-class dimension display labels Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW * test(service-analytics): record the measured ablation direction Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8645848 commit 113050e

4 files changed

Lines changed: 464 additions & 20 deletions

File tree

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
A dataset dimension over a `user` or `tree` field renders the referenced record's display name, the same way a `lookup` dimension already did. A "by person" chart's axis is people's names, not a column of user ids.
6+
7+
`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 this service already treated 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 (`lookup`, `master_detail`), so within a single dataset query one axis came back as a name and the other as a raw id, for two fields that differ in one word:
8+
9+
```
10+
Field.user({ label: 'Person' }) -> { type: 'user', reference: 'sys_user' }
11+
Field.lookup('sys_business_unit', { … }) -> { type: 'lookup', reference: 'sys_business_unit' }
12+
```
13+
14+
- **The subset is gone, not extended.** The resolver now asks `referenceTargetOf` (`@objectstack/spec/data`) — the declared single arbiter of "what does this reference field point at" — at all three sites that classified a dimension: the display pass, the `#3680` sort-key hook's `isLabelBearing`, and its `resolveLabels`. Adding two literals to a private `Set` would have left the next member of the class to be re-reported by the next user.
15+
- **A `user` field authored without `reference` resolves too.** `sys_user` is a constant of the type, which `referenceTargetOf` materializes; requiring an author to restate it is exactly the disagreement between two readers of one field that arbiter exists to end.
16+
- **The label read stays scoped (`#3602`).** Turning a user id into a name is a read of `sys_user`, and it travels the same `LabelScopeResolver` path every other member of the class travels — the referenced object's own RLS is resolved and ANDed into the lookup, and an unresolvable scope still fails closed to the raw id rather than fetching unscoped. This is the half of the change that had to land with it, not after it.
17+
- **Nothing degrades into an error or a blank.** An orphaned or RLS-hidden user id, a `sys_user` with no display field, and a user object unknown to the engine all leave the raw id in place and answer the query, which is the pre-existing contract for an unresolved lookup id.
18+
19+
No new authorable key and no new export: `DatasetDimensionSchema` is untouched, and a dimension's own declared `type` still does not decide this — the resolver reads the object field's type, as it always has.
Lines changed: 319 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,319 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #16390 — a `user` dimension renders the referenced record's display NAME, the
5+
* same way a `lookup` dimension already does.
6+
*
7+
* ## The measured asymmetry
8+
*
9+
* ONE dataset over the reporter's object, ONE `POST /api/v1/analytics/dataset/query`
10+
* — which is `AnalyticsService.queryDataset` one thin route away (`rest-server.ts`
11+
* dispatches to `svc.queryDataset` and does nothing to the rows):
12+
*
13+
* ```
14+
* dimensions: ['unit'] -> {"unit":"Customer Success", "avg_score":91.5} a NAME
15+
* dimensions: ['person'] -> {"person":"7zLYIwpX82If4Bvt…", "avg_score":124.25} an ID
16+
* ```
17+
*
18+
* Both fields carry exactly what the resolution needs, and differ in one word:
19+
*
20+
* ```
21+
* Field.user({ label: 'Person' }) -> { type: 'user', reference: 'sys_user' }
22+
* Field.lookup('sys_business_unit', {…}) -> { type: 'lookup', reference: 'sys_business_unit' }
23+
* ```
24+
*
25+
* so the fixture below builds its field map from those BUILDERS rather than from
26+
* hand-written literals — the premise is then re-measured by the pin on every run
27+
* instead of being asserted once in a card.
28+
*
29+
* ## The class is the unit of the fix, not the one member that was reported
30+
*
31+
* `REFERENCE_VALUE_TYPES` (`packages/spec/src/data/field-value.zod.ts`) declares
32+
* FOUR members — `lookup`, `master_detail`, `user`, `tree` — as one kind: "value
33+
* points at another record … a record-id string in stored form". This service
34+
* already treats them as one kind where it annotates measure result types
35+
* (`measure-result-type.ts` imports that very set), while the label resolver
36+
* hand-wrote a two-member subset of it. `user` and `tree` were BOTH outside that
37+
* subset, so both rendered raw ids; fixing only the reported member would re-seed
38+
* the same defect for the next reporter to find on the other one.
39+
*
40+
* The target object is read through spec's `referenceTargetOf`, the declared
41+
* SINGLE arbiter of "what does this reference field point at" — which also
42+
* supplies `sys_user` for a `user` field authored WITHOUT `reference`, a shape
43+
* that reaches production (`packages/objectql/src/query-expression-conformance.test.ts`
44+
* captures one) and that a `meta.reference &&` test would silently drop.
45+
*
46+
* ## Declaring the dimension `type: 'lookup'` is not, and never was, a way out
47+
*
48+
* `DatasetDimensionSchema.type` is `['string','number','date','boolean','lookup']` —
49+
* it has no `user` member at all — and the resolver reads the OBJECT field's type,
50+
* never the dimension's. Pinned below on both spellings, because it is the reason
51+
* the fix belongs at the resolution site and not in an author-facing declaration.
52+
*
53+
* ## Reverse verification — direction predicted BEFORE running
54+
*
55+
* Ordinary direction, no inversion and no count movement: the change ADDS
56+
* resolutions that were absent and narrows no rule, so restoring the two-member
57+
* subset must turn RED exactly the cases that depend on a `user` or `tree` axis,
58+
* and leave GREEN every `lookup`, `master_detail`, `select`, unresolved-id,
59+
* no-display-field and fail-closed case — `master_detail` was already inside the
60+
* old subset, so it is a control here, not a casualty. Predicted on that reading:
61+
* 7 red in this file and 3 in the `dimension-labels` sibling. Measured exactly
62+
* that (10 red / 33 green over the two files); the run is quoted in the PR body.
63+
*/
64+
65+
import { describe, it, expect } from 'vitest';
66+
import { DatasetSchema } from '@objectstack/spec/ui';
67+
import { Field, referenceTargetOf } from '@objectstack/spec/data';
68+
import type { ExecutionContext } from '@objectstack/spec/kernel';
69+
import { AnalyticsService } from '../analytics-service.js';
70+
import { pickDisplayField, type FieldMetaLite } from '../dimension-labels.js';
71+
72+
// ── the reporter's object, built from the real builders ─────────────────────
73+
74+
const KPI_RESULT_FIELDS: Record<string, FieldMetaLite> = {
75+
// The card's two fields, verbatim in shape.
76+
person: Field.user({ label: 'Person' }),
77+
unit: Field.lookup('sys_business_unit', { label: 'Business Unit' }),
78+
// The other two members of the same declared class.
79+
owner_team: Field.masterDetail('kpi_team', { label: 'Team' }),
80+
category: { type: 'tree', reference: 'kpi_category', label: 'Category' } as FieldMetaLite,
81+
// A `user` field authored WITHOUT `reference` — the target is a constant of
82+
// the type, so this metadata is fully specified, not under-specified.
83+
reviewer: { type: 'user', label: 'Reviewer' } as FieldMetaLite,
84+
score: Field.number({ label: 'Score' }),
85+
};
86+
87+
/** `sys_user`'s real primary-title pointer is `nameField: 'name'`. */
88+
const SYS_USER_FIELDS: Record<string, FieldMetaLite> = {
89+
name: { type: 'text' },
90+
email: { type: 'text' },
91+
};
92+
const UNIT_FIELDS: Record<string, FieldMetaLite> = { name: { type: 'text' } };
93+
const TEAM_FIELDS: Record<string, FieldMetaLite> = { name: { type: 'text' } };
94+
const CATEGORY_FIELDS: Record<string, FieldMetaLite> = { name: { type: 'text' } };
95+
96+
const FIELD_MAPS: Record<string, Record<string, FieldMetaLite>> = {
97+
kpi_result: KPI_RESULT_FIELDS,
98+
sys_user: SYS_USER_FIELDS,
99+
sys_business_unit: UNIT_FIELDS,
100+
kpi_team: TEAM_FIELDS,
101+
kpi_category: CATEGORY_FIELDS,
102+
};
103+
104+
/** id -> display name, per referenced object. `usr_orphan` is deliberately absent. */
105+
const NAMES: Record<string, Record<string, string>> = {
106+
sys_user: { usr_ada: 'Ada Lovelace', usr_bo: 'Bo Chen' },
107+
sys_business_unit: { bu_cs: 'Customer Success', bu_east: 'East China' },
108+
kpi_team: { team_a: 'Team Alpha' },
109+
kpi_category: { cat_q: 'Quality' },
110+
};
111+
112+
const dataset = DatasetSchema.parse({
113+
name: 'kpi_scores',
114+
label: 'KPI Scores',
115+
object: 'kpi_result',
116+
dimensions: [
117+
// `lookup` is the only reference-ish spelling the dimension schema offers,
118+
// which is what the reporter wrote for BOTH axes.
119+
{ name: 'unit', field: 'unit', type: 'lookup', label: 'Business Unit' },
120+
{ name: 'person', field: 'person', type: 'lookup', label: 'Person' },
121+
{ name: 'owner_team', field: 'owner_team', type: 'lookup' },
122+
{ name: 'category', field: 'category', type: 'lookup' },
123+
// The same user axis declared `string` — the declaration must not decide.
124+
{ name: 'reviewer', field: 'reviewer', type: 'string' },
125+
],
126+
measures: [{ name: 'avg_score', aggregate: 'avg', field: 'score' }],
127+
});
128+
129+
/** Rows the base aggregate returns, keyed by dimension name (raw stored ids). */
130+
const BASE_ROWS: Record<string, Record<string, unknown>[]> = {
131+
'unit,person': [
132+
{ unit: 'bu_cs', person: 'usr_ada', avg_score: 91.5 },
133+
{ unit: 'bu_east', person: 'usr_bo', avg_score: 106.12 },
134+
],
135+
person: [
136+
{ person: 'usr_ada', avg_score: 124.25 },
137+
{ person: 'usr_bo', avg_score: 128.6 },
138+
],
139+
owner_team: [{ owner_team: 'team_a', avg_score: 70 }],
140+
category: [{ category: 'cat_q', avg_score: 80 }],
141+
reviewer: [{ reviewer: 'usr_ada', avg_score: 60 }],
142+
};
143+
144+
interface Wiring {
145+
/** Referenced objects whose rows are visible; default: all of them. */
146+
names?: Record<string, Record<string, string>>;
147+
/** Field maps override — e.g. a `sys_user` with no display field. */
148+
fieldMaps?: Record<string, Record<string, FieldMetaLite>>;
149+
getReadScope?: (objectName: string, context?: ExecutionContext) => unknown;
150+
onFetch?: (targetObject: string, ids: unknown[], scope: unknown) => void;
151+
}
152+
153+
function service(w: Wiring = {}) {
154+
const fieldMaps = w.fieldMaps ?? FIELD_MAPS;
155+
const names = w.names ?? NAMES;
156+
return new AnalyticsService({
157+
queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }),
158+
executeAggregate: async (_object: string, { groupBy }: { groupBy?: string[] }) => {
159+
const key = (groupBy ?? []).join(',');
160+
return BASE_ROWS[key] ?? [];
161+
},
162+
...(w.getReadScope ? { getReadScope: w.getReadScope as never } : {}),
163+
labelResolver: {
164+
getObjectFields: (objectName) => fieldMaps[objectName],
165+
// Mirrors the plugin bridge: pick the target's display field, then read
166+
// `id -> that field` for the ids in hand. Absent ids simply do not come
167+
// back, exactly as an RLS-hidden or orphaned row does not.
168+
fetchRecordLabels: async (targetObject, ids, scope) => {
169+
w.onFetch?.(targetObject, ids, scope);
170+
const map = new Map<unknown, string>();
171+
if (!pickDisplayField(fieldMaps[targetObject])) return map;
172+
const table = names[targetObject] ?? {};
173+
for (const id of ids) if (table[String(id)]) map.set(id, table[String(id)]);
174+
return map;
175+
},
176+
},
177+
});
178+
}
179+
180+
const query = (svc: AnalyticsService, dimensions: string[]) =>
181+
svc.queryDataset(dataset, { dimensions, measures: ['avg_score'] });
182+
183+
// ── the premise, re-measured rather than recalled ───────────────────────────
184+
185+
describe('#16390 — the premise: a user field already carries its target', () => {
186+
it('Field.user() and Field.lookup() differ in one word, and both name a target', () => {
187+
expect(Field.user({ label: 'Person' })).toMatchObject({ type: 'user', reference: 'sys_user' });
188+
expect(Field.lookup('sys_business_unit', { label: 'Business Unit' }))
189+
.toMatchObject({ type: 'lookup', reference: 'sys_business_unit' });
190+
// The single arbiter answers for every member of the class, and answers for
191+
// a `user` field that omits `reference` too.
192+
expect(referenceTargetOf(KPI_RESULT_FIELDS.person)).toBe('sys_user');
193+
expect(referenceTargetOf(KPI_RESULT_FIELDS.reviewer)).toBe('sys_user');
194+
expect(referenceTargetOf(KPI_RESULT_FIELDS.unit)).toBe('sys_business_unit');
195+
expect(referenceTargetOf(KPI_RESULT_FIELDS.owner_team)).toBe('kpi_team');
196+
expect(referenceTargetOf(KPI_RESULT_FIELDS.category)).toBe('kpi_category');
197+
// …and refuses the measure column, so nothing non-referential is swept in.
198+
expect(referenceTargetOf(KPI_RESULT_FIELDS.score)).toBeUndefined();
199+
});
200+
201+
it("resolves sys_user through its nameField ('name'), the same convention a lookup target uses", () => {
202+
expect(pickDisplayField(SYS_USER_FIELDS)).toBe('name');
203+
});
204+
});
205+
206+
// ── the pin that matters: ONE query, BOTH axes ──────────────────────────────
207+
208+
describe('#16390 — one dataset query, a lookup dimension and a user dimension', () => {
209+
it('renders a display name for BOTH axes', async () => {
210+
const res = await query(service(), ['unit', 'person']);
211+
expect(res.rows).toEqual([
212+
{ unit: 'Customer Success', person: 'Ada Lovelace', avg_score: 91.5 },
213+
{ unit: 'East China', person: 'Bo Chen', avg_score: 106.12 },
214+
]);
215+
});
216+
217+
it('resolves the user axis on its own, too (the reporter\'s second query)', async () => {
218+
const res = await query(service(), ['person']);
219+
expect(res.rows).toEqual([
220+
{ person: 'Ada Lovelace', avg_score: 124.25 },
221+
{ person: 'Bo Chen', avg_score: 128.6 },
222+
]);
223+
});
224+
225+
it('reads the OBJECT field type, not the dimension declaration — a `string`-declared user axis resolves identically', async () => {
226+
const res = await query(service(), ['reviewer']);
227+
expect(res.rows).toEqual([{ reviewer: 'Ada Lovelace', avg_score: 60 }]);
228+
});
229+
230+
it('covers the whole declared reference class: master_detail and tree resolve as well', async () => {
231+
const md = await query(service(), ['owner_team']);
232+
expect(md.rows).toEqual([{ owner_team: 'Team Alpha', avg_score: 70 }]);
233+
const tree = await query(service(), ['category']);
234+
expect(tree.rows).toEqual([{ category: 'Quality', avg_score: 80 }]);
235+
});
236+
237+
it('asks for the REFERENCED object, never the base object', async () => {
238+
const targets: string[] = [];
239+
await query(service({ onFetch: (t) => targets.push(t) }), ['unit', 'person']);
240+
expect(new Set(targets)).toEqual(new Set(['sys_business_unit', 'sys_user']));
241+
});
242+
});
243+
244+
// ── the read scope (#3602) reaches the new members too ──────────────────────
245+
246+
describe('#16390 — the label read stays scoped for every member of the class', () => {
247+
it('resolves and forwards the referenced object read scope for a user dimension', async () => {
248+
const asked: string[] = [];
249+
const seen: Array<{ target: string; scope: unknown }> = [];
250+
await query(
251+
service({
252+
getReadScope: (objectName) => {
253+
asked.push(objectName);
254+
return objectName === 'sys_user' ? { organization_id: 'org_A' } : undefined;
255+
},
256+
onFetch: (target, _ids, scope) => seen.push({ target, scope }),
257+
}),
258+
['person'],
259+
);
260+
// Reading a user id into a name IS a read of `sys_user`; it must carry that
261+
// object's own RLS, exactly as a lookup target's label read does.
262+
expect(asked).toContain('sys_user');
263+
expect(seen).toContainEqual({ target: 'sys_user', scope: { organization_id: 'org_A' } });
264+
});
265+
266+
it('fails CLOSED for a user dimension: an unresolvable scope leaves the raw id, and fetches nothing', async () => {
267+
let fetched = false;
268+
const res = await query(
269+
service({
270+
getReadScope: (objectName) => {
271+
// The base object stays resolvable — only the label target fails, so
272+
// the query itself must still answer.
273+
if (objectName === 'sys_user') throw new Error('security service unavailable');
274+
return undefined;
275+
},
276+
onFetch: () => { fetched = true; },
277+
}),
278+
['person'],
279+
);
280+
expect(fetched).toBe(false);
281+
expect(res.rows).toEqual([
282+
{ person: 'usr_ada', avg_score: 124.25 },
283+
{ person: 'usr_bo', avg_score: 128.6 },
284+
]);
285+
});
286+
});
287+
288+
// ── C4 negative controls: degrade to the id, never to an error or a blank ───
289+
290+
describe('#16390 — an unresolvable user renders as itself, not as an error', () => {
291+
it('leaves an orphaned / RLS-hidden user id untouched, and still answers the query', async () => {
292+
const res = await query(service({ names: { ...NAMES, sys_user: { usr_ada: 'Ada Lovelace' } } }), ['person']);
293+
expect(res.rows).toEqual([
294+
{ person: 'Ada Lovelace', avg_score: 124.25 },
295+
{ person: 'usr_bo', avg_score: 128.6 }, // raw id survives — no blank, no throw
296+
]);
297+
});
298+
299+
it('leaves every id raw when sys_user carries no display field at all', async () => {
300+
const res = await query(
301+
service({ fieldMaps: { ...FIELD_MAPS, sys_user: { created_at: { type: 'date' } } } }),
302+
['person'],
303+
);
304+
expect(res.rows).toEqual([
305+
{ person: 'usr_ada', avg_score: 124.25 },
306+
{ person: 'usr_bo', avg_score: 128.6 },
307+
]);
308+
});
309+
310+
it('leaves the id raw when the user object is unknown to the engine', async () => {
311+
const withoutUser = { ...FIELD_MAPS };
312+
delete (withoutUser as Record<string, unknown>).sys_user;
313+
const res = await query(service({ fieldMaps: withoutUser }), ['person']);
314+
expect(res.rows).toEqual([
315+
{ person: 'usr_ada', avg_score: 124.25 },
316+
{ person: 'usr_bo', avg_score: 128.6 },
317+
]);
318+
});
319+
});

0 commit comments

Comments
 (0)