diff --git a/.changeset/dataset-authoring-form.md b/.changeset/dataset-authoring-form.md new file mode 100644 index 0000000000..f8bd5ce73e --- /dev/null +++ b/.changeset/dataset-authoring-form.md @@ -0,0 +1,20 @@ +--- +"@objectstack/spec": minor +--- + +feat(spec): dataset authoring form + derived measures without a dummy aggregate + +`dataset` was the only UI-authorable metadata type without a `defineForm` +layout, so Studio's create surface fell back to the auto-generated flat layout +(free-text `object`, no grouping). Adds `dataset.form.ts` (registered in +`METADATA_FORM_REGISTRY`): sectioned Basics / Source / Dimensions / Measures +with an `object` picker (`ref:object`) and guidance — matching the sibling +`report` editor. + +Also makes `DatasetMeasureSchema.aggregate` optional. A derived measure +(`derived: { op, of }`) combines other measures by name and `aggregate` is +ignored for it at compile time, but the schema still required it — so a derived +measure failed validation unless you added a meaningless aggregate. `aggregate` +is now required only for non-derived measures (enforced in the existing +`superRefine`). Backward compatible: existing measures that carry an aggregate +stay valid. diff --git a/.changeset/dataset-join-ambiguous-column.md b/.changeset/dataset-join-ambiguous-column.md new file mode 100644 index 0000000000..e909465afd --- /dev/null +++ b/.changeset/dataset-join-ambiguous-column.md @@ -0,0 +1,13 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(analytics): qualify base-object columns in joined dataset queries + +A dataset that joins a related object (`include` + a `relationship.field` +dimension/measure) emitted BARE base-table columns in SELECT/GROUP BY while the +joined columns were alias-qualified. When the base and joined tables share a +column name (e.g. both have `status`), the query failed at runtime with +"ambiguous column name". `NativeSQLStrategy` now qualifies plain base-column +identifiers with the base table when the cube has joins; single-object cubes +are unchanged (byte-for-byte identical SQL). diff --git a/packages/services/service-analytics/src/__tests__/native-sql-rls.test.ts b/packages/services/service-analytics/src/__tests__/native-sql-rls.test.ts index a2aca608d8..34d46bb15c 100644 --- a/packages/services/service-analytics/src/__tests__/native-sql-rls.test.ts +++ b/packages/services/service-analytics/src/__tests__/native-sql-rls.test.ts @@ -108,3 +108,51 @@ describe('NativeSQLStrategy — D-C RLS hardening', () => { expect(sql).toContain('$2'); }); }); + +describe('NativeSQLStrategy — base-column qualification under joins', () => { + // Regression: a joined dataset whose base table and joined table share a + // column name (e.g. `status`) produced `GROUP BY status` → "ambiguous column + // name" at runtime. Base columns must be qualified with the base table when + // the cube can join. (Found by dogfooding a joined dataset in Studio.) + const joinCube: Cube = { + name: 'sales', + title: 'Sales', + sql: 'opportunity', + measures: { revenue: { name: 'revenue', label: 'Revenue', type: 'sum', sql: 'amount' } }, + dimensions: { + status: { name: 'status', label: 'Status', type: 'string', sql: 'status' }, + region: { name: 'region', label: 'Region', type: 'string', sql: 'account.region' }, + }, + joins: { account: { name: 'account', relationship: 'many_to_one', sql: '' } }, + public: false, + }; + + it('qualifies a base-table dimension with the base table when the cube has joins', async () => { + const strategy = new NativeSQLStrategy(); + const ctx = ctxWith({ + getCube: (n) => (n === 'sales' ? joinCube : undefined), + getAllowedRelationships: () => new Set(['account']), + }); + const q: AnalyticsQuery = { cube: 'sales', measures: ['revenue'], dimensions: ['status', 'region'], timezone: 'UTC' }; + const { sql } = await strategy.generateSql(q, ctx); + expect(sql).toContain('"opportunity"."status"'); // base column qualified (was bare `status`) + expect(sql).toContain('"account"."region"'); // relationship column still qualified + expect(sql).toContain('LEFT JOIN "account"'); + expect(sql).not.toMatch(/GROUP BY\s+status\b/); // no bare, ambiguous base column + }); + + it('leaves base columns BARE for a single-object cube (no joins) — generated SQL unchanged', async () => { + const soloCube: Cube = { + name: 'tasks', title: 'Tasks', sql: 'task', + measures: { c: { name: 'c', label: 'Count', type: 'count', sql: '*' } }, + dimensions: { status: { name: 'status', label: 'Status', type: 'string', sql: 'status' } }, + public: false, + }; + const strategy = new NativeSQLStrategy(); + const ctx = ctxWith({ getCube: (n) => (n === 'tasks' ? soloCube : undefined) }); + const q: AnalyticsQuery = { cube: 'tasks', measures: ['c'], dimensions: ['status'], timezone: 'UTC' }; + const { sql } = await strategy.generateSql(q, ctx); + expect(sql).toContain('GROUP BY status'); // bare — unchanged for single-object cubes + expect(sql).not.toContain('"task"."status"'); + }); +}); diff --git a/packages/services/service-analytics/src/dataset-compiler.ts b/packages/services/service-analytics/src/dataset-compiler.ts index 083573d7eb..da273e5cb7 100644 --- a/packages/services/service-analytics/src/dataset-compiler.ts +++ b/packages/services/service-analytics/src/dataset-compiler.ts @@ -58,6 +58,11 @@ export type RelationshipResolver = ( /** Map a dataset measure's aggregate to the Cube metric `type`. */ function aggregateToMetricType(m: DatasetMeasure): Metric['type'] { + // Only reached for non-derived measures, where the spec refinement guarantees + // an aggregate; guard defensively so the type narrows from `optional`. + if (!m.aggregate) { + throw new Error(`[dataset-compiler] non-derived measure "${m.name}" has no aggregate`); + } if (UNSUPPORTED_AGGREGATES.has(m.aggregate)) { throw new Error( `[dataset-compiler] measure "${m.name}" uses aggregate "${m.aggregate}" which is ` + diff --git a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts index 03668fa328..d25aff5a52 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -234,7 +234,19 @@ export class NativeSQLStrategy implements AnalyticsStrategy { joins: Map, cube?: Cube, ): string { - if (!rawSql.includes('.')) return rawSql; + if (!rawSql.includes('.')) { + // Base-table column. When the cube can join other tables, a bare column + // that also exists on a joined table (e.g. base `status` vs joined + // `account.status`) makes the SQL engine raise "ambiguous column name". + // Qualify plain identifiers with the base table; leave SQL expressions + // and `*` untouched. Single-object cubes (no joins) keep bare columns so + // their generated SQL is byte-for-byte unchanged. + const canJoin = !!cube?.joins && Object.keys(cube.joins).length > 0; + if (canJoin && /^[A-Za-z_][A-Za-z0-9_]*$/.test(rawSql)) { + return `"${parentTable}"."${rawSql}"`; + } + return rawSql; + } // Only the first dotted hop is supported (single-level relation). const [alias, ...rest] = rawSql.split('.'); if (!alias || rest.length === 0) return rawSql; diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 43d8ce822c..78b495f336 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -3342,6 +3342,7 @@ "actionForm (const)", "appForm (const)", "dashboardForm (const)", + "datasetForm (const)", "defineAction (function)", "defineApp (function)", "defineDataset (function)", diff --git a/packages/spec/src/system/metadata-form-registry.ts b/packages/spec/src/system/metadata-form-registry.ts index 0d81b5234f..fb3114fb0c 100644 --- a/packages/spec/src/system/metadata-form-registry.ts +++ b/packages/spec/src/system/metadata-form-registry.ts @@ -37,6 +37,7 @@ import { viewForm, appForm, dashboardForm, + datasetForm, actionForm, pageForm, reportForm, @@ -67,6 +68,7 @@ export const METADATA_FORM_REGISTRY: Readonly> = Object field: fieldForm, hook: hookForm, report: reportForm, + dataset: datasetForm, view: viewForm, app: appForm, dashboard: dashboardForm, diff --git a/packages/spec/src/ui/dataset.form.ts b/packages/spec/src/ui/dataset.form.ts new file mode 100644 index 0000000000..63267aa22f --- /dev/null +++ b/packages/spec/src/ui/dataset.form.ts @@ -0,0 +1,63 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { defineForm } from './view.zod'; + +/** + * Form layout for the Dataset metadata editor (ADR-0021 analytics semantic layer). + * + * Bound to {@link DatasetSchema}. Until this entry existed, `dataset` was the + * only UI-authorable metadata type WITHOUT a registered {@link FormView}, so the + * metadata-admin create surface fell back to the auto-generated single-section + * layout. That fallback (a) silently DROPPED the optional `include` (joins) and + * `filter` (intrinsic scope) fields — making joined / scoped datasets + * un-authorable online — and (b) rendered the base `object` and dimension/ + * measure `field`s as bare free-text inputs with no object context. + * + * This layout restores `include` + `filter`, groups the surface into sections + * with guidance, and uses pickers (`ref:object`, `filter-builder` scoped to the + * base object) so a business user can author a dataset without memorising + * machine names. Mirrors {@link reportForm} — the sibling analytics editor. + */ +export const datasetForm = defineForm({ + schemaId: 'dataset', + type: 'simple', + sections: [ + { + name: 'basics', + label: 'Basics', + description: 'Dataset identity.', + columns: 2, + fields: [ + { field: 'name', type: 'text', colSpan: 1, required: true, immutable: true, helpText: 'snake_case unique identifier' }, + { field: 'label', type: 'text', colSpan: 1, required: true, helpText: 'Display name' }, + { field: 'description', type: 'textarea', colSpan: 2, helpText: 'What this dataset measures' }, + ], + }, + { + name: 'source', + label: 'Source', + description: 'The base object, the relationships to join, and the dataset’s intrinsic scope. Joins are derived from the object graph — pick relationship (lookup / master_detail) names, never write an ON clause.', + fields: [ + { field: 'object', widget: 'ref:object', required: true, helpText: 'Base object — the FROM' }, + { field: 'include', widget: 'string-tags', helpText: 'Relationship (lookup / master_detail) field names to join — enables `relationship.field` dimensions/measures (e.g. include "account" → group by account.region)' }, + { field: 'filter', widget: 'filter-builder', dependsOn: 'object', helpText: 'Intrinsic scope filter (e.g. exclude soft-deleted records), ANDed into every query' }, + ], + }, + { + name: 'dimensions', + label: 'Dimensions', + description: 'Groupable axes. Use a base field, or `relationship.field` (e.g. account.region) for a relationship included above.', + fields: [ + { field: 'dimensions', type: 'repeater', required: true, helpText: 'Each: name (referenced by presentations), field, type, and — for dates — a default bucketing granularity' }, + ], + }, + { + name: 'measures', + label: 'Measures', + description: 'Aggregatable values defined once and referenced by name. A measure is sum/avg/count/… of a field; a derived measure combines other measures (ratio/sum/difference/product). Measure-scoped filters and derived ops are edited per-row in the dataset designer.', + fields: [ + { field: 'measures', type: 'repeater', required: true, helpText: 'Each: name, aggregate, field (optional for count), display format/currency, and a “certified” governance flag' }, + ], + }, + ], +}); diff --git a/packages/spec/src/ui/dataset.test.ts b/packages/spec/src/ui/dataset.test.ts index 88cf52d16c..b8d3264612 100644 --- a/packages/spec/src/ui/dataset.test.ts +++ b/packages/spec/src/ui/dataset.test.ts @@ -88,3 +88,26 @@ describe('DatasetSchema', () => { expect(d).toBe(base); }); }); + +describe('DatasetSchema — derived measure aggregate optionality', () => { + // Regression: a derived measure had to carry a meaningless `aggregate` or + // validation failed, even though `aggregate` is ignored for derived measures. + it('accepts a derived measure with NO aggregate', () => { + expect(() => + DatasetSchema.parse({ + ...base, + measures: [ + { name: 'won_amount', aggregate: 'sum', field: 'amount' }, + { name: 'win_rate', derived: { op: 'ratio', of: ['won_amount', 'revenue'] } }, + { name: 'revenue', aggregate: 'sum', field: 'amount' }, + ], + }), + ).not.toThrow(); + }); + + it('still rejects a NON-derived measure with no aggregate', () => { + expect(() => + DatasetSchema.parse({ ...base, measures: [{ name: 'revenue', field: 'amount' }] }), + ).toThrowError(/requires `aggregate`/); + }); +}); diff --git a/packages/spec/src/ui/dataset.zod.ts b/packages/spec/src/ui/dataset.zod.ts index 9a9ea54a92..0f8f3570b9 100644 --- a/packages/spec/src/ui/dataset.zod.ts +++ b/packages/spec/src/ui/dataset.zod.ts @@ -65,7 +65,7 @@ export const DatasetMeasureSchema = lazySchema(() => z.object({ name: SnakeCaseIdentifierSchema.describe('Measure name — e.g. "revenue"; defined once'), label: I18nLabelSchema.optional(), /** Aggregation function — reuses the canonical query.zod enum. */ - aggregate: AggregationFunction.describe('Aggregation (sum/avg/count/...)'), + aggregate: AggregationFunction.optional().describe('Aggregation (sum/avg/count/...); omit when `derived` is set'), /** Base or `relationship.field`. Optional for `count` (count(*)). */ field: z.string().optional().describe('Aggregated field; optional for count(*)'), /** Measure-scoped filter (e.g. only won deals for "won_amount"). */ @@ -152,8 +152,16 @@ export const DatasetSchema = lazySchema(() => z.object({ // Derived measures may only reference OTHER measures declared in this dataset. for (const m of ds.measures) { if (!m.derived) { - // A non-derived measure needs a field unless it is a plain count. - if (!m.field && m.aggregate !== 'count') { + // A non-derived measure must declare an aggregate (a derived measure + // omits it — it combines other measures by name instead). + if (!m.aggregate) { + ctx.addIssue({ + code: 'custom', + message: `measure "${m.name}" requires \`aggregate\` (or a \`derived\` spec)`, + path: ['measures'], + }); + } else if (!m.field && m.aggregate !== 'count') { + // A non-derived measure needs a field unless it is a plain count. ctx.addIssue({ code: 'custom', message: `measure "${m.name}" requires \`field\` (only \`count\` may omit it)`, diff --git a/packages/spec/src/ui/index.ts b/packages/spec/src/ui/index.ts index c1ea9636cc..2b537db3e2 100644 --- a/packages/spec/src/ui/index.ts +++ b/packages/spec/src/ui/index.ts @@ -22,6 +22,7 @@ export { reportForm } from './report.form'; export { viewForm } from './view.form'; export { appForm } from './app.form'; export { dashboardForm } from './dashboard.form'; +export { datasetForm } from './dataset.form'; export { actionForm } from './action.form'; export { pageForm } from './page.form'; export * from './action.zod';