Repository navigation
Add granular query operation capabilities to driver schema - #49
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 目前实现已经非常完整,我只有一个小的建议供参考: • 默认值考虑:目前所有新标志的默认值似乎都是 false(在 JSON Schema 中显式设为 false,Zod 中虽未显式但在测试中体现了这一点)。这是一个安全的默认策略(“悲观策略”),确保了如果驱动没声明支持,引擎会兜底处理。这很好。 |
There was a problem hiding this comment.
Pull request overview
This pull request adds granular query operation capabilities to the driver schema, enabling ObjectQL to determine which query operations can be pushed to the database driver versus computed in-memory. The PR introduces 6 new capability flags for common query operations (filters, aggregations, sorting, pagination, window functions, and subqueries).
Changes:
- Added 6 new query capability boolean flags to
DriverCapabilitiesSchemaandDatasourceCapabilitiesschemas - Updated all test fixtures with realistic capability matrices for PostgreSQL, MongoDB, Salesforce, Redis, and a new memory driver example
- Regenerated JSON schemas and documentation with new capability fields
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/spec/src/system/driver.zod.ts | Added 6 query capability fields to DriverCapabilitiesSchema with comprehensive JSDoc documentation and .describe() calls |
| packages/spec/src/system/datasource.zod.ts | Added 6 query capability fields to DatasourceCapabilities; removed aggregation field in favor of queryAggregations |
| packages/spec/src/system/driver.test.ts | Updated all test fixtures with new capability flags; added realistic examples for PostgreSQL, MongoDB, Salesforce, Redis, and memory drivers |
| packages/spec/json-schema/DriverInterface.json | Auto-generated JSON schema updated with new required capability fields |
| packages/spec/json-schema/DriverDefinition.json | Auto-generated JSON schema updated with new capability fields and defaults |
| packages/spec/json-schema/DriverCapabilities.json | Auto-generated JSON schema updated with new required capability fields |
| packages/spec/json-schema/DatasourceCapabilities.json | Auto-generated JSON schema updated with new capability fields and defaults |
| packages/spec/json-schema/Datasource.json | Auto-generated JSON schema updated with new capability fields |
| content/docs/references/system/integration/DatasourceCapabilities.mdx | Auto-generated documentation updated with new capability fields (missing descriptions) |
| content/docs/references/system/DriverCapabilities.mdx | Auto-generated documentation updated with new capability fields and descriptions |
| /** Can execute WHERE clause filters natively? */ | ||
| queryFilters: z.boolean().default(false), | ||
|
|
||
| /** Can perform aggregation (group by, sum, avg)? */ | ||
| queryAggregations: z.boolean().default(false), | ||
|
|
||
| /** Can perform ORDER BY sorting? */ | ||
| querySorting: z.boolean().default(false), | ||
|
|
||
| /** Can perform LIMIT/OFFSET pagination? */ | ||
| queryPagination: z.boolean().default(false), | ||
|
|
||
| /** Can perform window functions? */ | ||
| queryWindowFunctions: z.boolean().default(false), | ||
|
|
||
| /** Can perform subqueries? */ | ||
| querySubqueries: z.boolean().default(false), |
There was a problem hiding this comment.
The new query capability fields in DatasourceCapabilities lack .describe() method calls, which causes the auto-generated documentation to have empty descriptions. For consistency with DriverCapabilitiesSchema (which uses both JSDoc comments and .describe() calls) and to ensure proper documentation generation, each field should include a .describe() call with the description text.
For example:
queryFilters: z.boolean().default(false).describe('Can execute WHERE clause filters natively?')queryAggregations: z.boolean().default(false).describe('Can perform aggregation (group by, sum, avg)?')
This would ensure the generated documentation tables include meaningful descriptions for each capability.
|
|
||
| /** Can execute WHERE clause filters natively? */ | ||
| queryFilters: z.boolean().default(false), | ||
|
|
There was a problem hiding this comment.
The removal of the aggregation capability field and its replacement with queryAggregations is a breaking change. While this rename provides better clarity and consistency with the new query capability naming scheme, any existing code that references capabilities.aggregation will break.
Consider:
- Adding a migration guide or deprecation notice in the PR description
- If backwards compatibility is required, temporarily supporting both field names with the old one marked as deprecated
- Documenting this breaking change in a CHANGELOG or migration guide
| /** | |
| * @deprecated Use `queryAggregations` instead. | |
| * Legacy aggregation capability flag kept for backwards compatibility. | |
| */ | |
| aggregation: z.boolean().default(false).describe('Deprecated: use queryAggregations'), | |
Add missing query operation capabilities (queryFilters, queryAggregations, querySorting, queryPagination, queryWindowFunctions, querySubqueries) to match the updated DriverCapabilitiesSchema. Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Based on code review, updated capability flags to accurately reflect the current implementation. Only queryPagination, jsonFields, and arrayFields are actually supported. Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…the bare sys_file id, switched per deployment on the adr-0104-file-references flag Records the maintainer's decision-batch #49 item 1 ruling (Option A) as a 2026-09-05 addendum to ADR-0104: the media family's (image / file / avatar / video / audio) single-value physical column is a string column holding the bare sys_file id — the generator's VARCHAR(2048) / table.string is the ruled end-state and the SQL driver moves to it; the encoding switch is per deployment, keyed on the existing adr-0104-file-references sys_migration row and never on a version, with the column move as a further step of `os migrate files-to-references --apply` after zero blocking findings; the dual-encoding window this implies, its invariant (column type and write encoding never disagree on one deployment), the three populations it must hold over, and its end (the first protocol major after the driver lands, with a loud boot refusal for un-moved deployments); the two confidence gaps carried from the measurement stated as gaps with what closes them; and the sequencing behind the driver card. Governed surface (docs/adr). No code, schema, generated artifact or changeset moves with this commit. Anchors are symbol / file anchors only. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M59rPZZFzqhfMUPFqqZTkf
…the bare sys_file id, switched per deployment on the adr-0104-file-references flag (objectstack-ai#15041) (objectstack-ai#16014) * docs(adr): ADR-0104 addendum — the file family's stored column holds the bare sys_file id, switched per deployment on the adr-0104-file-references flag Records the maintainer's decision-batch objectstack-ai#49 item 1 ruling (Option A) as a 2026-09-05 addendum to ADR-0104: the media family's (image / file / avatar / video / audio) single-value physical column is a string column holding the bare sys_file id — the generator's VARCHAR(2048) / table.string is the ruled end-state and the SQL driver moves to it; the encoding switch is per deployment, keyed on the existing adr-0104-file-references sys_migration row and never on a version, with the column move as a further step of `os migrate files-to-references --apply` after zero blocking findings; the dual-encoding window this implies, its invariant (column type and write encoding never disagree on one deployment), the three populations it must hold over, and its end (the first protocol major after the driver lands, with a loud boot refusal for un-moved deployments); the two confidence gaps carried from the measurement stated as gaps with what closes them; and the sequencing behind the driver card. Governed surface (docs/adr). No code, schema, generated artifact or changeset moves with this commit. Anchors are symbol / file anchors only. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M59rPZZFzqhfMUPFqqZTkf * docs(adr): ADR-0104 Status bullet indexes the 2026-09-05 addendum One sentence in the ADR's running index of addenda, in the spelling the existing entries use: the 2026-09-05 addendum rules the media family's physical column — a string column holding the bare sys_file id, switched per deployment on the adr-0104-file-references flag, never per version; driver card objectstack-ai#15989 implements it. Seat ruling on the report's open question 1 (B). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M59rPZZFzqhfMUPFqqZTkf --------- Co-authored-by: Claude <noreply@anthropic.com>
…ult unchanged (objectstack-ai#17863) Fixes objectstack-ai#15484 Clause-②: no ⛔ **Corrected at review — the seat's act, not the dev's.** This line declared `` `Clause-②: yes` `` when the PR was opened (backticked and mid-line —⚠️ which this repo's reader does in fact **accept**: `readClause2Line` returns `declared: yes` on that exact string, measured. The bare, line-initial form is what the protocol asks for, and it is what this line now carries), on the operator-visible-surface axis argued under "Clause-② measurement" below. **The maintainer had already judged that axis for this card**, in decision batch objectstack-ai#49's own text: 「**Changeset:** `@objectstack/rest` `patch` (default behaviour identical; a new declared environment seam is documented, **not a contract key**)」. Record: the contract review at [`5646806136`](objectstack-ai#17863 (comment)), head `7e6e9e044`, which also corrects this seat's own wrong reading at [`5646786543`](objectstack-ai#17863 (comment)). ⛔ The measurement below is left exactly as the dev wrote it. Decision batch objectstack-ai#49 item 2 ruled option **A**: a declared level seam on `packages/rest`'s `logError`, in the `OS_REGISTRY_LOG` shape, **with the shipped default unchanged**. That is what this delivers — a declaration a gate can read, not a quieter product.⚠️ **One row of the dispatch order's file face is deliberately NOT executed here, and that is the main thing to review.** The suite-wide opt-down turns out to cost far more than the four pins the order inherited, and it is written up under "The opt-down: measured, and stopped" below. Everything else landed. ## What landed | file | what | |:--|:--| | `packages/rest/src/log.ts` | the seam. `OS_REST_LOG`, five levels, shipped default `'info'` — today's behaviour byte-for-byte. `logWarn` falls through the same ladder. | | `packages/rest/src/rest-log-declared-level-seam.test.ts` | 14 assertions pinning the shipped default, the fallback direction, and the ladder. | | `scripts/check-rest-log-declared.mjs` | the gate — 19-case self-test, wired into `package.json` and `lint.yml`. | | `packages/rest/vitest.config.ts` | the declaration, in the root block and in **both** inline projects. | | `packages/rest/README.md` | where an operator learns the variable. | | `docs/audits/2026-09-test-log-volume-census.md` | lines 419–420's reservation discharged, per the ruling's own instruction. | The seam is one contract with `OS_REGISTRY_LOG`, not a second ad-hoc variable: same five-level vocabulary, same `'info'` default, same "unrecognised value falls back to the default" direction. The gate holds the two vocabularies **equal**, reading each from its own source rather than carrying a copy. The gate's population is narrow **by decision, and says so in its header**: the package that owns the seam (located by its environment read, never hardcoded) plus any package that opts in by declaring the key. Measured at time of writing, 21 workspace packages reference `@objectstack/rest` from their own test sources — `packages/spec` among them — and conscripting all of them is a bigger change than the one that was ruled. ## Re-derived measurement — nothing quoted from the card The card's counts were 8 days stale and its `[Registry]` control is spent. Both replaced, and **the live control was validated in the same capture it was used in**: | pattern | card | measured now | |:--|--:|--:| | total captured lines | 5,971 | 5,709 | | indented `at ` frames | 1,922 | **2,095** (36.7% of output) | | `at file://` | 688 | 742 | | control — `[sql-driver] DATABASE_ERROR` | — | **362** (fires) | | control — `[REST]` | — | **272** (fires) | | spent control — `[Registry]` | 528 | **0** (as documented) | Attribution, by the header preceding each frame block: `error-response.ts` **1,197 (57.1%)**, `rest-server.ts` 841 (40.1%), `cause` chains 57. **100% of the frames arrive through `logError`** — and the `[sql-driver]` population contributes **zero**, which re-confirms the disjointness claim the card asked not to lose. **The default did not move, and that is a measurement, not a claim.** Same suite, before and after: frames **2,095 → 2,095**, control 362 → 362. Test counts move only by the new file: 190 files / 3,182 passed → **191 files / 3,196 passed**. ## The ablation Two legs, direction predicted in writing first, each mutation proved on disk before anything was read, restore proved by blob hash and a clean whole-tree status. **Leg 1 — move only the shipped default** (`'info'` → `'silent'`; harness declaration untouched). On-disk proof: removed-text 1→0, injected-text 1, blob `51fdb14b` vs HEAD `3720e0a0`. - the gate → **exit 1**, printing `REST_LOG_DEFAULT_LEVEL is 'silent', which stops at least one of this shim's two sites from reporting at all for EVERY real caller` - tests → `1 failed | 2 passed`; the failing file is the new seam pin, and **the two files carrying the four inherited pins stayed GREEN** That second result was predicted, and it is the point: an explicit harness declaration outranks the default, so a default move alone is invisible to a suite that declares. Which is why there is a leg 2. **Leg 2 — the world in which the default actually governs** (default `'silent'` **and** the harness declaration removed). On-disk proof: declarations 3→0, blob `70c8244c` vs HEAD `84331960`. - the four inherited pins (objectstack-ai#5437 / objectstack-ai#4886 / objectstack-ai#5489) → **`2 failed`, 8 assertions red** - frames in that capture → **0** ⇒ the frames are load-bearing, and it is the shipped default that keeps them reachable. **Restore:** `git checkout HEAD --` on absolute paths under a trap; `log.ts` blob `3720e0a0` == HEAD, `vitest.config.ts` blob `84331960` == HEAD, `git diff HEAD` empty, `git status --porcelain` empty across the whole tree. An empty hash was coded as failure, not as "nothing to compare". ## Clause-② measurement Measured from the delivered diff, not inherited: - **Export list of `packages/rest/src/index.ts`, before and after, order-insensitively — IDENTICAL.** Taken two independent ways off the built entry so a type-only export cannot hide: 12 runtime value exports (`Object.keys` of the ESM entry) and 31 declared export names parsed out of `dist/index.d.ts`. `diff` of the two readings is empty. ⛔ Not a `[+-].*export` matcher. - **`packages/rest/package.json`'s `exports` map — did not move.** `git diff` against the merge base over that file is empty. - **No wire byte, no published payload key.** The objectstack-ai#14696 mechanical floor is **not** met. ⇒ **Outcome, added at review and kept apart from the text below it: the declaration is `no`.** The export-axis reading in this section is correct and is confirmed independently in the review of record (`log.ts` gains four exports, none reachable — `index.ts` does not re-export it, the `exports` map has one `"."` entry with no wildcard, and `files[]` ships no source). The operator-visible-surface axis argued next is a real axis, and it is the one the ruling already answered with 「not a contract key」. ⛔ The paragraph below is left as written. ⇒ On the export axis the honest answer is `no`. **The declaration is `yes` on the operator-visible-surface axis**, which is the one triage actually raised: this ships a **new environment variable** that changes what an operator sees, documented in `packages/rest/README.md` — a file inside this package's published `files[]`. `log.ts` itself stays un-re-exported, an internal shim as its docblock says. **The env seam's name is `OS_REST_LOG`, and an operator learns it from `packages/rest/README.md`'s new `### Environment` section** (also from `log.ts`'s docblock and the audit's closing section). ## The opt-down: measured, and stopped The order's file face asks `packages/rest/vitest.config.ts` to opt the suite down. **Measured before choosing:** `OS_REST_LOG: 'silent'` does take frames **2,095 → 0**. It also does this: - **28 assertions across 15 files go RED** — not the four pins the order inherited, 7x that. They are the "the operator still gets the words" half of the contract, and they read the fault through a `vi.spyOn(console, 'error')` mock, so they never printed any of the volume in the first place. - **8 files assert the OTHER half — that an expected 4xx logs NOTHING** (`expect(unhandledLogs()).toHaveLength(0)` and siblings, 4 in `rest-expected-error-logging.test.ts` alone). A silenced suite makes those pass **for the wrong reason**: they stay green with every expected 4xx logged loudly. That is a gate weakened into a phantom check by a legitimate-looking declaration — the same way this card's own `[Registry]` control was silently spent by objectstack-ai#15425. Landing that needs every file asserting on the fault log to declare the loud level for itself, plus a guard pairing the two so a future test cannot assert silence into a silenced suite — **~20 files the order did not name**. The order's own stop condition covers exactly this, so the declaration ships here at `'info'` (the shipped default: real, valid, gate-read, behaviour-identical) and the value is left as the one-line choice it is. The measurement is recorded in the config's own root `env` block so the next reader does not have to re-derive it. ## Verification Gate set derived, not guessed — `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`, 109 commands, run against `7e6e9e044` with every exit code captured **before** any pipe. - **109/109 exit 0.** Three needed a second look and none was a finding: `check:dual-build-cjs-loads` and `check:type-check-debt` first returned **exit 3 — `PREREQUISITE NOT MET`**, satisfied by `turbo run build` over all packages (72/72 successful) and then **both exit 0**; `check:pm-dispatch-gates` first returned 124, which was a local runner timeout and not a verdict — re-run unbounded it is **exit 0** (1,678 self-test cases). - `pnpm lint` — the full union, `eslint . --no-inline-config`, **exit 0**, no narrowing. (This lane's known blind spot: `dispatch-gates` does not name it.) - `pnpm --filter @objectstack/rest run typecheck` — **exit 0**; `tsc --noEmit` plus `check:test-typecheck`, 0 files / 0 errors in the debt ledger. - `pnpm --filter @objectstack/rest test` — **191 files / 3,196 passed | 1 skipped**. - `node scripts/check-rest-log-declared.mjs --self-test` — **19/19**. Heavy runs were serialised through `scripts/pm/os-verify-lock.sh`; every verdict above is read from a command's own printed line or from the wrapper's `VERDICT command-exit` line. ## Acceptance notes Out of scope, noted, not filed — each names who would meet it: - **`packages/rest/vitest.config.ts`'s comment is now accurate where it was not.** It stated in the present tense that the suite "measures 528 residual `[Registry]` lines here", on the lines immediately above the declaration that makes it 0. That text is untouched by this PR, but the block it sits in is now the one a reader of `OS_REST_LOG` lands on. Carrier: the next PR editing this config. Raised on this card at `5550786137`; still not filed, because `scripts/check-registry-log-declared.mjs`'s header repeats the same 528 as a live reading and the two should be corrected together, by whoever owns that gate. - **`scripts/check-rest-log-declared.mjs` carries a second spelling of `check-registry-log-declared.mjs`'s brace-matching and env-block reader.** Stated in its own header rather than left to be discovered. Extracting one shared reader is the right follow-up; it is not done here because that gate's self-test carries a battery floor this card has no mandate to move. Carrier: whoever adds the third seam of this shape — at which point the duplication stops being a note and starts being a population. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c --- _Generated by [Claude Code](https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c)_ --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Drivers currently can only declare high-level capabilities (transactions, joins, full-text search). No mechanism exists to specify support for individual query operations, preventing ObjectQL from determining whether to push operations to the driver or compute in-memory.
Changes
Schema Enhancements
DriverCapabilitiesSchemaandDatasourceCapabilities:queryFilters- WHERE clause supportqueryAggregations- GROUP BY/aggregation functionsquerySorting- ORDER BY supportqueryPagination- LIMIT/OFFSET supportqueryWindowFunctions- Window functions with OVER clausequerySubqueries- Nested SELECT supportTest Coverage
Generated Artifacts
Example
ObjectQL can now inspect these flags to determine whether to push query operations to the driver or handle them in-memory.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.