Skip to content

Commit 2570ab0

Browse files
Elon Muskclaude
andauthored
fix(objectql): the primary key is never a __search companion source (#10290) (#10459)
* fix(objectql): the primary key is never a `__search` companion source (#10290) * test(objectql,pinyin-search): negative + positive controls for the PK companion source (#10290) * chore: changeset for the primary-key companion refusal (#10290) --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent a718ee3 commit 2570ab0

6 files changed

Lines changed: 388 additions & 0 deletions

File tree

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
The `__search` companion is no longer provisioned or backfilled on objects whose only companion source is the primary key (#10290)
6+
7+
`resolveSearchCompanionSources` resolves the companion's source through
8+
ADR-0079's `resolveDisplayField`. That derivation ends at "first title-eligible
9+
field by declaration order", and on a table whose only text column IS its
10+
primary key — system tables, junction tables, append-only logs — it lands on
11+
`id`. `id` is `type: 'text'`, not hidden and carries no `requiredPermissions`,
12+
so it passed the eligibility gate: `provisionSearchCompanion` declared a
13+
`__search` column on those objects and `plugin-pinyin-search`'s backfill walked
14+
them at every boot.
15+
16+
That work is doomed by construction rather than merely unlikely. Both writers —
17+
the `beforeInsert`/`beforeUpdate` stamp and the boot backfill — gate on
18+
`containsCJK(row[source])`, and a platform-generated primary key is ASCII by
19+
construction, so the predicate can never be true. Measured on a real
20+
`bootStack` of `examples/app-showcase`: **20 of the 66 objects** the backfill
21+
enumerated were in this state, walking whole platform tables to compute nothing
22+
`sys_secret`, `sys_oauth_access_token` and `sys_jwks` among them.
23+
24+
`resolveSearchCompanionSources` now returns `[]` when the resolved display
25+
field is the record's primary key, and `isPrimaryKeyField` is exported as the
26+
named judgement behind it.
27+
28+
**Keyed on the field's ROLE, not on "resolved by fallback".** The registry's
29+
materialization seam runs `provisionPrimary(schema, { synthesize: false })`
30+
before this module — a contractual order — and that pass writes `nameField:
31+
'id'` onto the document, so by the time provisioning asks, a derived fallback
32+
and an author's explicit pointer are byte-identical. The role is readable from
33+
the name because that is where the platform keeps it: the driver provisions
34+
`id` on every physical table unconditionally and there is no per-field
35+
`primaryKey` marker in the spec, which is why `isPreservableUnderAudit` already
36+
keys on `SystemFieldName.ID` for the same reason. `_id` is refused as the
37+
alternate spelling of the same address.
38+
39+
**This interprets ADR-0079, it does not amend it.** The title contract is
40+
untouched: `resolveDisplayField` still resolves `id`, `provisionPrimary` still
41+
designates it, and `resolveRecordDisplayName` still renders the `Record #<id>`
42+
floor. Only the search normalizer declines to take its input from there — the
43+
same distinction #4483 drew one seam over on the READ path, where the display
44+
field's job in the `$search` auto-default is to ORDER the set and never to
45+
ADMIT a field the exclusions already rejected (`SEARCH_AUTO_EXCLUDED_FIELDS`
46+
names `id` and `_id`).
47+
48+
**What does not change.** Existing permanently-NULL `__search` columns on
49+
already-migrated tables stay: ADR-0045 migrations are additive and dropping a
50+
physical column is a separate decision. Those deployments still stop walking —
51+
the backfill skips an object whose sources resolve empty even when its schema
52+
still declares the column. Objects with a real name/title field are unaffected:
53+
provisioning, write-time stamping and the query-time `$or` clause all behave
54+
exactly as before, including when the object also declares an `id` field and
55+
when its display field is a plain text column that is not named `name`/`title`.

packages/objectql/src/core.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ export {
3131
provisionSearchCompanion,
3232
resolveSearchCompanionSources,
3333
isCompanionSourceEligible,
34+
isPrimaryKeyField,
3435
isCompanionMatchableTerm,
3536
isSearchCompanionRequested,
3637
stripSearchCompanion,

packages/objectql/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ export {
4949
provisionSearchCompanion,
5050
resolveSearchCompanionSources,
5151
isCompanionSourceEligible,
52+
isPrimaryKeyField,
5253
isCompanionMatchableTerm,
5354
isSearchCompanionRequested,
5455
stripSearchCompanion,

packages/objectql/src/search-companion.test.ts

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,14 @@
77

88
import { describe, it, expect } from 'vitest';
99
import type { ServiceObject } from '@objectstack/spec/data';
10+
import { resolveDisplayField } from '@objectstack/spec/data';
1011
import {
1112
SEARCH_COMPANION_FIELD,
1213
provisionSearchCompanion,
1314
resolveSearchCompanionSources,
1415
isCompanionSourceEligible,
1516
isCompanionMatchableTerm,
17+
isPrimaryKeyField,
1618
containsCJK,
1719
} from './search-companion';
1820
import { expandSearchToFilter, resolveSearchFields } from './search-filter';
@@ -195,3 +197,141 @@ describe('containsCJK / isCompanionMatchableTerm', () => {
195197
expect(isCompanionMatchableTerm('123')).toBe(false);
196198
});
197199
});
200+
201+
// ─────────────────────────────────────────────────────────────────────────────
202+
// [#10290] The primary key is never a companion source.
203+
//
204+
// The two blocks below are a matched pair and are meant to be read together:
205+
// the first is the NEGATIVE control (the defect — it fails on the pre-fix
206+
// source), the second is the POSITIVE control (the capability survives — it
207+
// fails if the refusal over-reaches).
208+
// ─────────────────────────────────────────────────────────────────────────────
209+
210+
/**
211+
* The measured shape: a platform table whose only title-eligible column IS its
212+
* primary key. `id` is declared first, so ADR-0079's derivation reaches tier 3
213+
* ("first title-eligible field by declaration order") and lands on it — which
214+
* is how `sys_jwks`, `sys_secret`, `sys_oauth_access_token` and 17 more came to
215+
* carry a companion column their values can never fill.
216+
*/
217+
const pkOnly = (): any => ({
218+
name: 'sys_jwks',
219+
fields: {
220+
id: { type: 'text', label: 'ID', readonly: true },
221+
created_at: { type: 'datetime' },
222+
expires_at: { type: 'datetime' },
223+
},
224+
});
225+
226+
/**
227+
* The shape the registry ACTUALLY hands this seam. `materializeBaseLayer` runs
228+
* `provisionPrimary(schema, { synthesize: false })` first — contractual order —
229+
* so the derived fallback has already been written down as an explicit
230+
* `nameField` pointer by the time provisioning asks. A refusal keyed on "the
231+
* display field resolved by fallback" could not see any difference here, which
232+
* is why the refusal is keyed on the field's role instead.
233+
*/
234+
const pkOnlyDesignated = (): any => ({ ...pkOnly(), nameField: 'id' });
235+
236+
describe('[#10290] negative control — the primary key is never a companion source', () => {
237+
it('refuses a tier-3 derived display field that is the primary key', () => {
238+
expect(resolveDisplayField(pkOnly())).toBe('id'); // ADR-0079 is unchanged…
239+
expect(resolveSearchCompanionSources(pkOnly())).toEqual([]); // …the companion declines it
240+
});
241+
242+
it('refuses it just the same once `provisionPrimary` has written it down as `nameField`', () => {
243+
expect(resolveSearchCompanionSources(pkOnlyDesignated())).toEqual([]);
244+
});
245+
246+
it('refuses the `_id` spelling of the same address', () => {
247+
expect(resolveSearchCompanionSources({
248+
name: 'sys_legacy',
249+
nameField: '_id',
250+
fields: { _id: { type: 'text' }, created_at: { type: 'datetime' } },
251+
})).toEqual([]);
252+
});
253+
254+
it('declares no `__search` column on such an object (returns it by reference)', () => {
255+
const before = pkOnly();
256+
expect(provisionSearchCompanion(before)).toBe(before);
257+
expect(before.fields[SEARCH_COMPANION_FIELD]).toBeUndefined();
258+
259+
const designated = pkOnlyDesignated();
260+
expect(provisionSearchCompanion(designated)).toBe(designated);
261+
expect(designated.fields[SEARCH_COMPANION_FIELD]).toBeUndefined();
262+
});
263+
264+
it('SchemaRegistry: registering one provisions no companion, and ADR-0079 still designates the title', () => {
265+
const registry = new SchemaRegistry({ multiTenant: false, searchCompanion: true });
266+
registry.registerObject(pkOnly(), 'test-pkg', 'sys');
267+
const schema = registry.getObject('sys_jwks')!;
268+
expect(schema.fields![SEARCH_COMPANION_FIELD]).toBeUndefined();
269+
// The title contract is INTERPRETED, not amended: the pointer is still set.
270+
expect((schema as any).nameField).toBe('id');
271+
});
272+
273+
it('isPrimaryKeyField answers for the address spellings only', () => {
274+
expect(isPrimaryKeyField('id')).toBe(true);
275+
expect(isPrimaryKeyField('_id')).toBe(true);
276+
expect(isPrimaryKeyField(undefined)).toBe(false);
277+
expect(isPrimaryKeyField('')).toBe(false);
278+
});
279+
});
280+
281+
describe('[#10290] positive control — objects with a real title keep the companion', () => {
282+
/** A contact as the platform really registers it: `id` present, `name` too. */
283+
const contactWithId = (): any => ({
284+
name: 'crm_contact',
285+
fields: {
286+
id: { type: 'text', label: 'ID', readonly: true },
287+
name: { type: 'text', label: 'Name' },
288+
email: { type: 'email' },
289+
},
290+
});
291+
292+
it('an object carrying an `id` field still gets its companion from the name field', () => {
293+
expect(resolveSearchCompanionSources(contactWithId())).toEqual(['name']);
294+
const out = provisionSearchCompanion(contactWithId());
295+
expect(out.fields[SEARCH_COMPANION_FIELD]).toBeDefined();
296+
});
297+
298+
it('a plain text display field that is not named `name`/`title` is still accepted', () => {
299+
// Explicit pointer …
300+
expect(resolveSearchCompanionSources({
301+
name: 'crm_ticket',
302+
nameField: 'subject',
303+
fields: { id: { type: 'text' }, subject: { type: 'text' } },
304+
})).toEqual(['subject']);
305+
// … and a tier-3 DERIVED text column (the same derivation path the negative
306+
// control exercises), so the refusal cannot be "all `type: 'text'` sources".
307+
expect(resolveSearchCompanionSources({
308+
name: 'crm_code',
309+
fields: { code: { type: 'text' }, note: { type: 'textarea' } },
310+
})).toEqual(['code']);
311+
});
312+
313+
it('a field whose NAME merely contains "id" is not the address', () => {
314+
for (const fname of ['identifier', 'id_card', 'bid', 'valid_name', 'external_id_label']) {
315+
expect(isPrimaryKeyField(fname)).toBe(false);
316+
expect(resolveSearchCompanionSources({
317+
name: 'crm_thing',
318+
nameField: fname,
319+
fields: { [fname]: { type: 'text' } },
320+
})).toEqual([fname]);
321+
}
322+
});
323+
324+
it('SchemaRegistry: a titled object is still provisioned end-to-end', () => {
325+
const registry = new SchemaRegistry({ multiTenant: false, searchCompanion: true });
326+
registry.registerObject(contactWithId(), 'test-pkg', 'crm');
327+
const schema = registry.getObject('crm_contact')!;
328+
expect(schema.fields![SEARCH_COMPANION_FIELD]).toBeDefined();
329+
expect((schema.fields![SEARCH_COMPANION_FIELD] as any).hidden).toBe(true);
330+
});
331+
332+
it('the query-time OR clause still fires for a companion-bearing object', () => {
333+
const fields = provisionSearchCompanion(contactWithId()).fields as any;
334+
const filter = expandSearchToFilter('zhangwei', { fields });
335+
expect(filter.$or).toContainEqual({ [SEARCH_COMPANION_FIELD]: { $contains: 'zhangwei' } });
336+
});
337+
});

packages/objectql/src/search-companion.ts

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
*/
4242

4343
import { resolveDisplayField, TITLE_ELIGIBLE_TYPES } from '@objectstack/spec/data';
44+
import { SystemFieldName } from '@objectstack/spec/system';
4445

4546
/**
4647
* Name of the hidden companion column. Double-underscore prefixed so it can
@@ -92,6 +93,34 @@ function isCompanionSourceType(type: string | undefined): boolean {
9293
return TITLE_ELIGIBLE_TYPES.has(type);
9394
}
9495

96+
/**
97+
* [#10290] Field names that carry the record's PRIMARY KEY — its address —
98+
* rather than any human-authored text.
99+
*
100+
* Keyed on the NAME because that is where the role lives: the driver
101+
* provisions `id` on every physical table unconditionally
102+
* (`resolveInjectedSystemColumns` reports it even under `systemFields: false`),
103+
* and there is no per-field `primaryKey` marker in the spec for a def-based
104+
* test to read — the same reasoning `isPreservableUnderAudit`
105+
* (`validation/rule-validator.ts`) already keys on {@link SystemFieldName.ID}.
106+
* `_id` is the alternate spelling of the same address: `resolveRecordDisplayName`
107+
* reads `record.id ?? record._id`, and the read path's own exclusion set
108+
* (`SEARCH_AUTO_EXCLUDED_FIELDS`, `@objectstack/spec/data`) names both, for
109+
* this refusal, one seam over.
110+
*/
111+
const RECORD_ADDRESS_FIELD_NAMES: ReadonlySet<string> = new Set<string>([SystemFieldName.ID, '_id']);
112+
113+
/**
114+
* [#10290] Is `fieldName` the object's primary key?
115+
*
116+
* Exported so the refusal in {@link resolveSearchCompanionSources} is one
117+
* named judgement a caller can ask about, rather than a literal buried in a
118+
* conditional.
119+
*/
120+
export function isPrimaryKeyField(fieldName: string | undefined | null): boolean {
121+
return typeof fieldName === 'string' && RECORD_ADDRESS_FIELD_NAMES.has(fieldName);
122+
}
123+
95124
/**
96125
* May `fieldMeta` feed the companion column?
97126
*
@@ -119,11 +148,62 @@ export function isCompanionSourceEligible(fieldMeta: CompanionFieldMeta | undefi
119148
* ({@link provisionSearchCompanion}) and the `plugin-pinyin-search` populate
120149
* hook — deriving both from the same function means there is no stored
121150
* mapping to drift.
151+
*
152+
* ## [#10290] The PRIMARY KEY is never a source
153+
*
154+
* ADR-0079's derivation ends at "first title-eligible field by declaration
155+
* order", and on a table whose only text column IS its primary key — system
156+
* tables, junction tables, append-only logs — that is `id`. `id` is
157+
* `type: 'text'`, not hidden and carries no `requiredPermissions`, so it
158+
* passed {@link isCompanionSourceEligible} and the companion was provisioned
159+
* on it.
160+
*
161+
* The work that follows is doomed by construction rather than merely
162+
* unlikely. The companion stores search-normalized forms of CJK text and
163+
* nothing else: both writers gate on {@link containsCJK} of the source value
164+
* (the `beforeInsert`/`beforeUpdate` stamp and the boot backfill). A primary
165+
* key is a platform-generated identifier, ASCII by construction, so
166+
* `containsCJK` can never be true — the boot backfill's keyset walk over such
167+
* an object scans a whole platform table to compute nothing, forever.
168+
* Measured on a real `bootStack` of `examples/app-showcase`: **20 of the 66
169+
* objects** the backfill enumerated were in this state, `sys_secret`,
170+
* `sys_oauth_access_token` and `sys_jwks` among them.
171+
*
172+
* ⛔ The refusal is keyed on the field's ROLE, NOT on "the display field
173+
* resolved by fallback", and that is forced rather than chosen. The registry's
174+
* materialization seam (`registry.ts` `materializeBaseLayer`) runs
175+
* `provisionPrimary(schema, { synthesize: false })` BEFORE this module — a
176+
* contractual order — and that pass WRITES `nameField: 'id'` onto the
177+
* document. By the time provisioning asks this function, a derived fallback
178+
* and an author's explicit pointer are byte-identical, so "was this a
179+
* fallback?" is not a question this seam can answer. It does not need to: the
180+
* values cannot qualify either way.
181+
*
182+
* This INTERPRETS ADR-0079, it does not amend it. The title contract is
183+
* untouched — `resolveDisplayField` still resolves `id`, `provisionPrimary`
184+
* still designates it, and `resolveRecordDisplayName` still renders the
185+
* `Record #<id>` floor. What changes is only that the search normalizer
186+
* declines to take its input from there. That is the same distinction #4483
187+
* drew on the READ path one seam over: the display field's job in the
188+
* `$search` auto-default is to ORDER the set, never to ADMIT a field the
189+
* exclusions already rejected (`SEARCH_AUTO_EXCLUDED_FIELDS`, which names `id`
190+
* and `_id`). This is that rule on the WRITE/provisioning path.
191+
*
192+
* Existing permanently-NULL `__search` columns on already-migrated tables are
193+
* deliberately left in place: ADR-0045 migrations are additive, and dropping a
194+
* physical column is a separate decision. What stops here is new provisioning
195+
* and the per-boot walk — the backfill skips an object whose sources resolve
196+
* empty, and {@link provisionSearchCompanion} no longer declares the column at
197+
* all, so `plugin-pinyin-search`'s own `fields[SEARCH_COMPANION_FIELD]`
198+
* early-out closes the walk one guard earlier still.
122199
*/
123200
export function resolveSearchCompanionSources(schema: CompanionObjectMeta | undefined | null): string[] {
124201
if (!schema?.fields) return [];
125202
const display = resolveDisplayField(schema as any);
126203
if (!display) return [];
204+
// [#10290] Before the metadata gate: the primary key's METADATA is a
205+
// perfectly ordinary readable text column, so only its name can refuse it.
206+
if (isPrimaryKeyField(display)) return [];
127207
const meta = schema.fields[display];
128208
return isCompanionSourceEligible(meta) ? [display] : [];
129209
}

0 commit comments

Comments
 (0)