Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .changeset/6302-aggregate-filter-lowering.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
---
'@object-ui/data-objectstack': patch
---

`ObjectStackAdapter.aggregate()` lowers rule-shaped filter arrays before the
analytics wire, reusing the lowering `find()` already runs (objectui#6302).

`find()` has translated `[{ field, operator, value }, ...]` into the server's
filter AST for as long as `convertQueryParams` has existed. The analytics path
did not: `aggregate()` assigned `payload.where = params.filter` verbatim and
posted it to `/analytics/query`.

The two doors are not equally forgiving, so the gap had a user-visible end.
`lowerAnalyticsWhere` in `@objectstack/service-analytics` — shared by both
aggregation strategies — accepts AST tuples and throws on an array of rule
objects. A stored `ViewFilterRule[]` that a LIST renders correctly therefore
rendered `element:number` into its error state on every analytics-capable
deployment, which is the default one because the CLI always loads analytics.

An array filter now goes through the same `translateFilterArray` the `find()`
path uses — one lowering, so the two paths cannot disagree about one stored
filter. Rules spread into a logical node (`['and', ...rules, ...tuples]`, the
commonest composite there is) are lowered at depth, as they already were on
`find()`. Non-array filters are untouched: the MongoDB-style object this branch
was written for is what `/analytics/query` already accepts, and translating it
would be a semantic change this fix does not make. Already-AST arrays,
record-shaped filters, and the no-filter case are byte-unchanged.
28 changes: 28 additions & 0 deletions packages/data-objectstack/README.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,6 +147,34 @@ const ast = [
];
```

#### Rule-shaped arrays, on `find()` **and** `aggregate()`

Server-driven view configs store their conditions as an array of rules
(`ViewFilterRule[]`), not as a MongoDB-style object:

```typescript
const filter = [{ field: 'stage', operator: 'equals', value: 'won' }];
```

Both read paths lower that array to the same AST before it reaches the wire —
`find()` via `$filter`, and `aggregate()` via the analytics `where`. They share
one translator, so a stored filter cannot mean one thing on a list and another
on a KPI:

```typescript
// find(): filter=["stage","=","won"]
// aggregate(): { ..., where: ["stage", "=", "won"] }
```

Operator aliases (`equals`, `greater_than_or_equal`, `not_in`, `before`, ...)
map to the canonical AST symbols, and rules spread into a logical node
(`['and', ...rules, ...tuples]`) are lowered at depth. A rule that cannot be
translated raises `MalformedFilterError` rather than being dropped — dropping
one condition of an `and` would widen the result set and report success.

Non-array filters are passed through unchanged on the aggregate path: a
MongoDB-style object is already what `/analytics/query` accepts.

### Sorting

```typescript
Expand Down
261 changes: 261 additions & 0 deletions packages/data-objectstack/src/aggregate-filter-lowering.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* `aggregate()` lowers rule-shaped filter arrays before the analytics wire.
*
* WHY THIS FILE EXISTS (objectui#6302). `find()` has translated
* `[{ field, operator, value }, ...]` into the server's filter AST since the
* day `convertQueryParams` learned to — see `filter-entry-translation.test.ts`,
* which runs every shape down both `find()` routes. The analytics path did not:
* `aggregate()` assigned `payload.where = params.filter` verbatim and posted it
* to `/analytics/query`.
*
* The two doors are not equally forgiving, which is why the gap had a
* user-visible end. `lowerAnalyticsWhere` in `@objectstack/service-analytics`
* — shared by BOTH aggregation strategies, so there is no deployment where the
* lenient reading applies — accepts AST tuples and THROWS on an array of rule
* objects ("[analytics] received a 'where' array that is not a filter"). The
* spec's own `isFilterAST` gate says the same thing about the same value, and
* the tests below assert on it directly so the refusal is pinned by the
* contract rather than by a message string:
*
* isFilterAST([{ field: 'stage', operator: 'equals', value: 'won' }]) // false
* isFilterAST(['stage', '=', 'won']) // true
*
* Net effect before the fix: a stored `ViewFilterRule[]` that a LIST renders
* correctly rendered `element:number` into its error state on every
* analytics-capable deployment — and analytics is the default one, because the
* CLI always loads it.
*
* The fix reuses `translateFilterArray` rather than adding a second lowering.
* That is load-bearing and is asserted as such below: the cross-path parity
* block requires `aggregate()`'s `where` and `find()`'s `filter=` to be the
* SAME value for the same input, so the two paths cannot drift the way the two
* `find()` routes once did. Non-array filters are deliberately untouched — the
* MongoDB-style object this branch was written for is already what the
* analytics endpoint accepts, and translating it would be a semantic change
* this fix does not make.
*/

import { describe, it, expect, beforeEach, vi } from 'vitest';
import { isFilterAST } from '@objectstack/spec/data';
import { ObjectStackAdapter, clearSharedDiscoveryCache, isMalformedFilterError } from './index';

/** Rows that carry the requested measure, so nothing degrades to the fallback. */
const ANALYTICS_ROWS = { rows: [{ amount_sum: 150 }] };

function makeAdapter() {
/** Every parsed `/analytics/query` request body, in order. */
const analyticsBodies: any[] = [];
const urls: string[] = [];
const fetchImpl = vi.fn(async (url: any, init?: any) => {
const u = String(url);
urls.push(u);
if (u.includes('/api/v1/discovery')) {
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { version: 'v1', routes: {} } }),
} as any;
}
if (u.includes('/api/v1/analytics/query')) {
analyticsBodies.push(init?.body ? JSON.parse(String(init.body)) : undefined);
return { ok: true, status: 200, statusText: 'OK', json: async () => ANALYTICS_ROWS } as any;
}
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { object: 'opportunity', records: [], total: 0 } }),
} as any;
});
const adapter = new ObjectStackAdapter({
baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any,
});
return { adapter, analyticsBodies, urls };
}

const SUM_BY_STAGE = { function: 'sum', field: 'amount', groupBy: '_all' };

/**
* The `where` this filter put on the analytics wire.
*
* `HAS_NO_WHERE` distinguishes "the key was absent" from "the key was present
* and undefined" — the empty-filter cases below turn on exactly that.
*/
const HAS_NO_WHERE = Symbol('no where key');

async function whereOnWire(filter: unknown): Promise<unknown> {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', { ...SUM_BY_STAGE, filter });
expect(analyticsBodies).toHaveLength(1);
const body = analyticsBodies[0];
return 'where' in body ? body.where : HAS_NO_WHERE;
}

/** The `filter=` the SAME value produces on the plain `find()` route. */
async function filterOnFindWire(filter: unknown): Promise<unknown> {
const { adapter, urls } = makeAdapter();
await adapter.find('opportunity', { $filter: filter } as any);
const dataCall = urls.filter((u) => u.includes('/data/opportunity')).pop();
const raw = dataCall ? new URL(dataCall).searchParams.get('filter') : null;
return raw === null ? HAS_NO_WHERE : JSON.parse(raw);
}

describe('aggregate() lowers a rule-shaped filter array before `client.analytics.query`', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('translates a single rule into an AST tuple', async () => {
const where = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
expect(where).toEqual(['stage', '=', 'won']);
});

it('the lowered value passes the AST gate the raw one fails', async () => {
const rules = [{ field: 'stage', operator: 'equals', value: 'won' }];
// Negative control: this is what used to reach the wire, and it is exactly
// the value `lowerAnalyticsWhere` refuses. Without this line the test above
// could pass against a lowering that produced some OTHER non-AST shape.
expect(isFilterAST(rules)).toBe(false);
expect(isFilterAST(await whereOnWire(rules) as any)).toBe(true);
});

it('maps operator aliases the way the find() path does', async () => {
expect(await whereOnWire([{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]))
.toEqual(['amount', '>=', 3]);
});

it('joins several rules with `and`', async () => {
expect(await whereOnWire([
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
])).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('lowers rules SPREAD into a logical node, not just top-level ones', async () => {
// The commonest composite there is: a view's stored filter plus one the
// user added in the panel. The head is the string `and`, so a top-level-only
// check would call the whole thing "already AST" and ship the rule raw.
const composite = ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]];
expect(isFilterAST(composite as any)).toBe(false);
const where = await whereOnWire(composite);
expect(where).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
expect(isFilterAST(where as any)).toBe(true);
});

it('produces the SAME `where` as the AST-tuple equivalent (the acceptance criterion)', async () => {
const fromRules = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
const fromTuple = await whereOnWire(['stage', '=', 'won']);
expect(fromRules).toEqual(fromTuple);
});
});

describe('aggregate() leaves every already-correct filter shape byte-unchanged', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('an AST tuple passes through untouched', async () => {
expect(await whereOnWire(['stage', '=', 'won'])).toEqual(['stage', '=', 'won']);
});

it('a logical AST node passes through untouched', async () => {
expect(await whereOnWire(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]))
.toEqual(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]);
});

it('a legacy nested array of nodes passes through untouched', async () => {
expect(await whereOnWire([['stage', '=', 'won'], ['amount', '>', 100]]))
.toEqual([['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('a record-shaped (MongoDB-style) filter is NOT translated', async () => {
// The shape this branch was written for. `/analytics/query` accepts it, so
// lowering it here would be a semantic change, not a fix.
expect(await whereOnWire({ stage: 'won' })).toEqual({ stage: 'won' });
});

it('a record-shaped filter with an operator object is NOT translated either', async () => {
expect(await whereOnWire({ amount: { $gt: 100 } })).toEqual({ amount: { $gt: 100 } });
});

it('an empty array still reaches the wire as an empty array', async () => {
// Unchanged on purpose: `if (params.filter)` is truthy for `[]`, and this
// fix moves no boundary it did not have to move.
expect(await whereOnWire([])).toEqual([]);
});

it('no filter means no `where` key at all', async () => {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', SUM_BY_STAGE);
expect(analyticsBodies[0]).not.toHaveProperty('where');
});
});

describe('the find() path is unchanged, and the two paths share one lowering', () => {
beforeEach(() => clearSharedDiscoveryCache());

// Each row is one input. Both sides are measured on the wire, so a change to
// either path that the other does not make turns this red.
const SHARED_CASES: Array<[string, unknown]> = [
['a single rule', [{ field: 'stage', operator: 'equals', value: 'won' }]],
['an aliased operator', [{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]],
['several rules', [
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
]],
['rules spread into a logical node', ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]]],
['an AST tuple', ['stage', '=', 'won']],
['a logical AST node', ['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]],
];

for (const [name, filter] of SHARED_CASES) {
it(`aggregate() and find() agree on ${name}`, async () => {
const viaFind = await filterOnFindWire(filter);
expect(viaFind).not.toBe(HAS_NO_WHERE);
expect(await whereOnWire(filter)).toEqual(viaFind);
});
}

it('find() still lowers a single rule exactly as it did before', async () => {
// The `find()` half of the card's acceptance, stated independently of
// `aggregate()` so a regression there cannot hide behind the parity rows.
expect(await filterOnFindWire([{ field: 'stage', operator: 'equals', value: 'won' }]))
.toEqual(['stage', '=', 'won']);
});

it('find() still sends no filter for an empty array', async () => {
// The one place the two paths legitimately differ: `convertQueryParams`
// drops an empty filter, the analytics payload keeps `[]`. Recorded, not
// reconciled — reconciling it is a behaviour change this card does not make.
expect(await filterOnFindWire([])).toBe(HAS_NO_WHERE);
expect(await whereOnWire([])).toEqual([]);
});
});

describe('a rule the adapter cannot translate refuses on the aggregate path too', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('throws the same malformed-filter refusal `find()` raises, without inventing numbers', async () => {
// Dropping the untranslatable entry would WIDEN the result set and report
// success — the silent over-fetch `MalformedFilterError` exists to stop.
// Sharing the lowering means the analytics path inherits that refusal.
const { adapter, analyticsBodies, urls } = makeAdapter();
const err = await adapter
.aggregate('opportunity', {
...SUM_BY_STAGE,
filter: [
{ field: 'stage', operator: 'eq', value: 'won' },
{ operator: 'eq', value: 'no field here' },
],
})
.then(() => null, (e) => e);

expect(err).toBeInstanceOf(Error);
expect(isMalformedFilterError(err)).toBe(true);
// Nothing was posted to analytics, and no plausible-looking number came
// back from the fallback instead.
expect(analyticsBodies).toHaveLength(0);
expect(urls.some((u) => u.includes('/data/opportunity'))).toBe(false);
});
});
25 changes: 24 additions & 1 deletion packages/data-objectstack/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -4598,7 +4598,30 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
// spec/ui/dashboard.zod.ts). Send via the canonical `where`
// field of the analytics endpoint, matching the unified Query
// DSL (spec/data/query.zod.ts).
payload.where = params.filter;
//
// An ARRAY filter goes through the same `translateFilterArray` the
// `find()` path runs in `convertQueryParams`, because an authored
// `ViewFilterRule[]` reaches this method exactly as it reaches that
// one. It used to ship RAW from here, and the analytics door is
// stricter than the data door: `lowerAnalyticsWhere`
// (`@objectstack/service-analytics`, shared by both aggregation
// strategies) THROWS "[analytics] received a 'where' array that is
// not a filter" on an array of rule objects, while accepting AST
// tuples. So a stored filter that a list renders correctly rendered
// `element:number` into its error state on every analytics-capable
// deployment — and analytics is the default one, since the CLI always
// loads it (objectui#6302).
//
// One lowering, not two: the same function, so the analytics path and
// the `find()` path cannot disagree about one stored filter — which is
// the whole reason `translateFilterArray` was made a single definition
// (see its header). Non-array filters keep passing through untouched:
// the MongoDB-style object this branch was written for is what
// `/analytics/query` already accepts, and translating it here would be
// a semantic change this fix is expressly not making.
payload.where = Array.isArray(params.filter)
? translateFilterArray(params.filter)
: params.filter;
}

const data = await this.client.analytics.query(payload);
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(data-objectstack): lower rule-shaped filter arrays on aggregate()'s analytics path by claude[bot] · Pull Request #6828 · objectstack-ai/objectui · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .changeset/6302-aggregate-filter-lowering.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
---
'@object-ui/data-objectstack': patch
---

`ObjectStackAdapter.aggregate()` lowers rule-shaped filter arrays before the
analytics wire, reusing the lowering `find()` already runs (objectui#6302).

`find()` has translated `[{ field, operator, value }, ...]` into the server's
filter AST for as long as `convertQueryParams` has existed. The analytics path
did not: `aggregate()` assigned `payload.where = params.filter` verbatim and
posted it to `/analytics/query`.

The two doors are not equally forgiving, so the gap had a user-visible end.
`lowerAnalyticsWhere` in `@objectstack/service-analytics` — shared by both
aggregation strategies — accepts AST tuples and throws on an array of rule
objects. A stored `ViewFilterRule[]` that a LIST renders correctly therefore
rendered `element:number` into its error state on every analytics-capable
deployment, which is the default one because the CLI always loads analytics.

An array filter now goes through the same `translateFilterArray` the `find()`
path uses — one lowering, so the two paths cannot disagree about one stored
filter. Rules spread into a logical node (`['and', ...rules, ...tuples]`, the
commonest composite there is) are lowered at depth, as they already were on
`find()`. Non-array filters are untouched: the MongoDB-style object this branch
was written for is what `/analytics/query` already accepts, and translating it
would be a semantic change this fix does not make. Already-AST arrays,
record-shaped filters, and the no-filter case are byte-unchanged.
28 changes: 28 additions & 0 deletions packages/data-objectstack/README.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,6 +147,34 @@ const ast = [
];
```

#### Rule-shaped arrays, on `find()` **and** `aggregate()`

Server-driven view configs store their conditions as an array of rules
(`ViewFilterRule[]`), not as a MongoDB-style object:

```typescript
const filter = [{ field: 'stage', operator: 'equals', value: 'won' }];
```

Both read paths lower that array to the same AST before it reaches the wire —
`find()` via `$filter`, and `aggregate()` via the analytics `where`. They share
one translator, so a stored filter cannot mean one thing on a list and another
on a KPI:

```typescript
// find(): filter=["stage","=","won"]
// aggregate(): { ..., where: ["stage", "=", "won"] }
```

Operator aliases (`equals`, `greater_than_or_equal`, `not_in`, `before`, ...)
map to the canonical AST symbols, and rules spread into a logical node
(`['and', ...rules, ...tuples]`) are lowered at depth. A rule that cannot be
translated raises `MalformedFilterError` rather than being dropped — dropping
one condition of an `and` would widen the result set and report success.

Non-array filters are passed through unchanged on the aggregate path: a
MongoDB-style object is already what `/analytics/query` accepts.

### Sorting

```typescript
Expand Down
261 changes: 261 additions & 0 deletions packages/data-objectstack/src/aggregate-filter-lowering.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* `aggregate()` lowers rule-shaped filter arrays before the analytics wire.
*
* WHY THIS FILE EXISTS (objectui#6302). `find()` has translated
* `[{ field, operator, value }, ...]` into the server's filter AST since the
* day `convertQueryParams` learned to — see `filter-entry-translation.test.ts`,
* which runs every shape down both `find()` routes. The analytics path did not:
* `aggregate()` assigned `payload.where = params.filter` verbatim and posted it
* to `/analytics/query`.
*
* The two doors are not equally forgiving, which is why the gap had a
* user-visible end. `lowerAnalyticsWhere` in `@objectstack/service-analytics`
* — shared by BOTH aggregation strategies, so there is no deployment where the
* lenient reading applies — accepts AST tuples and THROWS on an array of rule
* objects ("[analytics] received a 'where' array that is not a filter"). The
* spec's own `isFilterAST` gate says the same thing about the same value, and
* the tests below assert on it directly so the refusal is pinned by the
* contract rather than by a message string:
*
* isFilterAST([{ field: 'stage', operator: 'equals', value: 'won' }]) // false
* isFilterAST(['stage', '=', 'won']) // true
*
* Net effect before the fix: a stored `ViewFilterRule[]` that a LIST renders
* correctly rendered `element:number` into its error state on every
* analytics-capable deployment — and analytics is the default one, because the
* CLI always loads it.
*
* The fix reuses `translateFilterArray` rather than adding a second lowering.
* That is load-bearing and is asserted as such below: the cross-path parity
* block requires `aggregate()`'s `where` and `find()`'s `filter=` to be the
* SAME value for the same input, so the two paths cannot drift the way the two
* `find()` routes once did. Non-array filters are deliberately untouched — the
* MongoDB-style object this branch was written for is already what the
* analytics endpoint accepts, and translating it would be a semantic change
* this fix does not make.
*/

import { describe, it, expect, beforeEach, vi } from 'vitest';
import { isFilterAST } from '@objectstack/spec/data';
import { ObjectStackAdapter, clearSharedDiscoveryCache, isMalformedFilterError } from './index';

/** Rows that carry the requested measure, so nothing degrades to the fallback. */
const ANALYTICS_ROWS = { rows: [{ amount_sum: 150 }] };

function makeAdapter() {
/** Every parsed `/analytics/query` request body, in order. */
const analyticsBodies: any[] = [];
const urls: string[] = [];
const fetchImpl = vi.fn(async (url: any, init?: any) => {
const u = String(url);
urls.push(u);
if (u.includes('/api/v1/discovery')) {
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { version: 'v1', routes: {} } }),
} as any;
}
if (u.includes('/api/v1/analytics/query')) {
analyticsBodies.push(init?.body ? JSON.parse(String(init.body)) : undefined);
return { ok: true, status: 200, statusText: 'OK', json: async () => ANALYTICS_ROWS } as any;
}
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { object: 'opportunity', records: [], total: 0 } }),
} as any;
});
const adapter = new ObjectStackAdapter({
baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any,
});
return { adapter, analyticsBodies, urls };
}

const SUM_BY_STAGE = { function: 'sum', field: 'amount', groupBy: '_all' };

/**
* The `where` this filter put on the analytics wire.
*
* `HAS_NO_WHERE` distinguishes "the key was absent" from "the key was present
* and undefined" — the empty-filter cases below turn on exactly that.
*/
const HAS_NO_WHERE = Symbol('no where key');

async function whereOnWire(filter: unknown): Promise<unknown> {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', { ...SUM_BY_STAGE, filter });
expect(analyticsBodies).toHaveLength(1);
const body = analyticsBodies[0];
return 'where' in body ? body.where : HAS_NO_WHERE;
}

/** The `filter=` the SAME value produces on the plain `find()` route. */
async function filterOnFindWire(filter: unknown): Promise<unknown> {
const { adapter, urls } = makeAdapter();
await adapter.find('opportunity', { $filter: filter } as any);
const dataCall = urls.filter((u) => u.includes('/data/opportunity')).pop();
const raw = dataCall ? new URL(dataCall).searchParams.get('filter') : null;
return raw === null ? HAS_NO_WHERE : JSON.parse(raw);
}

describe('aggregate() lowers a rule-shaped filter array before `client.analytics.query`', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('translates a single rule into an AST tuple', async () => {
const where = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
expect(where).toEqual(['stage', '=', 'won']);
});

it('the lowered value passes the AST gate the raw one fails', async () => {
const rules = [{ field: 'stage', operator: 'equals', value: 'won' }];
// Negative control: this is what used to reach the wire, and it is exactly
// the value `lowerAnalyticsWhere` refuses. Without this line the test above
// could pass against a lowering that produced some OTHER non-AST shape.
expect(isFilterAST(rules)).toBe(false);
expect(isFilterAST(await whereOnWire(rules) as any)).toBe(true);
});

it('maps operator aliases the way the find() path does', async () => {
expect(await whereOnWire([{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]))
.toEqual(['amount', '>=', 3]);
});

it('joins several rules with `and`', async () => {
expect(await whereOnWire([
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
])).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('lowers rules SPREAD into a logical node, not just top-level ones', async () => {
// The commonest composite there is: a view's stored filter plus one the
// user added in the panel. The head is the string `and`, so a top-level-only
// check would call the whole thing "already AST" and ship the rule raw.
const composite = ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]];
expect(isFilterAST(composite as any)).toBe(false);
const where = await whereOnWire(composite);
expect(where).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
expect(isFilterAST(where as any)).toBe(true);
});

it('produces the SAME `where` as the AST-tuple equivalent (the acceptance criterion)', async () => {
const fromRules = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
const fromTuple = await whereOnWire(['stage', '=', 'won']);
expect(fromRules).toEqual(fromTuple);
});
});

describe('aggregate() leaves every already-correct filter shape byte-unchanged', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('an AST tuple passes through untouched', async () => {
expect(await whereOnWire(['stage', '=', 'won'])).toEqual(['stage', '=', 'won']);
});

it('a logical AST node passes through untouched', async () => {
expect(await whereOnWire(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]))
.toEqual(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]);
});

it('a legacy nested array of nodes passes through untouched', async () => {
expect(await whereOnWire([['stage', '=', 'won'], ['amount', '>', 100]]))
.toEqual([['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('a record-shaped (MongoDB-style) filter is NOT translated', async () => {
// The shape this branch was written for. `/analytics/query` accepts it, so
// lowering it here would be a semantic change, not a fix.
expect(await whereOnWire({ stage: 'won' })).toEqual({ stage: 'won' });
});

it('a record-shaped filter with an operator object is NOT translated either', async () => {
expect(await whereOnWire({ amount: { $gt: 100 } })).toEqual({ amount: { $gt: 100 } });
});

it('an empty array still reaches the wire as an empty array', async () => {
// Unchanged on purpose: `if (params.filter)` is truthy for `[]`, and this
// fix moves no boundary it did not have to move.
expect(await whereOnWire([])).toEqual([]);
});

it('no filter means no `where` key at all', async () => {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', SUM_BY_STAGE);
expect(analyticsBodies[0]).not.toHaveProperty('where');
});
});

describe('the find() path is unchanged, and the two paths share one lowering', () => {
beforeEach(() => clearSharedDiscoveryCache());

// Each row is one input. Both sides are measured on the wire, so a change to
// either path that the other does not make turns this red.
const SHARED_CASES: Array<[string, unknown]> = [
['a single rule', [{ field: 'stage', operator: 'equals', value: 'won' }]],
['an aliased operator', [{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]],
['several rules', [
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
]],
['rules spread into a logical node', ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]]],
['an AST tuple', ['stage', '=', 'won']],
['a logical AST node', ['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]],
];

for (const [name, filter] of SHARED_CASES) {
it(`aggregate() and find() agree on ${name}`, async () => {
const viaFind = await filterOnFindWire(filter);
expect(viaFind).not.toBe(HAS_NO_WHERE);
expect(await whereOnWire(filter)).toEqual(viaFind);
});
}

it('find() still lowers a single rule exactly as it did before', async () => {
// The `find()` half of the card's acceptance, stated independently of
// `aggregate()` so a regression there cannot hide behind the parity rows.
expect(await filterOnFindWire([{ field: 'stage', operator: 'equals', value: 'won' }]))
.toEqual(['stage', '=', 'won']);
});

it('find() still sends no filter for an empty array', async () => {
// The one place the two paths legitimately differ: `convertQueryParams`
// drops an empty filter, the analytics payload keeps `[]`. Recorded, not
// reconciled — reconciling it is a behaviour change this card does not make.
expect(await filterOnFindWire([])).toBe(HAS_NO_WHERE);
expect(await whereOnWire([])).toEqual([]);
});
});

describe('a rule the adapter cannot translate refuses on the aggregate path too', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('throws the same malformed-filter refusal `find()` raises, without inventing numbers', async () => {
// Dropping the untranslatable entry would WIDEN the result set and report
// success — the silent over-fetch `MalformedFilterError` exists to stop.
// Sharing the lowering means the analytics path inherits that refusal.
const { adapter, analyticsBodies, urls } = makeAdapter();
const err = await adapter
.aggregate('opportunity', {
...SUM_BY_STAGE,
filter: [
{ field: 'stage', operator: 'eq', value: 'won' },
{ operator: 'eq', value: 'no field here' },
],
})
.then(() => null, (e) => e);

expect(err).toBeInstanceOf(Error);
expect(isMalformedFilterError(err)).toBe(true);
// Nothing was posted to analytics, and no plausible-looking number came
// back from the fallback instead.
expect(analyticsBodies).toHaveLength(0);
expect(urls.some((u) => u.includes('/data/opportunity'))).toBe(false);
});
});
25 changes: 24 additions & 1 deletion packages/data-objectstack/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -4598,7 +4598,30 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
// spec/ui/dashboard.zod.ts). Send via the canonical `where`
// field of the analytics endpoint, matching the unified Query
// DSL (spec/data/query.zod.ts).
payload.where = params.filter;
//
// An ARRAY filter goes through the same `translateFilterArray` the
// `find()` path runs in `convertQueryParams`, because an authored
// `ViewFilterRule[]` reaches this method exactly as it reaches that
// one. It used to ship RAW from here, and the analytics door is
// stricter than the data door: `lowerAnalyticsWhere`
// (`@objectstack/service-analytics`, shared by both aggregation
// strategies) THROWS "[analytics] received a 'where' array that is
// not a filter" on an array of rule objects, while accepting AST
// tuples. So a stored filter that a list renders correctly rendered
// `element:number` into its error state on every analytics-capable
// deployment — and analytics is the default one, since the CLI always
// loads it (objectui#6302).
//
// One lowering, not two: the same function, so the analytics path and
// the `find()` path cannot disagree about one stored filter — which is
// the whole reason `translateFilterArray` was made a single definition
// (see its header). Non-array filters keep passing through untouched:
// the MongoDB-style object this branch was written for is what
// `/analytics/query` already accepts, and translating it here would be
// a semantic change this fix is expressly not making.
payload.where = Array.isArray(params.filter)
? translateFilterArray(params.filter)
: params.filter;
}

const data = await this.client.analytics.query(payload);
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(data-objectstack): lower rule-shaped filter arrays on aggregate()'s analytics path by claude[bot] · Pull Request #6828 · objectstack-ai/objectui · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .changeset/6302-aggregate-filter-lowering.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
---
'@object-ui/data-objectstack': patch
---

`ObjectStackAdapter.aggregate()` lowers rule-shaped filter arrays before the
analytics wire, reusing the lowering `find()` already runs (objectui#6302).

`find()` has translated `[{ field, operator, value }, ...]` into the server's
filter AST for as long as `convertQueryParams` has existed. The analytics path
did not: `aggregate()` assigned `payload.where = params.filter` verbatim and
posted it to `/analytics/query`.

The two doors are not equally forgiving, so the gap had a user-visible end.
`lowerAnalyticsWhere` in `@objectstack/service-analytics` — shared by both
aggregation strategies — accepts AST tuples and throws on an array of rule
objects. A stored `ViewFilterRule[]` that a LIST renders correctly therefore
rendered `element:number` into its error state on every analytics-capable
deployment, which is the default one because the CLI always loads analytics.

An array filter now goes through the same `translateFilterArray` the `find()`
path uses — one lowering, so the two paths cannot disagree about one stored
filter. Rules spread into a logical node (`['and', ...rules, ...tuples]`, the
commonest composite there is) are lowered at depth, as they already were on
`find()`. Non-array filters are untouched: the MongoDB-style object this branch
was written for is what `/analytics/query` already accepts, and translating it
would be a semantic change this fix does not make. Already-AST arrays,
record-shaped filters, and the no-filter case are byte-unchanged.
28 changes: 28 additions & 0 deletions packages/data-objectstack/README.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,6 +147,34 @@ const ast = [
];
```

#### Rule-shaped arrays, on `find()` **and** `aggregate()`

Server-driven view configs store their conditions as an array of rules
(`ViewFilterRule[]`), not as a MongoDB-style object:

```typescript
const filter = [{ field: 'stage', operator: 'equals', value: 'won' }];
```

Both read paths lower that array to the same AST before it reaches the wire —
`find()` via `$filter`, and `aggregate()` via the analytics `where`. They share
one translator, so a stored filter cannot mean one thing on a list and another
on a KPI:

```typescript
// find(): filter=["stage","=","won"]
// aggregate(): { ..., where: ["stage", "=", "won"] }
```

Operator aliases (`equals`, `greater_than_or_equal`, `not_in`, `before`, ...)
map to the canonical AST symbols, and rules spread into a logical node
(`['and', ...rules, ...tuples]`) are lowered at depth. A rule that cannot be
translated raises `MalformedFilterError` rather than being dropped — dropping
one condition of an `and` would widen the result set and report success.

Non-array filters are passed through unchanged on the aggregate path: a
MongoDB-style object is already what `/analytics/query` accepts.

### Sorting

```typescript
Expand Down
261 changes: 261 additions & 0 deletions packages/data-objectstack/src/aggregate-filter-lowering.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* `aggregate()` lowers rule-shaped filter arrays before the analytics wire.
*
* WHY THIS FILE EXISTS (objectui#6302). `find()` has translated
* `[{ field, operator, value }, ...]` into the server's filter AST since the
* day `convertQueryParams` learned to — see `filter-entry-translation.test.ts`,
* which runs every shape down both `find()` routes. The analytics path did not:
* `aggregate()` assigned `payload.where = params.filter` verbatim and posted it
* to `/analytics/query`.
*
* The two doors are not equally forgiving, which is why the gap had a
* user-visible end. `lowerAnalyticsWhere` in `@objectstack/service-analytics`
* — shared by BOTH aggregation strategies, so there is no deployment where the
* lenient reading applies — accepts AST tuples and THROWS on an array of rule
* objects ("[analytics] received a 'where' array that is not a filter"). The
* spec's own `isFilterAST` gate says the same thing about the same value, and
* the tests below assert on it directly so the refusal is pinned by the
* contract rather than by a message string:
*
* isFilterAST([{ field: 'stage', operator: 'equals', value: 'won' }]) // false
* isFilterAST(['stage', '=', 'won']) // true
*
* Net effect before the fix: a stored `ViewFilterRule[]` that a LIST renders
* correctly rendered `element:number` into its error state on every
* analytics-capable deployment — and analytics is the default one, because the
* CLI always loads it.
*
* The fix reuses `translateFilterArray` rather than adding a second lowering.
* That is load-bearing and is asserted as such below: the cross-path parity
* block requires `aggregate()`'s `where` and `find()`'s `filter=` to be the
* SAME value for the same input, so the two paths cannot drift the way the two
* `find()` routes once did. Non-array filters are deliberately untouched — the
* MongoDB-style object this branch was written for is already what the
* analytics endpoint accepts, and translating it would be a semantic change
* this fix does not make.
*/

import { describe, it, expect, beforeEach, vi } from 'vitest';
import { isFilterAST } from '@objectstack/spec/data';
import { ObjectStackAdapter, clearSharedDiscoveryCache, isMalformedFilterError } from './index';

/** Rows that carry the requested measure, so nothing degrades to the fallback. */
const ANALYTICS_ROWS = { rows: [{ amount_sum: 150 }] };

function makeAdapter() {
/** Every parsed `/analytics/query` request body, in order. */
const analyticsBodies: any[] = [];
const urls: string[] = [];
const fetchImpl = vi.fn(async (url: any, init?: any) => {
const u = String(url);
urls.push(u);
if (u.includes('/api/v1/discovery')) {
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { version: 'v1', routes: {} } }),
} as any;
}
if (u.includes('/api/v1/analytics/query')) {
analyticsBodies.push(init?.body ? JSON.parse(String(init.body)) : undefined);
return { ok: true, status: 200, statusText: 'OK', json: async () => ANALYTICS_ROWS } as any;
}
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { object: 'opportunity', records: [], total: 0 } }),
} as any;
});
const adapter = new ObjectStackAdapter({
baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any,
});
return { adapter, analyticsBodies, urls };
}

const SUM_BY_STAGE = { function: 'sum', field: 'amount', groupBy: '_all' };

/**
* The `where` this filter put on the analytics wire.
*
* `HAS_NO_WHERE` distinguishes "the key was absent" from "the key was present
* and undefined" — the empty-filter cases below turn on exactly that.
*/
const HAS_NO_WHERE = Symbol('no where key');

async function whereOnWire(filter: unknown): Promise<unknown> {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', { ...SUM_BY_STAGE, filter });
expect(analyticsBodies).toHaveLength(1);
const body = analyticsBodies[0];
return 'where' in body ? body.where : HAS_NO_WHERE;
}

/** The `filter=` the SAME value produces on the plain `find()` route. */
async function filterOnFindWire(filter: unknown): Promise<unknown> {
const { adapter, urls } = makeAdapter();
await adapter.find('opportunity', { $filter: filter } as any);
const dataCall = urls.filter((u) => u.includes('/data/opportunity')).pop();
const raw = dataCall ? new URL(dataCall).searchParams.get('filter') : null;
return raw === null ? HAS_NO_WHERE : JSON.parse(raw);
}

describe('aggregate() lowers a rule-shaped filter array before `client.analytics.query`', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('translates a single rule into an AST tuple', async () => {
const where = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
expect(where).toEqual(['stage', '=', 'won']);
});

it('the lowered value passes the AST gate the raw one fails', async () => {
const rules = [{ field: 'stage', operator: 'equals', value: 'won' }];
// Negative control: this is what used to reach the wire, and it is exactly
// the value `lowerAnalyticsWhere` refuses. Without this line the test above
// could pass against a lowering that produced some OTHER non-AST shape.
expect(isFilterAST(rules)).toBe(false);
expect(isFilterAST(await whereOnWire(rules) as any)).toBe(true);
});

it('maps operator aliases the way the find() path does', async () => {
expect(await whereOnWire([{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]))
.toEqual(['amount', '>=', 3]);
});

it('joins several rules with `and`', async () => {
expect(await whereOnWire([
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
])).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('lowers rules SPREAD into a logical node, not just top-level ones', async () => {
// The commonest composite there is: a view's stored filter plus one the
// user added in the panel. The head is the string `and`, so a top-level-only
// check would call the whole thing "already AST" and ship the rule raw.
const composite = ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]];
expect(isFilterAST(composite as any)).toBe(false);
const where = await whereOnWire(composite);
expect(where).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
expect(isFilterAST(where as any)).toBe(true);
});

it('produces the SAME `where` as the AST-tuple equivalent (the acceptance criterion)', async () => {
const fromRules = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
const fromTuple = await whereOnWire(['stage', '=', 'won']);
expect(fromRules).toEqual(fromTuple);
});
});

describe('aggregate() leaves every already-correct filter shape byte-unchanged', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('an AST tuple passes through untouched', async () => {
expect(await whereOnWire(['stage', '=', 'won'])).toEqual(['stage', '=', 'won']);
});

it('a logical AST node passes through untouched', async () => {
expect(await whereOnWire(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]))
.toEqual(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]);
});

it('a legacy nested array of nodes passes through untouched', async () => {
expect(await whereOnWire([['stage', '=', 'won'], ['amount', '>', 100]]))
.toEqual([['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('a record-shaped (MongoDB-style) filter is NOT translated', async () => {
// The shape this branch was written for. `/analytics/query` accepts it, so
// lowering it here would be a semantic change, not a fix.
expect(await whereOnWire({ stage: 'won' })).toEqual({ stage: 'won' });
});

it('a record-shaped filter with an operator object is NOT translated either', async () => {
expect(await whereOnWire({ amount: { $gt: 100 } })).toEqual({ amount: { $gt: 100 } });
});

it('an empty array still reaches the wire as an empty array', async () => {
// Unchanged on purpose: `if (params.filter)` is truthy for `[]`, and this
// fix moves no boundary it did not have to move.
expect(await whereOnWire([])).toEqual([]);
});

it('no filter means no `where` key at all', async () => {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', SUM_BY_STAGE);
expect(analyticsBodies[0]).not.toHaveProperty('where');
});
});

describe('the find() path is unchanged, and the two paths share one lowering', () => {
beforeEach(() => clearSharedDiscoveryCache());

// Each row is one input. Both sides are measured on the wire, so a change to
// either path that the other does not make turns this red.
const SHARED_CASES: Array<[string, unknown]> = [
['a single rule', [{ field: 'stage', operator: 'equals', value: 'won' }]],
['an aliased operator', [{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]],
['several rules', [
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
]],
['rules spread into a logical node', ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]]],
['an AST tuple', ['stage', '=', 'won']],
['a logical AST node', ['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]],
];

for (const [name, filter] of SHARED_CASES) {
it(`aggregate() and find() agree on ${name}`, async () => {
const viaFind = await filterOnFindWire(filter);
expect(viaFind).not.toBe(HAS_NO_WHERE);
expect(await whereOnWire(filter)).toEqual(viaFind);
});
}

it('find() still lowers a single rule exactly as it did before', async () => {
// The `find()` half of the card's acceptance, stated independently of
// `aggregate()` so a regression there cannot hide behind the parity rows.
expect(await filterOnFindWire([{ field: 'stage', operator: 'equals', value: 'won' }]))
.toEqual(['stage', '=', 'won']);
});

it('find() still sends no filter for an empty array', async () => {
// The one place the two paths legitimately differ: `convertQueryParams`
// drops an empty filter, the analytics payload keeps `[]`. Recorded, not
// reconciled — reconciling it is a behaviour change this card does not make.
expect(await filterOnFindWire([])).toBe(HAS_NO_WHERE);
expect(await whereOnWire([])).toEqual([]);
});
});

describe('a rule the adapter cannot translate refuses on the aggregate path too', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('throws the same malformed-filter refusal `find()` raises, without inventing numbers', async () => {
// Dropping the untranslatable entry would WIDEN the result set and report
// success — the silent over-fetch `MalformedFilterError` exists to stop.
// Sharing the lowering means the analytics path inherits that refusal.
const { adapter, analyticsBodies, urls } = makeAdapter();
const err = await adapter
.aggregate('opportunity', {
...SUM_BY_STAGE,
filter: [
{ field: 'stage', operator: 'eq', value: 'won' },
{ operator: 'eq', value: 'no field here' },
],
})
.then(() => null, (e) => e);

expect(err).toBeInstanceOf(Error);
expect(isMalformedFilterError(err)).toBe(true);
// Nothing was posted to analytics, and no plausible-looking number came
// back from the fallback instead.
expect(analyticsBodies).toHaveLength(0);
expect(urls.some((u) => u.includes('/data/opportunity'))).toBe(false);
});
});
25 changes: 24 additions & 1 deletion packages/data-objectstack/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -4598,7 +4598,30 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
// spec/ui/dashboard.zod.ts). Send via the canonical `where`
// field of the analytics endpoint, matching the unified Query
// DSL (spec/data/query.zod.ts).
payload.where = params.filter;
//
// An ARRAY filter goes through the same `translateFilterArray` the
// `find()` path runs in `convertQueryParams`, because an authored
// `ViewFilterRule[]` reaches this method exactly as it reaches that
// one. It used to ship RAW from here, and the analytics door is
// stricter than the data door: `lowerAnalyticsWhere`
// (`@objectstack/service-analytics`, shared by both aggregation
// strategies) THROWS "[analytics] received a 'where' array that is
// not a filter" on an array of rule objects, while accepting AST
// tuples. So a stored filter that a list renders correctly rendered
// `element:number` into its error state on every analytics-capable
// deployment — and analytics is the default one, since the CLI always
// loads it (objectui#6302).
//
// One lowering, not two: the same function, so the analytics path and
// the `find()` path cannot disagree about one stored filter — which is
// the whole reason `translateFilterArray` was made a single definition
// (see its header). Non-array filters keep passing through untouched:
// the MongoDB-style object this branch was written for is what
// `/analytics/query` already accepts, and translating it here would be
// a semantic change this fix is expressly not making.
payload.where = Array.isArray(params.filter)
? translateFilterArray(params.filter)
: params.filter;
}

const data = await this.client.analytics.query(payload);
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(data-objectstack): lower rule-shaped filter arrays on aggregate()'s analytics path by claude[bot] · Pull Request #6828 · objectstack-ai/objectui · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .changeset/6302-aggregate-filter-lowering.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
---
'@object-ui/data-objectstack': patch
---

`ObjectStackAdapter.aggregate()` lowers rule-shaped filter arrays before the
analytics wire, reusing the lowering `find()` already runs (objectui#6302).

`find()` has translated `[{ field, operator, value }, ...]` into the server's
filter AST for as long as `convertQueryParams` has existed. The analytics path
did not: `aggregate()` assigned `payload.where = params.filter` verbatim and
posted it to `/analytics/query`.

The two doors are not equally forgiving, so the gap had a user-visible end.
`lowerAnalyticsWhere` in `@objectstack/service-analytics` — shared by both
aggregation strategies — accepts AST tuples and throws on an array of rule
objects. A stored `ViewFilterRule[]` that a LIST renders correctly therefore
rendered `element:number` into its error state on every analytics-capable
deployment, which is the default one because the CLI always loads analytics.

An array filter now goes through the same `translateFilterArray` the `find()`
path uses — one lowering, so the two paths cannot disagree about one stored
filter. Rules spread into a logical node (`['and', ...rules, ...tuples]`, the
commonest composite there is) are lowered at depth, as they already were on
`find()`. Non-array filters are untouched: the MongoDB-style object this branch
was written for is what `/analytics/query` already accepts, and translating it
would be a semantic change this fix does not make. Already-AST arrays,
record-shaped filters, and the no-filter case are byte-unchanged.
28 changes: 28 additions & 0 deletions packages/data-objectstack/README.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,6 +147,34 @@ const ast = [
];
```

#### Rule-shaped arrays, on `find()` **and** `aggregate()`

Server-driven view configs store their conditions as an array of rules
(`ViewFilterRule[]`), not as a MongoDB-style object:

```typescript
const filter = [{ field: 'stage', operator: 'equals', value: 'won' }];
```

Both read paths lower that array to the same AST before it reaches the wire —
`find()` via `$filter`, and `aggregate()` via the analytics `where`. They share
one translator, so a stored filter cannot mean one thing on a list and another
on a KPI:

```typescript
// find(): filter=["stage","=","won"]
// aggregate(): { ..., where: ["stage", "=", "won"] }
```

Operator aliases (`equals`, `greater_than_or_equal`, `not_in`, `before`, ...)
map to the canonical AST symbols, and rules spread into a logical node
(`['and', ...rules, ...tuples]`) are lowered at depth. A rule that cannot be
translated raises `MalformedFilterError` rather than being dropped — dropping
one condition of an `and` would widen the result set and report success.

Non-array filters are passed through unchanged on the aggregate path: a
MongoDB-style object is already what `/analytics/query` accepts.

### Sorting

```typescript
Expand Down
261 changes: 261 additions & 0 deletions packages/data-objectstack/src/aggregate-filter-lowering.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* `aggregate()` lowers rule-shaped filter arrays before the analytics wire.
*
* WHY THIS FILE EXISTS (objectui#6302). `find()` has translated
* `[{ field, operator, value }, ...]` into the server's filter AST since the
* day `convertQueryParams` learned to — see `filter-entry-translation.test.ts`,
* which runs every shape down both `find()` routes. The analytics path did not:
* `aggregate()` assigned `payload.where = params.filter` verbatim and posted it
* to `/analytics/query`.
*
* The two doors are not equally forgiving, which is why the gap had a
* user-visible end. `lowerAnalyticsWhere` in `@objectstack/service-analytics`
* — shared by BOTH aggregation strategies, so there is no deployment where the
* lenient reading applies — accepts AST tuples and THROWS on an array of rule
* objects ("[analytics] received a 'where' array that is not a filter"). The
* spec's own `isFilterAST` gate says the same thing about the same value, and
* the tests below assert on it directly so the refusal is pinned by the
* contract rather than by a message string:
*
* isFilterAST([{ field: 'stage', operator: 'equals', value: 'won' }]) // false
* isFilterAST(['stage', '=', 'won']) // true
*
* Net effect before the fix: a stored `ViewFilterRule[]` that a LIST renders
* correctly rendered `element:number` into its error state on every
* analytics-capable deployment — and analytics is the default one, because the
* CLI always loads it.
*
* The fix reuses `translateFilterArray` rather than adding a second lowering.
* That is load-bearing and is asserted as such below: the cross-path parity
* block requires `aggregate()`'s `where` and `find()`'s `filter=` to be the
* SAME value for the same input, so the two paths cannot drift the way the two
* `find()` routes once did. Non-array filters are deliberately untouched — the
* MongoDB-style object this branch was written for is already what the
* analytics endpoint accepts, and translating it would be a semantic change
* this fix does not make.
*/

import { describe, it, expect, beforeEach, vi } from 'vitest';
import { isFilterAST } from '@objectstack/spec/data';
import { ObjectStackAdapter, clearSharedDiscoveryCache, isMalformedFilterError } from './index';

/** Rows that carry the requested measure, so nothing degrades to the fallback. */
const ANALYTICS_ROWS = { rows: [{ amount_sum: 150 }] };

function makeAdapter() {
/** Every parsed `/analytics/query` request body, in order. */
const analyticsBodies: any[] = [];
const urls: string[] = [];
const fetchImpl = vi.fn(async (url: any, init?: any) => {
const u = String(url);
urls.push(u);
if (u.includes('/api/v1/discovery')) {
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { version: 'v1', routes: {} } }),
} as any;
}
if (u.includes('/api/v1/analytics/query')) {
analyticsBodies.push(init?.body ? JSON.parse(String(init.body)) : undefined);
return { ok: true, status: 200, statusText: 'OK', json: async () => ANALYTICS_ROWS } as any;
}
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { object: 'opportunity', records: [], total: 0 } }),
} as any;
});
const adapter = new ObjectStackAdapter({
baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any,
});
return { adapter, analyticsBodies, urls };
}

const SUM_BY_STAGE = { function: 'sum', field: 'amount', groupBy: '_all' };

/**
* The `where` this filter put on the analytics wire.
*
* `HAS_NO_WHERE` distinguishes "the key was absent" from "the key was present
* and undefined" — the empty-filter cases below turn on exactly that.
*/
const HAS_NO_WHERE = Symbol('no where key');

async function whereOnWire(filter: unknown): Promise<unknown> {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', { ...SUM_BY_STAGE, filter });
expect(analyticsBodies).toHaveLength(1);
const body = analyticsBodies[0];
return 'where' in body ? body.where : HAS_NO_WHERE;
}

/** The `filter=` the SAME value produces on the plain `find()` route. */
async function filterOnFindWire(filter: unknown): Promise<unknown> {
const { adapter, urls } = makeAdapter();
await adapter.find('opportunity', { $filter: filter } as any);
const dataCall = urls.filter((u) => u.includes('/data/opportunity')).pop();
const raw = dataCall ? new URL(dataCall).searchParams.get('filter') : null;
return raw === null ? HAS_NO_WHERE : JSON.parse(raw);
}

describe('aggregate() lowers a rule-shaped filter array before `client.analytics.query`', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('translates a single rule into an AST tuple', async () => {
const where = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
expect(where).toEqual(['stage', '=', 'won']);
});

it('the lowered value passes the AST gate the raw one fails', async () => {
const rules = [{ field: 'stage', operator: 'equals', value: 'won' }];
// Negative control: this is what used to reach the wire, and it is exactly
// the value `lowerAnalyticsWhere` refuses. Without this line the test above
// could pass against a lowering that produced some OTHER non-AST shape.
expect(isFilterAST(rules)).toBe(false);
expect(isFilterAST(await whereOnWire(rules) as any)).toBe(true);
});

it('maps operator aliases the way the find() path does', async () => {
expect(await whereOnWire([{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]))
.toEqual(['amount', '>=', 3]);
});

it('joins several rules with `and`', async () => {
expect(await whereOnWire([
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
])).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('lowers rules SPREAD into a logical node, not just top-level ones', async () => {
// The commonest composite there is: a view's stored filter plus one the
// user added in the panel. The head is the string `and`, so a top-level-only
// check would call the whole thing "already AST" and ship the rule raw.
const composite = ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]];
expect(isFilterAST(composite as any)).toBe(false);
const where = await whereOnWire(composite);
expect(where).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
expect(isFilterAST(where as any)).toBe(true);
});

it('produces the SAME `where` as the AST-tuple equivalent (the acceptance criterion)', async () => {
const fromRules = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
const fromTuple = await whereOnWire(['stage', '=', 'won']);
expect(fromRules).toEqual(fromTuple);
});
});

describe('aggregate() leaves every already-correct filter shape byte-unchanged', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('an AST tuple passes through untouched', async () => {
expect(await whereOnWire(['stage', '=', 'won'])).toEqual(['stage', '=', 'won']);
});

it('a logical AST node passes through untouched', async () => {
expect(await whereOnWire(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]))
.toEqual(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]);
});

it('a legacy nested array of nodes passes through untouched', async () => {
expect(await whereOnWire([['stage', '=', 'won'], ['amount', '>', 100]]))
.toEqual([['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('a record-shaped (MongoDB-style) filter is NOT translated', async () => {
// The shape this branch was written for. `/analytics/query` accepts it, so
// lowering it here would be a semantic change, not a fix.
expect(await whereOnWire({ stage: 'won' })).toEqual({ stage: 'won' });
});

it('a record-shaped filter with an operator object is NOT translated either', async () => {
expect(await whereOnWire({ amount: { $gt: 100 } })).toEqual({ amount: { $gt: 100 } });
});

it('an empty array still reaches the wire as an empty array', async () => {
// Unchanged on purpose: `if (params.filter)` is truthy for `[]`, and this
// fix moves no boundary it did not have to move.
expect(await whereOnWire([])).toEqual([]);
});

it('no filter means no `where` key at all', async () => {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', SUM_BY_STAGE);
expect(analyticsBodies[0]).not.toHaveProperty('where');
});
});

describe('the find() path is unchanged, and the two paths share one lowering', () => {
beforeEach(() => clearSharedDiscoveryCache());

// Each row is one input. Both sides are measured on the wire, so a change to
// either path that the other does not make turns this red.
const SHARED_CASES: Array<[string, unknown]> = [
['a single rule', [{ field: 'stage', operator: 'equals', value: 'won' }]],
['an aliased operator', [{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]],
['several rules', [
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
]],
['rules spread into a logical node', ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]]],
['an AST tuple', ['stage', '=', 'won']],
['a logical AST node', ['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]],
];

for (const [name, filter] of SHARED_CASES) {
it(`aggregate() and find() agree on ${name}`, async () => {
const viaFind = await filterOnFindWire(filter);
expect(viaFind).not.toBe(HAS_NO_WHERE);
expect(await whereOnWire(filter)).toEqual(viaFind);
});
}

it('find() still lowers a single rule exactly as it did before', async () => {
// The `find()` half of the card's acceptance, stated independently of
// `aggregate()` so a regression there cannot hide behind the parity rows.
expect(await filterOnFindWire([{ field: 'stage', operator: 'equals', value: 'won' }]))
.toEqual(['stage', '=', 'won']);
});

it('find() still sends no filter for an empty array', async () => {
// The one place the two paths legitimately differ: `convertQueryParams`
// drops an empty filter, the analytics payload keeps `[]`. Recorded, not
// reconciled — reconciling it is a behaviour change this card does not make.
expect(await filterOnFindWire([])).toBe(HAS_NO_WHERE);
expect(await whereOnWire([])).toEqual([]);
});
});

describe('a rule the adapter cannot translate refuses on the aggregate path too', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('throws the same malformed-filter refusal `find()` raises, without inventing numbers', async () => {
// Dropping the untranslatable entry would WIDEN the result set and report
// success — the silent over-fetch `MalformedFilterError` exists to stop.
// Sharing the lowering means the analytics path inherits that refusal.
const { adapter, analyticsBodies, urls } = makeAdapter();
const err = await adapter
.aggregate('opportunity', {
...SUM_BY_STAGE,
filter: [
{ field: 'stage', operator: 'eq', value: 'won' },
{ operator: 'eq', value: 'no field here' },
],
})
.then(() => null, (e) => e);

expect(err).toBeInstanceOf(Error);
expect(isMalformedFilterError(err)).toBe(true);
// Nothing was posted to analytics, and no plausible-looking number came
// back from the fallback instead.
expect(analyticsBodies).toHaveLength(0);
expect(urls.some((u) => u.includes('/data/opportunity'))).toBe(false);
});
});
25 changes: 24 additions & 1 deletion packages/data-objectstack/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -4598,7 +4598,30 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
// spec/ui/dashboard.zod.ts). Send via the canonical `where`
// field of the analytics endpoint, matching the unified Query
// DSL (spec/data/query.zod.ts).
payload.where = params.filter;
//
// An ARRAY filter goes through the same `translateFilterArray` the
// `find()` path runs in `convertQueryParams`, because an authored
// `ViewFilterRule[]` reaches this method exactly as it reaches that
// one. It used to ship RAW from here, and the analytics door is
// stricter than the data door: `lowerAnalyticsWhere`
// (`@objectstack/service-analytics`, shared by both aggregation
// strategies) THROWS "[analytics] received a 'where' array that is
// not a filter" on an array of rule objects, while accepting AST
// tuples. So a stored filter that a list renders correctly rendered
// `element:number` into its error state on every analytics-capable
// deployment — and analytics is the default one, since the CLI always
// loads it (objectui#6302).
//
// One lowering, not two: the same function, so the analytics path and
// the `find()` path cannot disagree about one stored filter — which is
// the whole reason `translateFilterArray` was made a single definition
// (see its header). Non-array filters keep passing through untouched:
// the MongoDB-style object this branch was written for is what
// `/analytics/query` already accepts, and translating it here would be
// a semantic change this fix is expressly not making.
payload.where = Array.isArray(params.filter)
? translateFilterArray(params.filter)
: params.filter;
}

const data = await this.client.analytics.query(payload);
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(data-objectstack): lower rule-shaped filter arrays on aggregate()'s analytics path by claude[bot] · Pull Request #6828 · objectstack-ai/objectui · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .changeset/6302-aggregate-filter-lowering.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
---
'@object-ui/data-objectstack': patch
---

`ObjectStackAdapter.aggregate()` lowers rule-shaped filter arrays before the
analytics wire, reusing the lowering `find()` already runs (objectui#6302).

`find()` has translated `[{ field, operator, value }, ...]` into the server's
filter AST for as long as `convertQueryParams` has existed. The analytics path
did not: `aggregate()` assigned `payload.where = params.filter` verbatim and
posted it to `/analytics/query`.

The two doors are not equally forgiving, so the gap had a user-visible end.
`lowerAnalyticsWhere` in `@objectstack/service-analytics` — shared by both
aggregation strategies — accepts AST tuples and throws on an array of rule
objects. A stored `ViewFilterRule[]` that a LIST renders correctly therefore
rendered `element:number` into its error state on every analytics-capable
deployment, which is the default one because the CLI always loads analytics.

An array filter now goes through the same `translateFilterArray` the `find()`
path uses — one lowering, so the two paths cannot disagree about one stored
filter. Rules spread into a logical node (`['and', ...rules, ...tuples]`, the
commonest composite there is) are lowered at depth, as they already were on
`find()`. Non-array filters are untouched: the MongoDB-style object this branch
was written for is what `/analytics/query` already accepts, and translating it
would be a semantic change this fix does not make. Already-AST arrays,
record-shaped filters, and the no-filter case are byte-unchanged.
28 changes: 28 additions & 0 deletions packages/data-objectstack/README.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,6 +147,34 @@ const ast = [
];
```

#### Rule-shaped arrays, on `find()` **and** `aggregate()`

Server-driven view configs store their conditions as an array of rules
(`ViewFilterRule[]`), not as a MongoDB-style object:

```typescript
const filter = [{ field: 'stage', operator: 'equals', value: 'won' }];
```

Both read paths lower that array to the same AST before it reaches the wire —
`find()` via `$filter`, and `aggregate()` via the analytics `where`. They share
one translator, so a stored filter cannot mean one thing on a list and another
on a KPI:

```typescript
// find(): filter=["stage","=","won"]
// aggregate(): { ..., where: ["stage", "=", "won"] }
```

Operator aliases (`equals`, `greater_than_or_equal`, `not_in`, `before`, ...)
map to the canonical AST symbols, and rules spread into a logical node
(`['and', ...rules, ...tuples]`) are lowered at depth. A rule that cannot be
translated raises `MalformedFilterError` rather than being dropped — dropping
one condition of an `and` would widen the result set and report success.

Non-array filters are passed through unchanged on the aggregate path: a
MongoDB-style object is already what `/analytics/query` accepts.

### Sorting

```typescript
Expand Down
261 changes: 261 additions & 0 deletions packages/data-objectstack/src/aggregate-filter-lowering.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* `aggregate()` lowers rule-shaped filter arrays before the analytics wire.
*
* WHY THIS FILE EXISTS (objectui#6302). `find()` has translated
* `[{ field, operator, value }, ...]` into the server's filter AST since the
* day `convertQueryParams` learned to — see `filter-entry-translation.test.ts`,
* which runs every shape down both `find()` routes. The analytics path did not:
* `aggregate()` assigned `payload.where = params.filter` verbatim and posted it
* to `/analytics/query`.
*
* The two doors are not equally forgiving, which is why the gap had a
* user-visible end. `lowerAnalyticsWhere` in `@objectstack/service-analytics`
* — shared by BOTH aggregation strategies, so there is no deployment where the
* lenient reading applies — accepts AST tuples and THROWS on an array of rule
* objects ("[analytics] received a 'where' array that is not a filter"). The
* spec's own `isFilterAST` gate says the same thing about the same value, and
* the tests below assert on it directly so the refusal is pinned by the
* contract rather than by a message string:
*
* isFilterAST([{ field: 'stage', operator: 'equals', value: 'won' }]) // false
* isFilterAST(['stage', '=', 'won']) // true
*
* Net effect before the fix: a stored `ViewFilterRule[]` that a LIST renders
* correctly rendered `element:number` into its error state on every
* analytics-capable deployment — and analytics is the default one, because the
* CLI always loads it.
*
* The fix reuses `translateFilterArray` rather than adding a second lowering.
* That is load-bearing and is asserted as such below: the cross-path parity
* block requires `aggregate()`'s `where` and `find()`'s `filter=` to be the
* SAME value for the same input, so the two paths cannot drift the way the two
* `find()` routes once did. Non-array filters are deliberately untouched — the
* MongoDB-style object this branch was written for is already what the
* analytics endpoint accepts, and translating it would be a semantic change
* this fix does not make.
*/

import { describe, it, expect, beforeEach, vi } from 'vitest';
import { isFilterAST } from '@objectstack/spec/data';
import { ObjectStackAdapter, clearSharedDiscoveryCache, isMalformedFilterError } from './index';

/** Rows that carry the requested measure, so nothing degrades to the fallback. */
const ANALYTICS_ROWS = { rows: [{ amount_sum: 150 }] };

function makeAdapter() {
/** Every parsed `/analytics/query` request body, in order. */
const analyticsBodies: any[] = [];
const urls: string[] = [];
const fetchImpl = vi.fn(async (url: any, init?: any) => {
const u = String(url);
urls.push(u);
if (u.includes('/api/v1/discovery')) {
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { version: 'v1', routes: {} } }),
} as any;
}
if (u.includes('/api/v1/analytics/query')) {
analyticsBodies.push(init?.body ? JSON.parse(String(init.body)) : undefined);
return { ok: true, status: 200, statusText: 'OK', json: async () => ANALYTICS_ROWS } as any;
}
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { object: 'opportunity', records: [], total: 0 } }),
} as any;
});
const adapter = new ObjectStackAdapter({
baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any,
});
return { adapter, analyticsBodies, urls };
}

const SUM_BY_STAGE = { function: 'sum', field: 'amount', groupBy: '_all' };

/**
* The `where` this filter put on the analytics wire.
*
* `HAS_NO_WHERE` distinguishes "the key was absent" from "the key was present
* and undefined" — the empty-filter cases below turn on exactly that.
*/
const HAS_NO_WHERE = Symbol('no where key');

async function whereOnWire(filter: unknown): Promise<unknown> {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', { ...SUM_BY_STAGE, filter });
expect(analyticsBodies).toHaveLength(1);
const body = analyticsBodies[0];
return 'where' in body ? body.where : HAS_NO_WHERE;
}

/** The `filter=` the SAME value produces on the plain `find()` route. */
async function filterOnFindWire(filter: unknown): Promise<unknown> {
const { adapter, urls } = makeAdapter();
await adapter.find('opportunity', { $filter: filter } as any);
const dataCall = urls.filter((u) => u.includes('/data/opportunity')).pop();
const raw = dataCall ? new URL(dataCall).searchParams.get('filter') : null;
return raw === null ? HAS_NO_WHERE : JSON.parse(raw);
}

describe('aggregate() lowers a rule-shaped filter array before `client.analytics.query`', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('translates a single rule into an AST tuple', async () => {
const where = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
expect(where).toEqual(['stage', '=', 'won']);
});

it('the lowered value passes the AST gate the raw one fails', async () => {
const rules = [{ field: 'stage', operator: 'equals', value: 'won' }];
// Negative control: this is what used to reach the wire, and it is exactly
// the value `lowerAnalyticsWhere` refuses. Without this line the test above
// could pass against a lowering that produced some OTHER non-AST shape.
expect(isFilterAST(rules)).toBe(false);
expect(isFilterAST(await whereOnWire(rules) as any)).toBe(true);
});

it('maps operator aliases the way the find() path does', async () => {
expect(await whereOnWire([{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]))
.toEqual(['amount', '>=', 3]);
});

it('joins several rules with `and`', async () => {
expect(await whereOnWire([
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
])).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('lowers rules SPREAD into a logical node, not just top-level ones', async () => {
// The commonest composite there is: a view's stored filter plus one the
// user added in the panel. The head is the string `and`, so a top-level-only
// check would call the whole thing "already AST" and ship the rule raw.
const composite = ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]];
expect(isFilterAST(composite as any)).toBe(false);
const where = await whereOnWire(composite);
expect(where).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
expect(isFilterAST(where as any)).toBe(true);
});

it('produces the SAME `where` as the AST-tuple equivalent (the acceptance criterion)', async () => {
const fromRules = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
const fromTuple = await whereOnWire(['stage', '=', 'won']);
expect(fromRules).toEqual(fromTuple);
});
});

describe('aggregate() leaves every already-correct filter shape byte-unchanged', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('an AST tuple passes through untouched', async () => {
expect(await whereOnWire(['stage', '=', 'won'])).toEqual(['stage', '=', 'won']);
});

it('a logical AST node passes through untouched', async () => {
expect(await whereOnWire(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]))
.toEqual(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]);
});

it('a legacy nested array of nodes passes through untouched', async () => {
expect(await whereOnWire([['stage', '=', 'won'], ['amount', '>', 100]]))
.toEqual([['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('a record-shaped (MongoDB-style) filter is NOT translated', async () => {
// The shape this branch was written for. `/analytics/query` accepts it, so
// lowering it here would be a semantic change, not a fix.
expect(await whereOnWire({ stage: 'won' })).toEqual({ stage: 'won' });
});

it('a record-shaped filter with an operator object is NOT translated either', async () => {
expect(await whereOnWire({ amount: { $gt: 100 } })).toEqual({ amount: { $gt: 100 } });
});

it('an empty array still reaches the wire as an empty array', async () => {
// Unchanged on purpose: `if (params.filter)` is truthy for `[]`, and this
// fix moves no boundary it did not have to move.
expect(await whereOnWire([])).toEqual([]);
});

it('no filter means no `where` key at all', async () => {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', SUM_BY_STAGE);
expect(analyticsBodies[0]).not.toHaveProperty('where');
});
});

describe('the find() path is unchanged, and the two paths share one lowering', () => {
beforeEach(() => clearSharedDiscoveryCache());

// Each row is one input. Both sides are measured on the wire, so a change to
// either path that the other does not make turns this red.
const SHARED_CASES: Array<[string, unknown]> = [
['a single rule', [{ field: 'stage', operator: 'equals', value: 'won' }]],
['an aliased operator', [{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]],
['several rules', [
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
]],
['rules spread into a logical node', ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]]],
['an AST tuple', ['stage', '=', 'won']],
['a logical AST node', ['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]],
];

for (const [name, filter] of SHARED_CASES) {
it(`aggregate() and find() agree on ${name}`, async () => {
const viaFind = await filterOnFindWire(filter);
expect(viaFind).not.toBe(HAS_NO_WHERE);
expect(await whereOnWire(filter)).toEqual(viaFind);
});
}

it('find() still lowers a single rule exactly as it did before', async () => {
// The `find()` half of the card's acceptance, stated independently of
// `aggregate()` so a regression there cannot hide behind the parity rows.
expect(await filterOnFindWire([{ field: 'stage', operator: 'equals', value: 'won' }]))
.toEqual(['stage', '=', 'won']);
});

it('find() still sends no filter for an empty array', async () => {
// The one place the two paths legitimately differ: `convertQueryParams`
// drops an empty filter, the analytics payload keeps `[]`. Recorded, not
// reconciled — reconciling it is a behaviour change this card does not make.
expect(await filterOnFindWire([])).toBe(HAS_NO_WHERE);
expect(await whereOnWire([])).toEqual([]);
});
});

describe('a rule the adapter cannot translate refuses on the aggregate path too', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('throws the same malformed-filter refusal `find()` raises, without inventing numbers', async () => {
// Dropping the untranslatable entry would WIDEN the result set and report
// success — the silent over-fetch `MalformedFilterError` exists to stop.
// Sharing the lowering means the analytics path inherits that refusal.
const { adapter, analyticsBodies, urls } = makeAdapter();
const err = await adapter
.aggregate('opportunity', {
...SUM_BY_STAGE,
filter: [
{ field: 'stage', operator: 'eq', value: 'won' },
{ operator: 'eq', value: 'no field here' },
],
})
.then(() => null, (e) => e);

expect(err).toBeInstanceOf(Error);
expect(isMalformedFilterError(err)).toBe(true);
// Nothing was posted to analytics, and no plausible-looking number came
// back from the fallback instead.
expect(analyticsBodies).toHaveLength(0);
expect(urls.some((u) => u.includes('/data/opportunity'))).toBe(false);
});
});
25 changes: 24 additions & 1 deletion packages/data-objectstack/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -4598,7 +4598,30 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
// spec/ui/dashboard.zod.ts). Send via the canonical `where`
// field of the analytics endpoint, matching the unified Query
// DSL (spec/data/query.zod.ts).
payload.where = params.filter;
//
// An ARRAY filter goes through the same `translateFilterArray` the
// `find()` path runs in `convertQueryParams`, because an authored
// `ViewFilterRule[]` reaches this method exactly as it reaches that
// one. It used to ship RAW from here, and the analytics door is
// stricter than the data door: `lowerAnalyticsWhere`
// (`@objectstack/service-analytics`, shared by both aggregation
// strategies) THROWS "[analytics] received a 'where' array that is
// not a filter" on an array of rule objects, while accepting AST
// tuples. So a stored filter that a list renders correctly rendered
// `element:number` into its error state on every analytics-capable
// deployment — and analytics is the default one, since the CLI always
// loads it (objectui#6302).
//
// One lowering, not two: the same function, so the analytics path and
// the `find()` path cannot disagree about one stored filter — which is
// the whole reason `translateFilterArray` was made a single definition
// (see its header). Non-array filters keep passing through untouched:
// the MongoDB-style object this branch was written for is what
// `/analytics/query` already accepts, and translating it here would be
// a semantic change this fix is expressly not making.
payload.where = Array.isArray(params.filter)
? translateFilterArray(params.filter)
: params.filter;
}

const data = await this.client.analytics.query(payload);
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(data-objectstack): lower rule-shaped filter arrays on aggregate()'s analytics path by claude[bot] · Pull Request #6828 · objectstack-ai/objectui · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .changeset/6302-aggregate-filter-lowering.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
---
'@object-ui/data-objectstack': patch
---

`ObjectStackAdapter.aggregate()` lowers rule-shaped filter arrays before the
analytics wire, reusing the lowering `find()` already runs (objectui#6302).

`find()` has translated `[{ field, operator, value }, ...]` into the server's
filter AST for as long as `convertQueryParams` has existed. The analytics path
did not: `aggregate()` assigned `payload.where = params.filter` verbatim and
posted it to `/analytics/query`.

The two doors are not equally forgiving, so the gap had a user-visible end.
`lowerAnalyticsWhere` in `@objectstack/service-analytics` — shared by both
aggregation strategies — accepts AST tuples and throws on an array of rule
objects. A stored `ViewFilterRule[]` that a LIST renders correctly therefore
rendered `element:number` into its error state on every analytics-capable
deployment, which is the default one because the CLI always loads analytics.

An array filter now goes through the same `translateFilterArray` the `find()`
path uses — one lowering, so the two paths cannot disagree about one stored
filter. Rules spread into a logical node (`['and', ...rules, ...tuples]`, the
commonest composite there is) are lowered at depth, as they already were on
`find()`. Non-array filters are untouched: the MongoDB-style object this branch
was written for is what `/analytics/query` already accepts, and translating it
would be a semantic change this fix does not make. Already-AST arrays,
record-shaped filters, and the no-filter case are byte-unchanged.
28 changes: 28 additions & 0 deletions packages/data-objectstack/README.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,6 +147,34 @@ const ast = [
];
```

#### Rule-shaped arrays, on `find()` **and** `aggregate()`

Server-driven view configs store their conditions as an array of rules
(`ViewFilterRule[]`), not as a MongoDB-style object:

```typescript
const filter = [{ field: 'stage', operator: 'equals', value: 'won' }];
```

Both read paths lower that array to the same AST before it reaches the wire —
`find()` via `$filter`, and `aggregate()` via the analytics `where`. They share
one translator, so a stored filter cannot mean one thing on a list and another
on a KPI:

```typescript
// find(): filter=["stage","=","won"]
// aggregate(): { ..., where: ["stage", "=", "won"] }
```

Operator aliases (`equals`, `greater_than_or_equal`, `not_in`, `before`, ...)
map to the canonical AST symbols, and rules spread into a logical node
(`['and', ...rules, ...tuples]`) are lowered at depth. A rule that cannot be
translated raises `MalformedFilterError` rather than being dropped — dropping
one condition of an `and` would widen the result set and report success.

Non-array filters are passed through unchanged on the aggregate path: a
MongoDB-style object is already what `/analytics/query` accepts.

### Sorting

```typescript
Expand Down
261 changes: 261 additions & 0 deletions packages/data-objectstack/src/aggregate-filter-lowering.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* `aggregate()` lowers rule-shaped filter arrays before the analytics wire.
*
* WHY THIS FILE EXISTS (objectui#6302). `find()` has translated
* `[{ field, operator, value }, ...]` into the server's filter AST since the
* day `convertQueryParams` learned to — see `filter-entry-translation.test.ts`,
* which runs every shape down both `find()` routes. The analytics path did not:
* `aggregate()` assigned `payload.where = params.filter` verbatim and posted it
* to `/analytics/query`.
*
* The two doors are not equally forgiving, which is why the gap had a
* user-visible end. `lowerAnalyticsWhere` in `@objectstack/service-analytics`
* — shared by BOTH aggregation strategies, so there is no deployment where the
* lenient reading applies — accepts AST tuples and THROWS on an array of rule
* objects ("[analytics] received a 'where' array that is not a filter"). The
* spec's own `isFilterAST` gate says the same thing about the same value, and
* the tests below assert on it directly so the refusal is pinned by the
* contract rather than by a message string:
*
* isFilterAST([{ field: 'stage', operator: 'equals', value: 'won' }]) // false
* isFilterAST(['stage', '=', 'won']) // true
*
* Net effect before the fix: a stored `ViewFilterRule[]` that a LIST renders
* correctly rendered `element:number` into its error state on every
* analytics-capable deployment — and analytics is the default one, because the
* CLI always loads it.
*
* The fix reuses `translateFilterArray` rather than adding a second lowering.
* That is load-bearing and is asserted as such below: the cross-path parity
* block requires `aggregate()`'s `where` and `find()`'s `filter=` to be the
* SAME value for the same input, so the two paths cannot drift the way the two
* `find()` routes once did. Non-array filters are deliberately untouched — the
* MongoDB-style object this branch was written for is already what the
* analytics endpoint accepts, and translating it would be a semantic change
* this fix does not make.
*/

import { describe, it, expect, beforeEach, vi } from 'vitest';
import { isFilterAST } from '@objectstack/spec/data';
import { ObjectStackAdapter, clearSharedDiscoveryCache, isMalformedFilterError } from './index';

/** Rows that carry the requested measure, so nothing degrades to the fallback. */
const ANALYTICS_ROWS = { rows: [{ amount_sum: 150 }] };

function makeAdapter() {
/** Every parsed `/analytics/query` request body, in order. */
const analyticsBodies: any[] = [];
const urls: string[] = [];
const fetchImpl = vi.fn(async (url: any, init?: any) => {
const u = String(url);
urls.push(u);
if (u.includes('/api/v1/discovery')) {
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { version: 'v1', routes: {} } }),
} as any;
}
if (u.includes('/api/v1/analytics/query')) {
analyticsBodies.push(init?.body ? JSON.parse(String(init.body)) : undefined);
return { ok: true, status: 200, statusText: 'OK', json: async () => ANALYTICS_ROWS } as any;
}
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { object: 'opportunity', records: [], total: 0 } }),
} as any;
});
const adapter = new ObjectStackAdapter({
baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any,
});
return { adapter, analyticsBodies, urls };
}

const SUM_BY_STAGE = { function: 'sum', field: 'amount', groupBy: '_all' };

/**
* The `where` this filter put on the analytics wire.
*
* `HAS_NO_WHERE` distinguishes "the key was absent" from "the key was present
* and undefined" — the empty-filter cases below turn on exactly that.
*/
const HAS_NO_WHERE = Symbol('no where key');

async function whereOnWire(filter: unknown): Promise<unknown> {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', { ...SUM_BY_STAGE, filter });
expect(analyticsBodies).toHaveLength(1);
const body = analyticsBodies[0];
return 'where' in body ? body.where : HAS_NO_WHERE;
}

/** The `filter=` the SAME value produces on the plain `find()` route. */
async function filterOnFindWire(filter: unknown): Promise<unknown> {
const { adapter, urls } = makeAdapter();
await adapter.find('opportunity', { $filter: filter } as any);
const dataCall = urls.filter((u) => u.includes('/data/opportunity')).pop();
const raw = dataCall ? new URL(dataCall).searchParams.get('filter') : null;
return raw === null ? HAS_NO_WHERE : JSON.parse(raw);
}

describe('aggregate() lowers a rule-shaped filter array before `client.analytics.query`', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('translates a single rule into an AST tuple', async () => {
const where = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
expect(where).toEqual(['stage', '=', 'won']);
});

it('the lowered value passes the AST gate the raw one fails', async () => {
const rules = [{ field: 'stage', operator: 'equals', value: 'won' }];
// Negative control: this is what used to reach the wire, and it is exactly
// the value `lowerAnalyticsWhere` refuses. Without this line the test above
// could pass against a lowering that produced some OTHER non-AST shape.
expect(isFilterAST(rules)).toBe(false);
expect(isFilterAST(await whereOnWire(rules) as any)).toBe(true);
});

it('maps operator aliases the way the find() path does', async () => {
expect(await whereOnWire([{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]))
.toEqual(['amount', '>=', 3]);
});

it('joins several rules with `and`', async () => {
expect(await whereOnWire([
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
])).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('lowers rules SPREAD into a logical node, not just top-level ones', async () => {
// The commonest composite there is: a view's stored filter plus one the
// user added in the panel. The head is the string `and`, so a top-level-only
// check would call the whole thing "already AST" and ship the rule raw.
const composite = ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]];
expect(isFilterAST(composite as any)).toBe(false);
const where = await whereOnWire(composite);
expect(where).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
expect(isFilterAST(where as any)).toBe(true);
});

it('produces the SAME `where` as the AST-tuple equivalent (the acceptance criterion)', async () => {
const fromRules = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
const fromTuple = await whereOnWire(['stage', '=', 'won']);
expect(fromRules).toEqual(fromTuple);
});
});

describe('aggregate() leaves every already-correct filter shape byte-unchanged', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('an AST tuple passes through untouched', async () => {
expect(await whereOnWire(['stage', '=', 'won'])).toEqual(['stage', '=', 'won']);
});

it('a logical AST node passes through untouched', async () => {
expect(await whereOnWire(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]))
.toEqual(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]);
});

it('a legacy nested array of nodes passes through untouched', async () => {
expect(await whereOnWire([['stage', '=', 'won'], ['amount', '>', 100]]))
.toEqual([['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('a record-shaped (MongoDB-style) filter is NOT translated', async () => {
// The shape this branch was written for. `/analytics/query` accepts it, so
// lowering it here would be a semantic change, not a fix.
expect(await whereOnWire({ stage: 'won' })).toEqual({ stage: 'won' });
});

it('a record-shaped filter with an operator object is NOT translated either', async () => {
expect(await whereOnWire({ amount: { $gt: 100 } })).toEqual({ amount: { $gt: 100 } });
});

it('an empty array still reaches the wire as an empty array', async () => {
// Unchanged on purpose: `if (params.filter)` is truthy for `[]`, and this
// fix moves no boundary it did not have to move.
expect(await whereOnWire([])).toEqual([]);
});

it('no filter means no `where` key at all', async () => {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', SUM_BY_STAGE);
expect(analyticsBodies[0]).not.toHaveProperty('where');
});
});

describe('the find() path is unchanged, and the two paths share one lowering', () => {
beforeEach(() => clearSharedDiscoveryCache());

// Each row is one input. Both sides are measured on the wire, so a change to
// either path that the other does not make turns this red.
const SHARED_CASES: Array<[string, unknown]> = [
['a single rule', [{ field: 'stage', operator: 'equals', value: 'won' }]],
['an aliased operator', [{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]],
['several rules', [
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
]],
['rules spread into a logical node', ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]]],
['an AST tuple', ['stage', '=', 'won']],
['a logical AST node', ['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]],
];

for (const [name, filter] of SHARED_CASES) {
it(`aggregate() and find() agree on ${name}`, async () => {
const viaFind = await filterOnFindWire(filter);
expect(viaFind).not.toBe(HAS_NO_WHERE);
expect(await whereOnWire(filter)).toEqual(viaFind);
});
}

it('find() still lowers a single rule exactly as it did before', async () => {
// The `find()` half of the card's acceptance, stated independently of
// `aggregate()` so a regression there cannot hide behind the parity rows.
expect(await filterOnFindWire([{ field: 'stage', operator: 'equals', value: 'won' }]))
.toEqual(['stage', '=', 'won']);
});

it('find() still sends no filter for an empty array', async () => {
// The one place the two paths legitimately differ: `convertQueryParams`
// drops an empty filter, the analytics payload keeps `[]`. Recorded, not
// reconciled — reconciling it is a behaviour change this card does not make.
expect(await filterOnFindWire([])).toBe(HAS_NO_WHERE);
expect(await whereOnWire([])).toEqual([]);
});
});

describe('a rule the adapter cannot translate refuses on the aggregate path too', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('throws the same malformed-filter refusal `find()` raises, without inventing numbers', async () => {
// Dropping the untranslatable entry would WIDEN the result set and report
// success — the silent over-fetch `MalformedFilterError` exists to stop.
// Sharing the lowering means the analytics path inherits that refusal.
const { adapter, analyticsBodies, urls } = makeAdapter();
const err = await adapter
.aggregate('opportunity', {
...SUM_BY_STAGE,
filter: [
{ field: 'stage', operator: 'eq', value: 'won' },
{ operator: 'eq', value: 'no field here' },
],
})
.then(() => null, (e) => e);

expect(err).toBeInstanceOf(Error);
expect(isMalformedFilterError(err)).toBe(true);
// Nothing was posted to analytics, and no plausible-looking number came
// back from the fallback instead.
expect(analyticsBodies).toHaveLength(0);
expect(urls.some((u) => u.includes('/data/opportunity'))).toBe(false);
});
});
25 changes: 24 additions & 1 deletion packages/data-objectstack/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -4598,7 +4598,30 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
// spec/ui/dashboard.zod.ts). Send via the canonical `where`
// field of the analytics endpoint, matching the unified Query
// DSL (spec/data/query.zod.ts).
payload.where = params.filter;
//
// An ARRAY filter goes through the same `translateFilterArray` the
// `find()` path runs in `convertQueryParams`, because an authored
// `ViewFilterRule[]` reaches this method exactly as it reaches that
// one. It used to ship RAW from here, and the analytics door is
// stricter than the data door: `lowerAnalyticsWhere`
// (`@objectstack/service-analytics`, shared by both aggregation
// strategies) THROWS "[analytics] received a 'where' array that is
// not a filter" on an array of rule objects, while accepting AST
// tuples. So a stored filter that a list renders correctly rendered
// `element:number` into its error state on every analytics-capable
// deployment — and analytics is the default one, since the CLI always
// loads it (objectui#6302).
//
// One lowering, not two: the same function, so the analytics path and
// the `find()` path cannot disagree about one stored filter — which is
// the whole reason `translateFilterArray` was made a single definition
// (see its header). Non-array filters keep passing through untouched:
// the MongoDB-style object this branch was written for is what
// `/analytics/query` already accepts, and translating it here would be
// a semantic change this fix is expressly not making.
payload.where = Array.isArray(params.filter)
? translateFilterArray(params.filter)
: params.filter;
}

const data = await this.client.analytics.query(payload);
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(data-objectstack): lower rule-shaped filter arrays on aggregate()'s analytics path by claude[bot] · Pull Request #6828 · objectstack-ai/objectui · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .changeset/6302-aggregate-filter-lowering.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
---
'@object-ui/data-objectstack': patch
---

`ObjectStackAdapter.aggregate()` lowers rule-shaped filter arrays before the
analytics wire, reusing the lowering `find()` already runs (objectui#6302).

`find()` has translated `[{ field, operator, value }, ...]` into the server's
filter AST for as long as `convertQueryParams` has existed. The analytics path
did not: `aggregate()` assigned `payload.where = params.filter` verbatim and
posted it to `/analytics/query`.

The two doors are not equally forgiving, so the gap had a user-visible end.
`lowerAnalyticsWhere` in `@objectstack/service-analytics` — shared by both
aggregation strategies — accepts AST tuples and throws on an array of rule
objects. A stored `ViewFilterRule[]` that a LIST renders correctly therefore
rendered `element:number` into its error state on every analytics-capable
deployment, which is the default one because the CLI always loads analytics.

An array filter now goes through the same `translateFilterArray` the `find()`
path uses — one lowering, so the two paths cannot disagree about one stored
filter. Rules spread into a logical node (`['and', ...rules, ...tuples]`, the
commonest composite there is) are lowered at depth, as they already were on
`find()`. Non-array filters are untouched: the MongoDB-style object this branch
was written for is what `/analytics/query` already accepts, and translating it
would be a semantic change this fix does not make. Already-AST arrays,
record-shaped filters, and the no-filter case are byte-unchanged.
28 changes: 28 additions & 0 deletions packages/data-objectstack/README.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,6 +147,34 @@ const ast = [
];
```

#### Rule-shaped arrays, on `find()` **and** `aggregate()`

Server-driven view configs store their conditions as an array of rules
(`ViewFilterRule[]`), not as a MongoDB-style object:

```typescript
const filter = [{ field: 'stage', operator: 'equals', value: 'won' }];
```

Both read paths lower that array to the same AST before it reaches the wire —
`find()` via `$filter`, and `aggregate()` via the analytics `where`. They share
one translator, so a stored filter cannot mean one thing on a list and another
on a KPI:

```typescript
// find(): filter=["stage","=","won"]
// aggregate(): { ..., where: ["stage", "=", "won"] }
```

Operator aliases (`equals`, `greater_than_or_equal`, `not_in`, `before`, ...)
map to the canonical AST symbols, and rules spread into a logical node
(`['and', ...rules, ...tuples]`) are lowered at depth. A rule that cannot be
translated raises `MalformedFilterError` rather than being dropped — dropping
one condition of an `and` would widen the result set and report success.

Non-array filters are passed through unchanged on the aggregate path: a
MongoDB-style object is already what `/analytics/query` accepts.

### Sorting

```typescript
Expand Down
261 changes: 261 additions & 0 deletions packages/data-objectstack/src/aggregate-filter-lowering.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* `aggregate()` lowers rule-shaped filter arrays before the analytics wire.
*
* WHY THIS FILE EXISTS (objectui#6302). `find()` has translated
* `[{ field, operator, value }, ...]` into the server's filter AST since the
* day `convertQueryParams` learned to — see `filter-entry-translation.test.ts`,
* which runs every shape down both `find()` routes. The analytics path did not:
* `aggregate()` assigned `payload.where = params.filter` verbatim and posted it
* to `/analytics/query`.
*
* The two doors are not equally forgiving, which is why the gap had a
* user-visible end. `lowerAnalyticsWhere` in `@objectstack/service-analytics`
* — shared by BOTH aggregation strategies, so there is no deployment where the
* lenient reading applies — accepts AST tuples and THROWS on an array of rule
* objects ("[analytics] received a 'where' array that is not a filter"). The
* spec's own `isFilterAST` gate says the same thing about the same value, and
* the tests below assert on it directly so the refusal is pinned by the
* contract rather than by a message string:
*
* isFilterAST([{ field: 'stage', operator: 'equals', value: 'won' }]) // false
* isFilterAST(['stage', '=', 'won']) // true
*
* Net effect before the fix: a stored `ViewFilterRule[]` that a LIST renders
* correctly rendered `element:number` into its error state on every
* analytics-capable deployment — and analytics is the default one, because the
* CLI always loads it.
*
* The fix reuses `translateFilterArray` rather than adding a second lowering.
* That is load-bearing and is asserted as such below: the cross-path parity
* block requires `aggregate()`'s `where` and `find()`'s `filter=` to be the
* SAME value for the same input, so the two paths cannot drift the way the two
* `find()` routes once did. Non-array filters are deliberately untouched — the
* MongoDB-style object this branch was written for is already what the
* analytics endpoint accepts, and translating it would be a semantic change
* this fix does not make.
*/

import { describe, it, expect, beforeEach, vi } from 'vitest';
import { isFilterAST } from '@objectstack/spec/data';
import { ObjectStackAdapter, clearSharedDiscoveryCache, isMalformedFilterError } from './index';

/** Rows that carry the requested measure, so nothing degrades to the fallback. */
const ANALYTICS_ROWS = { rows: [{ amount_sum: 150 }] };

function makeAdapter() {
/** Every parsed `/analytics/query` request body, in order. */
const analyticsBodies: any[] = [];
const urls: string[] = [];
const fetchImpl = vi.fn(async (url: any, init?: any) => {
const u = String(url);
urls.push(u);
if (u.includes('/api/v1/discovery')) {
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { version: 'v1', routes: {} } }),
} as any;
}
if (u.includes('/api/v1/analytics/query')) {
analyticsBodies.push(init?.body ? JSON.parse(String(init.body)) : undefined);
return { ok: true, status: 200, statusText: 'OK', json: async () => ANALYTICS_ROWS } as any;
}
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { object: 'opportunity', records: [], total: 0 } }),
} as any;
});
const adapter = new ObjectStackAdapter({
baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any,
});
return { adapter, analyticsBodies, urls };
}

const SUM_BY_STAGE = { function: 'sum', field: 'amount', groupBy: '_all' };

/**
* The `where` this filter put on the analytics wire.
*
* `HAS_NO_WHERE` distinguishes "the key was absent" from "the key was present
* and undefined" — the empty-filter cases below turn on exactly that.
*/
const HAS_NO_WHERE = Symbol('no where key');

async function whereOnWire(filter: unknown): Promise<unknown> {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', { ...SUM_BY_STAGE, filter });
expect(analyticsBodies).toHaveLength(1);
const body = analyticsBodies[0];
return 'where' in body ? body.where : HAS_NO_WHERE;
}

/** The `filter=` the SAME value produces on the plain `find()` route. */
async function filterOnFindWire(filter: unknown): Promise<unknown> {
const { adapter, urls } = makeAdapter();
await adapter.find('opportunity', { $filter: filter } as any);
const dataCall = urls.filter((u) => u.includes('/data/opportunity')).pop();
const raw = dataCall ? new URL(dataCall).searchParams.get('filter') : null;
return raw === null ? HAS_NO_WHERE : JSON.parse(raw);
}

describe('aggregate() lowers a rule-shaped filter array before `client.analytics.query`', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('translates a single rule into an AST tuple', async () => {
const where = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
expect(where).toEqual(['stage', '=', 'won']);
});

it('the lowered value passes the AST gate the raw one fails', async () => {
const rules = [{ field: 'stage', operator: 'equals', value: 'won' }];
// Negative control: this is what used to reach the wire, and it is exactly
// the value `lowerAnalyticsWhere` refuses. Without this line the test above
// could pass against a lowering that produced some OTHER non-AST shape.
expect(isFilterAST(rules)).toBe(false);
expect(isFilterAST(await whereOnWire(rules) as any)).toBe(true);
});

it('maps operator aliases the way the find() path does', async () => {
expect(await whereOnWire([{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]))
.toEqual(['amount', '>=', 3]);
});

it('joins several rules with `and`', async () => {
expect(await whereOnWire([
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
])).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('lowers rules SPREAD into a logical node, not just top-level ones', async () => {
// The commonest composite there is: a view's stored filter plus one the
// user added in the panel. The head is the string `and`, so a top-level-only
// check would call the whole thing "already AST" and ship the rule raw.
const composite = ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]];
expect(isFilterAST(composite as any)).toBe(false);
const where = await whereOnWire(composite);
expect(where).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
expect(isFilterAST(where as any)).toBe(true);
});

it('produces the SAME `where` as the AST-tuple equivalent (the acceptance criterion)', async () => {
const fromRules = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
const fromTuple = await whereOnWire(['stage', '=', 'won']);
expect(fromRules).toEqual(fromTuple);
});
});

describe('aggregate() leaves every already-correct filter shape byte-unchanged', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('an AST tuple passes through untouched', async () => {
expect(await whereOnWire(['stage', '=', 'won'])).toEqual(['stage', '=', 'won']);
});

it('a logical AST node passes through untouched', async () => {
expect(await whereOnWire(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]))
.toEqual(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]);
});

it('a legacy nested array of nodes passes through untouched', async () => {
expect(await whereOnWire([['stage', '=', 'won'], ['amount', '>', 100]]))
.toEqual([['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('a record-shaped (MongoDB-style) filter is NOT translated', async () => {
// The shape this branch was written for. `/analytics/query` accepts it, so
// lowering it here would be a semantic change, not a fix.
expect(await whereOnWire({ stage: 'won' })).toEqual({ stage: 'won' });
});

it('a record-shaped filter with an operator object is NOT translated either', async () => {
expect(await whereOnWire({ amount: { $gt: 100 } })).toEqual({ amount: { $gt: 100 } });
});

it('an empty array still reaches the wire as an empty array', async () => {
// Unchanged on purpose: `if (params.filter)` is truthy for `[]`, and this
// fix moves no boundary it did not have to move.
expect(await whereOnWire([])).toEqual([]);
});

it('no filter means no `where` key at all', async () => {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', SUM_BY_STAGE);
expect(analyticsBodies[0]).not.toHaveProperty('where');
});
});

describe('the find() path is unchanged, and the two paths share one lowering', () => {
beforeEach(() => clearSharedDiscoveryCache());

// Each row is one input. Both sides are measured on the wire, so a change to
// either path that the other does not make turns this red.
const SHARED_CASES: Array<[string, unknown]> = [
['a single rule', [{ field: 'stage', operator: 'equals', value: 'won' }]],
['an aliased operator', [{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]],
['several rules', [
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
]],
['rules spread into a logical node', ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]]],
['an AST tuple', ['stage', '=', 'won']],
['a logical AST node', ['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]],
];

for (const [name, filter] of SHARED_CASES) {
it(`aggregate() and find() agree on ${name}`, async () => {
const viaFind = await filterOnFindWire(filter);
expect(viaFind).not.toBe(HAS_NO_WHERE);
expect(await whereOnWire(filter)).toEqual(viaFind);
});
}

it('find() still lowers a single rule exactly as it did before', async () => {
// The `find()` half of the card's acceptance, stated independently of
// `aggregate()` so a regression there cannot hide behind the parity rows.
expect(await filterOnFindWire([{ field: 'stage', operator: 'equals', value: 'won' }]))
.toEqual(['stage', '=', 'won']);
});

it('find() still sends no filter for an empty array', async () => {
// The one place the two paths legitimately differ: `convertQueryParams`
// drops an empty filter, the analytics payload keeps `[]`. Recorded, not
// reconciled — reconciling it is a behaviour change this card does not make.
expect(await filterOnFindWire([])).toBe(HAS_NO_WHERE);
expect(await whereOnWire([])).toEqual([]);
});
});

describe('a rule the adapter cannot translate refuses on the aggregate path too', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('throws the same malformed-filter refusal `find()` raises, without inventing numbers', async () => {
// Dropping the untranslatable entry would WIDEN the result set and report
// success — the silent over-fetch `MalformedFilterError` exists to stop.
// Sharing the lowering means the analytics path inherits that refusal.
const { adapter, analyticsBodies, urls } = makeAdapter();
const err = await adapter
.aggregate('opportunity', {
...SUM_BY_STAGE,
filter: [
{ field: 'stage', operator: 'eq', value: 'won' },
{ operator: 'eq', value: 'no field here' },
],
})
.then(() => null, (e) => e);

expect(err).toBeInstanceOf(Error);
expect(isMalformedFilterError(err)).toBe(true);
// Nothing was posted to analytics, and no plausible-looking number came
// back from the fallback instead.
expect(analyticsBodies).toHaveLength(0);
expect(urls.some((u) => u.includes('/data/opportunity'))).toBe(false);
});
});
25 changes: 24 additions & 1 deletion packages/data-objectstack/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -4598,7 +4598,30 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
// spec/ui/dashboard.zod.ts). Send via the canonical `where`
// field of the analytics endpoint, matching the unified Query
// DSL (spec/data/query.zod.ts).
payload.where = params.filter;
//
// An ARRAY filter goes through the same `translateFilterArray` the
// `find()` path runs in `convertQueryParams`, because an authored
// `ViewFilterRule[]` reaches this method exactly as it reaches that
// one. It used to ship RAW from here, and the analytics door is
// stricter than the data door: `lowerAnalyticsWhere`
// (`@objectstack/service-analytics`, shared by both aggregation
// strategies) THROWS "[analytics] received a 'where' array that is
// not a filter" on an array of rule objects, while accepting AST
// tuples. So a stored filter that a list renders correctly rendered
// `element:number` into its error state on every analytics-capable
// deployment — and analytics is the default one, since the CLI always
// loads it (objectui#6302).
//
// One lowering, not two: the same function, so the analytics path and
// the `find()` path cannot disagree about one stored filter — which is
// the whole reason `translateFilterArray` was made a single definition
// (see its header). Non-array filters keep passing through untouched:
// the MongoDB-style object this branch was written for is what
// `/analytics/query` already accepts, and translating it here would be
// a semantic change this fix is expressly not making.
payload.where = Array.isArray(params.filter)
? translateFilterArray(params.filter)
: params.filter;
}

const data = await this.client.analytics.query(payload);
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix(data-objectstack): lower rule-shaped filter arrays on aggregate()'s analytics path by claude[bot] · Pull Request #6828 · objectstack-ai/objectui · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .changeset/6302-aggregate-filter-lowering.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
---
'@object-ui/data-objectstack': patch
---

`ObjectStackAdapter.aggregate()` lowers rule-shaped filter arrays before the
analytics wire, reusing the lowering `find()` already runs (objectui#6302).

`find()` has translated `[{ field, operator, value }, ...]` into the server's
filter AST for as long as `convertQueryParams` has existed. The analytics path
did not: `aggregate()` assigned `payload.where = params.filter` verbatim and
posted it to `/analytics/query`.

The two doors are not equally forgiving, so the gap had a user-visible end.
`lowerAnalyticsWhere` in `@objectstack/service-analytics` — shared by both
aggregation strategies — accepts AST tuples and throws on an array of rule
objects. A stored `ViewFilterRule[]` that a LIST renders correctly therefore
rendered `element:number` into its error state on every analytics-capable
deployment, which is the default one because the CLI always loads analytics.

An array filter now goes through the same `translateFilterArray` the `find()`
path uses — one lowering, so the two paths cannot disagree about one stored
filter. Rules spread into a logical node (`['and', ...rules, ...tuples]`, the
commonest composite there is) are lowered at depth, as they already were on
`find()`. Non-array filters are untouched: the MongoDB-style object this branch
was written for is what `/analytics/query` already accepts, and translating it
would be a semantic change this fix does not make. Already-AST arrays,
record-shaped filters, and the no-filter case are byte-unchanged.
28 changes: 28 additions & 0 deletions packages/data-objectstack/README.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,6 +147,34 @@ const ast = [
];
```

#### Rule-shaped arrays, on `find()` **and** `aggregate()`

Server-driven view configs store their conditions as an array of rules
(`ViewFilterRule[]`), not as a MongoDB-style object:

```typescript
const filter = [{ field: 'stage', operator: 'equals', value: 'won' }];
```

Both read paths lower that array to the same AST before it reaches the wire —
`find()` via `$filter`, and `aggregate()` via the analytics `where`. They share
one translator, so a stored filter cannot mean one thing on a list and another
on a KPI:

```typescript
// find(): filter=["stage","=","won"]
// aggregate(): { ..., where: ["stage", "=", "won"] }
```

Operator aliases (`equals`, `greater_than_or_equal`, `not_in`, `before`, ...)
map to the canonical AST symbols, and rules spread into a logical node
(`['and', ...rules, ...tuples]`) are lowered at depth. A rule that cannot be
translated raises `MalformedFilterError` rather than being dropped — dropping
one condition of an `and` would widen the result set and report success.

Non-array filters are passed through unchanged on the aggregate path: a
MongoDB-style object is already what `/analytics/query` accepts.

### Sorting

```typescript
Expand Down
261 changes: 261 additions & 0 deletions packages/data-objectstack/src/aggregate-filter-lowering.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* `aggregate()` lowers rule-shaped filter arrays before the analytics wire.
*
* WHY THIS FILE EXISTS (objectui#6302). `find()` has translated
* `[{ field, operator, value }, ...]` into the server's filter AST since the
* day `convertQueryParams` learned to — see `filter-entry-translation.test.ts`,
* which runs every shape down both `find()` routes. The analytics path did not:
* `aggregate()` assigned `payload.where = params.filter` verbatim and posted it
* to `/analytics/query`.
*
* The two doors are not equally forgiving, which is why the gap had a
* user-visible end. `lowerAnalyticsWhere` in `@objectstack/service-analytics`
* — shared by BOTH aggregation strategies, so there is no deployment where the
* lenient reading applies — accepts AST tuples and THROWS on an array of rule
* objects ("[analytics] received a 'where' array that is not a filter"). The
* spec's own `isFilterAST` gate says the same thing about the same value, and
* the tests below assert on it directly so the refusal is pinned by the
* contract rather than by a message string:
*
* isFilterAST([{ field: 'stage', operator: 'equals', value: 'won' }]) // false
* isFilterAST(['stage', '=', 'won']) // true
*
* Net effect before the fix: a stored `ViewFilterRule[]` that a LIST renders
* correctly rendered `element:number` into its error state on every
* analytics-capable deployment — and analytics is the default one, because the
* CLI always loads it.
*
* The fix reuses `translateFilterArray` rather than adding a second lowering.
* That is load-bearing and is asserted as such below: the cross-path parity
* block requires `aggregate()`'s `where` and `find()`'s `filter=` to be the
* SAME value for the same input, so the two paths cannot drift the way the two
* `find()` routes once did. Non-array filters are deliberately untouched — the
* MongoDB-style object this branch was written for is already what the
* analytics endpoint accepts, and translating it would be a semantic change
* this fix does not make.
*/

import { describe, it, expect, beforeEach, vi } from 'vitest';
import { isFilterAST } from '@objectstack/spec/data';
import { ObjectStackAdapter, clearSharedDiscoveryCache, isMalformedFilterError } from './index';

/** Rows that carry the requested measure, so nothing degrades to the fallback. */
const ANALYTICS_ROWS = { rows: [{ amount_sum: 150 }] };

function makeAdapter() {
/** Every parsed `/analytics/query` request body, in order. */
const analyticsBodies: any[] = [];
const urls: string[] = [];
const fetchImpl = vi.fn(async (url: any, init?: any) => {
const u = String(url);
urls.push(u);
if (u.includes('/api/v1/discovery')) {
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { version: 'v1', routes: {} } }),
} as any;
}
if (u.includes('/api/v1/analytics/query')) {
analyticsBodies.push(init?.body ? JSON.parse(String(init.body)) : undefined);
return { ok: true, status: 200, statusText: 'OK', json: async () => ANALYTICS_ROWS } as any;
}
return {
ok: true, status: 200, statusText: 'OK',
json: async () => ({ success: true, data: { object: 'opportunity', records: [], total: 0 } }),
} as any;
});
const adapter = new ObjectStackAdapter({
baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any,
});
return { adapter, analyticsBodies, urls };
}

const SUM_BY_STAGE = { function: 'sum', field: 'amount', groupBy: '_all' };

/**
* The `where` this filter put on the analytics wire.
*
* `HAS_NO_WHERE` distinguishes "the key was absent" from "the key was present
* and undefined" — the empty-filter cases below turn on exactly that.
*/
const HAS_NO_WHERE = Symbol('no where key');

async function whereOnWire(filter: unknown): Promise<unknown> {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', { ...SUM_BY_STAGE, filter });
expect(analyticsBodies).toHaveLength(1);
const body = analyticsBodies[0];
return 'where' in body ? body.where : HAS_NO_WHERE;
}

/** The `filter=` the SAME value produces on the plain `find()` route. */
async function filterOnFindWire(filter: unknown): Promise<unknown> {
const { adapter, urls } = makeAdapter();
await adapter.find('opportunity', { $filter: filter } as any);
const dataCall = urls.filter((u) => u.includes('/data/opportunity')).pop();
const raw = dataCall ? new URL(dataCall).searchParams.get('filter') : null;
return raw === null ? HAS_NO_WHERE : JSON.parse(raw);
}

describe('aggregate() lowers a rule-shaped filter array before `client.analytics.query`', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('translates a single rule into an AST tuple', async () => {
const where = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
expect(where).toEqual(['stage', '=', 'won']);
});

it('the lowered value passes the AST gate the raw one fails', async () => {
const rules = [{ field: 'stage', operator: 'equals', value: 'won' }];
// Negative control: this is what used to reach the wire, and it is exactly
// the value `lowerAnalyticsWhere` refuses. Without this line the test above
// could pass against a lowering that produced some OTHER non-AST shape.
expect(isFilterAST(rules)).toBe(false);
expect(isFilterAST(await whereOnWire(rules) as any)).toBe(true);
});

it('maps operator aliases the way the find() path does', async () => {
expect(await whereOnWire([{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]))
.toEqual(['amount', '>=', 3]);
});

it('joins several rules with `and`', async () => {
expect(await whereOnWire([
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
])).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('lowers rules SPREAD into a logical node, not just top-level ones', async () => {
// The commonest composite there is: a view's stored filter plus one the
// user added in the panel. The head is the string `and`, so a top-level-only
// check would call the whole thing "already AST" and ship the rule raw.
const composite = ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]];
expect(isFilterAST(composite as any)).toBe(false);
const where = await whereOnWire(composite);
expect(where).toEqual(['and', ['stage', '=', 'won'], ['amount', '>', 100]]);
expect(isFilterAST(where as any)).toBe(true);
});

it('produces the SAME `where` as the AST-tuple equivalent (the acceptance criterion)', async () => {
const fromRules = await whereOnWire([{ field: 'stage', operator: 'equals', value: 'won' }]);
const fromTuple = await whereOnWire(['stage', '=', 'won']);
expect(fromRules).toEqual(fromTuple);
});
});

describe('aggregate() leaves every already-correct filter shape byte-unchanged', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('an AST tuple passes through untouched', async () => {
expect(await whereOnWire(['stage', '=', 'won'])).toEqual(['stage', '=', 'won']);
});

it('a logical AST node passes through untouched', async () => {
expect(await whereOnWire(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]))
.toEqual(['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]);
});

it('a legacy nested array of nodes passes through untouched', async () => {
expect(await whereOnWire([['stage', '=', 'won'], ['amount', '>', 100]]))
.toEqual([['stage', '=', 'won'], ['amount', '>', 100]]);
});

it('a record-shaped (MongoDB-style) filter is NOT translated', async () => {
// The shape this branch was written for. `/analytics/query` accepts it, so
// lowering it here would be a semantic change, not a fix.
expect(await whereOnWire({ stage: 'won' })).toEqual({ stage: 'won' });
});

it('a record-shaped filter with an operator object is NOT translated either', async () => {
expect(await whereOnWire({ amount: { $gt: 100 } })).toEqual({ amount: { $gt: 100 } });
});

it('an empty array still reaches the wire as an empty array', async () => {
// Unchanged on purpose: `if (params.filter)` is truthy for `[]`, and this
// fix moves no boundary it did not have to move.
expect(await whereOnWire([])).toEqual([]);
});

it('no filter means no `where` key at all', async () => {
const { adapter, analyticsBodies } = makeAdapter();
await adapter.aggregate('opportunity', SUM_BY_STAGE);
expect(analyticsBodies[0]).not.toHaveProperty('where');
});
});

describe('the find() path is unchanged, and the two paths share one lowering', () => {
beforeEach(() => clearSharedDiscoveryCache());

// Each row is one input. Both sides are measured on the wire, so a change to
// either path that the other does not make turns this red.
const SHARED_CASES: Array<[string, unknown]> = [
['a single rule', [{ field: 'stage', operator: 'equals', value: 'won' }]],
['an aliased operator', [{ field: 'amount', operator: 'greater_than_or_equal', value: 3 }]],
['several rules', [
{ field: 'stage', operator: 'eq', value: 'won' },
{ field: 'amount', operator: 'gt', value: 100 },
]],
['rules spread into a logical node', ['and', { field: 'stage', operator: 'eq', value: 'won' }, ['amount', '>', 100]]],
['an AST tuple', ['stage', '=', 'won']],
['a logical AST node', ['or', ['stage', '=', 'won'], ['stage', '=', 'lost']]],
];

for (const [name, filter] of SHARED_CASES) {
it(`aggregate() and find() agree on ${name}`, async () => {
const viaFind = await filterOnFindWire(filter);
expect(viaFind).not.toBe(HAS_NO_WHERE);
expect(await whereOnWire(filter)).toEqual(viaFind);
});
}

it('find() still lowers a single rule exactly as it did before', async () => {
// The `find()` half of the card's acceptance, stated independently of
// `aggregate()` so a regression there cannot hide behind the parity rows.
expect(await filterOnFindWire([{ field: 'stage', operator: 'equals', value: 'won' }]))
.toEqual(['stage', '=', 'won']);
});

it('find() still sends no filter for an empty array', async () => {
// The one place the two paths legitimately differ: `convertQueryParams`
// drops an empty filter, the analytics payload keeps `[]`. Recorded, not
// reconciled — reconciling it is a behaviour change this card does not make.
expect(await filterOnFindWire([])).toBe(HAS_NO_WHERE);
expect(await whereOnWire([])).toEqual([]);
});
});

describe('a rule the adapter cannot translate refuses on the aggregate path too', () => {
beforeEach(() => clearSharedDiscoveryCache());

it('throws the same malformed-filter refusal `find()` raises, without inventing numbers', async () => {
// Dropping the untranslatable entry would WIDEN the result set and report
// success — the silent over-fetch `MalformedFilterError` exists to stop.
// Sharing the lowering means the analytics path inherits that refusal.
const { adapter, analyticsBodies, urls } = makeAdapter();
const err = await adapter
.aggregate('opportunity', {
...SUM_BY_STAGE,
filter: [
{ field: 'stage', operator: 'eq', value: 'won' },
{ operator: 'eq', value: 'no field here' },
],
})
.then(() => null, (e) => e);

expect(err).toBeInstanceOf(Error);
expect(isMalformedFilterError(err)).toBe(true);
// Nothing was posted to analytics, and no plausible-looking number came
// back from the fallback instead.
expect(analyticsBodies).toHaveLength(0);
expect(urls.some((u) => u.includes('/data/opportunity'))).toBe(false);
});
});
25 changes: 24 additions & 1 deletion packages/data-objectstack/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -4598,7 +4598,30 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
// spec/ui/dashboard.zod.ts). Send via the canonical `where`
// field of the analytics endpoint, matching the unified Query
// DSL (spec/data/query.zod.ts).
payload.where = params.filter;
//
// An ARRAY filter goes through the same `translateFilterArray` the
// `find()` path runs in `convertQueryParams`, because an authored
// `ViewFilterRule[]` reaches this method exactly as it reaches that
// one. It used to ship RAW from here, and the analytics door is
// stricter than the data door: `lowerAnalyticsWhere`
// (`@objectstack/service-analytics`, shared by both aggregation
// strategies) THROWS "[analytics] received a 'where' array that is
// not a filter" on an array of rule objects, while accepting AST
// tuples. So a stored filter that a list renders correctly rendered
// `element:number` into its error state on every analytics-capable
// deployment — and analytics is the default one, since the CLI always
// loads it (objectui#6302).
//
// One lowering, not two: the same function, so the analytics path and
// the `find()` path cannot disagree about one stored filter — which is
// the whole reason `translateFilterArray` was made a single definition
// (see its header). Non-array filters keep passing through untouched:
// the MongoDB-style object this branch was written for is what
// `/analytics/query` already accepts, and translating it here would be
// a semantic change this fix is expressly not making.
payload.where = Array.isArray(params.filter)
? translateFilterArray(params.filter)
: params.filter;
}

const data = await this.client.analytics.query(payload);
Expand Down
Loading