diff --git a/.changeset/24284-search-all-internal-skip.md b/.changeset/24284-search-all-internal-skip.md new file mode 100644 index 00000000000..ed9563af5e9 --- /dev/null +++ b/.changeset/24284-search-all-internal-skip.md @@ -0,0 +1,9 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +fix(metadata-protocol): global search judges an object's searchable set without its `internal: true` fields, the set the engine scans + +Global search (`searchAll`, `GET /api/v1/search`) passes over an object with nothing to scan before it queries. It judged that from the object's fields including `internal: true` ones, which the engine never searches. An object whose only text field is internal therefore passed the skip and reached the engine, which now refuses a search over an empty set, so the whole sweep would have failed. The skip now leaves internal fields out, so such an object is passed over and the sweep answers `200` with the other objects' hits. + +What to do. Nothing. diff --git a/.changeset/24284-search-empty-set-refused.md b/.changeset/24284-search-empty-set-refused.md new file mode 100644 index 00000000000..19d91b164b5 --- /dev/null +++ b/.changeset/24284-search-empty-set-refused.md @@ -0,0 +1,16 @@ +--- +'@objectstack/objectql': minor +--- + +feat(objectql)!: a `search` over an object with no searchable field is refused with `400 INVALID_FIELD` instead of answering every row + +Clause-②: no (narrowing) + + + +A search term is matched against the object's resolved search fields: its `searchableFields` declaration (entries that name a stored field), or else the default set (the name/title field and the text, email, phone, url, autonumber, textarea, markdown and select fields). On an object where that set is empty, such as a junction object made of two lookups and a formula title, the term used to add no filter, so `?search=zzzz-nothing` and `?search=Dune` returned the same rows, and a user read every row as a match. + +- **What changes.** `find`, `findOne`, `count` and `aggregate` answer a non-blank `search` over an empty resolved set with `400 INVALID_FIELD` (`param: 'search'`). That covers every door that reaches the engine: the REST list and query routes (and the list's `total`), the grouped read, the dispatcher, the SDK and a hook's `ctx.api`. The message names the object, says why the set is empty, and gives the two remedies. +- **Unchanged.** A blank or whitespace term is not a search and still answers every row. An object with a searchable field filters as before. An explicit `searchFields` naming a field outside the set keeps its own `400 INVALID_FIELD` at the data door. Global search (`GET /api/v1/search`) still passes over an object with nothing to scan and answers `200`. + +What to do. If a read now answers `400 INVALID_FIELD` with "has no searchable field", declare `searchableFields` on that object naming a stored field, or mirror the composed title onto a stored text field and search that. A client that offers a search box on such an object should hide it, or show the refusal. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index a36457b9291..8a1b5360a43 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -15847,8 +15847,21 @@ export class ObjectStackProtocolImplementation implements // `search` instead of a pre-built `where` routes through it anyway — // and leaves ONE producer of search clauses rather than two callers // of one helper, which is the stronger form of the same fix. + // + // [#24284] …and the cliff in (1) is now a REFUSAL one call down: + // the engine answers a non-blank term over an empty resolved set + // with `400 INVALID_FIELD` rather than every row. The skip stays, + // and it is what keeps the palette `200` — the sweep passes over an + // object with nothing to scan instead of failing on it, since this + // method chose the object, not the caller. So the set judged here + // must be the set the engine resolves: the `internal: true` columns + // it never scans are left out of it too, through the same flag + // reader the #22646 ingress gate uses. Without that, an object + // whose only text column is internal would pass this skip, reach + // `find`, and fail the whole sweep. + const internalFields = new Set(collectInternalWriteResponseFields(obj)); const fieldMetaByName: Record = {}; - for (const f of fields) if (f?.name) fieldMetaByName[f.name] = f; + for (const f of fields) if (f?.name && !internalFields.has(f.name)) fieldMetaByName[f.name] = f; const { allowed: declaredSearchFields } = resolveSearchFieldResolution({ fields: fieldMetaByName, searchableFields: obj.searchableFields, diff --git a/packages/objectql/src/engine-search-empty-set.test.ts b/packages/objectql/src/engine-search-empty-set.test.ts new file mode 100644 index 00000000000..575243eb1d9 --- /dev/null +++ b/packages/objectql/src/engine-search-empty-set.test.ts @@ -0,0 +1,240 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#24284] A non-blank `search` over an object whose RESOLVED search set is + * empty is refused — `400 INVALID_FIELD`, on every verb that reaches + * `expandSearchOnAst` — instead of answering every row. + * + * The shape is the junction object: two lookups and a composed formula title. + * The auto-default scans textual and enum types only, so it resolves no field, + * `expandSearchToFilter` answers `null`, and before this refusal the term was + * dropped with `where` untouched: `search: 'zzzz-nothing'` and `search: 'Dune'` + * returned the same five rows, read by a user as "everything matched". + * + * What stays as it was, pinned beside the refusal so a fix that over-reaches + * goes red here too: a blank term is not a search; a control object with a + * stored text field still filters; an explicit override intersected to nothing + * on an object WITH a searchable set falls back to that set (filtered, never + * every row). + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import type { EngineQueryOptions } from '@objectstack/spec/data'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { ObjectQL } from './engine.js'; + +type Search = EngineQueryOptions['search']; + +type Row = Record; + +const LOANS: Row[] = [ + { id: 'l1', book: 'b1', reader: 'r1' }, + { id: 'l2', book: 'b2', reader: 'r1' }, + { id: 'l3', book: 'b3', reader: 'r1' }, + { id: 'l4', book: 'b1', reader: 'r1' }, + { id: 'l5', book: 'b2', reader: 'r1' }, +]; +const BOOKS: Row[] = [ + { id: 'b1', title: 'Dune' }, + { id: 'b2', title: 'Emma' }, + { id: 'b3', title: 'Ulysses' }, +]; + +function matches(row: Row, where: unknown): boolean { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where as Row)) { + if (k === '$and') { if (!(v as unknown[]).every((w) => matches(row, w))) return false; continue; } + if (k === '$or') { if (!(v as unknown[]).some((w) => matches(row, w))) return false; continue; } + const op = v as Row; + if (op && typeof op === 'object' && '$icontains' in op) { + if (!String(row[k] ?? '').toLowerCase().includes(String(op.$icontains).toLowerCase())) return false; + continue; + } + if (row[k] !== v) return false; + } + return true; +} + +function makeDriver() { + const reads: Array<{ object: string; via: string }> = []; + const tables: Record = { lib_loan: LOANS, lib_book: BOOKS, lib_token: [{ id: 't1', digest: 'abc' }] }; + const read = (object: string, ast: any) => { + const rows = (tables[object] ?? []).filter((r) => matches(r, ast?.where)); + return typeof ast?.limit === 'number' ? rows.slice(0, ast.limit) : rows; + }; + const driver: any = { + name: 'mem', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(object: string, ast: any) { reads.push({ object, via: 'find' }); return read(object, ast).map((r) => ({ ...r })); }, + async findOne(object: string, ast: any) { reads.push({ object, via: 'findOne' }); return read(object, ast)[0] ?? null; }, + async count(object: string, ast: any) { reads.push({ object, via: 'count' }); return read(object, ast).length; }, + async create(_o: string, d: any) { return d; }, + async update(_o: string, _id: string, d: any) { return d; }, + async delete() { return true; }, + async bulkCreate(_o: string, r: any[]) { return r; }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, reads }; +} + +const REFUSAL = { + code: 'INVALID_FIELD', + status: 400, + object: 'lib_loan', + param: 'search', +}; + +describe('[#24284] a search over an empty resolved search set is refused', () => { + let engine: ObjectQL; + let reads: ReturnType['reads']; + + beforeEach(async () => { + const made = makeDriver(); + reads = made.reads; + engine = new ObjectQL(); + engine.registerDriver(made.driver, true); + await engine.init(); + const registry = engine.registry as any; + registry.registerObject({ name: 'lib_book', fields: { title: { type: 'text' } } }, 'test'); + registry.registerObject({ + name: 'lib_loan', + nameField: 'title', + fields: { + book: { type: 'lookup', reference: 'lib_book' }, + reader: { type: 'lookup', reference: 'lib_reader' }, + title: { type: 'formula', expression: "record.book.title + ' · ' + record.reader.name" }, + }, + }, 'test'); + // A textual column that is `internal: true` — excluded from the scan, + // so the resolved set is empty for the same reason by another road. + registry.registerObject({ + name: 'lib_token', + fields: { digest: { type: 'text', internal: true } }, + }, 'test'); + }); + + const verbs: Array<[string, (e: ObjectQL, search: Search) => Promise]> = [ + ['find', (e, search) => e.find('lib_loan', { search })], + ['findOne', (e, search) => e.findOne('lib_loan', { search })], + ['count', (e, search) => e.count('lib_loan', { search })], + ['aggregate', (e, search) => e.aggregate('lib_loan', { + search, groupBy: ['book'], aggregations: [{ function: 'count', alias: 'n' }], + })], + ]; + + it.each(verbs)('%s answers 400 INVALID_FIELD naming the object and both remedies — and reads nothing', async (_verb, run) => { + for (const term of ['zzzz-nothing', 'Dune', { query: 'Dune' }] as Search[]) { + const err: any = await run(engine, term).then(() => null, (e) => e); + expect(err, `term ${JSON.stringify(term)} was answered`).not.toBeNull(); + expect(err).toMatchObject(REFUSAL); + expect(String(err.message)).toMatch(/^Object 'lib_loan' has no searchable field, so 'search' cannot be honoured on it/); + expect(String(err.message)).toContain("it declares no 'searchableFields'"); + expect(String(err.message)).toContain("Declare 'searchableFields' on 'lib_loan' naming a stored field"); + expect(String(err.message)).toContain('mirror the composed title onto a stored text field'); + } + expect(reads.filter((r) => r.object === 'lib_loan')).toEqual([]); + }); + + it('an explicit searchFields narrowing does not rescue it — the allowed set is empty, so the fallback is too', async () => { + for (const searchFields of [['title'], ['book'], ['nope'], []]) { + const err: any = await engine.find('lib_loan', { search: 'Dune', searchFields }).then(() => null, (e) => e); + expect(err, `searchFields ${JSON.stringify(searchFields)}`).toMatchObject(REFUSAL); + } + }); + + it('a declaration naming only unscannable entries names them in the refusal', async () => { + (engine.registry as any).registerObject({ + name: 'lib_loan', + searchableFields: ['title', 'gone'], + fields: { + book: { type: 'lookup', reference: 'lib_book' }, + title: { type: 'formula', expression: 'record.book.title' }, + }, + }, 'test'); + const err: any = await engine.find('lib_loan', { search: 'Dune' }).then(() => null, (e) => e); + expect(err).toMatchObject(REFUSAL); + expect(String(err.message)).toContain("its 'searchableFields' (title, gone) names no stored field"); + }); + + it('an object whose only text column is internal: true is refused the same way, saying why', async () => { + const err: any = await engine.find('lib_token', { search: 'abc' }).then(() => null, (e) => e); + expect(err).toMatchObject({ ...REFUSAL, object: 'lib_token' }); + expect(String(err.message)).toContain("('internal: true' fields are never searched)"); + expect(reads.filter((r) => r.object === 'lib_token')).toEqual([]); + }); + + it('a blank or whitespace term is not a search: every row, where untouched, on every verb', async () => { + for (const search of ['', ' ', { query: ' ' }] as Search[]) { + expect(await engine.find('lib_loan', { search })).toHaveLength(5); + expect(await engine.count('lib_loan', { search })).toBe(5); + expect(await engine.find('lib_loan', { search, where: { book: 'b1' } })).toHaveLength(2); + } + }); + + it('control: an object with a stored text field still filters, and an override intersected to nothing falls back to its set', async () => { + expect((await engine.find('lib_book', { search: 'Dune' })).map((r) => r.id)).toEqual(['b1']); + expect(await engine.find('lib_book', { search: 'zzzz-nothing' })).toEqual([]); + expect(await engine.count('lib_book', { search: 'zzzz-nothing' })).toBe(0); + // A direct engine caller naming only fields outside the allowed set: the + // tolerant intersection falls back to the allowed set — a FILTERED + // answer, never every row. + expect((await engine.find('lib_book', { search: 'emma', searchFields: ['nope'] })).map((r) => r.id)).toEqual(['b2']); + }); +}); + +/** + * The internal producers and the data door, over a real engine: `searchAll` + * (the ⌘K palette) hands `search` to objects it did not choose, so it must + * keep skipping an object with nothing to scan BEFORE it queries — a refusal + * there would fail the whole sweep. `findData` (the REST list / query door) + * surfaces the refusal as the read's own answer. + */ +describe('[#24284] the palette keeps its skip; the data door surfaces the refusal', () => { + async function makeHarness() { + const made = makeDriver(); + const engine = new ObjectQL(); + engine.registerDriver(made.driver, true); + await engine.init(); + const registry = engine.registry as any; + registry.registerObject({ name: 'lib_book', fields: { title: { type: 'text' } } }, 'test'); + registry.registerObject({ + name: 'lib_loan', + nameField: 'title', + fields: { + book: { type: 'lookup', reference: 'lib_book' }, + reader: { type: 'lookup', reference: 'lib_reader' }, + title: { type: 'formula', expression: "record.book.title + ' · ' + record.reader.name" }, + }, + }, 'test'); + registry.registerObject({ name: 'lib_token', fields: { digest: { type: 'text', internal: true } } }, 'test'); + const protocol = new ObjectStackProtocolImplementation(engine as never); + return { protocol, reads: made.reads }; + } + + it('searchAll answers the other objects\' hits, and leaves the junction and the internal-only object out of objectsScanned', async () => { + const { protocol, reads } = await makeHarness(); + const objects = ['lib_book', 'lib_loan', 'lib_token']; + const res: any = await protocol.searchAll({ q: 'Dune', objects, perObject: 5, limit: 25 }); + expect(res.hits.map((h: any) => [h.object, h.id])).toEqual([['lib_book', 'b1']]); + expect(res.totalObjects).toBe(1); + expect(reads.filter((r) => r.object !== 'lib_book')).toEqual([]); + const none: any = await protocol.searchAll({ q: 'Dune', objects: ['lib_loan', 'lib_token'], perObject: 5, limit: 25 }); + expect(none).toMatchObject({ hits: [], totalObjects: 0, totalHits: 0 }); + }); + + it('findData over the junction answers the refusal, on the list and the query shapes', async () => { + const { protocol, reads } = await makeHarness(); + for (const query of [{ search: 'zzzz-nothing' }, { search: 'Dune', searchFields: '' }, { search: { query: 'Dune' } }]) { + const err: any = await (protocol as any).findData({ object: 'lib_loan', query }).then(() => null, (e: any) => e); + expect(err, JSON.stringify(query)).toMatchObject(REFUSAL); + } + expect(reads.filter((r) => r.object === 'lib_loan')).toEqual([]); + // …and the control object and the blank term are untouched. + const book: any = await (protocol as any).findData({ object: 'lib_book', query: { search: 'Dune' } }); + expect(book.records.map((r: any) => r.id)).toEqual(['b1']); + const blank: any = await (protocol as any).findData({ object: 'lib_loan', query: { search: '' } }); + expect(blank.records).toHaveLength(5); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index f1f881c5134..2d3bb84abc7 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -265,7 +265,7 @@ import { assertGroupByNamesNoJsonStoredField } from './group-by-structured-json- import { assertAggregationFieldTypesAccepted } from './aggregate-field-type-door.js'; import { applyProtection, pluralToSingular, ExternalWriteForbiddenError } from '@objectstack/spec/shared'; import { SchemaRegistry, computeFQN, type ArtifactInstallScope } from './registry.js'; -import { expandSearchToFilter } from './search-filter.js'; +import { expandSearchToFilter, normalizeSearch, SEARCHABLE_ENUM_TYPES, SEARCHABLE_TEXTUAL_TYPES } from './search-filter.js'; import { isSearchCompanionRequested, stripSearchCompanion } from './search-companion.js'; import { ExpressionEngine, toEvalPermissions, type EvalError, type EvalPermissions } from '@objectstack/formula'; import type { Expression } from '@objectstack/spec'; @@ -2015,6 +2015,42 @@ function assertProjectionHasNoDottedPaths( throw err; } +/** + * [#24284] The refusal for a non-blank `search` over an object whose RESOLVED + * search set is empty — no `searchableFields` entry naming a stored field, and + * no field of a type the auto-default scans (the junction shape: lookups plus + * a composed formula title). Without it the expansion added no predicate and + * the read answered every row. + * + * `INVALID_FIELD`, the code #4254's explicit-override gate answers on this + * same axis (and the one a search naming a formula title gets), not + * `INVALID_QUERY`: the request is well-formed, and what cannot be honoured is + * a fact about the object's FIELDS. 400 on every door, and the same answer for + * every caller — it is derived from the declaration, never from the caller's + * permissions (a narrowing by field-level security happens later, in the + * predicate guard, and `searchAll` hands the engine only the queryable subset). + */ +function emptySearchSetError(object: string, schema: any, internalExcluded: boolean): Error { + const declared: unknown[] = Array.isArray(schema?.searchableFields) ? schema.searchableFields : []; + const err: any = new Error( + `Object '${object}' has no searchable field, so 'search' cannot be honoured on it` + + (declared.length > 0 + ? `: its 'searchableFields' (${declared.join(', ')}) names no stored field` + : `: it declares no 'searchableFields', and none of its fields is a type the default ` + + `search scans (${[...SEARCHABLE_TEXTUAL_TYPES, ...SEARCHABLE_ENUM_TYPES].join(' / ')})`) + + (internalExcluded ? " ('internal: true' fields are never searched)" : '') + + '. Answering every row instead would read as "everything matched". Declare ' + + `'searchableFields' on '${object}' naming a stored field, or mirror the composed title ` + + `onto a stored text field and search that.`, + ); + err.status = 400; + err.httpStatus = 400; + err.code = 'INVALID_FIELD'; + err.object = object; + err.param = 'search'; + return err; +} + /** * [#8682 / #8738] The DECLARED-FIELD DOOR on the WRITE paths — `insert()` and, * since #8738, `update()`: a key the object does not declare is refused by the @@ -13729,6 +13765,20 @@ export class ObjectQL implements IObjectQLEngine { }); if (searchFilter) { ast.where = ast.where ? { $and: [ast.where, searchFilter] } : searchFilter; + } else if (normalizeSearch(raw).query.trim()) { + // [#24284] A non-blank term that expanded to nothing means the RESOLVED + // search set is empty: `expandSearchToFilter` answers `null` for + // exactly two reasons, and the other one (a blank term) is excluded + // here. Leaving `where` untouched would answer the search with EVERY + // row, indistinguishable from "everything matched" — the silent + // widening #4254 refuses on the explicit-override axis. Judged on the + // set the expansion itself resolved (no second resolver), so it holds + // for every verb that reaches here (`find` / `findOne` / `aggregate` / + // the count) and every door (REST, dispatcher, SDK, a hook's + // `ctx.api`). An explicit override intersected to nothing falls back + // to the allowed set inside `resolveSearchFields`, so it reaches this + // only when that set is empty too. + throw emptySearchSetError(ast.object, schema, internalFields.length > 0); } } delete (ast as any).search; diff --git a/packages/objectql/src/search-filter.ts b/packages/objectql/src/search-filter.ts index 5a2afee3709..eea4dd2f2f1 100644 --- a/packages/objectql/src/search-filter.ts +++ b/packages/objectql/src/search-filter.ts @@ -199,9 +199,16 @@ function companionWithinSearchFields(searchFields: readonly string[], opts: Expa /** * Expand a `$search` term into a `{ $or: [...] }` (single term) or - * `{ $and: [{ $or: [...] }, ...] }` (multi-term) filter. Returns `null` when - * there's nothing to search (empty query or no resolvable fields) so the caller - * leaves the existing filter untouched. + * `{ $and: [{ $or: [...] }, ...] }` (multi-term) filter. Returns `null` in + * two cases, and they are NOT the same answer one layer up: + * + * - an empty or whitespace query — not a search at all, so the caller leaves + * the existing filter untouched; + * - [#24284] a real term over an EMPTY resolved field set — a search that + * cannot be honoured. The engine's one caller (`expandSearchOnAst`) refuses + * that with `400 INVALID_FIELD` rather than leaving `where` untouched, which + * would answer the search with every row. This function stays pure and + * non-throwing; the judgement lives at the call site every verb reaches. */ export function expandSearchToFilter(raw: unknown, opts: ExpandSearchOptions): any | null { const { query, fields: requested } = normalizeSearch(raw);