From 3009d036ec4f3e7e5c55ae10f31b58308e7aae73 Mon Sep 17 00:00:00 2001 From: Toby Brain Date: Fri, 29 May 2026 11:26:18 +1000 Subject: [PATCH] [OAS] Detect colliding shared schema ids during OpenAPI generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Throw a descriptive `OasSchemaCollisionError` from `@kbn/router-to-openapispec` when two distinct shapes are registered under the same shared-schema id during a single generation pass. Catches the silent component-overwrite class of bug behind https://github.com/elastic/kibana/issues/271809 (e.g. `Base.extends({...})` inheriting `meta.id` from its base). Companion to https://github.com/elastic/kibana/pull/271812 (Path A — short-term Fleet-side fix that removes the existing collisions). Co-authored-by: Cursor --- .../shared/kbn-router-to-openapispec/index.ts | 2 + .../src/generate_oas.test.ts | 72 +++++++++- .../src/oas_converter/index.ts | 34 ++++- .../oas_converter/kbn_config_schema/lib.ts | 10 +- .../post_process_mutations/context.test.ts | 109 +++++++++++++++ .../post_process_mutations/context.ts | 29 ++++ .../post_process_mutations/index.ts | 4 +- .../schema_collision.ts | 129 ++++++++++++++++++ .../kbn-router-to-openapispec/src/type.ts | 2 + 9 files changed, 385 insertions(+), 6 deletions(-) create mode 100644 src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/schema_collision.ts diff --git a/src/platform/packages/shared/kbn-router-to-openapispec/index.ts b/src/platform/packages/shared/kbn-router-to-openapispec/index.ts index 37a9edcc14e77..f450a07deaa1f 100644 --- a/src/platform/packages/shared/kbn-router-to-openapispec/index.ts +++ b/src/platform/packages/shared/kbn-router-to-openapispec/index.ts @@ -12,4 +12,6 @@ export { type GenerateOpenApiDocumentOptionsFilters, } from './src/generate_oas'; +export { OasSchemaCollisionError } from './src/oas_converter'; + export type { OasMetaExtensions } from './src/oas_converter/zod/lib'; diff --git a/src/platform/packages/shared/kbn-router-to-openapispec/src/generate_oas.test.ts b/src/platform/packages/shared/kbn-router-to-openapispec/src/generate_oas.test.ts index ede70f4847631..cce263dde8fae 100644 --- a/src/platform/packages/shared/kbn-router-to-openapispec/src/generate_oas.test.ts +++ b/src/platform/packages/shared/kbn-router-to-openapispec/src/generate_oas.test.ts @@ -30,7 +30,12 @@ import { generateOpenApiDocument } from './generate_oas'; import { processRouter } from './process_router'; import { processVersionedRouter } from './process_versioned_router'; import type { CreateTestRouterArgs } from './generate_oas.test.util'; -import { createTestRouters, createRouter, createVersionedRouter } from './generate_oas.test.util'; +import { + createTestRouters, + createRouter, + createVersionedRouter, + getRouterDefaults, +} from './generate_oas.test.util'; import { sharedOas, createSharedZodSchema, @@ -825,4 +830,69 @@ describe('generateOpenApiDocument', () => { }, }); }); + + describe('shared schema id collisions (issue #271809)', () => { + const buildRouters = (firstBody: Type, secondBody: Type) => { + const makeRoute = (path: string, body: Type) => ({ + ...getRouterDefaults(), + method: 'post' as const, + path, + validationSchemas: { + request: { body }, + response: { 200: { description: 'ok' } }, + }, + }); + const firstRouter = createRouter({ routes: [makeRoute('/first', firstBody)] }); + const secondRouter = createRouter({ routes: [makeRoute('/second', secondBody)] }); + return { routers: [firstRouter, secondRouter], versionedRouters: [] as never[] }; + }; + + it('throws when two routes register the same meta.id with different shapes', async () => { + // Reproduces the Fleet pattern: a base schema with `meta: { id }` is used + // by one route; a derived schema produced via `Base.extends({...})` — + // which inherits `meta.id` from the base when no override is passed — + // is used by another route. Without the guardrail this silently + // overwrites the registered component (last write wins). + const baseShape = schema.object( + { id: schema.string(), success: schema.boolean() }, + { meta: { id: 'package_policy_status_response' } } + ); + const derivedSameId = schema.object( + { + id: schema.string(), + success: schema.boolean(), + output_id: schema.maybe(schema.oneOf([schema.literal(null), schema.string()])), + policy_ids: schema.arrayOf(schema.string()), + }, + { meta: { id: 'package_policy_status_response' } } + ); + + const { routers, versionedRouters } = buildRouters(derivedSameId, baseShape); + + await expect( + generateOpenApiDocument( + { routers, versionedRouters }, + { title: 'test', baseUrl: 'https://test.oas', version: '99.99.99' } + ) + ).rejects.toThrowError(/OAS shared schema collision for id "package_policy_status_response"/); + }); + + it('does not throw when two routes register the same id with the same shape', async () => { + // Re-using the same shared schema across multiple routes is the common, + // intended case. It must remain a no-op. + const sharedSchema = schema.object( + { id: schema.string(), success: schema.boolean() }, + { meta: { id: 'reused_status_response' } } + ); + + const { routers, versionedRouters } = buildRouters(sharedSchema, sharedSchema); + + const oas = await generateOpenApiDocument( + { routers, versionedRouters }, + { title: 'test', baseUrl: 'https://test.oas', version: '99.99.99' } + ); + + expect(oas.components?.schemas?.reused_status_response).toBeDefined(); + }); + }); }); diff --git a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/index.ts b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/index.ts index d4fd37b2466c9..5440e95f2fcdc 100644 --- a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/index.ts +++ b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/index.ts @@ -10,13 +10,30 @@ import type { OpenAPIV3 } from 'openapi-types'; import type { KnownParameters, OpenAPIConverter } from '../type'; import type { Env } from '../generate_oas'; +import type { OnCollision } from './kbn_config_schema/post_process_mutations'; +import { + describeSchemaCollision, + OasSchemaCollisionError, + schemasMatch, +} from './kbn_config_schema/post_process_mutations/schema_collision'; import { kbnConfigSchemaConverter } from './kbn_config_schema'; import { zodConverter } from './zod'; import { catchAllConverter } from './catch_all'; +export interface OasConverterOptions { + /** + * Behaviour when a shared schema id is registered with two different shapes + * within a single generation pass. Defaults to `'throw'`. + * + * @see {@link OnCollision} + */ + onCollision?: OnCollision; +} + export class OasConverter { readonly #env: Env; + readonly #onCollision: OnCollision; readonly #converters: OpenAPIConverter[] = [ kbnConfigSchemaConverter, zodConverter, @@ -24,8 +41,9 @@ export class OasConverter { ]; readonly #sharedSchemas = new Map(); - constructor(env: Env = { serverless: false }) { + constructor(env: Env = { serverless: false }, options: OasConverterOptions = {}) { this.#env = env; + this.#onCollision = options.onCollision ?? 'throw'; } /** @@ -45,6 +63,17 @@ export class OasConverter { #addComponents(components: { [id: string]: OpenAPIV3.SchemaObject }) { Object.entries(components).forEach(([id, schema]) => { + if (this.#onCollision !== 'ignore') { + const existing = this.#sharedSchemas.get(id); + if (existing && !schemasMatch(existing, schema)) { + const message = describeSchemaCollision(id, existing, schema); + if (this.#onCollision === 'throw') { + throw new OasSchemaCollisionError(message, id); + } + // eslint-disable-next-line no-console + console.warn(message); + } + } this.#sharedSchemas.set(id, schema); }); } @@ -58,6 +87,7 @@ export class OasConverter { const { schema: oasSchema, shared } = this.#getConverter(unwrapped)!.convert(unwrapped, { env: this.#env, sharedSchemas: this.#sharedSchemas, + onCollision: this.#onCollision, }); this.#addComponents(shared); return oasSchema as OpenAPIV3.SchemaObject; @@ -86,3 +116,5 @@ export class OasConverter { }; } } + +export { OasSchemaCollisionError } from './kbn_config_schema/post_process_mutations/schema_collision'; diff --git a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/lib.ts b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/lib.ts index 8d8d6aa5c849d..dcf3355872c70 100644 --- a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/lib.ts +++ b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/lib.ts @@ -70,9 +70,15 @@ export const unwrapKbnConfigSchema = (schema: unknown): joi.Schema => { return schema.getSchema(); }; -export const convert = (kbnConfigSchema: unknown, { sharedSchemas, env }: ConvertOptions = {}) => { +export const convert = ( + kbnConfigSchema: unknown, + { sharedSchemas, env, onCollision }: ConvertOptions = {} +) => { const schema = unwrapKbnConfigSchema(kbnConfigSchema); - const { result, shared } = parse({ schema, ctx: createCtx({ sharedSchemas, env }) }); + const { result, shared } = parse({ + schema, + ctx: createCtx({ sharedSchemas, env, onCollision }), + }); return { schema: result, shared }; }; diff --git a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/context.test.ts b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/context.test.ts index 2d48b69b665b5..7506931e59651 100644 --- a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/context.test.ts +++ b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/context.test.ts @@ -10,6 +10,7 @@ import { schema } from '@kbn/config-schema'; import { joi2JsonInternal } from '../parse'; import { createCtx } from './context'; +import { OasSchemaCollisionError } from './schema_collision'; it('records schemas as expected', () => { const ctx = createCtx(); @@ -26,3 +27,111 @@ it('records schemas as expected', () => { b: { properties: {} }, }); }); + +describe('shared-schema id collision detection', () => { + const baseShape = { type: 'object' as const, properties: { a: { type: 'string' as const } } }; + const extendedShape = { + type: 'object' as const, + properties: { + a: { type: 'string' as const }, + b: { type: 'number' as const }, + }, + }; + + it('is a no-op when the same id is registered with the same shape', () => { + const ctx = createCtx(); + ctx.addSharedSchema('reused', { ...baseShape }); + expect(() => ctx.addSharedSchema('reused', { ...baseShape })).not.toThrow(); + expect(ctx.getSharedSchemas()).toEqual({ reused: baseShape }); + }); + + it('throws OasSchemaCollisionError when the same id is registered with a different shape', () => { + const ctx = createCtx(); + ctx.addSharedSchema('id_collision', { ...baseShape }); + expect(() => ctx.addSharedSchema('id_collision', { ...extendedShape })).toThrowError( + OasSchemaCollisionError + ); + }); + + it('includes the conflicting id and a key-set diff in the error message', () => { + const ctx = createCtx(); + ctx.addSharedSchema('id_collision', { ...baseShape }); + let captured: Error | undefined; + try { + ctx.addSharedSchema('id_collision', { ...extendedShape }); + } catch (error) { + captured = error as Error; + } + expect(captured).toBeDefined(); + expect(captured!.message).toContain('"id_collision"'); + expect(captured!.message).toContain('properties only in the second registration: `b`'); + expect(captured!.message).toContain('Base.extends({...})'); + expect(captured!.message).toContain('https://github.com/elastic/kibana/issues/271809'); + }); + + it('warns instead of throwing when onCollision is "warn"', () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const ctx = createCtx({ onCollision: 'warn' }); + ctx.addSharedSchema('id_collision', { ...baseShape }); + ctx.addSharedSchema('id_collision', { ...extendedShape }); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('OAS shared schema collision'); + // last write still wins under 'warn' + expect(ctx.getSharedSchemas().id_collision).toEqual(extendedShape); + } finally { + warn.mockRestore(); + } + }); + + it('preserves legacy last-write-wins semantics when onCollision is "ignore"', () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const ctx = createCtx({ onCollision: 'ignore' }); + ctx.addSharedSchema('id_collision', { ...baseShape }); + ctx.addSharedSchema('id_collision', { ...extendedShape }); + expect(warn).not.toHaveBeenCalled(); + expect(ctx.getSharedSchemas().id_collision).toEqual(extendedShape); + } finally { + warn.mockRestore(); + } + }); + + it('reports the collision when reproducing the issue #271809 pattern via @kbn/config-schema', () => { + const ctx = createCtx(); + // Two distinct schemas that share the same `meta.id` — what `Base.extends({...})` + // (without overriding meta.id) produces in practice. + const baseSchema = schema.object( + { a: schema.string(), b: schema.boolean() }, + { meta: { id: 'package_policy_status_response_demo' } } + ); + const extendedSchema = schema.object( + { + a: schema.string(), + b: schema.boolean(), + output_id: schema.maybe(schema.oneOf([schema.literal(null), schema.string()])), + policy_ids: schema.arrayOf(schema.string()), + }, + { meta: { id: 'package_policy_status_response_demo' } } + ); + + const baseJson = joi2JsonInternal(baseSchema.getSchema()) as unknown as { + schemas: Record; + }; + const extendedJson = joi2JsonInternal(extendedSchema.getSchema()) as unknown as { + schemas: Record; + }; + + ctx.addSharedSchema( + 'package_policy_status_response_demo', + extendedJson.schemas.package_policy_status_response_demo as never + ); + + expect(() => + ctx.addSharedSchema( + 'package_policy_status_response_demo', + baseJson.schemas.package_policy_status_response_demo as never + ) + ).toThrowError(/OAS shared schema collision for id "package_policy_status_response_demo"/); + }); +}); diff --git a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/context.ts b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/context.ts index 61e1c34ca569b..e33dd3c7b6681 100644 --- a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/context.ts +++ b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/context.ts @@ -10,6 +10,19 @@ import type { OpenAPIV3 } from 'openapi-types'; import type { Env } from '../../../generate_oas'; import { getIdFromRefString } from './mutations/utils'; +import { describeSchemaCollision, OasSchemaCollisionError, schemasMatch } from './schema_collision'; + +/** + * Behaviour when a shared schema is registered under an id that is already + * present in the registry with a different shape. + * + * - `'throw'` (default): raise {@link OasSchemaCollisionError} with a diff of + * the conflicting properties. Catches silent overwrite bugs at OAS generate + * time (see https://github.com/elastic/kibana/issues/271809). + * - `'warn'`: log a warning and let the new shape win (transitional). + * - `'ignore'`: keep the existing legacy semantics (last-write-wins, silent). + */ +export type OnCollision = 'throw' | 'warn' | 'ignore'; export interface IContext { addSharedSchema: (id: string, schema: OpenAPIV3.SchemaObject) => void; @@ -21,18 +34,32 @@ export interface IContext { interface Options { sharedSchemas?: Map; env?: Env; + onCollision?: OnCollision; } class Context implements IContext { private readonly sharedSchemas: Map; private readonly namespace?: string; private readonly env: Env; + private readonly onCollision: OnCollision; constructor(opts: Options) { this.sharedSchemas = opts.sharedSchemas ?? new Map(); this.env = opts.env ?? { serverless: false }; + this.onCollision = opts.onCollision ?? 'throw'; } public addSharedSchema(id: string, schema: OpenAPIV3.SchemaObject): void { + if (this.onCollision !== 'ignore') { + const existing = this.sharedSchemas.get(id); + if (existing && !schemasMatch(existing, schema)) { + const message = describeSchemaCollision(id, existing, schema); + if (this.onCollision === 'throw') { + throw new OasSchemaCollisionError(message, id); + } + // eslint-disable-next-line no-console + console.warn(message); + } + } this.sharedSchemas.set(id, schema); } @@ -57,3 +84,5 @@ class Context implements IContext { export const createCtx = (opts: Options = { sharedSchemas: new Map() }) => { return new Context(opts); }; + +export { OasSchemaCollisionError } from './schema_collision'; diff --git a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/index.ts b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/index.ts index 4f129efc4ab0d..0e2a56fee66cc 100644 --- a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/index.ts +++ b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/index.ts @@ -72,5 +72,5 @@ const walkSchema = (ctx: IContext, schema: Schema): void => { } }; -export { createCtx } from './context'; -export type { IContext } from './context'; +export { createCtx, OasSchemaCollisionError } from './context'; +export type { IContext, OnCollision } from './context'; diff --git a/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/schema_collision.ts b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/schema_collision.ts new file mode 100644 index 0000000000000..af82a91756621 --- /dev/null +++ b/src/platform/packages/shared/kbn-router-to-openapispec/src/oas_converter/kbn_config_schema/post_process_mutations/schema_collision.ts @@ -0,0 +1,129 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { metaFields } from '@kbn/config-schema'; +import type { OpenAPIV3 } from 'openapi-types'; + +const ISSUE_URL = 'https://github.com/elastic/kibana/issues/271809'; + +const { META_FIELD_X_OAS_OPTIONAL } = metaFields; + +/** + * Internal annotations the OAS converter writes onto schemas mid-parse and + * strips at the end of {@link parse}. Ignored when comparing two registrations + * of the same shared schema id: the existing entry in the master map has + * already been stripped, but a freshly generated entry from a `maybe(shared)` + * site still has them attached at registration time. + */ +const TRANSIENT_KEYS = new Set([META_FIELD_X_OAS_OPTIONAL]); + +const visibleKeys = (value: object): string[] => { + return Object.keys(value).filter((key) => !TRANSIENT_KEYS.has(key)); +}; + +/** + * Deep-equality that ignores transient internal annotations on schema objects + * (see {@link TRANSIENT_KEYS}). Drop-in replacement for `lodash.isEqual` for + * the shared-schema comparison, scoped to plain JSON-shaped values. + */ +export const schemasMatch = (a: unknown, b: unknown): boolean => { + if (a === b) return true; + if (typeof a !== 'object' || typeof b !== 'object' || a === null || b === null) { + return false; + } + if (Array.isArray(a)) { + if (!Array.isArray(b) || a.length !== b.length) return false; + for (let i = 0; i < a.length; i++) { + if (!schemasMatch(a[i], b[i])) return false; + } + return true; + } + if (Array.isArray(b)) return false; + + const aKeys = visibleKeys(a as object); + const bKeys = visibleKeys(b as object); + if (aKeys.length !== bKeys.length) return false; + + const bRecord = b as Record; + for (const key of aKeys) { + if (!Object.prototype.hasOwnProperty.call(bRecord, key)) return false; + if (!schemasMatch((a as Record)[key], bRecord[key])) return false; + } + return true; +}; + +const propertyNames = (schema: OpenAPIV3.SchemaObject): string[] => { + return schema.properties ? Object.keys(schema.properties).sort() : []; +}; + +const sortedDifference = (a: readonly string[], b: readonly string[]): string[] => { + const bSet = new Set(b); + return a.filter((key) => !bSet.has(key)); +}; + +const formatKeyList = (keys: readonly string[]): string => { + if (keys.length === 0) return '(none)'; + return keys.map((key) => `\`${key}\``).join(', '); +}; + +/** + * Build the human-readable diagnostic for a shared-schema id collision. + * + * Two shapes are considered colliding when they share an id but differ + * in any way (deep-equal mismatch). The diagnostic lists property-key + * differences first because that is the dominant failure mode driven by + * `Base.extends({...})` patterns inheriting `meta.id` from the base. + */ +export const describeSchemaCollision = ( + id: string, + previous: OpenAPIV3.SchemaObject, + next: OpenAPIV3.SchemaObject +): string => { + const previousKeys = propertyNames(previous); + const nextKeys = propertyNames(next); + const onlyInPrevious = sortedDifference(previousKeys, nextKeys); + const onlyInNext = sortedDifference(nextKeys, previousKeys); + + const lines = [ + `OAS shared schema collision for id "${id}": the same id was registered with two different shapes.`, + ]; + + if (onlyInPrevious.length > 0 || onlyInNext.length > 0) { + lines.push( + ` properties only in the first registration: ${formatKeyList(onlyInPrevious)}`, + ` properties only in the second registration: ${formatKeyList(onlyInNext)}` + ); + } else { + lines.push(' property key sets match; the shapes differ in property definitions or metadata.'); + } + + lines.push( + 'This usually means a `Base.extends({...})` call inherited `meta.id` from its base, ' + + 'or two unrelated schemas accidentally chose the same id string. ' + + "Pass `{ meta: { id: 'distinct_id' } }` as the second argument to `extends()` " + + `to give the derived schema its own component name. See ${ISSUE_URL}.` + ); + + return lines.join('\n'); +}; + +/** + * Thrown by the OAS converter when a shared schema id is registered with two + * different shapes within a single generation pass. + */ +export class OasSchemaCollisionError extends Error { + public readonly schemaId: string; + constructor(message: string, schemaId: string) { + super(message); + this.name = 'OasSchemaCollisionError'; + this.schemaId = schemaId; + + Object.setPrototypeOf(this, new.target.prototype); + } +} diff --git a/src/platform/packages/shared/kbn-router-to-openapispec/src/type.ts b/src/platform/packages/shared/kbn-router-to-openapispec/src/type.ts index 2b72300ee4ac2..5ee0111f8eecf 100644 --- a/src/platform/packages/shared/kbn-router-to-openapispec/src/type.ts +++ b/src/platform/packages/shared/kbn-router-to-openapispec/src/type.ts @@ -10,6 +10,7 @@ import type { Router } from '@kbn/core-http-router-server-internal'; import type { OpenAPIV3 } from '../openapi-types'; import type { Env } from './generate_oas'; +import type { OnCollision } from './oas_converter/kbn_config_schema/post_process_mutations'; export type { OpenAPIV3 } from '../openapi-types'; export interface KnownParameters { [paramName: string]: { optional: boolean }; @@ -18,6 +19,7 @@ export interface KnownParameters { export interface ConvertOptions { sharedSchemas?: Map; env?: Env; + onCollision?: OnCollision; } export interface OpenAPIConverter {