|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | + |
| 3 | +/** |
| 4 | + * [#10759] `ObjectQLStrategy`'s two doors judge one query by ONE member view. |
| 5 | + * |
| 6 | + * ## What was wrong, measured on `origin/main` before the change |
| 7 | + * |
| 8 | + * `planCrossObject` reads `Object.keys(filter)` and nothing else, and the two |
| 9 | + * call sites handed it different things. `generateSql()` handed it every member |
| 10 | + * the `where` touches, flattened out of the tree. `execute()` handed it the |
| 11 | + * built ENGINE FILTER — where an AND-ed leaf sits at the top level and is seen, |
| 12 | + * but anything structural (`$or`, `$not`, a nested `$and` that cannot merge) has |
| 13 | + * been folded into `filter.$and`, so the only key readable for it was the |
| 14 | + * literal `$and`, which is never a field name. |
| 15 | + * |
| 16 | + * So one query got two answers. Measured, both doors fired in one run over one |
| 17 | + * fixture (`where: { $or: [{ 'account.region': 'West' }, { stage: 'won' }] }`): |
| 18 | + * |
| 19 | + * ``` |
| 20 | + * BEFORE execute() ACCEPTED -> engine.aggregate got |
| 21 | + * {"$and":[{"$or":[{"account.region":"West"},…]}]} |
| 22 | + * generateSql() REFUSED cannot evaluate a cross-object filter ("account.region") |
| 23 | + * AFTER execute() REFUSED (same message, same INVALID_FIELD / 400) |
| 24 | + * generateSql() REFUSED unchanged |
| 25 | + * ``` |
| 26 | + * |
| 27 | + * `engine.aggregate` cannot join. The accepted half did not answer a |
| 28 | + * cross-object query — it answered a NARROWER one, silently, because the branch |
| 29 | + * naming a column the base object does not have can never match. That is the |
| 30 | + * silent mis-bucket #3654's loud refusal exists to prevent, and the file already |
| 31 | + * stated the invariant it was breaking: *"`generateSql()` calls this too, so the |
| 32 | + * preview accepts/rejects the same set."* |
| 33 | + * |
| 34 | + * ## Why this file pins FOUR directions, not one |
| 35 | + * |
| 36 | + * Pinning only the new refusal would go green on an implementation that refuses |
| 37 | + * every combinator — which would break every legitimate `$or` query shipping |
| 38 | + * today. So the accepting neighbours are pinned in the same file, one character |
| 39 | + * away from the refused ones: |
| 40 | + * |
| 41 | + * ① a cross-object member nested in `$or` / `$not` is REFUSED on both doors |
| 42 | + * ② a combinator with NO cross-object member still passes both doors, and |
| 43 | + * still reaches `engine.aggregate` carrying its disjunction |
| 44 | + * ③ the `generateSql()` door is UNCHANGED — it was already right, and the |
| 45 | + * top-level case it always refused is refused with the same words |
| 46 | + * ④ the #10413-phase-1 dataset-level `filter` conjunct (PR #10758) is not |
| 47 | + * misread as a cross-object reference — an ordinary definition-level scope |
| 48 | + * travels in `$and` exactly like a combinator does, and reads as a member |
| 49 | + * of nothing |
| 50 | + * |
| 51 | + * ## The scope line this file also draws |
| 52 | + * |
| 53 | + * A dataset whose DEFINITION-LEVEL filter is itself cross-object is accepted by |
| 54 | + * BOTH doors, before and after this change — neither call site's view contains |
| 55 | + * the dataset scope. The doors AGREE there, so it is not the invariant this card |
| 56 | + * restores; it is a separate defect and is pinned here as measured-and-known |
| 57 | + * rather than left to be rediscovered. See the last block. |
| 58 | + */ |
| 59 | + |
| 60 | +import { describe, it, expect } from 'vitest'; |
| 61 | +import type { ExecutionContext } from '@objectstack/spec/kernel'; |
| 62 | +import type { AnalyticsQuery } from '@objectstack/spec/contracts'; |
| 63 | +import { DatasetSchema, type Dataset } from '@objectstack/spec/ui'; |
| 64 | +import { AnalyticsService } from '../analytics-service.js'; |
| 65 | + |
| 66 | +const ctxA = { tenantId: 'org_A', userId: 'u_a' } as ExecutionContext; |
| 67 | + |
| 68 | +interface Refusal extends Error { code?: string; status?: number; member?: string; param?: string } |
| 69 | +interface AggCall { object: string; filter?: unknown } |
| 70 | + |
| 71 | +/** A cube with one base dimension, one cross-object dimension, one base measure. */ |
| 72 | +const SALES_BY_ACCOUNT: Dataset = DatasetSchema.parse({ |
| 73 | + name: 'sales_by_account', |
| 74 | + label: 'Sales by account', |
| 75 | + object: 'opportunity', |
| 76 | + include: ['account'], |
| 77 | + dimensions: [ |
| 78 | + { name: 'stage', field: 'stage', type: 'string' }, |
| 79 | + { name: 'region', field: 'account.region', type: 'string' }, |
| 80 | + ], |
| 81 | + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], |
| 82 | +}) as Dataset; |
| 83 | + |
| 84 | +/** |
| 85 | + * The #10413 phase-1 shape: the SAME cube plus a definition-level `filter`. |
| 86 | + * PR #10758 pushes that filter onto `execute()`'s `conjuncts` list, so it lands |
| 87 | + * inside `filter.$and` — the very place a combinator lands. Its presence must |
| 88 | + * not by itself make a query look cross-object. |
| 89 | + */ |
| 90 | +const SCOPED_SALES: Dataset = DatasetSchema.parse({ |
| 91 | + name: 'scoped_sales', |
| 92 | + label: 'Scoped sales', |
| 93 | + object: 'opportunity', |
| 94 | + include: ['account'], |
| 95 | + filter: { is_deleted: false }, |
| 96 | + dimensions: [{ name: 'stage', field: 'stage', type: 'string' }], |
| 97 | + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], |
| 98 | +}) as Dataset; |
| 99 | + |
| 100 | +/** A dataset whose definition-level filter is ITSELF cross-object. */ |
| 101 | +const XOBJ_SCOPED_SALES: Dataset = DatasetSchema.parse({ |
| 102 | + name: 'xobj_scoped_sales', |
| 103 | + label: 'Cross-object scoped sales', |
| 104 | + object: 'opportunity', |
| 105 | + include: ['account'], |
| 106 | + filter: { 'account.region': 'West' }, |
| 107 | + dimensions: [{ name: 'stage', field: 'stage', type: 'string' }], |
| 108 | + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], |
| 109 | +}) as Dataset; |
| 110 | + |
| 111 | +/** |
| 112 | + * `nativeSql: false` makes `NativeSQLStrategy` decline, so every query below |
| 113 | + * routes to `ObjectQLStrategy` — the door this card is about. |
| 114 | + * |
| 115 | + * The stub RETURNS ROWS rather than throwing, so a refusal that failed to fire |
| 116 | + * produces a passing-looking success with garbage in it: a green rejection test |
| 117 | + * here proves the guard, not luck. |
| 118 | + */ |
| 119 | +function serviceFor(defs: Dataset[]) { |
| 120 | + const calls: AggCall[] = []; |
| 121 | + const svc = new AnalyticsService({ |
| 122 | + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), |
| 123 | + executeAggregate: async (object: string, options: { filter?: unknown }) => { |
| 124 | + calls.push({ object, filter: options.filter }); |
| 125 | + return [{ stage: 'won', revenue: 42 }]; |
| 126 | + }, |
| 127 | + }); |
| 128 | + for (const d of defs) svc.registerDataset(d); |
| 129 | + return { svc, calls }; |
| 130 | +} |
| 131 | + |
| 132 | +async function refusalFrom(thunk: () => Promise<unknown>): Promise<Refusal | undefined> { |
| 133 | + try { |
| 134 | + await thunk(); |
| 135 | + return undefined; |
| 136 | + } catch (e) { |
| 137 | + return e as Refusal; |
| 138 | + } |
| 139 | +} |
| 140 | + |
| 141 | +/** Both doors, one query, one tree — the disagreement is a measurement. */ |
| 142 | +async function bothDoors(cube: string, query: Omit<AnalyticsQuery, 'cube'>, defs: Dataset[]) { |
| 143 | + const { svc, calls } = serviceFor(defs); |
| 144 | + const q = { ...query, cube } as AnalyticsQuery; |
| 145 | + return { |
| 146 | + execute: await refusalFrom(() => svc.query(q, ctxA)), |
| 147 | + generateSql: await refusalFrom(() => svc.generateSql(q, ctxA)), |
| 148 | + calls, |
| 149 | + }; |
| 150 | +} |
| 151 | + |
| 152 | +const CROSS_OBJECT_MESSAGE = /cannot evaluate a cross-object filter \("account\.region"\)/; |
| 153 | + |
| 154 | +// ───────────────────────────────────────────────────────────────────────────── |
| 155 | +// ① the refusal that was missing on the execution door |
| 156 | +// ───────────────────────────────────────────────────────────────────────────── |
| 157 | + |
| 158 | +/** |
| 159 | + * Each entry nests the SAME cross-object member one level down, in a different |
| 160 | + * combinator, so the pin is on "structure hides the member" rather than on the |
| 161 | + * `$or` spelling alone. |
| 162 | + */ |
| 163 | +const NESTED: Array<{ name: string; where: Record<string, unknown> }> = [ |
| 164 | + { name: '$or', where: { $or: [{ 'account.region': 'West' }, { stage: 'won' }] } }, |
| 165 | + { name: '$not', where: { $not: { 'account.region': 'West' } } }, |
| 166 | + { name: '$or nested two deep', where: { $or: [{ $and: [{ 'account.region': 'West' }, { stage: 'won' }] }, { stage: 'lost' }] } }, |
| 167 | +]; |
| 168 | + |
| 169 | +describe('[#10759] a cross-object member nested in a combinator is refused on BOTH doors', () => { |
| 170 | + for (const c of NESTED) { |
| 171 | + it(`${c.name}: execute() refuses with the ADR-0112 envelope`, async () => { |
| 172 | + const { execute, calls } = await bothDoors('sales_by_account', { |
| 173 | + dimensions: ['stage'], measures: ['revenue'], where: c.where, |
| 174 | + }, [SALES_BY_ACCOUNT]); |
| 175 | + |
| 176 | + expect(execute, 'accepted — the member was invisible to the envelope check').toBeInstanceOf(Error); |
| 177 | + expect(String(execute?.message)).toMatch(CROSS_OBJECT_MESSAGE); |
| 178 | + // Read exactly as `rest-server.ts`'s catch reads them: a 4xx status AND a |
| 179 | + // code, or the route falls through to 500 ANALYTICS_QUERY_FAILED. Asserting |
| 180 | + // only that it throws would pass on a bare `Error` and report the platform |
| 181 | + // broken for a caller mistake. |
| 182 | + expect(execute?.code, 'no `code` ⇒ 500 ANALYTICS_QUERY_FAILED').toBe('INVALID_FIELD'); |
| 183 | + expect(execute?.status, 'no `status` ⇒ 500 ANALYTICS_QUERY_FAILED').toBe(400); |
| 184 | + // The member is named as the REQUEST spelled it, and `where` is the key to |
| 185 | + // go fix — the refusal is actionable without reading this file. |
| 186 | + expect(execute?.member).toBe('account.region'); |
| 187 | + expect(execute?.param).toBe('where'); |
| 188 | + // Refused BEFORE the engine was asked, not after it mis-bucketed. |
| 189 | + expect(calls, 'engine.aggregate was reached — it cannot join').toEqual([]); |
| 190 | + }); |
| 191 | + |
| 192 | + it(`${c.name}: both doors agree`, async () => { |
| 193 | + const { execute, generateSql } = await bothDoors('sales_by_account', { |
| 194 | + dimensions: ['stage'], measures: ['revenue'], where: c.where, |
| 195 | + }, [SALES_BY_ACCOUNT]); |
| 196 | + // The invariant `planCrossObject` states for itself, asserted as one fact |
| 197 | + // about one query rather than as two independent expectations. |
| 198 | + expect( |
| 199 | + [execute === undefined, generateSql === undefined], |
| 200 | + 'the preview and the execution door accept/reject the same set', |
| 201 | + ).toEqual([false, false]); |
| 202 | + expect(String(generateSql?.message)).toMatch(CROSS_OBJECT_MESSAGE); |
| 203 | + }); |
| 204 | + } |
| 205 | + |
| 206 | + it('the KNOWN-PRESENT control: the same member at the top level was always refused', async () => { |
| 207 | + // The counter-check for every "refused" above. This shape predates #10759 and |
| 208 | + // is refused on both doors before AND after it — so it proves the fixture, |
| 209 | + // the cube and the detection path work, and cannot be read as evidence for |
| 210 | + // the change. The nested rows above are what moved. |
| 211 | + const { execute, generateSql } = await bothDoors('sales_by_account', { |
| 212 | + dimensions: ['stage'], measures: ['revenue'], where: { 'account.region': 'West' }, |
| 213 | + }, [SALES_BY_ACCOUNT]); |
| 214 | + expect(String(execute?.message)).toMatch(CROSS_OBJECT_MESSAGE); |
| 215 | + expect(String(generateSql?.message)).toMatch(CROSS_OBJECT_MESSAGE); |
| 216 | + }); |
| 217 | +}); |
| 218 | + |
| 219 | +// ───────────────────────────────────────────────────────────────────────────── |
| 220 | +// ② the accepting neighbours — no combinator was refused wholesale |
| 221 | +// ───────────────────────────────────────────────────────────────────────────── |
| 222 | + |
| 223 | +describe('[#10759] a combinator with NO cross-object member still passes', () => { |
| 224 | + const CLEAN: Array<{ name: string; where: Record<string, unknown> }> = [ |
| 225 | + { name: '$or over base fields', where: { $or: [{ stage: 'won' }, { stage: 'lost' }] } }, |
| 226 | + { name: '$not over a base field', where: { $not: { stage: 'won' } } }, |
| 227 | + { name: 'a mixed tree over base fields', where: { $or: [{ $and: [{ stage: 'won' }, { amount: 5 }] }, { stage: 'lost' }] } }, |
| 228 | + ]; |
| 229 | + |
| 230 | + for (const c of CLEAN) { |
| 231 | + it(`${c.name}: accepted on both doors`, async () => { |
| 232 | + const { execute, generateSql, calls } = await bothDoors('sales_by_account', { |
| 233 | + dimensions: ['stage'], measures: ['revenue'], where: c.where, |
| 234 | + }, [SALES_BY_ACCOUNT]); |
| 235 | + expect(execute, `execute() refused a clean combinator: ${execute?.message}`).toBeUndefined(); |
| 236 | + expect(generateSql, `generateSql() refused a clean combinator: ${generateSql?.message}`).toBeUndefined(); |
| 237 | + // Reached the engine, and reached it carrying the disjunction — a refusal |
| 238 | + // is not the only way to break these queries; dropping the predicate would |
| 239 | + // widen the answer just as silently. |
| 240 | + expect(calls).toHaveLength(1); |
| 241 | + expect(JSON.stringify(calls[0].filter)).toContain('$'); |
| 242 | + }); |
| 243 | + } |
| 244 | + |
| 245 | + it('an in-envelope cross-object DIMENSION still compiles — only FILTERS were widened', async () => { |
| 246 | + // `region` resolves to `account.region` and is served by FK-expand. If the |
| 247 | + // new view had been read as "any cross-object member anywhere", this would |
| 248 | + // have started failing too. |
| 249 | + const { generateSql } = await bothDoors('sales_by_account', { |
| 250 | + dimensions: ['region'], measures: ['revenue'], |
| 251 | + }, [SALES_BY_ACCOUNT]); |
| 252 | + expect(generateSql).toBeUndefined(); |
| 253 | + }); |
| 254 | +}); |
| 255 | + |
| 256 | +// ───────────────────────────────────────────────────────────────────────────── |
| 257 | +// ③ + ④ the #10758 dataset-scope conjunct |
| 258 | +// ───────────────────────────────────────────────────────────────────────────── |
| 259 | + |
| 260 | +describe('[#10759] the #10413-phase-1 dataset filter conjunct is not misread', () => { |
| 261 | + it('an ordinary definition-level filter is accepted and still reaches the engine', async () => { |
| 262 | + const { execute, generateSql, calls } = await bothDoors('scoped_sales', { |
| 263 | + dimensions: ['stage'], measures: ['revenue'], |
| 264 | + }, [SCOPED_SALES]); |
| 265 | + expect(execute, `execute() refused a scoped dataset: ${execute?.message}`).toBeUndefined(); |
| 266 | + expect(generateSql).toBeUndefined(); |
| 267 | + // PR #10758's own guarantee, re-pinned from this side: the scope travels as |
| 268 | + // an `$and` conjunct, which is exactly the position a combinator occupies — |
| 269 | + // so this is the pin that a "refuse anything under `$and`" implementation |
| 270 | + // would fail. |
| 271 | + expect(JSON.stringify(calls[0]?.filter)).toContain('is_deleted'); |
| 272 | + }); |
| 273 | + |
| 274 | + it('a dataset scope does not shield a cross-object member in the caller’s own $or', async () => { |
| 275 | + const { execute, generateSql, calls } = await bothDoors('scoped_sales', { |
| 276 | + dimensions: ['stage'], measures: ['revenue'], |
| 277 | + where: { $or: [{ 'account.region': 'West' }, { stage: 'won' }] }, |
| 278 | + }, [SCOPED_SALES]); |
| 279 | + expect(String(execute?.message)).toMatch(CROSS_OBJECT_MESSAGE); |
| 280 | + expect(String(generateSql?.message)).toMatch(CROSS_OBJECT_MESSAGE); |
| 281 | + expect(calls).toEqual([]); |
| 282 | + }); |
| 283 | + |
| 284 | + /** |
| 285 | + * MEASURED AND DELIBERATELY LEFT OPEN — not a latent pass. |
| 286 | + * |
| 287 | + * A cross-object DEFINITION-LEVEL filter is accepted by both doors, and |
| 288 | + * `engine.aggregate` receives `{"$and":[{"account.region":"West"}]}`, which it |
| 289 | + * cannot join. PR #10758 created this instance by giving the dataset scope a |
| 290 | + * route onto the ObjectQL door at all; #10759 is not it, because the two doors |
| 291 | + * AGREE here — neither call site's member view contains the dataset scope, so |
| 292 | + * there is no preview/execution divergence to restore. |
| 293 | + * |
| 294 | + * Filed separately rather than widened into this PR: refusing it is a real |
| 295 | + * decision (query-time refusal versus a compile-time rejection in |
| 296 | + * `dataset-compiler.ts`, which is the contract-first placement), and it is not |
| 297 | + * the invariant this file restores. The expectation below is written to the |
| 298 | + * behaviour as it IS, so the day that decision lands this pin goes red and |
| 299 | + * points at the paragraph explaining why. |
| 300 | + */ |
| 301 | + it('a CROSS-OBJECT definition-level filter is still accepted by both doors (filed separately)', async () => { |
| 302 | + const { execute, generateSql, calls } = await bothDoors('xobj_scoped_sales', { |
| 303 | + dimensions: ['stage'], measures: ['revenue'], |
| 304 | + }, [XOBJ_SCOPED_SALES]); |
| 305 | + expect(execute).toBeUndefined(); |
| 306 | + expect(generateSql).toBeUndefined(); |
| 307 | + expect(JSON.stringify(calls[0]?.filter)).toContain('account.region'); |
| 308 | + }); |
| 309 | +}); |
0 commit comments