Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…nse 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>
…ety, guard against array entries Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 仔细评估是否已经完成了任务中要求的所有开发 |
…and document GET-by-ID allowed params Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
已逐项核查全部 5 个问题的完成情况: 问题1 — filter/filters/$filter 命名统一 ✅
问题2 — AST 数组格式转换 ✅
问题3 — Dispatcher 响应字段不匹配 ✅
问题4 — GET by ID 参数污染 ✅
问题5 — isFilterAST 检测不严谨 ✅
测试 & 文档:19 个新测试,229 文件 6475 测试全部通过,ROADMAP 已更新。 |
There was a problem hiding this comment.
Pull request overview
This pull request comprehensively addresses five interrelated filter parameter protocol issues across the ObjectStack codebase: inconsistent naming (filter/filters/$filter), naive AST detection, incorrect response field mapping, GET-by-ID parameter pollution, and lack of shared AST validation.
Changes:
- Introduced
isFilterAST()structural validator andVALID_AST_OPERATORSconstant in@objectstack/spec/datato replace naiveArray.isArraychecks with recursive AST validation - Unified filter parameter naming: canonical
filter(singular) for HTTP,filters(plural) accepted for backward compatibility, with normalization in protocol layer - Fixed dispatcher response field mapping from incorrect
result.data/result.countto spec-correctresult.records/result.total - Implemented GET-by-ID parameter allowlisting (only
select/expandpermitted) to prevent query parameter pollution - Added
HttpFindQueryParamsSchemadocumenting canonical HTTP query parameter names with coercion support
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/spec/src/data/filter.zod.ts | Added isFilterAST() recursive validator and VALID_AST_OPERATORS Set for structural AST validation beyond naive array checks |
| packages/spec/src/data/filter.test.ts | Added 19 comprehensive tests covering isFilterAST() validation: comparison nodes, logical nodes, legacy arrays, invalid arrays, recursive validation |
| packages/spec/src/api/protocol.zod.ts | Added HttpFindQueryParamsSchema with canonical filter param (deprecated filters for compat); updated GetDataRequestSchema to explicitly allow only select/expand; corrected FindDataRequest example to use structured QuerySchema format |
| packages/spec/src/api/protocol.test.ts | Added 50+ tests for GetDataRequestSchema select/expand validation and HttpFindQueryParamsSchema coercion/naming |
| packages/runtime/src/http-dispatcher.ts | Fixed GET-by-ID to allowlist only select/expand params; corrected handleApiEndpoint response mapping to use result.records/result.total |
| packages/objectql/src/protocol.ts | Added filters → filter normalization; replaced Array.isArray(options.filter) with isFilterAST() for AST detection |
| packages/client/src/index.ts | Added canonical QueryOptions.filter field (deprecated filters); delegated AST detection to spec's isFilterAST(); sends filter (singular) HTTP param |
| content/docs/guides/api-reference.mdx | Updated parameter table to canonical unprefixed names (filter, select, sort); documented GET-by-ID allowlisted params; added OData endpoint note |
| ROADMAP.md | Documented completion of 4 protocol hardening items: filter parameter naming, isFilterAST() validation, GET-by-ID allowlisting, dispatcher field mapping |
| if (this.isFilterAST(filterValue) || Array.isArray(filterValue)) { | ||
| // AST or any array → serialize as JSON in `filter` param |
There was a problem hiding this comment.
The condition this.isFilterAST(filterValue) || Array.isArray(filterValue) has a logical issue. Since isFilterAST() already returns false for invalid arrays (like [1, 2, 3]), the fallback || Array.isArray(filterValue) will cause ANY array (even non-filter arrays) to be JSON-serialized and sent to the server.
This means invalid filter arrays like [1, 2, 3] will pass client-side validation and be sent to the server, where they will either fail validation or be misinterpreted.
Consider either:
- Remove the
|| Array.isArray(filterValue)fallback so only valid AST arrays are serialized - Add a comment explaining why invalid arrays should be passed to the server for validation
The current implementation defeats the purpose of the new isFilterAST() structural validation.
| if (this.isFilterAST(filterValue) || Array.isArray(filterValue)) { | |
| // AST or any array → serialize as JSON in `filter` param | |
| if (this.isFilterAST(filterValue)) { | |
| // Valid filter AST → serialize as JSON in `filter` param |
Filter handling across the stack had five interrelated issues: inconsistent
filter/filters/$filternaming, naiveArray.isArrayAST detection, wrong response field names in the dispatcher, unconstrained query param passthrough on GET-by-ID, and no shared AST validation utility.Spec:
isFilterAST()with structural validationReplaces the client's
Array.isArray(filter)with a recursive validator exported from@objectstack/spec/datathat checks for valid AST shapes (comparison nodes with known operators, logical nodes with valid children, legacy flat arrays).Spec:
HttpFindQueryParamsSchema+GetDataRequestSchematighteningHttpFindQueryParamsSchemadocuments canonical HTTP query params.filter(singular) is the standard;filters(plural) accepted for backward compat.GetDataRequestSchemanow explicitly declaresselect/expandas the only allowed query params for GET-by-ID.Runtime: dispatcher fixes
handleApiEndpoint:result.data/result.count→result.records/result.totalto matchFindDataResponseSchema/data/:object/:id: onlyselect/expandforwarded; all other query params discardedProtocol:
filters→filternormalizationobjectql/protocol.tsnow normalizesfilterstofilterbefore parsing, and usesisFilterAST()instead ofArray.isArray.Client SDK
QueryOptions.filteradded as canonical field;filtersmarked@deprecatedisFilterASTdelegated to spec instead of inlineArray.isArrayfilter(singular) as HTTP query paramObject.entrieson array valuesDocumentation sync
api-reference.mdxto use canonicalfilter(not$filter) for the standard REST API parameter table, with a note clarifying that OData-style$-prefixed parameters are supported via the dedicated OData endpoint.select,expand) forGET /data/:object/:idin the API reference.Tests
19 new tests covering
isFilterASTrecursive validation,VALID_AST_OPERATORS,GetDataRequestSchemaselect/expand,HttpFindQueryParamsSchemacoercion. Full suite: 229 files, 6475 tests passing.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.