diff --git a/.changeset/search-capability-serveability-predicate.md b/.changeset/search-capability-serveability-predicate.md new file mode 100644 index 0000000000..06a2ebe34e --- /dev/null +++ b/.changeset/search-capability-serveability-predicate.md @@ -0,0 +1,57 @@ +--- +"@objectstack/metadata-protocol": patch +"@objectstack/rest": patch +"@objectstack/spec": patch +--- + +fix(metadata-protocol,rest,spec): derive `capabilities.search` from what serves `/search`, not from an empty service slot (#7541) + +Every REST host advertised `capabilities.search = { enabled: false }` in +`/discovery` while `GET /api/v1/search?q=…` answered `200` with real hits. This +is Prime Directive #10 inverted: not an advertised endpoint that 404s, but a +live endpoint **no conforming client will ever call**, because the document +whose only job is to say what is available said it was not. + +**Two producers, two unrelated predicates.** The capability bit came from a +registered `search` service slot (`registeredServices.has('search')`), while the +route refused on something else entirely — `registerSearchEndpoints` returns +`501 NOT_IMPLEMENTED` exactly when `typeof protocol.searchAll !== 'function'`. +Nothing in either repository registers that slot (`CORE_SERVICE_PROVIDER` +records this, verified), and the protocol implements `searchAll` +unconditionally, so the two answers were not merely capable of disagreeing — +they disagreed on every host that exists. + +`search` was the last well-known capability still on bare slot presence. Its +neighbours were moved onto serveability with the rule stated in the builder — +*"the predicate is deliberately the SAME one that decides whether the route is +advertised — what we advertise and what we claim cannot disagree"* — most +recently `chunkedUpload` in #5672. This brings `search` onto that footing: **one +predicate, both ends.** + +- `@objectstack/metadata-protocol` — `capabilities.search` is now + `typeof this.searchAll === 'function'`, the route's own refusal predicate. +- `@objectstack/rest` — the `/discovery` producer ANDs that with + `api.enableSearch`, the flag that decides whether this server mounts the route + at all. Exactly the two-layer conjunction `transactionalBatch` already uses + with `api.enableBatch`: the protocol states what it can serve, the server + states what it mounted, and a deployment that opts out reports `false` rather + than promising a 404. Nothing was added to the route itself. + +**`services.search` is unchanged, and deliberately so.** The slot answers a +different question — `CoreServiceName` declares it "Search Engine +(Elastic/Meili)" and `ISearchService` is an index/query contract — so it still +reports *which engine occupies the slot*, while the capability reports *whether +the surface is served*. On an ordinary host those now differ +(`capabilities.search.enabled: true` beside `services.search.status: +'unavailable'`), and both statements are true. So that the two halves of one +document do not read as contradicting each other, `@objectstack/spec` gives the +slot a `REMEDY_DETAIL` sentence — the same treatment `ui` carries for the same +shape (#4146) — which keeps the unchanged "no implementation ships" fact and +adds which question the entry answers. The `status` itself stays +`unavailable`: no engine is registered, and saying otherwise would be the +original defect pointed the other way. + +**Client impact.** A client that gated its search UI on +`capabilities.search.enabled` was hiding a working feature on every deployment; +it now sees `true` wherever the endpoint really serves, and `false` when the +protocol cannot search (route `501`) or the server did not mount it (`404`). diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7ee497373a..bc2ea876ee 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -3477,7 +3477,34 @@ export class ObjectStackProtocolImplementation implements comments: !!this.engine.registry?.getObject?.('sys_comment'), automation: registeredServices.has('automation'), cron: registeredServices.has('job'), - search: registeredServices.has('search'), + // [#7541] Serveability-gated on the protocol's OWN search + // implementation, was slot presence. This is the same predicate the + // route refuses on: `registerSearchEndpoints` + // (packages/rest/src/rest-server.ts) 501s exactly when + // `typeof protocol.searchAll !== 'function'`, so the two ends can no + // longer answer the same question differently — the rule stated at + // the top of this block, applied to the key that was still exempt. + // + // The old predicate was wrong in the direction discovery exists to + // prevent: `searchAll` is implemented by this class unconditionally, + // nothing in either repository registers the `search` slot + // (CORE_SERVICE_PROVIDER records that, verified), so every REST host + // served `GET /api/v1/search` 200 while advertising + // `capabilities.search = false`. A conforming client — one that + // trusts the document instead of probing — skipped a working + // surface. Prime Directive #10 inverted. + // + // `services.search` is deliberately NOT collapsed into this. The + // slot is a distinct question with its own answer: `CoreServiceName` + // declares it "Search Engine (Elastic/Meili)" and `ISearchService` + // is an index/query contract, so `services.search` reports WHICH + // ENGINE occupies the slot while this bit reports WHETHER THE + // SURFACE IS SERVED (`WellKnownCapabilitiesSchema.search`: "whether + // the backend supports full-text search"). They may legitimately + // differ — an empty slot with a served endpoint is today's normal + // host — and `serviceUnavailableMessage('search')` now says so in + // the same document, the way `ui` does for the same shape (#4146). + search: typeof this.searchAll === 'function', export: registeredServices.has('automation') || registeredServices.has('queue'), // [#5672] Serveability-gated, was presence-only. Two reasons, and // the second is the binding one: diff --git a/packages/objectql/src/protocol-discovery.test.ts b/packages/objectql/src/protocol-discovery.test.ts index 0ade020953..4af85770e7 100644 --- a/packages/objectql/src/protocol-discovery.test.ts +++ b/packages/objectql/src/protocol-discovery.test.ts @@ -448,17 +448,24 @@ describe('ObjectStackProtocolImplementation - Dynamic Service Discovery', () => expect(discovery.capabilities).toBeDefined(); // ui is registered but doesn't map to a well-known capability directly expect(discovery.services.ui.enabled).toBe(true); - // All well-known capabilities should be disabled since ui doesn't map to any - // (comments derives from the sys_comment object, which is not registered here). + // The SLOT-DERIVED well-known capabilities should be disabled since ui maps + // to none of them (comments derives from the sys_comment object, which is + // not registered here). expect(discovery.capabilities!.comments).toEqual({ enabled: false }); expect(discovery.capabilities!.automation).toEqual({ enabled: false }); expect(discovery.capabilities!.cron).toEqual({ enabled: false }); - expect(discovery.capabilities!.search).toEqual({ enabled: false }); expect(discovery.capabilities!.export).toEqual({ enabled: false }); expect(discovery.capabilities!.chunkedUpload).toEqual({ enabled: false }); + // [#7541] `search` is NOT in that list any more. It is no longer derived + // from a service slot at all — it reports whether this protocol can serve + // `/search` (`typeof searchAll === 'function'`, the predicate the route's + // own 501 uses), and this class always can. Asserting `false` here was + // asserting the defect: the endpoint served 200s while the document said + // the capability was off. + expect(discovery.capabilities!.search).toEqual({ enabled: true }); }); - it('should set all capabilities to false when no services are registered', async () => { + it('should set all slot-derived capabilities to false when no services are registered', async () => { protocol = new ObjectStackProtocolImplementation(engine); const discovery = await protocol.getDiscovery(); @@ -466,9 +473,18 @@ describe('ObjectStackProtocolImplementation - Dynamic Service Discovery', () => expect(discovery.capabilities!.comments).toEqual({ enabled: false }); expect(discovery.capabilities!.automation).toEqual({ enabled: false }); expect(discovery.capabilities!.cron).toEqual({ enabled: false }); - expect(discovery.capabilities!.search).toEqual({ enabled: false }); expect(discovery.capabilities!.export).toEqual({ enabled: false }); expect(discovery.capabilities!.chunkedUpload).toEqual({ enabled: false }); + // [#7541] Same reason as above — and this is the exact host the issue was + // reported against: an empty registry, a live `/search`. The two halves of + // the document stay independent and both stay honest: the SLOT is still + // empty here... + expect(discovery.capabilities!.search).toEqual({ enabled: true }); + expect(discovery.services.search.enabled).toBe(false); + expect(discovery.services.search.status).toBe('unavailable'); + // ...and its message now says which question that answers, instead of + // reading as "search is dead on this host". + expect(discovery.services.search.message).toMatch(/capabilities\.search/); }); it('should dynamically set capabilities based on registered services', async () => { @@ -482,11 +498,18 @@ describe('ObjectStackProtocolImplementation - Dynamic Service Discovery', () => expect(discovery.capabilities!.automation).toEqual({ enabled: true }); expect(discovery.capabilities!.cron).toEqual({ enabled: false }); - expect(discovery.capabilities!.search).toEqual({ enabled: true }); expect(discovery.capabilities!.export).toEqual({ enabled: true }); expect(discovery.capabilities!.chunkedUpload).toEqual({ enabled: true }); // comments is independent of services — it tracks the sys_comment object (#3180). expect(discovery.capabilities!.comments).toEqual({ enabled: false }); + // [#7541] `search` is true here too, but NOT because the slot above is + // filled — this line proves nothing about the slot and is kept only so the + // reader is not left thinking it does. The discriminating cases live in + // `packages/rest/src/discovery-search-capability-agreement.test.ts`, which + // drives the capability builder and the route together. + expect(discovery.capabilities!.search).toEqual({ enabled: true }); + // What the slot DOES still decide, unchanged: the `services` half. + expect(discovery.services.search.enabled).toBe(true); }); // ── Atomic cross-object batch capability (#3298 / #1604 / ADR-0034) ───────── diff --git a/packages/rest/src/discovery-search-capability-agreement.test.ts b/packages/rest/src/discovery-search-capability-agreement.test.ts new file mode 100644 index 0000000000..4ef82f3e1b --- /dev/null +++ b/packages/rest/src/discovery-search-capability-agreement.test.ts @@ -0,0 +1,188 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7541] The two producers of "can this host search" must answer the same +// question the same way. +// +// The defect: `capabilities.search` came from a registered `search` SERVICE +// SLOT, while `GET {basePath}/search` refused on something else entirely — +// `typeof protocol.searchAll !== 'function'`. Nothing in either repository +// registers that slot and the protocol implements `searchAll` unconditionally, +// so every REST host advertised `capabilities.search = {enabled:false}` while +// serving 200s with real hits. A client that trusts the discovery document — +// which is the document's only purpose — skipped a working surface. Prime +// Directive #10 inverted: not an advertised endpoint that 404s, but a live +// endpoint no conforming client will ever call. +// +// WHAT THIS FILE ASSERTS, and why it is shaped this way: it does NOT assert +// `enabled === true`. That assertion passes again the day someone hardcodes the +// bit, which is the same class of defect one layer over. It asserts AGREEMENT — +// `declared === served` — with both sides MEASURED from the real producers in +// the same test: `capabilities.search` off the real `getDiscovery()` through +// the real `/discovery` handler, and the served status off the real +// `registerSearchEndpoints` handler. Three hosts that genuinely differ (below) +// keep the agreement from holding vacuously. + +import { describe, it, expect, vi } from 'vitest'; +import type { IHttpRequest } from '@objectstack/spec/contracts'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from './rest-server.js'; + +/** + * A complete `IHttpRequest`, typed against the contract rather than cast to + * `any`. Not ceremony: `enforceAuth` — which runs before either predicate under + * test — reads `method` and `path`, so a partial literal would exercise the + * gate with `undefined` on both. Building the real shape is what makes the + * measured statuses below the statuses a real caller gets. + */ +function request(path: string, query: Record = {}): IHttpRequest { + return { params: {}, query, headers: {}, method: 'GET', path }; +} + +function createMockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), + use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; +} + +/** + * An engine carrying ONE searchable object with ONE matching row, so the + * served path is a real 200-with-hits rather than a 200-with-nothing — the + * exact reproduction in the issue (`?q=audit` → 200, real hits). + */ +function createEngine() { + const widget = { + name: 'widget', + fields: [{ name: 'title', type: 'text', searchable: true }], + }; + return { + registry: { + getObject: (n: string) => (n === 'widget' ? widget : undefined), + getAllObjects: () => [widget], + getRegisteredTypes: () => [], + }, + find: async () => [{ id: 'w1', title: 'audit trail' }], + }; +} + +type Host = { + /** `capabilities.search.enabled` as the composed `/discovery` body reports it. */ + declared: boolean; + /** HTTP status `GET {basePath}/search?q=audit` actually answers. */ + status: number; + /** Hit count when it answered 200. */ + hits: number; +}; + +/** + * Boot a REST server over the REAL protocol and read BOTH producers off it. + * + * `enableSearch` selects whether this server mounts the route at all; + * `withSearchAll: false` removes the protocol's own implementation, which is + * the input the route's 501 branch exists for. + */ +async function measure(opts: { + enableSearch?: boolean; + withSearchAll?: boolean; +} = {}): Promise { + const protocol: any = new ObjectStackProtocolImplementation( + createEngine() as any, + () => new Map(), + ); + if (opts.withSearchAll === false) { + // Shadow the prototype method on the instance. Both predicates read the + // same property off the same object, so this single override is what makes + // the "protocol cannot search" host measurable at all — and it is why the + // test cannot pass by two independent predicates coincidentally agreeing. + Object.defineProperty(protocol, 'searchAll', { value: undefined, configurable: true }); + } + + const config: any = { + api: { + requireAuth: false, + ...(opts.enableSearch === false ? { enableSearch: false } : {}), + }, + }; + const rest = new RestServer(createMockServer() as any, protocol as any, config); + // Authenticated caller — step 1 of the issue's reproduction. `enforceAuth` + // runs BEFORE the `searchAll` probe, so an anonymous request 401s and never + // reaches either predicate; this is the house stub the other rest tests use + // for authed handlers, and it is upstream of everything under test here. + (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + const routes = rest.getRouteManager(); + + const discoveryEntry = routes.get('GET', '/api/v1/discovery'); + if (!discoveryEntry) throw new Error('discovery route not registered'); + let discoveryBody: any; + const discoveryRes: any = { + json: (b: any) => { discoveryBody = b; }, + status: () => discoveryRes, + }; + await discoveryEntry.handler(request('/api/v1/discovery'), discoveryRes); + const declared = discoveryBody?.capabilities?.search?.enabled; + + const searchEntry = routes.get('GET', '/api/v1/search'); + if (!searchEntry) { + // Not mounted — a client calling it gets the router's 404. That IS the + // served answer for this host. + return { declared, status: 404, hits: 0 }; + } + let status = 200; + let searchBody: any; + const searchRes: any = { + status: (s: number) => { status = s; return searchRes; }, + json: (b: any) => { searchBody = b; }, + }; + await searchEntry.handler(request('/api/v1/search', { q: 'audit' }), searchRes); + return { declared, status, hits: searchBody?.hits?.length ?? 0 }; +} + +/** Served ⇔ a caller can get search results out of this host. */ +const isServed = (h: Host) => h.status !== 404 && h.status !== 501; + +describe('[#7541] `capabilities.search` and the /search route answer one question', () => { + it('agrees on the ordinary host — where the document used to contradict the endpoint', async () => { + const host = await measure(); + + // The symptom, measured: the endpoint really does serve real hits here. + expect(host.status).toBe(200); + expect(host.hits).toBeGreaterThan(0); + + // The pin: whatever the endpoint does, the document says the same thing. + // Before the fix `declared` was false against a 200 — the inversion. + expect(host.declared).toBe(isServed(host)); + }); + + it('agrees on a host that does not mount the route (`api.enableSearch: false`)', async () => { + const host = await measure({ enableSearch: false }); + + expect(host.status).toBe(404); + expect(host.declared).toBe(isServed(host)); + }); + + it('agrees on a protocol that cannot search — both ends refuse on the same predicate', async () => { + const host = await measure({ withSearchAll: false }); + + // The route's own 501 branch, reached through the real handler. + expect(host.status).toBe(501); + expect(host.declared).toBe(isServed(host)); + }); + + it('anti-vacuity: the three hosts are genuinely discriminated, in both directions', async () => { + const [served, unmounted, unimplemented] = await Promise.all([ + measure(), + measure({ enableSearch: false }), + measure({ withSearchAll: false }), + ]); + + // Without this, `declared === served` would hold for the empty reason if + // some future edit pinned the bit — or the route — to one constant. + expect([served.declared, unmounted.declared, unimplemented.declared]) + .toEqual([true, false, false]); + expect([served.status, unmounted.status, unimplemented.status]) + .toEqual([200, 404, 501]); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 973b446155..457cde42e6 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -3790,6 +3790,25 @@ export class RestServer { + '{ $ref: } parent references (#1604 / ADR-0034).', }; + // [#7541] Global search — the same two-layer AND, for the + // same reason. The protocol answered whether IT can serve a + // search (`typeof searchAll === 'function'`, the predicate + // `registerSearchEndpoints` 501s on); this server answers + // whether it MOUNTED the route at all (`api.enableSearch`, + // the flag gated in registerRoutes). A deployment that opts + // out gets a 404, so advertising the protocol's `true` + // unqualified would re-open the declared ≠ enforced gap one + // layer up from the one this issue closed. Neither half is a + // fallback for a wrong bit: each layer states the fact only + // it knows, and `enabled` is their conjunction. + // + // The flag is read with the mount's own `?? true` spelling + // rather than the equivalent `!== false` — same predicate, + // same characters, so the two cannot be edited apart. + caps.search = { + enabled: !!caps.search?.enabled && (this.config.api.enableSearch ?? true), + }; + // Attach scoping metadata so clients can detect dual-mode routing. (discovery as any).scoping = { enabled: this.config.api.enableProjectScoping, diff --git a/packages/spec/src/system/core-services.zod.ts b/packages/spec/src/system/core-services.zod.ts index 6469461a92..6672eb715c 100644 --- a/packages/spec/src/system/core-services.zod.ts +++ b/packages/spec/src/system/core-services.zod.ts @@ -97,12 +97,16 @@ export const CORE_SERVICE_PROVIDER: Readonly> = { // the `ui` slot itself has no implementation anywhere (#4093 / #4146). 'ui': '@objectstack/metadata-protocol', // `null` means "no name belongs in an `Install X` sentence", which covers two - // different situations — see REMEDY_DETAIL, which is how the second one still - // gets an accurate message. + // different situations — see REMEDY_DETAIL, which is how BOTH of them still + // get an accurate message. // // Nothing provides the slot at all: `search` (no consumer either — // ADR-0115 Evidence 5). Verified across BOTH repositories: nothing in - // `objectstack-ai/cloud` registers it. + // `objectstack-ai/cloud` registers it. Still true, and since #7541 it is + // also no longer the whole story a reader needs: the cross-object + // `/search` endpoint is served by the protocol whether or not this slot is + // filled, so `search` carries a REMEDY_DETAIL sentence saying which of the + // two questions this entry answers. // // A provider exists but cannot be installed: `ai`. `@objectstack/service-ai` // registers this slot in `objectstack-ai/cloud` and is `private: true`, so @@ -131,9 +135,27 @@ export const CORE_SERVICE_PROVIDER: Readonly> = { * `/ui` is the case (#4146): it is served by the `protocol` service, and * nothing anywhere registers `ui` — so the sentence has to say what actually * serves it, not just what to install. + * + * `search` is the same shape (#7541), reached from the other side. Its + * capability bit is now derived from what actually serves the endpoint + * (`protocol.searchAll` — the predicate `registerSearchEndpoints` refuses on) + * rather than from this slot, so an ordinary REST host reports + * `capabilities.search.enabled: true` beside `services.search.status: + * 'unavailable'`. Both are true, and they answer different questions — but the + * bare "nothing ships" sentence read as "search is dead here", which is exactly + * how the two halves of one discovery document come to look like they + * contradict each other. The slot keeps its honest `unavailable`; the message + * now says which question it is answering. */ const REMEDY_DETAIL: Readonly> = { 'ui': 'Served by the protocol service — register MetadataPlugin (@objectstack/metadata-protocol) to enable', + // Deliberately keeps the generic sentence's "No implementation ships for the + // 'search' slot" opening: that fact is unchanged, and it is pinned by + // core-service-provider.test.ts. Everything after it is the disambiguation. + 'search': "No implementation ships for the 'search' slot — a dedicated search engine (Elasticsearch/Meilisearch); " + + 'register a service under it to enable. Cross-object search does not depend on it: ' + + 'GET {basePath}/search is served by the protocol itself, and whether it is served on this host is ' + + 'reported by capabilities.search — not by this slot.', // A provider EXISTS — `@objectstack/service-ai` registers this slot — but it // lives in the `objectstack-ai/cloud` repository and is `private: true`, so // there is nothing to install and no name that belongs in an "Install X"