Skip to content

Commit ee6d064

Browse files
Copilothotlong
andcommitted
fix: unify filter protocol - isFilterAST, naming, param safety, response fields
- Add isFilterAST() with structural validation to spec (replaces naive Array.isArray) - Add VALID_AST_OPERATORS constant for AST operator validation - Add HttpFindQueryParamsSchema standardizing HTTP query param names (canonical `filter` singular) - Add select/expand fields to GetDataRequestSchema to prevent parameter pollution - Fix handleApiEndpoint response field mismatch (result.records/total instead of result.data/count) - Fix GET by ID parameter whitelisting (only select/expand allowed) - Update client SDK to use spec isFilterAST and canonical `filter` param name - Update protocol.ts to normalize filters→filter and use isFilterAST - Add 17 new tests covering isFilterAST, VALID_AST_OPERATORS, GetData select/expand, HttpFindQueryParams Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
1 parent 1b60b9c commit ee6d064

7 files changed

Lines changed: 316 additions & 22 deletions

File tree

packages/client/src/index.ts

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

3-
import { QueryAST, SortNode, AggregationNode } from '@objectstack/spec/data';
3+
import { QueryAST, SortNode, AggregationNode, isFilterAST } from '@objectstack/spec/data';
44
import {
55
BatchUpdateRequest,
66
BatchUpdateResponse,
@@ -115,7 +115,10 @@ export type DiscoveryResult = GetDiscoveryResponse;
115115

116116
export interface QueryOptions {
117117
select?: string[]; // Simplified Selection
118-
filters?: Record<string, any>; // Map or AST
118+
/** @canonical Preferred filter parameter (singular). */
119+
filter?: Record<string, any> | unknown[]; // Map or AST
120+
/** @deprecated Use `filter` (singular). Kept for backward compatibility. */
121+
filters?: Record<string, any> | unknown[]; // Map or AST
119122
sort?: string | string[] | SortNode[]; // 'name' or ['-created_at'] or AST
120123
top?: number;
121124
skip?: number;
@@ -1445,14 +1448,17 @@ export class ObjectStackClient {
14451448
}
14461449

14471450
// 4. Handle Filters (Simple vs AST)
1448-
if (options.filters) {
1451+
// Canonical HTTP param name: `filter` (singular). `filters` (plural) is accepted
1452+
// for backward compatibility but `filter` is the standard going forward.
1453+
const filterValue = options.filter ?? options.filters;
1454+
if (filterValue) {
14491455
// Detect AST filter format vs simple key-value map. AST filters use an array structure
1450-
// with [field, operator, value] or [logicOp, ...nodes] shape (see isFilterAST).
1456+
// with [field, operator, value] or [logicOp, ...nodes] shape (see isFilterAST from spec).
14511457
// For complex filter expressions, use .query() which builds a proper QueryAST.
1452-
if (this.isFilterAST(options.filters)) {
1453-
queryParams.set('filters', JSON.stringify(options.filters));
1458+
if (this.isFilterAST(filterValue)) {
1459+
queryParams.set('filter', JSON.stringify(filterValue));
14541460
} else {
1455-
Object.entries(options.filters).forEach(([k, v]) => {
1461+
Object.entries(filterValue).forEach(([k, v]) => {
14561462
if (v !== undefined && v !== null) {
14571463
queryParams.append(k, String(v));
14581464
}
@@ -1571,10 +1577,9 @@ export class ObjectStackClient {
15711577
*/
15721578

15731579
private isFilterAST(filter: any): boolean {
1574-
// Basic check: if array, it's [field, op, val] or [logic, node, node]
1575-
// If object but not basic KV map... harder to tell without schema
1576-
// For now, assume if it passes Array.isArray it's an AST root
1577-
return Array.isArray(filter);
1580+
// Delegate to the spec-exported structural validator instead of naive Array.isArray.
1581+
// This checks for valid AST shapes: [field, op, val], [logic, ...nodes], or [[cond], ...].
1582+
return isFilterAST(filter);
15781583
}
15791584

15801585
/**

packages/objectql/src/protocol.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import type {
1010
} from '@objectstack/spec/api';
1111
import type { MetadataCacheRequest, MetadataCacheResponse, ServiceInfo, ApiRoutes, WellKnownCapabilities } from '@objectstack/spec/api';
1212
import type { IFeedService } from '@objectstack/spec/contracts';
13-
import { parseFilterAST } from '@objectstack/spec/data';
13+
import { parseFilterAST, isFilterAST } from '@objectstack/spec/data';
1414

1515
// We import SchemaRegistry directly since this class lives in the same package
1616
import { SchemaRegistry } from './registry.js';
@@ -304,17 +304,21 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
304304
delete options.orderBy;
305305
}
306306

307+
// Filter: normalize `filter`/`filters` (plural) → `filter` (singular, canonical)
308+
// Accept both `filter` and `filters` for backward compatibility.
309+
if (options.filters !== undefined && options.filter === undefined) {
310+
options.filter = options.filters;
311+
}
312+
delete options.filters;
313+
307314
// Filter: JSON string → object
308315
if (typeof options.filter === 'string') {
309316
try { options.filter = JSON.parse(options.filter); } catch { /* keep as-is */ }
310317
}
311-
if (typeof options.filters === 'string') {
312-
try { options.filter = JSON.parse(options.filters); delete options.filters; } catch { /* keep as-is */ }
313-
}
314318

315319
// Filter AST array → FilterCondition object
316320
// Converts ["and", ["field", "=", "val"], ...] to { $and: [{ field: "val" }, ...] }
317-
if (Array.isArray(options.filter)) {
321+
if (isFilterAST(options.filter)) {
318322
options.filter = parseFilterAST(options.filter);
319323
}
320324

packages/runtime/src/http-dispatcher.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -377,8 +377,14 @@ export class HttpDispatcher {
377377
// GET /data/:object/:id
378378
if (parts.length === 2 && m === 'GET') {
379379
const id = parts[1];
380+
// Spec: Only select/expand are allowlisted query params for GET by ID.
381+
// All other query parameters are discarded to prevent parameter pollution.
382+
const { select, expand } = query || {};
383+
const allowedParams: Record<string, any> = {};
384+
if (select != null) allowedParams.select = select;
385+
if (expand != null) allowedParams.expand = expand;
380386
// Spec: broker returns GetDataResponse = { object, id, record }
381-
const result = await broker.call('data.get', { object: objectName, id, ...query }, { request: context.request });
387+
const result = await broker.call('data.get', { object: objectName, id, ...allowedParams }, { request: context.request });
382388
return { handled: true, response: this.success(result) };
383389
}
384390

@@ -942,7 +948,8 @@ export class HttpDispatcher {
942948
// Map standard CRUD operations
943949
if (operation === 'find') {
944950
const result = await broker.call('data.query', { object, query }, { request: context.request });
945-
return { handled: true, response: this.success(result.data, { count: result.count }) };
951+
// Spec: FindDataResponse = { object, records, total?, hasMore? }
952+
return { handled: true, response: this.success(result.records, { total: result.total }) };
946953
}
947954
if (operation === 'get' && query.id) {
948955
const result = await broker.call('data.get', { object, id: query.id }, { request: context.request });

packages/spec/src/api/protocol.test.ts

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
GetDataResponseSchema,
66
FindDataRequestSchema,
77
FindDataResponseSchema,
8+
HttpFindQueryParamsSchema,
89
CreateDataRequestSchema,
910
CreateDataResponseSchema,
1011
UpdateDataRequestSchema,
@@ -367,3 +368,102 @@ describe('GetDiscoveryResponseSchema (capabilities)', () => {
367368
expect(result.success).toBe(false);
368369
});
369370
});
371+
372+
// ==========================================
373+
// GetDataRequestSchema — select/expand params
374+
// ==========================================
375+
376+
describe('GetDataRequestSchema (select/expand)', () => {
377+
it('should accept request with select and expand', () => {
378+
const result = GetDataRequestSchema.safeParse({
379+
object: 'contact',
380+
id: 'c1',
381+
select: ['name', 'email'],
382+
expand: ['account', 'owner'],
383+
});
384+
expect(result.success).toBe(true);
385+
if (result.success) {
386+
expect(result.data.select).toEqual(['name', 'email']);
387+
expect(result.data.expand).toEqual(['account', 'owner']);
388+
}
389+
});
390+
391+
it('should accept request without select/expand (optional)', () => {
392+
const result = GetDataRequestSchema.safeParse({
393+
object: 'contact',
394+
id: 'c1',
395+
});
396+
expect(result.success).toBe(true);
397+
if (result.success) {
398+
expect(result.data.select).toBeUndefined();
399+
expect(result.data.expand).toBeUndefined();
400+
}
401+
});
402+
403+
it('should strip unknown query parameters via strict parsing', () => {
404+
const result = GetDataRequestSchema.strict().safeParse({
405+
object: 'contact',
406+
id: 'c1',
407+
select: ['name'],
408+
// Unknown params that should be rejected by strict mode
409+
unknownParam: 'should-be-stripped',
410+
});
411+
expect(result.success).toBe(false);
412+
});
413+
});
414+
415+
// ==========================================
416+
// HttpFindQueryParamsSchema — HTTP parameter naming
417+
// ==========================================
418+
419+
describe('HttpFindQueryParamsSchema', () => {
420+
it('should accept canonical filter (singular) parameter', () => {
421+
const result = HttpFindQueryParamsSchema.safeParse({
422+
filter: '{"status":"active"}',
423+
select: 'name,email',
424+
top: 10,
425+
});
426+
expect(result.success).toBe(true);
427+
if (result.success) {
428+
expect(result.data.filter).toBe('{"status":"active"}');
429+
expect(result.data.top).toBe(10);
430+
}
431+
});
432+
433+
it('should accept deprecated filters (plural) parameter for backward compat', () => {
434+
const result = HttpFindQueryParamsSchema.safeParse({
435+
filters: '{"status":"active"}',
436+
});
437+
expect(result.success).toBe(true);
438+
if (result.success) {
439+
expect(result.data.filters).toBe('{"status":"active"}');
440+
}
441+
});
442+
443+
it('should coerce string numbers for top/skip', () => {
444+
const result = HttpFindQueryParamsSchema.safeParse({
445+
top: '25',
446+
skip: '50',
447+
});
448+
expect(result.success).toBe(true);
449+
if (result.success) {
450+
expect(result.data.top).toBe(25);
451+
expect(result.data.skip).toBe(50);
452+
}
453+
});
454+
455+
it('should accept sort, orderBy, expand, search, distinct, count', () => {
456+
const result = HttpFindQueryParamsSchema.safeParse({
457+
sort: 'name asc,created_at desc',
458+
expand: 'owner,account',
459+
search: 'John',
460+
distinct: true,
461+
count: true,
462+
});
463+
expect(result.success).toBe(true);
464+
});
465+
466+
it('should accept empty object (all params optional)', () => {
467+
expect(HttpFindQueryParamsSchema.safeParse({}).success).toBe(true);
468+
});
469+
});

packages/spec/src/api/protocol.zod.ts

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -240,9 +240,9 @@ export const GetUiViewResponseSchema = ViewSchema;
240240
* {
241241
* "object": "customers",
242242
* "query": {
243-
* "filters": [["status", "=", "active"], ["revenue", ">", 10000]],
244-
* "sort": "name desc",
245-
* "top": 10
243+
* "where": { "status": "active", "revenue": { "$gt": 10000 } },
244+
* "orderBy": [{ "field": "name", "order": "desc" }],
245+
* "limit": 10
246246
* }
247247
* }
248248
*/
@@ -263,19 +263,55 @@ export const FindDataResponseSchema = z.object({
263263
hasMore: z.boolean().optional().describe('True if there are more records available (pagination).'),
264264
});
265265

266+
/**
267+
* HTTP Find Query Parameters
268+
*
269+
* Canonical HTTP query parameter names for GET /data/:object list endpoints.
270+
* The canonical filter parameter is `filter` (singular). The plural `filters` is
271+
* accepted for backward compatibility but `filter` is the standard going forward.
272+
*
273+
* This schema defines the allowlisted query parameters for HTTP GET list requests.
274+
* Server-side parsers (protocol.ts) should accept both `filter` and `filters` and
275+
* normalize to `filter` (singular) internally.
276+
*
277+
* @example
278+
* GET /api/v1/data/contacts?filter={"status":"active"}&select=name,email&top=10&sort=name
279+
*/
280+
export const HttpFindQueryParamsSchema = z.object({
281+
/** @canonical Singular form — the standard going forward. JSON string of filter expression or AST. */
282+
filter: z.string().optional().describe('JSON-encoded filter expression (canonical, singular).'),
283+
/** @deprecated Use `filter` (singular). Accepted for backward compatibility. */
284+
filters: z.string().optional().describe('JSON-encoded filter expression (deprecated plural alias).'),
285+
select: z.string().optional().describe('Comma-separated list of fields to retrieve.'),
286+
sort: z.string().optional().describe('Sort expression (e.g. "name asc,created_at desc" or "-created_at").'),
287+
orderBy: z.string().optional().describe('Alias for sort (OData compatibility).'),
288+
top: z.coerce.number().optional().describe('Max records to return (limit).'),
289+
skip: z.coerce.number().optional().describe('Records to skip (offset).'),
290+
expand: z.string().optional().describe('Comma-separated list of relations to eager-load.'),
291+
search: z.string().optional().describe('Full-text search query.'),
292+
distinct: z.coerce.boolean().optional().describe('SELECT DISTINCT flag.'),
293+
count: z.coerce.boolean().optional().describe('Include total count in response.'),
294+
});
295+
266296
/**
267297
* Get Data Request
268298
* Retrieval of a single record by its unique identifier.
299+
* Only `select` and `expand` are permitted as additional query parameters.
300+
* All other query parameters should be discarded to prevent parameter pollution.
269301
*
270302
* @example
271303
* {
272304
* "object": "contracts",
273-
* "id": "cnt_123456"
305+
* "id": "cnt_123456",
306+
* "select": ["name", "status", "amount"],
307+
* "expand": ["owner", "account"]
274308
* }
275309
*/
276310
export const GetDataRequestSchema = z.object({
277311
object: z.string().describe('The object name.'),
278312
id: z.string().describe('The unique record identifier (primary key).'),
313+
select: z.array(z.string()).optional().describe('Fields to include in the response (allowlisted query param).'),
314+
expand: z.array(z.string()).optional().describe('Relations to eager-load (allowlisted query param).'),
279315
});
280316

281317
/**

packages/spec/src/data/filter.test.ts

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import {
1414
LOGICAL_OPERATORS,
1515
ALL_OPERATORS,
1616
parseFilterAST,
17+
isFilterAST,
18+
VALID_AST_OPERATORS,
1719
type Filter,
1820
type QueryFilter,
1921
type FieldOperators,
@@ -921,3 +923,80 @@ describe('parseFilterAST', () => {
921923
expect(() => FilterConditionSchema.parse(result)).not.toThrow();
922924
});
923925
});
926+
927+
// ============================================================================
928+
// isFilterAST — structural validation
929+
// ============================================================================
930+
931+
describe('isFilterAST', () => {
932+
it('should return false for null/undefined/empty', () => {
933+
expect(isFilterAST(null)).toBe(false);
934+
expect(isFilterAST(undefined)).toBe(false);
935+
expect(isFilterAST([])).toBe(false);
936+
});
937+
938+
it('should return false for non-array types', () => {
939+
expect(isFilterAST('not an array')).toBe(false);
940+
expect(isFilterAST(42)).toBe(false);
941+
expect(isFilterAST(true)).toBe(false);
942+
expect(isFilterAST({ status: 'active' })).toBe(false);
943+
});
944+
945+
it('should detect valid comparison node', () => {
946+
expect(isFilterAST(['status', '=', 'active'])).toBe(true);
947+
expect(isFilterAST(['age', '>', 18])).toBe(true);
948+
expect(isFilterAST(['age', '>=', 18])).toBe(true);
949+
expect(isFilterAST(['role', 'in', ['admin', 'editor']])).toBe(true);
950+
expect(isFilterAST(['name', 'contains', 'John'])).toBe(true);
951+
expect(isFilterAST(['name', 'like', 'John'])).toBe(true);
952+
expect(isFilterAST(['created_at', 'between', ['2024-01-01', '2024-12-31']])).toBe(true);
953+
expect(isFilterAST(['deleted_at', 'is_null', null])).toBe(true);
954+
});
955+
956+
it('should detect valid logical nodes', () => {
957+
expect(isFilterAST(['and', ['status', '=', 'active'], ['priority', '=', 'high']])).toBe(true);
958+
expect(isFilterAST(['or', ['role', '=', 'admin'], ['role', '=', 'editor']])).toBe(true);
959+
expect(isFilterAST(['AND', ['status', '=', 'active']])).toBe(true);
960+
expect(isFilterAST(['OR', ['a', '=', 1], ['b', '=', 2]])).toBe(true);
961+
});
962+
963+
it('should detect legacy flat array format', () => {
964+
expect(isFilterAST([['status', '=', 'active'], ['priority', '=', 'high']])).toBe(true);
965+
});
966+
967+
it('should reject invalid arrays that are not filter ASTs', () => {
968+
// Arbitrary number arrays
969+
expect(isFilterAST([1, 2, 3])).toBe(false);
970+
// Array of strings that aren't a valid comparison
971+
expect(isFilterAST(['hello', 'world'])).toBe(false);
972+
// Array where second element is not a known operator
973+
expect(isFilterAST(['field', 'UNKNOWN_OP', 'value'])).toBe(false);
974+
// Mixed array types
975+
expect(isFilterAST([true, false, null])).toBe(false);
976+
});
977+
978+
it('should reject "and"/"or" with no children', () => {
979+
expect(isFilterAST(['and'])).toBe(false);
980+
expect(isFilterAST(['or'])).toBe(false);
981+
});
982+
983+
it('should reject "and"/"or" with non-array children', () => {
984+
expect(isFilterAST(['and', 'not-an-array'])).toBe(false);
985+
expect(isFilterAST(['or', 123])).toBe(false);
986+
});
987+
});
988+
989+
// ============================================================================
990+
// VALID_AST_OPERATORS constant
991+
// ============================================================================
992+
993+
describe('VALID_AST_OPERATORS', () => {
994+
it('should contain all standard comparison operators', () => {
995+
const expected = ['=', '==', '!=', '<>', '>', '>=', '<', '<=', 'in', 'nin', 'not_in',
996+
'contains', 'like', 'startswith', 'starts_with', 'endswith', 'ends_with',
997+
'between', 'is_null', 'is_not_null'];
998+
for (const op of expected) {
999+
expect(VALID_AST_OPERATORS.has(op)).toBe(true);
1000+
}
1001+
});
1002+
});

0 commit comments

Comments
 (0)