Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -825,4 +830,69 @@ describe('generateOpenApiDocument', () => {
},
});
});

describe('shared schema id collisions (issue #271809)', () => {
const buildRouters = (firstBody: Type<unknown>, secondBody: Type<unknown>) => {
const makeRoute = (path: string, body: Type<unknown>) => ({
...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();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,40 @@
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,
catchAllConverter,
];
readonly #sharedSchemas = new Map<string, OpenAPIV3.SchemaObject>();

constructor(env: Env = { serverless: false }) {
constructor(env: Env = { serverless: false }, options: OasConverterOptions = {}) {
this.#env = env;
this.#onCollision = options.onCollision ?? 'throw';
}

/**
Expand All @@ -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);
});
}
Expand All @@ -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;
Expand Down Expand Up @@ -86,3 +116,5 @@ export class OasConverter {
};
}
}

export { OasSchemaCollisionError } from './kbn_config_schema/post_process_mutations/schema_collision';
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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<string, object>;
};
const extendedJson = joi2JsonInternal(extendedSchema.getSchema()) as unknown as {
schemas: Record<string, object>;
};

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"/);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -21,18 +34,32 @@ export interface IContext {
interface Options {
sharedSchemas?: Map<string, OpenAPIV3.SchemaObject>;
env?: Env;
onCollision?: OnCollision;
}

class Context implements IContext {
private readonly sharedSchemas: Map<string, OpenAPIV3.SchemaObject>;
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);
}

Expand All @@ -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';
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Loading
Loading