Repository navigation
Commit 97680bb
fix(service-analytics)!: the /analytics/sql dry run refuses a selection asking one column twice, as /analytics/query does (#28878)
domain:server#3 · os-project-manager · session_01FEkQ5kbUJTTWAxjV9nqr1P
<!-- claude-projects-attribution: {"github_login":"os-project-manager"}
-->
_Requested by **Leehom** · [project
thread](https://claude.ai/code/project/chan_01TenzsRopZj4jMbvjX8uz1X?thread=cmsg_01TenzsRopZj4jMbvjX8uz1XB1hsVtuncZ5F5C4zpiftHC)_
Fixes #28841
Clause-②: no (narrowing) — `/analytics/sql`
(`AnalyticsService.generateSql`) now refuses `INVALID_QUERY` / 400 a
selection that names one column twice in its groups (two granularities
of one time dimension, the column raw beside its own bucket, or two
spellings of it bucketed twice), with the refusal `/analytics/query`
gives the same selection, field for field. It used to render a statement
for each. Nothing is widened: no package-root export, public member,
answer member, error code or status is added.
Metadata-protocol: none
Skills-docs: none — no file under `skills/**` or `content/docs` says the
`/analytics/sql` dry run renders these selections.
**BREAKING** accept-set narrowing on the `/analytics/sql` dry run
(`@objectstack/service-analytics`). `/analytics/query` and
`queryDataset` are unchanged.
Card thread: `session_01FEkQ5kbUJTTWAxjV9nqr1P`.
## 维护者速读
- **改了什么**:`/analytics/sql`(只出 SQL、不执行的预演口)遇到同一列被要两次的选择,现在和
`/analytics/query` 一样拒绝,逐字段一致(`INVALID_QUERY` /
400)。涉及的选择有三种:同一时间维度两个粒度、原始列加它自己的时间桶、同一列两种写法各分一次桶。以前预演口照样给出一段
SQL,而真正的查询会拒绝。
- **为什么改**:卡 #28841(p3)。来源是 PR #28837 合约评审的 should-fix,也是我在 #28799
报告里的范围外发现。席位裁定:`generateSql` 通过 #28798 的同一个数据集门、按 `execute`
构建的同一组键拒绝;回显里「每维度只记一个粒度」的旧表删掉,而不是打补丁。
- **风险与回滚**:这是收窄,以前能出 SQL 的预演现在被拒,但只限查询本身就拒绝的选择。查询能服务的预演一字不差(已钉住)。回滚即
revert。
-
**先看**:`packages/services/service-analytics/src/strategies/objectql-strategy.ts`,看
`generateSql` 里新增的 `assertDatasetGroupColumnsDistinct` 调用,以及新的模块级
`crossObjectGroupKeys`(从 `executeCrossObject` 原样移出)。
- **两分钟验证**:`pnpm --filter @objectstack/service-analytics exec vitest
run src/__tests__/objectql-echo-column-twice.test.ts`,预期 13 passed。
- **需要你做的**:无。席位跑合约评审并落地。
## What changes
- **`generateSql` refuses through the one dataset door.** Right after
`planCrossObject`, before any bucket expression renders, the echo calls
`assertDatasetGroupColumnsDistinct` (`dataset-group-column-door.ts`,
#28798). It judges the keys `execute()` would hand the engine:
- `aggregateGroupKeys(query, cube)` on the direct path;
- `crossObjectGroupKeys(query, cube, crossByDim)` when `plan` routes the
query to the cross-object path, as `execute()` does.
It runs on the same object and with the same member reader, so the
refusal is the query's, message included. No third copy of the refusal
is added.
- **The one-granularity map is gone.** `granByDim` (`Map<string,
string>`, last entry wins) is replaced by
`granularitiesByDimension(query)`, the reader both key builders use.
`dimExpr` reads a dimension's first granularity. That is equivalent on
every selection the echo still renders, because the door has refused any
dimension asked at two different granularities by then, and a
granularity repeated is one bucket.
- **`crossObjectGroupKeys`** is `executeCrossObject`'s key build, moved
verbatim (comments included) to a module-level, non-exported function,
so the cross-object path and the echo build the same keys.
`executeCrossObject` now calls it.
- **Untouched.** Nothing is exported or added to `ObjectQLStrategy`.
`packages/objectql`, the door and `analytics-service.ts` are unchanged.
- Tests: new `src/__tests__/objectql-echo-column-twice.test.ts` (13
cases, real SQLite, the default composition). Changeset:
`.changeset/28841-sql-dry-run-refuses-column-twice.md`,
`@objectstack/service-analytics` patch.
## Why
Measured on `main` `7e60dc23` with a probe over real SQLite, default
composition, comparing `query()` with `generateSql()` (the body `POST
/analytics/sql` answers with). `query()` refused every selection below
with `INVALID_QUERY` / 400. The other `generateSql` caller,
`execute()`'s best-effort echo, runs after the engine and is caught, so
`/analytics/query` was already right.
| selection | `generateSql` on `main` |
|:--|:--|
| `closed_at` month and year | `SELECT strftime('%Y', …) AS "closed_at"
… GROUP BY strftime('%Y', …)`: the month entry was dropped |
| `closed_at` raw beside `CUBE.closed_at` month | `GROUP BY closed_at,
strftime('%Y-%m', …)` |
| `closed_day` (over `closed_at`) beside `closed_at` month | the same
two columns |
| `closed_at` month beside `CUBE.closed_at` year | both buckets |
| the inferred cube, `closed_at` raw beside `OBJECT.closed_at` month |
both columns |
| `account.name` beside `closed_at` month and year | the join, with one
`year` bucket |
| `closed_at` month and year at `America/New_York` | `NOT_IMPLEMENTED` /
501, "The query itself is SERVED" |
After the change, every row answers the query's refusal, field for
field. The served echoes (one bucket; two columns; region beside a
bucket asked twice at one granularity; a cross-object dimension beside
one bucket) printed the same bytes before and after.
## Tests
<details><summary>Red first, green, ablations</summary>
- **Red on base.** `objectql-strategy.ts` was restored to `7e60dc23`,
with the new test at HEAD. Result: `Tests 7 failed | 5 passed (12)` on
the 12-case file. Every refusal case failed on the dry run resolving,
and the five served pins passed. The 13th case (two dimensions through
one relationship) was added afterwards for ablation B2. Restored, then
`git status` was clean.
- **Green.** `13 passed (13)`. The full service-analytics suite gave
`Test Files 204 passed (204)`, `Tests 4889 passed | 282 skipped`. The
REST analytics tests and `tsc --noEmit` passed, and scoped eslint exited
0.
- **Ablations** used `node scripts/ablation-replace.mjs` with the fix
committed first, running this file plus
`preview-time-filter-fk-parity.test.ts`. Each anchor went 1 → 0, and
each restore came back with blob == HEAD:
- B1: make the echo's door call a no-op. Result: `8 failed | 42 passed`,
every refusal case.
- B2: judge `aggregateGroupKeys` on the cross-object route too. Result:
`1 failed | 49 passed`. Two dimensions through one relationship share
one FK key on that path, so the refusal names `groupBy.2.field` there,
not `groupBy.3.field`.
- B3: put the old one-granularity map back, with the door in place.
Result: `50 passed`. This is expected: with the door in place, the map
only feeds rendering of selections the door let through, where one
granularity per dimension is all there is. With the door removed as
well, that is the base tree, red above.
</details>
- **Dogfood** ran on `348838c8` under the verify lock. Result: `Test
Files 4 failed | 377 passed | 3 skipped (384)`, `Tests 4 failed | 3130
passed | 28 skipped (3162)`. The 4 failures are the 18.0-database
migration files: `audit-key-material`, `audit-mail-bodies`,
`identity-mail-read-gate` (the `sys_activity` mirror) and
`upgrade-18x-harness` (`no such table: sys_activity__r20261019`). The
same 4 fail in `main`'s own Dogfood Regression Gate (1/3, 2/3, 3/3) at
`3b920f6d`, the base of this branch. None of them reads analytics.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01FEkQ5kbUJTTWAxjV9nqr1P
---
_Generated by [Claude
Code](https://claude.ai/code/session_01FEkQ5kbUJTTWAxjV9nqr1P)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 61de599 commit 97680bb
6 files changed
Lines changed: 290 additions & 41 deletions
File tree
- .changeset
- packages
- client/src
- services/service-analytics/src
- __tests__
- strategies
- scripts
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
525 | 525 | | |
526 | 526 | | |
527 | 527 | | |
| 528 | + | |
| 529 | + | |
| 530 | + | |
| 531 | + | |
| 532 | + | |
| 533 | + | |
528 | 534 | | |
529 | 535 | | |
530 | 536 | | |
| |||
761 | 767 | | |
762 | 768 | | |
763 | 769 | | |
764 | | - | |
| 770 | + | |
| 771 | + | |
| 772 | + | |
| 773 | + | |
765 | 774 | | |
766 | 775 | | |
767 | 776 | | |
768 | 777 | | |
769 | 778 | | |
| 779 | + | |
770 | 780 | | |
771 | 781 | | |
772 | 782 | | |
| |||
814 | 824 | | |
815 | 825 | | |
816 | 826 | | |
817 | | - | |
| 827 | + | |
818 | 828 | | |
819 | 829 | | |
820 | 830 | | |
821 | 831 | | |
822 | | - | |
| 832 | + | |
| 833 | + | |
823 | 834 | | |
824 | 835 | | |
825 | 836 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
0 commit comments