Skip to content

Commit b100c40

Browse files
committed
fix(lint, service-analytics): refuse a dataset or cube dimension over a virtual field at authoring
Claude-Session: https://claude.ai/code/session_016WV4d1SZmabyrNd3YLCq3J Co-authored-by: Claude <noreply@anthropic.com>
1 parent 098b2b9 commit b100c40

7 files changed

Lines changed: 481 additions & 82 deletions

‎packages/lint/src/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ export {
6464
validateDatasetMeasureAggregates,
6565
MEASURE_AGGREGATE_FIELD_TYPE_REFUSED,
6666
DIMENSION_JSON_STORED_FIELD_REFUSED,
67+
DIMENSION_VIRTUAL_FIELD_REFUSED,
6768
MEASURE_DURATION_FIELD_TYPE_REFUSED,
6869
// [#23235] The dimension leg's group-key verdict on its own, so a consumer
6970
// deriving dataset dimensions (cloud's AI build) asks the door's two

‎packages/lint/src/rule-explanations.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2360,6 +2360,25 @@ const DIMENSION_JSON_STORED_FIELD_REFUSED_EXPLANATION: RuleExplanation = {
23602360
],
23612361
};
23622362

2363+
const DIMENSION_VIRTUAL_FIELD_REFUSED_EXPLANATION: RuleExplanation = {
2364+
rule: 'dimension-virtual-field-refused',
2365+
covers: 'why analytics does not group by a formula field',
2366+
paragraphs: [
2367+
'A dimension is a GROUP KEY: a dataset dimension compiles to a cube dimension whose `sql` is its ' +
2368+
'`field`, and a query that selects it groups (or date-buckets) by that column. A virtual field, a ' +
2369+
'`formula`, is computed on read and never stored, whatever its `returnType`: no driver materialises ' +
2370+
'a column for it, so there is nothing to group by. Every analytics face refuses such a group with ' +
2371+
'`400 INVALID_FIELD` at `groupBy.N`, and the dataset compile refuses a dataset declaring one over its ' +
2372+
'own object with `400 DATASET_INVALID`, so no report, dashboard or query is ever served from it. ' +
2373+
'Before those refusals the group key reached the driver as a column that does not exist, or an ' +
2374+
'engine that aggregates in memory put every row into one null group while the answer looked served.',
2375+
'Denormalise the value: store it in a field of its own on the object (written by a hook or a flow ' +
2376+
'when its source changes), and point the dimension at that stored field. This rule makes the same ' +
2377+
'refusal where the author is standing. A dotted path is judged on the leaf\'s own declaration.',
2378+
ANALYTICS_MEMBER_WALK,
2379+
],
2380+
};
2381+
23632382
const MEASURE_DURATION_FIELD_TYPE_REFUSED_EXPLANATION: RuleExplanation = {
23642383
rule: 'measure-duration-field-type-refused',
23652384
covers: 'which fields a duration measure measures between',
@@ -5737,6 +5756,7 @@ export const RULE_EXPLANATIONS: Readonly<Record<string, RuleExplanation>> = Obje
57375756
[VALIDATION_RULE_JSON_SCHEMA_UNKNOWN_FORMAT_EXPLANATION.rule]: VALIDATION_RULE_JSON_SCHEMA_UNKNOWN_FORMAT_EXPLANATION,
57385757
[MEASURE_AGGREGATE_FIELD_TYPE_REFUSED_EXPLANATION.rule]: MEASURE_AGGREGATE_FIELD_TYPE_REFUSED_EXPLANATION,
57395758
[DIMENSION_JSON_STORED_FIELD_REFUSED_EXPLANATION.rule]: DIMENSION_JSON_STORED_FIELD_REFUSED_EXPLANATION,
5759+
[DIMENSION_VIRTUAL_FIELD_REFUSED_EXPLANATION.rule]: DIMENSION_VIRTUAL_FIELD_REFUSED_EXPLANATION,
57405760
[MEASURE_DURATION_FIELD_TYPE_REFUSED_EXPLANATION.rule]: MEASURE_DURATION_FIELD_TYPE_REFUSED_EXPLANATION,
57415761
[REACT_CHART_DRILLDOWN_INVALID_EXPLANATION.rule]: REACT_CHART_DRILLDOWN_INVALID_EXPLANATION,
57425762
[REACT_CHART_AGGREGATE_INVALID_EXPLANATION.rule]: REACT_CHART_AGGREGATE_INVALID_EXPLANATION,

‎packages/lint/src/runtime-gate.dataset-writes.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,8 @@ const OBJECTS = [
6161
issued_on: { type: 'date', label: 'Issued' },
6262
total: { type: 'currency', label: 'Total' },
6363
account: { type: 'lookup', label: 'Account', reference: 'acme_account' },
64+
// [#30773] A virtual field: computed on read, never stored.
65+
total_label: { type: 'formula', label: 'Total label', expression: 'record.region', returnType: 'text' },
6466
},
6567
},
6668
{
@@ -229,6 +231,21 @@ describe('the door refuses each crossed rule s own defect (#19143)', () => {
229231
expect(result.rulesRun).toContain('validateDatasetMeasureAggregates');
230232
});
231233

234+
it('⭐ LIT — a dimension over a formula field, which no query can group by (#30773)', () => {
235+
// The dataset compile refuses it with `400 DATASET_INVALID` and every
236+
// analytics face refuses the group with `400 INVALID_FIELD`; this is the
237+
// same refusal at the save, where the author is standing.
238+
const result = expectRefusal(
239+
cleanDataset({ dimensions: [{ name: 'status', field: 'status' }, { name: 'total_label', field: 'total_label' }] }),
240+
'dimension-virtual-field-refused',
241+
);
242+
expect(result.errors.find((e) => e.rule === 'dimension-virtual-field-refused')?.path).toBe(
243+
// The door rewrites the snapshot's index to the written item's name.
244+
'datasets.acme_invoice_metrics.dimensions[1].field',
245+
);
246+
expect(result.rulesRun).toContain('validateDatasetMeasureAggregates');
247+
});
248+
232249
it('⭐ LIT — a base object that resolves to nothing (the split-off rung)', () => {
233250
// `validateObjectReferences` owns this one rather than
234251
// `validateDatasetReferences`, because the reference is an object NAME and

‎packages/lint/src/validate-dataset-measure-aggregates.test.ts‎

Lines changed: 158 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
MULTI_CAPABLE_TYPES,
2222
MULTI_OPTION_TYPES,
2323
NUMERIC_VALUE_TYPES,
24+
SEARCH_VIRTUAL_TYPES,
2425
STRUCTURED_JSON_TYPES,
2526
VIRTUAL_FIELD_AGGREGATE_REMEDY,
2627
isAggregateCompatibleWithFieldType,
@@ -32,6 +33,7 @@ import { runAuthoringRules } from './authoring-rules.js';
3233
import { explainRule } from './rule-explanations.js';
3334
import {
3435
DIMENSION_JSON_STORED_FIELD_REFUSED,
36+
DIMENSION_VIRTUAL_FIELD_REFUSED,
3537
MEASURE_AGGREGATE_FIELD_TYPE_REFUSED,
3638
groupKeyClassOf as moduleGroupKeyClassOf,
3739
validateDatasetMeasureAggregates as validateDatasetMeasureAggregatesUnrecorded,
@@ -1237,8 +1239,150 @@ describe('[#23235] groupKeyClassOf — the exported classifier and the rule agre
12371239
});
12381240
});
12391241

1240-
describe('[#22161] one-line verdicts — the two ids', () => {
1241-
const IDS = [MEASURE_AGGREGATE_FIELD_TYPE_REFUSED, DIMENSION_JSON_STORED_FIELD_REFUSED];
1242+
// ───────────────────────────────────────────────────────────────────────────
1243+
// [#30773] The virtual-dimension leg — `dimension-virtual-field-refused`. A
1244+
// dataset or cube dimension over a `formula` (computed on read, never stored)
1245+
// is a group key no query can serve: every analytics face refuses the group at
1246+
// query time (`400 INVALID_FIELD` at `groupBy.N`), and the dataset compile
1247+
// refuses a dataset declaring one. Measured on `origin/main` `2d04cd2b` before
1248+
// this leg: `runAuthoringRules` on validate / build / lint fired nothing for a
1249+
// dataset dimension or a cube dimension over a formula.
1250+
// ───────────────────────────────────────────────────────────────────────────
1251+
1252+
const VIRTUAL_RULE = DIMENSION_VIRTUAL_FIELD_REFUSED;
1253+
const FORMULA_TEXT = { type: 'formula', expression: 'record.name', returnType: 'text' };
1254+
const FORMULA_DATE = { type: 'formula', expression: 'record.name', returnType: 'date' };
1255+
1256+
const virtualFindings = (stack: unknown) =>
1257+
validateDatasetMeasureAggregates(stack).filter((f) => f.rule === VIRTUAL_RULE);
1258+
1259+
describe('dimension-virtual-field-refused — a dimension over a virtual field is refused at authoring', () => {
1260+
// ⭐ The card's pin and its control, one pair.
1261+
it('refuses a dataset dimension over a formula field, and accepts one over a text field', () => {
1262+
const found = validateDatasetMeasureAggregates(dimensionStack(FORMULA_TEXT));
1263+
expect(found).toHaveLength(1);
1264+
const issue = found[0];
1265+
expect(issue.severity).toBe('error');
1266+
expect(issue.rule).toBe(VIRTUAL_RULE);
1267+
expect(issue.path).toBe('datasets[0].dimensions[0].field');
1268+
expect(issue.where).toBe('dataset "opportunity_metrics" › dimension "the_dimension"');
1269+
expect(issue.message).toBe(
1270+
'groups by field "grouped" (`formula` on object "crm_opportunity"), a value computed on read and never ' +
1271+
'stored, so no query can group by it',
1272+
);
1273+
// The remedy the query-time doors name: store the value, group by that.
1274+
expect(issue.hint).toBe(
1275+
'Store the value in a stored field of "crm_opportunity" (written when its source changes) and point this ' +
1276+
'dimension at that field, or remove the dimension.',
1277+
);
1278+
expect(validateDatasetMeasureAggregates(dimensionStack({ type: 'text' }))).toEqual([]);
1279+
});
1280+
1281+
it('refuses it whatever the returnType — a date-typed dimension bucketed by month over a formula of date included', () => {
1282+
const stack = dimensionStack(FORMULA_DATE);
1283+
const ds = (stack.datasets as Array<Record<string, unknown>>)[0];
1284+
ds.dimensions = [{ name: 'closed_month', field: 'grouped', type: 'date', dateGranularity: 'month' }];
1285+
const [issue] = virtualFindings(stack);
1286+
expect(issue?.path).toBe('datasets[0].dimensions[0].field');
1287+
expect(issue?.where).toBe('dataset "opportunity_metrics" › dimension "closed_month"');
1288+
// …while a stored date dimension bucketed the same way stays clean.
1289+
expect(validateDatasetMeasureAggregates(dimensionStack({ type: 'date' }))).toEqual([]);
1290+
});
1291+
1292+
// The whole surface against the spec's one predicate, never a local list.
1293+
it('agrees with SEARCH_VIRTUAL_TYPES on every declared FieldType', () => {
1294+
expect(SEARCH_VIRTUAL_TYPES.size).toBeGreaterThan(0);
1295+
let refused = 0;
1296+
let accepted = 0;
1297+
for (const fieldType of FieldType.options) {
1298+
const fires = virtualFindings(dimensionStack({ type: fieldType })).length > 0;
1299+
expect(fires, fieldType).toBe(SEARCH_VIRTUAL_TYPES.has(fieldType));
1300+
if (fires) refused++;
1301+
else accepted++;
1302+
}
1303+
expect(refused).toBe(SEARCH_VIRTUAL_TYPES.size);
1304+
expect(accepted).toBeGreaterThan(40);
1305+
});
1306+
1307+
it('names the later position beside a stored dimension, and never lands under the JSON-stored id', () => {
1308+
const stack = dimensionStack(FORMULA_TEXT);
1309+
const ds = (stack.datasets as Array<Record<string, unknown>>)[0];
1310+
ds.dimensions = [{ name: 'by_name', field: 'name' }, { name: 'by_calc', field: 'grouped' }];
1311+
const found = validateDatasetMeasureAggregates(stack);
1312+
expect(found.map((f) => [f.rule, f.path])).toEqual([[VIRTUAL_RULE, 'datasets[0].dimensions[1].field']]);
1313+
});
1314+
1315+
it('judges a dimension over a joined field on the object the leaf lives on', () => {
1316+
const joined = (leaf: Record<string, unknown>) => ({
1317+
name: 'analytics_probe',
1318+
objects: [
1319+
{
1320+
name: 'crm_opportunity',
1321+
sharingModel: 'private',
1322+
fields: { name: { type: 'text' }, account: { type: 'lookup', reference: 'crm_account' } },
1323+
},
1324+
{ name: 'crm_account', sharingModel: 'private', fields: { name: { type: 'text' }, tier: leaf } },
1325+
],
1326+
datasets: [
1327+
{
1328+
name: 'opportunity_metrics',
1329+
object: 'crm_opportunity',
1330+
include: ['account'],
1331+
dimensions: [{ name: 'acct_tier', field: 'account.tier' }],
1332+
measures: [{ name: 'n', aggregate: 'count' }],
1333+
},
1334+
],
1335+
});
1336+
const found = virtualFindings(joined(FORMULA_TEXT));
1337+
expect(found).toHaveLength(1);
1338+
expect(found[0].message).toContain('"account.tier" (`formula` on joined object "crm_account")');
1339+
expect(found[0].hint).toContain('a stored field of "crm_account"');
1340+
expect(virtualFindings(joined({ type: 'text' }))).toEqual([]);
1341+
});
1342+
1343+
it('refuses a cube dimension whose sql names a formula column, and accepts a stored one', () => {
1344+
const stack = cubeStack({ dimensions: { calc: dim('calc'), stage: dim('stage') } }, {}, { calc: FORMULA_TEXT });
1345+
const found = validateDatasetMeasureAggregates(stack);
1346+
expect(found).toHaveLength(1);
1347+
expect(found[0].rule).toBe(VIRTUAL_RULE);
1348+
expect(found[0].path).toBe('analyticsCubes[0].dimensions.calc.sql');
1349+
expect(found[0].where).toBe('cube "fx_cube" › dimension "calc"');
1350+
expect(found[0].message).toContain('(`formula` on object "fx_ledger")');
1351+
expect(validateDatasetMeasureAggregates(cubeStack({ dimensions: { stage: dim('stage') } }))).toEqual([]);
1352+
});
1353+
1354+
it('never hands the predicate a guess — the same skips as the JSON-stored leg', () => {
1355+
expect(virtualFindings(dimensionStack(FORMULA_TEXT, { datasetObject: 'not_here' }))).toEqual([]);
1356+
expect(virtualFindings(dimensionStack(FORMULA_TEXT, { field: 'nope' }))).toEqual([]);
1357+
expect(virtualFindings(dimensionStack(FORMULA_TEXT, { field: undefined }))).toEqual([]);
1358+
});
1359+
1360+
it('leaves the measure leg over a formula as it was: a count accepted, a total refused under the measure id', () => {
1361+
expect(findings(stackWith('count', 'formula'))).toEqual([]);
1362+
const [total] = findings(stackWith('sum', 'formula'));
1363+
expect(total?.hint).toBe(VIRTUAL_FIELD_AGGREGATE_REMEDY);
1364+
expect(validateDatasetMeasureAggregates(stackWith('sum', 'formula')).map((f) => f.rule)).toEqual([RULE]);
1365+
});
1366+
1367+
it('fires through runAuthoringRules on all three commands, and only on the refused dimension', () => {
1368+
const stack = dimensionStack(FORMULA_TEXT);
1369+
for (const command of ['validate', 'build', 'lint'] as const) {
1370+
const found = runAuthoringRules(command, { normalized: stack, parsed: stack }).filter(
1371+
(f) => f.rule === VIRTUAL_RULE,
1372+
);
1373+
expect(found, command).toHaveLength(1);
1374+
expect(found[0].severity, command).toBe('error');
1375+
}
1376+
const control = dimensionStack({ type: 'text' });
1377+
expect(
1378+
runAuthoringRules('lint', { normalized: control, parsed: control }).filter((f) => f.rule === VIRTUAL_RULE),
1379+
).toEqual([]);
1380+
});
1381+
});
1382+
1383+
describe('[#22161] one-line verdicts — the three ids', () => {
1384+
// [#30773] `dimension-virtual-field-refused` joined the two #22161 shaped.
1385+
const IDS = [MEASURE_AGGREGATE_FIELD_TYPE_REFUSED, DIMENSION_JSON_STORED_FIELD_REFUSED, DIMENSION_VIRTUAL_FIELD_REFUSED];
12421386

12431387
it('every verdict the cases above fired is one line of at most 200 characters', () => {
12441388
// The coverage control first: each id fired at least once, so the shape
@@ -1254,9 +1398,10 @@ describe('[#22161] one-line verdicts — the two ids', () => {
12541398
const MOVED: Record<string, readonly string[]> = {
12551399
[MEASURE_AGGREGATE_FIELD_TYPE_REFUSED]: ['property of the SQL dialect', 'average YEAR', 'compares values for equality', '400 DATASET_INVALID', '`measure-aggregate-incoherent`'],
12561400
[DIMENSION_JSON_STORED_FIELD_REFUSED]: ['GROUP KEY', 'one groups each serialized value apart', 'before any SQL is built', 'instead of an answer'],
1401+
[DIMENSION_VIRTUAL_FIELD_REFUSED]: ['GROUP KEY', '`400 INVALID_FIELD` at `groupBy.N`', '`400 DATASET_INVALID`', 'one null group'],
12571402
};
12581403

1259-
it('covers exactly the two ids', () => {
1404+
it('covers exactly the three ids', () => {
12601405
expect(Object.keys(MOVED).sort()).toEqual([...IDS].sort());
12611406
});
12621407

@@ -1283,8 +1428,8 @@ describe('[#22161] one-line verdicts — the two ids', () => {
12831428
expect(text).toContain(`\`min\` and \`max\`: ${table.min.join(', ')}.`);
12841429
});
12851430

1286-
it('both explanations name the JSON-stored types the spec declares', () => {
1287-
for (const rule of IDS) {
1431+
it('both JSON-stored explanations name the JSON-stored types the spec declares', () => {
1432+
for (const rule of [MEASURE_AGGREGATE_FIELD_TYPE_REFUSED, DIMENSION_JSON_STORED_FIELD_REFUSED]) {
12881433
const text = explanationOf(rule);
12891434
expect(text, rule).toContain(`a structured-JSON type — ${[...STRUCTURED_JSON_TYPES].join(', ')} —`);
12901435
expect(text, rule).toContain(`an inherently multi option type (${[...MULTI_OPTION_TYPES].join(', ')})`);
@@ -1297,8 +1442,8 @@ describe('[#22161] one-line verdicts — the two ids', () => {
12971442
// declaration accepts, and the dimension id its one remedy per class; why
12981443
// `count` reaches further, the stored-numeric route, the doors that refuse the
12991444
// pair later and the per-member `$contains` query are the explanations'.
1300-
describe('[#22161] one-line fixes — the two ids', () => {
1301-
const IDS = [MEASURE_AGGREGATE_FIELD_TYPE_REFUSED, DIMENSION_JSON_STORED_FIELD_REFUSED];
1445+
describe('[#22161] one-line fixes — the three ids', () => {
1446+
const IDS = [MEASURE_AGGREGATE_FIELD_TYPE_REFUSED, DIMENSION_JSON_STORED_FIELD_REFUSED, DIMENSION_VIRTUAL_FIELD_REFUSED];
13021447

13031448
it('every `fix:` line the cases above fired is one line of at most 200 characters', () => {
13041449
const converted = fired.filter((f) => IDS.includes(f.rule));
@@ -1310,6 +1455,8 @@ describe('[#22161] one-line fixes — the two ids', () => {
13101455
const dimension = converted.filter((f) => f.rule === DIMENSION_JSON_STORED_FIELD_REFUSED).map((f) => f.hint);
13111456
expect(dimension.some((h) => h.startsWith('Group by a field that stores one scalar value'))).toBe(true);
13121457
expect(dimension.some((h) => h.includes('`$contains`'))).toBe(true);
1458+
const virtual = converted.filter((f) => f.rule === DIMENSION_VIRTUAL_FIELD_REFUSED).map((f) => f.hint);
1459+
expect(virtual.some((h) => h.startsWith('Store the value in a stored field'))).toBe(true);
13131460
for (const f of converted) {
13141461
expect(f.hint, f.rule).not.toContain('\n');
13151462
expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200);
@@ -1330,6 +1477,10 @@ describe('[#22161] one-line fixes — the two ids', () => {
13301477
'`where { FIELD: { $contains: VALUE } }`',
13311478
'one query per member',
13321479
],
1480+
[DIMENSION_VIRTUAL_FIELD_REFUSED]: [
1481+
'store it in a field of its own on the object',
1482+
'point the dimension at that stored field',
1483+
],
13331484
};
13341485
for (const rule of IDS) {
13351486
for (const fact of facts[rule]) expect(explanationOf(rule), `${rule} explanation names ${fact}`).toContain(fact);

0 commit comments

Comments
 (0)