diff --git a/.changeset/objectql-crossobject-conjunct-refusal.md b/.changeset/objectql-crossobject-conjunct-refusal.md new file mode 100644 index 0000000000..8d67f7dbd3 --- /dev/null +++ b/.changeset/objectql-crossobject-conjunct-refusal.md @@ -0,0 +1,50 @@ +--- +"@objectstack/service-analytics": minor +--- + +**BREAKING**: `/analytics/query` now refuses a cross-object filter nested inside a +combinator on the ObjectQL path, instead of silently answering the wrong number +(#10759). + +`ObjectQLStrategy` runs one cross-object envelope check, from two call sites. +`generateSql()` (the `/analytics/sql` preview) asked it about every member the +`where` touches, flattened out of the filter tree. `execute()` asked it about the +built engine filter — where an AND-ed leaf sits at the top level and is seen, but +anything structural (an `$or`, a `$not`, a nested `$and` that cannot merge) has +been folded into `filter.$and`, so the only key readable for it was the literal +`$and`, which is never a field name. + +One query therefore got two answers, measured over one fixture in one run: + +``` +where: { $or: [{ 'account.region': 'West' }, { stage: 'won' }] } + +before /analytics/sql 400 INVALID_FIELD cross-object filter "account.region" + /analytics/query 200, rows +after both 400 INVALID_FIELD cross-object filter "account.region" +``` + +`engine.aggregate` cannot join. The half that returned rows was not answering the +cross-object query: the disjunct naming a column the base object does not have +can never match, so the query silently collapsed to its remaining branches and +reported a narrower figure as if it were the answer. Both call sites now derive +the member list from one shared view, so the invariant the strategy already +stated for itself — the preview accepts and rejects the same set the execution +door does — holds by construction rather than by two call sites agreeing. + +Who is affected: a deployment whose driver reports `objectqlAggregate` but not +`nativeSql` (Mongo, the memory driver), running an analytics query that puts a +related object's field inside `$or` or `$not`. Such a query now returns +`400 INVALID_FIELD` naming the member. The refusal already existed and already +had these words; what changed is that the execution door reaches it too. Nothing +an author writes in metadata changes, no stored shape is affected, and queries +whose combinators name only base-object fields are untouched — that set is pinned +in `crossobject-conjunct-refusal.test.ts` alongside the new refusal, because a +fix that refused every combinator would have looked identical from the refusal +side alone. + +The remedy for an affected query is the one the error message has always carried: +run it on a native-SQL driver, which can join, or drop the cross-object member +from the filter. + + diff --git a/packages/services/service-analytics/src/__tests__/crossobject-conjunct-refusal.test.ts b/packages/services/service-analytics/src/__tests__/crossobject-conjunct-refusal.test.ts new file mode 100644 index 0000000000..305195af83 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/crossobject-conjunct-refusal.test.ts @@ -0,0 +1,309 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10759] `ObjectQLStrategy`'s two doors judge one query by ONE member view. + * + * ## What was wrong, measured on `origin/main` before the change + * + * `planCrossObject` reads `Object.keys(filter)` and nothing else, and the two + * call sites handed it different things. `generateSql()` handed it every member + * the `where` touches, flattened out of the tree. `execute()` handed it the + * built ENGINE FILTER — where an AND-ed leaf sits at the top level and is seen, + * but anything structural (`$or`, `$not`, a nested `$and` that cannot merge) has + * been folded into `filter.$and`, so the only key readable for it was the + * literal `$and`, which is never a field name. + * + * So one query got two answers. Measured, both doors fired in one run over one + * fixture (`where: { $or: [{ 'account.region': 'West' }, { stage: 'won' }] }`): + * + * ``` + * BEFORE execute() ACCEPTED -> engine.aggregate got + * {"$and":[{"$or":[{"account.region":"West"},…]}]} + * generateSql() REFUSED cannot evaluate a cross-object filter ("account.region") + * AFTER execute() REFUSED (same message, same INVALID_FIELD / 400) + * generateSql() REFUSED unchanged + * ``` + * + * `engine.aggregate` cannot join. The accepted half did not answer a + * cross-object query — it answered a NARROWER one, silently, because the branch + * naming a column the base object does not have can never match. That is the + * silent mis-bucket #3654's loud refusal exists to prevent, and the file already + * stated the invariant it was breaking: *"`generateSql()` calls this too, so the + * preview accepts/rejects the same set."* + * + * ## Why this file pins FOUR directions, not one + * + * Pinning only the new refusal would go green on an implementation that refuses + * every combinator — which would break every legitimate `$or` query shipping + * today. So the accepting neighbours are pinned in the same file, one character + * away from the refused ones: + * + * ① a cross-object member nested in `$or` / `$not` is REFUSED on both doors + * ② a combinator with NO cross-object member still passes both doors, and + * still reaches `engine.aggregate` carrying its disjunction + * ③ the `generateSql()` door is UNCHANGED — it was already right, and the + * top-level case it always refused is refused with the same words + * ④ the #10413-phase-1 dataset-level `filter` conjunct (PR #10758) is not + * misread as a cross-object reference — an ordinary definition-level scope + * travels in `$and` exactly like a combinator does, and reads as a member + * of nothing + * + * ## The scope line this file also draws + * + * A dataset whose DEFINITION-LEVEL filter is itself cross-object is accepted by + * BOTH doors, before and after this change — neither call site's view contains + * the dataset scope. The doors AGREE there, so it is not the invariant this card + * restores; it is a separate defect and is pinned here as measured-and-known + * rather than left to be rediscovered. See the last block. + */ + +import { describe, it, expect } from 'vitest'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import type { AnalyticsQuery } from '@objectstack/spec/contracts'; +import { DatasetSchema, type Dataset } from '@objectstack/spec/ui'; +import { AnalyticsService } from '../analytics-service.js'; + +const ctxA = { tenantId: 'org_A', userId: 'u_a' } as ExecutionContext; + +interface Refusal extends Error { code?: string; status?: number; member?: string; param?: string } +interface AggCall { object: string; filter?: unknown } + +/** A cube with one base dimension, one cross-object dimension, one base measure. */ +const SALES_BY_ACCOUNT: Dataset = DatasetSchema.parse({ + name: 'sales_by_account', + label: 'Sales by account', + object: 'opportunity', + include: ['account'], + dimensions: [ + { name: 'stage', field: 'stage', type: 'string' }, + { name: 'region', field: 'account.region', type: 'string' }, + ], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}) as Dataset; + +/** + * The #10413 phase-1 shape: the SAME cube plus a definition-level `filter`. + * PR #10758 pushes that filter onto `execute()`'s `conjuncts` list, so it lands + * inside `filter.$and` — the very place a combinator lands. Its presence must + * not by itself make a query look cross-object. + */ +const SCOPED_SALES: Dataset = DatasetSchema.parse({ + name: 'scoped_sales', + label: 'Scoped sales', + object: 'opportunity', + include: ['account'], + filter: { is_deleted: false }, + dimensions: [{ name: 'stage', field: 'stage', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}) as Dataset; + +/** A dataset whose definition-level filter is ITSELF cross-object. */ +const XOBJ_SCOPED_SALES: Dataset = DatasetSchema.parse({ + name: 'xobj_scoped_sales', + label: 'Cross-object scoped sales', + object: 'opportunity', + include: ['account'], + filter: { 'account.region': 'West' }, + dimensions: [{ name: 'stage', field: 'stage', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}) as Dataset; + +/** + * `nativeSql: false` makes `NativeSQLStrategy` decline, so every query below + * routes to `ObjectQLStrategy` — the door this card is about. + * + * The stub RETURNS ROWS rather than throwing, so a refusal that failed to fire + * produces a passing-looking success with garbage in it: a green rejection test + * here proves the guard, not luck. + */ +function serviceFor(defs: Dataset[]) { + const calls: AggCall[] = []; + const svc = new AnalyticsService({ + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async (object: string, options: { filter?: unknown }) => { + calls.push({ object, filter: options.filter }); + return [{ stage: 'won', revenue: 42 }]; + }, + }); + for (const d of defs) svc.registerDataset(d); + return { svc, calls }; +} + +async function refusalFrom(thunk: () => Promise): Promise { + try { + await thunk(); + return undefined; + } catch (e) { + return e as Refusal; + } +} + +/** Both doors, one query, one tree — the disagreement is a measurement. */ +async function bothDoors(cube: string, query: Omit, defs: Dataset[]) { + const { svc, calls } = serviceFor(defs); + const q = { ...query, cube } as AnalyticsQuery; + return { + execute: await refusalFrom(() => svc.query(q, ctxA)), + generateSql: await refusalFrom(() => svc.generateSql(q, ctxA)), + calls, + }; +} + +const CROSS_OBJECT_MESSAGE = /cannot evaluate a cross-object filter \("account\.region"\)/; + +// ───────────────────────────────────────────────────────────────────────────── +// ① the refusal that was missing on the execution door +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Each entry nests the SAME cross-object member one level down, in a different + * combinator, so the pin is on "structure hides the member" rather than on the + * `$or` spelling alone. + */ +const NESTED: Array<{ name: string; where: Record }> = [ + { name: '$or', where: { $or: [{ 'account.region': 'West' }, { stage: 'won' }] } }, + { name: '$not', where: { $not: { 'account.region': 'West' } } }, + { name: '$or nested two deep', where: { $or: [{ $and: [{ 'account.region': 'West' }, { stage: 'won' }] }, { stage: 'lost' }] } }, +]; + +describe('[#10759] a cross-object member nested in a combinator is refused on BOTH doors', () => { + for (const c of NESTED) { + it(`${c.name}: execute() refuses with the ADR-0112 envelope`, async () => { + const { execute, calls } = await bothDoors('sales_by_account', { + dimensions: ['stage'], measures: ['revenue'], where: c.where, + }, [SALES_BY_ACCOUNT]); + + expect(execute, 'accepted — the member was invisible to the envelope check').toBeInstanceOf(Error); + expect(String(execute?.message)).toMatch(CROSS_OBJECT_MESSAGE); + // Read exactly as `rest-server.ts`'s catch reads them: a 4xx status AND a + // code, or the route falls through to 500 ANALYTICS_QUERY_FAILED. Asserting + // only that it throws would pass on a bare `Error` and report the platform + // broken for a caller mistake. + expect(execute?.code, 'no `code` ⇒ 500 ANALYTICS_QUERY_FAILED').toBe('INVALID_FIELD'); + expect(execute?.status, 'no `status` ⇒ 500 ANALYTICS_QUERY_FAILED').toBe(400); + // The member is named as the REQUEST spelled it, and `where` is the key to + // go fix — the refusal is actionable without reading this file. + expect(execute?.member).toBe('account.region'); + expect(execute?.param).toBe('where'); + // Refused BEFORE the engine was asked, not after it mis-bucketed. + expect(calls, 'engine.aggregate was reached — it cannot join').toEqual([]); + }); + + it(`${c.name}: both doors agree`, async () => { + const { execute, generateSql } = await bothDoors('sales_by_account', { + dimensions: ['stage'], measures: ['revenue'], where: c.where, + }, [SALES_BY_ACCOUNT]); + // The invariant `planCrossObject` states for itself, asserted as one fact + // about one query rather than as two independent expectations. + expect( + [execute === undefined, generateSql === undefined], + 'the preview and the execution door accept/reject the same set', + ).toEqual([false, false]); + expect(String(generateSql?.message)).toMatch(CROSS_OBJECT_MESSAGE); + }); + } + + it('the KNOWN-PRESENT control: the same member at the top level was always refused', async () => { + // The counter-check for every "refused" above. This shape predates #10759 and + // is refused on both doors before AND after it — so it proves the fixture, + // the cube and the detection path work, and cannot be read as evidence for + // the change. The nested rows above are what moved. + const { execute, generateSql } = await bothDoors('sales_by_account', { + dimensions: ['stage'], measures: ['revenue'], where: { 'account.region': 'West' }, + }, [SALES_BY_ACCOUNT]); + expect(String(execute?.message)).toMatch(CROSS_OBJECT_MESSAGE); + expect(String(generateSql?.message)).toMatch(CROSS_OBJECT_MESSAGE); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// ② the accepting neighbours — no combinator was refused wholesale +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#10759] a combinator with NO cross-object member still passes', () => { + const CLEAN: Array<{ name: string; where: Record }> = [ + { name: '$or over base fields', where: { $or: [{ stage: 'won' }, { stage: 'lost' }] } }, + { name: '$not over a base field', where: { $not: { stage: 'won' } } }, + { name: 'a mixed tree over base fields', where: { $or: [{ $and: [{ stage: 'won' }, { amount: 5 }] }, { stage: 'lost' }] } }, + ]; + + for (const c of CLEAN) { + it(`${c.name}: accepted on both doors`, async () => { + const { execute, generateSql, calls } = await bothDoors('sales_by_account', { + dimensions: ['stage'], measures: ['revenue'], where: c.where, + }, [SALES_BY_ACCOUNT]); + expect(execute, `execute() refused a clean combinator: ${execute?.message}`).toBeUndefined(); + expect(generateSql, `generateSql() refused a clean combinator: ${generateSql?.message}`).toBeUndefined(); + // Reached the engine, and reached it carrying the disjunction — a refusal + // is not the only way to break these queries; dropping the predicate would + // widen the answer just as silently. + expect(calls).toHaveLength(1); + expect(JSON.stringify(calls[0].filter)).toContain('$'); + }); + } + + it('an in-envelope cross-object DIMENSION still compiles — only FILTERS were widened', async () => { + // `region` resolves to `account.region` and is served by FK-expand. If the + // new view had been read as "any cross-object member anywhere", this would + // have started failing too. + const { generateSql } = await bothDoors('sales_by_account', { + dimensions: ['region'], measures: ['revenue'], + }, [SALES_BY_ACCOUNT]); + expect(generateSql).toBeUndefined(); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// ③ + ④ the #10758 dataset-scope conjunct +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#10759] the #10413-phase-1 dataset filter conjunct is not misread', () => { + it('an ordinary definition-level filter is accepted and still reaches the engine', async () => { + const { execute, generateSql, calls } = await bothDoors('scoped_sales', { + dimensions: ['stage'], measures: ['revenue'], + }, [SCOPED_SALES]); + expect(execute, `execute() refused a scoped dataset: ${execute?.message}`).toBeUndefined(); + expect(generateSql).toBeUndefined(); + // PR #10758's own guarantee, re-pinned from this side: the scope travels as + // an `$and` conjunct, which is exactly the position a combinator occupies — + // so this is the pin that a "refuse anything under `$and`" implementation + // would fail. + expect(JSON.stringify(calls[0]?.filter)).toContain('is_deleted'); + }); + + it('a dataset scope does not shield a cross-object member in the caller’s own $or', async () => { + const { execute, generateSql, calls } = await bothDoors('scoped_sales', { + dimensions: ['stage'], measures: ['revenue'], + where: { $or: [{ 'account.region': 'West' }, { stage: 'won' }] }, + }, [SCOPED_SALES]); + expect(String(execute?.message)).toMatch(CROSS_OBJECT_MESSAGE); + expect(String(generateSql?.message)).toMatch(CROSS_OBJECT_MESSAGE); + expect(calls).toEqual([]); + }); + + /** + * MEASURED AND DELIBERATELY LEFT OPEN — not a latent pass. + * + * A cross-object DEFINITION-LEVEL filter is accepted by both doors, and + * `engine.aggregate` receives `{"$and":[{"account.region":"West"}]}`, which it + * cannot join. PR #10758 created this instance by giving the dataset scope a + * route onto the ObjectQL door at all; #10759 is not it, because the two doors + * AGREE here — neither call site's member view contains the dataset scope, so + * there is no preview/execution divergence to restore. + * + * Filed separately rather than widened into this PR: refusing it is a real + * decision (query-time refusal versus a compile-time rejection in + * `dataset-compiler.ts`, which is the contract-first placement), and it is not + * the invariant this file restores. The expectation below is written to the + * behaviour as it IS, so the day that decision lands this pin goes red and + * points at the paragraph explaining why. + */ + it('a CROSS-OBJECT definition-level filter is still accepted by both doors (filed separately)', async () => { + const { execute, generateSql, calls } = await bothDoors('xobj_scoped_sales', { + dimensions: ['stage'], measures: ['revenue'], + }, [XOBJ_SCOPED_SALES]); + expect(execute).toBeUndefined(); + expect(generateSql).toBeUndefined(); + expect(JSON.stringify(calls[0]?.filter)).toContain('account.region'); + }); +}); diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index 96b2bbb687..720c84d0f2 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -194,7 +194,20 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // multi-hop, non-recombinable measures) is REJECTED by `planCrossObject` — // the engine has no join, and a silent mis-bucket is worse than a loud // error. `null` ⇒ the query is base-only and takes the direct path below. - const plan = this.planCrossObject(cube, query, filter); + // + // [#10759] Judged on {@link filterMemberView} — EVERY member the `where` + // touches — and NOT on the engine filter built above. The engine filter is + // the wrong instrument for this question: an AND-ed leaf lands at its top + // level and is seen, but anything structural (an `$or`, a `$not`, a nested + // `$and` that cannot merge) is folded into `filter.$and`, so the only key + // `planCrossObject` could see for it was the literal `$and` — never a + // cross-object field name. A cross-object reference inside a combinator was + // therefore invisible HERE while `generateSql()` — which has always asked + // the flattened question — refused it, and the two doors answered + // differently for one query. `/analytics/query` reached `engine.aggregate` + // with a predicate the engine cannot join and silently mis-bucketed it, + // which is the exact outcome #3654's loud refusal exists to prevent. + const plan = this.planCrossObject(cube, query, this.filterMemberView(cube, query)); if (plan) { return this.executeCrossObject(cube, query, aggregations, filter, plan, ctx); } @@ -341,11 +354,12 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // serves it via FK-expand. // EVERY member the filter touches, including ones nested in an `$or` — // the envelope check rejects cross-object filters, so a member it cannot - // see is a filter it cannot reject. - const plan = this.planCrossObject(cube, query, Object.fromEntries( - collectFilterLeaves(normalizeAnalyticsFilterTree(query)) - .map((f) => [this.resolveFieldName(cube, f.member, 'any'), true]), - )); + // see is a filter it cannot reject. [#10759] The same + // {@link filterMemberView} `execute()` is judged on, as one expression + // rather than two copies: "the preview accepts/rejects the same set" is an + // invariant between two call sites, and two copies of a view can drift + // apart while each stays individually correct — which is how they drifted. + const plan = this.planCrossObject(cube, query, this.filterMemberView(cube, query)); const crossByDim = new Map((plan?.crossDims ?? []).map((cd) => [cd.outputName, cd])); const joinClauses: string[] = []; const dimExpr = (dim: string): string => { @@ -532,6 +546,41 @@ export class ObjectQLStrategy implements AnalyticsStrategy { return joinedObject !== baseObject; } + /** + * The member view {@link planCrossObject} judges a filter by: EVERY member + * the query's `where` touches, structure discarded, keyed by RESOLVED field + * name (#10759). + * + * Both call sites — `execute()` and `generateSql()` — are handed this and + * nothing else, which is what makes the invariant `planCrossObject` states + * for itself ("the preview accepts/rejects the same set") structural rather + * than a coincidence maintained by hand. They used to build the view + * separately: the echo flattened the tree, `execute()` passed the ENGINE + * FILTER, and a filter record answers a different question — it is a + * predicate to evaluate, not an inventory of members. An `$or`, a `$not` or + * an unmergeable nested `$and` travels in it as one opaque `$and` entry, so + * the members inside were unreadable from the outside and the envelope check + * could not reject what it could not see. + * + * `true` is a placeholder operand and never reaches a driver: `planCrossObject` + * reads `Object.keys` only. Structure is discarded on purpose — a member is + * cross-object or it is not, and which branch of a disjunction it sits in + * cannot make `engine.aggregate` able to join it. + * + * Time-dimension WINDOWS are deliberately absent (they live in + * `dateRangeBounds`, not in `where`). They need no arm here: a cross-object + * time dimension is refused by `planCrossObject`'s own first loop, over + * `query.timeDimensions`, and refused as the time dimension the author wrote + * rather than as the lowered predicate it becomes — which is the better + * diagnostic and the reason that loop runs first. + */ + private filterMemberView(cube: Cube, query: AnalyticsQuery): Record { + return Object.fromEntries( + collectFilterLeaves(normalizeAnalyticsFilterTree(query)) + .map((f) => [this.resolveFieldName(cube, f.member, 'any'), true]), + ); + } + /** * Plan how to serve cross-object references on this join-less path (#3654). * @@ -545,7 +594,10 @@ export class ObjectQLStrategy implements AnalyticsStrategy { * (needs a real join to evaluate), a MULTI-HOP dimension (`a.b.c`), or a * non-recombinable measure (`avg`/`count_distinct`, whose sub-bucket values * cannot be merged). A loud error beats the silent mis-bucket #3654 kills. - * `generateSql()` calls this too, so the preview accepts/rejects the same set. + * `generateSql()` calls this too, so the preview accepts/rejects the same set + * — and since #10759 both callers derive `filter` from the one + * {@link filterMemberView}, so that sentence is enforced by construction + * instead of restated at two call sites. * * [#5716] All four refusals below are `invalidMemberError` — `INVALID_FIELD` / * 400, naming the member — and the MESSAGES are unchanged (they are good