Skip to content

Commit 7bbf1e2

Browse files
committed
merge origin/main (a7b854f)
2 parents 5b73346 + a7b854f commit 7bbf1e2

23 files changed

Lines changed: 1629 additions & 52 deletions
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
fix(service-analytics): the three SQL compilers compare LIKE values literally (#5567)
6+
7+
`$contains` / `$notContains` / `$startsWith` / `$endsWith` build a `LIKE` pattern
8+
around the comparand the author wrote. All three of this package's SQL compilers
9+
concatenated that comparand straight into a wildcard position — no escaping, no
10+
`ESCAPE` clause — so `_` (LIKE's single-character wildcard) and `%` (its
11+
multi-character one) stopped being literals. Measured on real SQLite, over the
12+
rows `x_admin` / `xyadmin` / `off 50% now` / `off 5012 now`:
13+
14+
| `where` | returned | correct |
15+
|----------------------------------|---------------|---------|
16+
| `{name: {$contains: '_admin'}}` | `['1','2']` | `['1']` |
17+
| `{name: {$contains: '50%'}}` | `['3','4']` | `['3']` |
18+
| `{name: {$startsWith: 'x_'}}` | `['1','2']` | `['1']` |
19+
| `{name: {$endsWith: '0% now'}}` | `['3','4']` | `['3']` |
20+
21+
Every row is a **widening** — rows the author excluded came back — and
22+
`$notContains` is the mirror image, excluding rows the author kept. One of the
23+
three call sites is the ADR-0021 D-C read-scope (tenant + RLS) lowering, where a
24+
wider predicate is over-reach rather than a loose filter (the #5347 / #5324
25+
ruling on that same file). Prime Directive #3 forces machine names to
26+
`snake_case`, so essentially every machine-name comparand carries a `_` and hit
27+
this silently.
28+
29+
All three compilers now escape the comparand and bind an explicit
30+
`ESCAPE` argument, matching what `driver-sql`'s `applyLike` has always done — so
31+
the same filter selects the same rows whichever strategy answers, and the
32+
`/analytics/sql` echo describes the statement that ran instead of a wider one.
33+
34+
**No authoring change.** A comparand with no `_`, `%` or `\` binds exactly the
35+
pattern it bound before; only its meaning when it *does* carry one changes, from
36+
wildcard to literal. If you were relying on a comparand acting as a wildcard,
37+
that was never a declared capability of these operators — the spec describes them
38+
as substring / prefix / suffix matches — and `driver-sql` already read it
39+
literally, so the reading you got depended on which strategy served the query.
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
"@objectstack/runtime": patch
3+
"@objectstack/metadata-protocol": patch
4+
---
5+
6+
fix(runtime): `callData`'s ObjectQL fallback answers a missing record id with 404 `RECORD_NOT_FOUND` (#5138)
7+
8+
`callData` (the data bridge behind `/data`, the MCP bridge and the declarative
9+
endpoint executor) is protocol-first with an ObjectQL fallback. The fallback
10+
gave **three different answers to one fact** — that `id` names no row:
11+
12+
| verb | before | on the wire |
13+
|---|---|---|
14+
| `get` | `return … : null` | `200 { data: null }` |
15+
| `update` | `throw new Error('[ObjectStack] Not Found')` — no `.status` | **500** |
16+
| `delete` | no existence check at all | `200 { deleted: true }` |
17+
18+
The protocol path has answered `404 RECORD_NOT_FOUND` on all three verbs since
19+
#4435 (re-asserted for the batch path by #5088), so the answer to the same
20+
request depended on something no caller can see: whether the deployment
21+
registered the `protocol` slot (`MetadataPlugin` / `@objectstack/metadata-protocol`).
22+
All three fallback branches now throw the SAME envelope the protocol throws.
23+
24+
Two of these were actively harmful. `update` reported a caller mistake as an
25+
internal fault — every dispatcher exit reads `.status``.statusCode` → 500, so
26+
a 4xx fact entered error reporting and alerting as a 5xx. `delete` reported
27+
success for a row that never existed, which is the hardest class to notice: an
28+
integrator reading `200` records the cleanup as done.
29+
30+
The envelope is not re-spelled. `recordNotFoundError` is now exported from
31+
`@objectstack/metadata-protocol` and imported by the fallback, so there is one
32+
construction point and the two paths behind one `callData` cannot drift apart
33+
again.
34+
35+
**Upgrade note.** If you run an assembly WITHOUT the metadata-protocol plugin
36+
(lean hosts, and the MCP multi-env path that threads a raw driver), these three
37+
calls change their answer for a missing id — from `200`/`200`/`500` to `404
38+
{ code: 'RECORD_NOT_FOUND', message: 'Record <id> not found in <object>' }`.
39+
Deployments that DO register the protocol slot are unaffected: they already
40+
answered `404` and this release does not touch that path. A client that
41+
branched on `data === null` from `GET /data/:object/:id` should branch on the
42+
`404` instead; a client that treated `DELETE` as idempotent should treat `404`
43+
as "already gone". Declarative endpoints (`object_operation`) inherit the same
44+
answer, since they reuse `/data`'s delegation.
45+
46+
`delete`'s existence check is a `find` probe, not a read of what `ql.delete`
47+
returned: `IDataDriver.delete` declares `Promise< boolean >` and the protocol
48+
can read it, but `IDataEngine.delete` declares `Promise< any >` and the engine
49+
returns its driver's result through the hook chain — testing that for `false`
50+
would be reading a signal the contract does not promise, and it fails in the
51+
direction this fixes.
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
"@objectstack/rest": patch
3+
---
4+
5+
fix(rest): an unclassified route error answers a sanitised 500, not a 400 (#5489)
6+
7+
**升级须知 — 状态码行为变化。** `@objectstack/rest` 的错误映射 `mapDataError`
8+
在所有分类分支都不匹配时,原先的终局兜底是
9+
`{ status: 400, body: { error: <原始 message> } }`。这一支现在改为一个消毒过的
10+
服务端故障信封:
11+
12+
```
13+
500 {"error":"Internal server error","code":"INTERNAL_ERROR"}
14+
```
15+
16+
**为什么。** 400 的语义是「你请求错了」——SDK、fetch 封装、代理和重试策略都据此
17+
判定「不要重试,调用方得改点什么」。而真正落到这一支的错误恰恰相反:元数据存储
18+
读不到时 `matchEndpoint` 按契约抛错(它抛就是为了让 outage 不伪装成「没有声明
19+
任何 endpoint」,ADR-0110 D3),或者干脆是处理器自身的 `TypeError`。两者调用方都
20+
修不了,且都**应该**重试。实测:`GET /api/v1/meta/api` 对着一个抛
21+
`Error('metadata store unreachable')` 的存储,返回 HTTP 400。
22+
23+
同时,原始 message 是逐字下发的——而这偏偏是全文件里最没有证据表明可以下发的一
24+
条路径:走到这里的前提就是 `looksLikeInternalErrorLeak` 什么都没匹配上,而
25+
#5462 已经记过「关键词启发式沉默不等于安全」。实测到的一例:一个声明了
26+
`status: 502`、message 为 `connect ECONNREFUSED 10.0.0.5:5432 (internal pool)`
27+
的错误,经由数据路由直接调用 `mapDataError` 时,以 400 携带主机与端口下发。
28+
沿用 #5464 的纪律:原文进服务端日志,不进客户端(500 不在
29+
`isExpectedDataStatus` 内,`handleRouteError` 会打印完整错误对象)。
30+
31+
**真正的客户端错误一个都没有改变。** 改动前先做了测绘:给这一支加桩,跑完
32+
`@objectstack/rest` 全套(48 文件 / 719 用例),落到这一支的只有 6 个错误——本单
33+
的存储 outage、两个 502 的 ECONNREFUSED、三个 `TypeError`,没有一个是客户端
34+
错误。历史上唯一骑在这条兜底上的客户端错误家族(driver-sql 无法编译的 filter
35+
拒绝)已由 #4436**生产者侧**声明 `status: 400` + `INVALID_FILTER` 迁走。
36+
validation / permission / unknown object / unknown field / not-null 漂移 /
37+
unique 冲突 / 沙箱业务拒绝等全部仍由各自分支给出原本的 4xx。
38+
39+
**`INTERNAL_ERROR` 而非 `DATABASE_ERROR`** #5462`DATA_STORE_FAULT`
40+
(`500 DATABASE_ERROR`)用在证据**指名**了存储故障的地方(驱动的 missing-relation
41+
措辞、`looksLikeInternalErrorLeak` 命中);而这一支的定义性事实是「没有任何证据」,
42+
把处理器的 `TypeError` 报成 `DATABASE_ERROR` 会把运维指向一个其实健康的数据库。
43+
`INTERNAL_ERROR``standardErrorCodeForHttpStatus(500)` 的取值
44+
(`@objectstack/spec``HttpStatusErrorCodeMap`)——目录自己为「500 且无更具体
45+
code」定义的下限,不是第三套措辞;message 复用的也是
46+
`resolveErrorResponse` 声明式 5xx 分支已在用的 `INTERNAL_ERROR_MESSAGE`
47+
48+
**如果你的客户端把这条兜底当 400 处理过**:它现在是 5xx,可以重试;若你有生产者
49+
依赖「不声明 status 即可把 message 原文送达调用方」,请改为在抛出点声明
50+
`status``code`(契约优先),那是唯一仍会把措辞交给调用方的路径。

packages/metadata-protocol/src/index.ts

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

33
export { ObjectStackProtocolImplementation, ConcurrentUpdateError, normalizeViewMetadata, graftNormalizedOperators, stripReadDecorations } from './protocol.js';
4+
// [#5138] The 404 envelope every single-record path answers, exported so the
5+
// ObjectQL FALLBACK in `@objectstack/runtime`'s `callData` builds the SAME one
6+
// instead of minting a second not-found shape. See `recordNotFoundError`.
7+
export { recordNotFoundError } from './protocol.js';
48
export { createMetadataProtocolPlugin, assembleMetadataProtocol } from './plugin.js';
59
export type { MetadataProtocolPluginOptions } from './plugin.js';
610
export type { UninstallCleanup, UninstallCleanupOutcome } from './protocol.js';

packages/metadata-protocol/src/protocol.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,8 +340,19 @@ function resolveOverlaySchema(type: string, _item: unknown): z.ZodTypeAny | null
340340
* params, #4190 stopped dropping filters) — a write that touched zero rows
341341
* reporting 200 is that shape one level up, on the verb where it costs the
342342
* most.
343+
*
344+
* [#5138] EXPORTED, for the same "cannot disagree about it" reason one layer
345+
* out. `@objectstack/runtime`'s `callData` is protocol-first with an ObjectQL
346+
* FALLBACK, and the fallback had reinvented this fact three incompatible ways
347+
* (`get` → `null`, `update` → a bare `Error` with no status ⇒ 500, `delete` →
348+
* no check at all ⇒ `200 { deleted: true }` for a row that never existed). It
349+
* now calls THIS function, so the two paths behind one `callData` answer a
350+
* missing id identically — which is the only reason a caller may stop caring
351+
* which of them served it. Re-spelling the envelope there would have been a
352+
* second not-found envelope; `RECORD_NOT_FOUND` (#5088) is the one this repo
353+
* has.
343354
*/
344-
function recordNotFoundError(object: string, id: string | number): Error {
355+
export function recordNotFoundError(object: string, id: string | number): Error {
345356
const err = new Error(`Record ${id} not found in ${object}`) as Error & {
346357
code?: string;
347358
status?: number;

packages/plugins/driver-sql/src/sql-driver.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,12 @@ const SQLITE_TIME_EXPR_REFS = 8;
467467
* tail (attribution, issue numbers) may be cut. Keep the actionable part —
468468
* operator, field, path, what arrived, what the spec declares — at the FRONT.
469469
*
470+
* [#5489] The "without a status it reached the client verbatim" half is now
471+
* history: that terminal branch answers a sanitised 500 (`INTERNAL_ERROR`).
472+
* Declaring `status` + `code` at the throw site is therefore the ONLY way a
473+
* refusal's words reach the caller at all — which is the contract-first
474+
* arrangement #4436 wanted, no longer relying on a fallback that leaked.
475+
*
470476
* The `[sql-driver]` prefix these messages used to carry is GONE from the text:
471477
* it is driver-internal wording, and shipping it to clients is exactly what the
472478
* #3867 sanitiser exists to stop. The operator/field/vocabulary detail — the
@@ -6272,6 +6278,16 @@ export class SqlDriver implements IDataDriver {
62726278
* character (MySQL/Postgres do, but the explicit clause is correct for all
62736279
* three). `shape` positions the wildcard: `contains` → `%v%`, `starts` → `v%`,
62746280
* `ends` → `%v`.
6281+
*
6282+
* **Second implementation, deliberately** (#5567):
6283+
* `packages/services/service-analytics/src/like-pattern.ts` carries the same
6284+
* transform — same escaped character class, same three shapes, same bound
6285+
* `ESCAPE` — because `service-analytics` depends on no driver and this is a
6286+
* private method taking a knex builder, so there is nothing for it to import.
6287+
* That file's header explains the choice; it is held to THIS expression, character
6288+
* for character, by `service-analytics`'s `like-metacharacter-escape.test.ts`.
6289+
* A third hand-copy is the thing to refuse: import from one of the two, or add
6290+
* a consumer to that test.
62756291
*/
62766292
private applyLike(
62776293
builder: any,

packages/rest/src/rest-4xx-message-truncation.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,17 @@ describe('mapDataError: short 4xx messages are byte-for-byte unchanged (#5423)',
147147
Object.assign(new Error('connect ECONNREFUSED 10.0.0.5:5432 '.repeat(20)), { status: 502 }),
148148
);
149149
expect(r.status).not.toBe(502);
150+
// [#5489] `not.toBe(502)` was true of the OLD landing too, and that
151+
// landing was `400` with every byte of the ECONNREFUSED text — host and
152+
// port included — on the wire. The negative assertion could not tell
153+
// the two apart, so what it actually lands on is pinned here: this
154+
// declared 5xx now leaves `mapDataError` through the terminal
155+
// `UNCLASSIFIED_FAULT`, sanitised and in the server band. (The declared
156+
// 502 is still not preserved on this direct-call path — that is
157+
// `resolveErrorResponse`'s branch, and out of #5489's scope.)
158+
expect(r.status).toBe(500);
159+
expect(r.body.code).toBe('INTERNAL_ERROR');
160+
expect(String(r.body.error)).not.toContain('10.0.0.5');
150161
});
151162
});
152163

packages/rest/src/rest-5xx-message-sanitization.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,11 @@
4444
// -> 404 Object 'showcase_account' is not registered
4545
// 500 `Failed to delete customization overlay: connect ECONNREFUSED ...`
4646
// -> 400 with the driver text STILL verbatim (terminal fallback)
47+
// [#5489] that terminal fallback is now a sanitised 500, so this
48+
// third row's LEAK is closed at the source. The other two rows are
49+
// untouched — they are mis-classifications by the text heuristics,
50+
// not by the fallback — and the reason this fix stays in the branch
51+
// itself (keep the producer's declared status) is unchanged.
4752
//
4853
// So it re-labels a server fault as a client mistake, re-labels a capability
4954
// refusal as a missing object, and — for any 5xx whose wording matches no

packages/rest/src/rest-endpoint-surfaces-served-only.test.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -265,13 +265,16 @@ describe('#5224 — GET /meta/api announces only what the matcher serves', () =>
265265
const { rest } = mountRest(ALL_ENUMERATED, outage);
266266

267267
const res = await getMetaApi(rest);
268-
// The pin is that the request FAILS rather than answering a set. The exact
269-
// status is not this change's to decide: an unrecognised error reaching
270-
// `handleRouteError` lands on `mapDataError`'s terminal fallback, which
271-
// this route measured at 400 — a pre-existing classification shared by
272-
// every error on the metadata routes, not a consequence of the narrowing.
273-
// Asserting 5xx here would pin someone else's bug as if it were fixed.
274-
expect(res.statusCode).toBeGreaterThanOrEqual(400);
268+
// [#5489] Promoted from `>= 400` to the 5xx band. #5487 deliberately left
269+
// it at `>= 400` because the terminal fallback in `mapDataError` measured
270+
// 400 here, and asserting 5xx would have pinned someone else's bug as if it
271+
// were fixed. #5489 fixed it: an outage the mapper cannot attribute to the
272+
// request is a server fault, which is what an SDK must read to decide that
273+
// retrying is the right move. The route's own pin — that it FAILS rather
274+
// than confidently answering "this deployment declares no endpoints" — is
275+
// unchanged and is the second assertion.
276+
expect(res.statusCode).toBeGreaterThanOrEqual(500);
277+
expect(res.body?.code).toBe('INTERNAL_ERROR');
275278
expect(res.body?.items ?? res.body).not.toEqual([SERVED]);
276279
}, 60_000);
277280
});

packages/rest/src/rest-expected-error-logging.test.ts

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,13 @@
2525
// OPPOSITE overreach, a predicate widened to "any 4xx is expected", which
2626
// would silence the un-coded 400 that `mapDataError` degrades an
2727
// unrecognised error (a handler `TypeError`) to.
28+
//
29+
// [#5489] That last sentence describes the world before the unrecognised-error
30+
// fallback became a sanitised 500. The handler-bug case below now asserts 500;
31+
// its adversary is no longer a widened 4xx predicate but any future attempt to
32+
// add 500 to `isExpectedDataStatus`. The invariant it guards — a real handler
33+
// bug is never silent — is the same one, and is now carried by the status band
34+
// rather than by the absence of a `code`.
2835

2936
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
3037
import { RestServer } from './rest-server';
@@ -168,20 +175,28 @@ describe('metadata routes — genuine faults keep the loud log (#4886)', () => {
168175
expect(res.statusCode).toBe(500);
169176
});
170177

171-
it('an UNRECOGNISED error (handler bug) stays loud even though it maps to 400', async () => {
172-
// This is the case a blanket "any 4xx is expected" predicate would
173-
// wrongly silence: `mapDataError` degrades anything it recognises
174-
// nothing about to an UN-CODED 400, and that is where a real handler
175-
// bug lands. Silencing it would be the mirror-image of #4886.
178+
it('an UNRECOGNISED error (handler bug) stays loud — and is a 500, not a 400 (#5489)', async () => {
179+
// The loudness is what #4886 pinned, and it is unchanged. What moved is
180+
// WHY it is structural: this case used to land on `mapDataError`'s
181+
// un-coded 400 fallback, so the guard read "loud even though it maps to
182+
// 400" and its adversary was a predicate widened to "any 4xx is
183+
// expected". #5489 made that fallback a sanitised 500
184+
// (`UNCLASSIFIED_FAULT`) because a handler bug is not the caller's
185+
// fault and an SDK must not read "do not retry" off it. 500 is outside
186+
// `isExpectedDataStatus` entirely, so the log line no longer depends on
187+
// the predicate staying narrow in the 4xx band.
176188
const bug = new TypeError('Cannot read properties of undefined (reading \'name\')');
177189
const { rest } = setup({ getMetaItem: vi.fn().mockRejectedValue(bug) });
178190

179191
const res = await callMetaItem(rest, { type: 'object', name: 'showcase_account' });
180192

181193
expect(unhandledLogs()).toHaveLength(1);
182194
expect(unhandledLogs()[0][1]).toBe(bug);
183-
expect(res.statusCode).toBe(400);
184-
expect(res.body?.code).toBeUndefined();
195+
expect(res.statusCode).toBe(500);
196+
expect(res.body?.code).toBe('INTERNAL_ERROR');
197+
// The bug's own words are the operator's, not the client's — and the
198+
// log line above is where they went.
199+
expect(JSON.stringify(res.body)).not.toContain('Cannot read properties');
185200
});
186201
});
187202

0 commit comments

Comments
 (0)