Skip to content

Commit

Permalink
[Security Solution] Implement shared components conflict resolution f…
Browse files Browse the repository at this point in the history
…unctionality (elastic#188812)

**Resolves:** elastic#188817

This PR adds automatic shared components conflict resolution functionality for OpenAPI merger. It boils down to a similar result as `npx @redocly/cli join --prefix-components-with-info-prop title` produces by prefixing shared components with document's title in each source.

OpenAPI bundler intentionally won't solve conflicts automatically since it's focused on bundling domain APIs where conflicts are usually indicators of upstream problems.

While working with various OpenAPI specs it may happen that different specs use exactly the same name for some shared components but different definitions. It must be avoided inside one API domain but it's a usual situation when merging OpenAPI specs of different API domains. For example domains may define a shared `Id` or `404Response` schemas where `Id` is a string in one domain and a number in another.

OpenAPI merger implemented in elastic#188110 and OpenAPI bundler implemented in elastic#171526 do not solve shared components related conflicts automatically. It works perfectly for a single API domain forcing engineers choosing shared schema names carefully.

This PR adds automatic shared components conflict resolution for OpenAPI merger. It prefixes shared component names with a normalized document's title.

OpenAPI bundler intentionally won't solve conflicts automatically since it's focused on bundling domain APIs where conflicts are usually indicators of upstream problems.

Consider two following OpenAPI specs each defining local `MySchema`

**spec1.schema.yaml**
```yaml
openapi: 3.0.3
info:
  title: My endpoint
  version: '2023-10-31'
paths:
  /api/some_api:
    get:
      operationId: MyEndpointGet
      responses:
        '200':
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/MySchema'

components:
  schemas:
    MySchema:
      type: string
      enum:
        - value1
```

**spec2.schema.yaml**
```yaml
openapi: 3.0.3
info:
  title: My another endpoint
  version: '2023-10-31'
paths:
  /api/another_api:
    get:
      operationId: MyAnotherEndpointGet
      responses:
        '200':
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/MySchema'

components:
  schemas:
    MySchema:
      type: number
```

and a script to merge them

```js
require('../../src/setup_node_env');
const { resolve } = require('path');
const { merge } = require('@kbn/openapi-bundler');
const { REPO_ROOT } = require('@kbn/repo-info');

(async () => {
  await merge({
    sourceGlobs: [
      `${REPO_ROOT}/oas_docs/spec1.schema.yaml`,
      `${REPO_ROOT}/oas_docs/spec2.schema.yaml`,
    ],
    outputFilePath: resolve(`${REPO_ROOT}/oas_docs/merged.yaml`),
    options: {
      mergedSpecInfo: {
        title: 'Merge result',
        version: 'my version',
      },
    },
  });
})();
```

will be merged successfully to

**merged.yaml**
```yaml
openapi: 3.0.3
info:
  title: Merge result
  version: 'my version'
paths:
  /api/another_api:
    get:
      operationId: MyAnotherEndpointGet
      responses:
        '200':
          content:
            application/json; Elastic-Api-Version=2023-10-31:
              schema:
                $ref: '#/components/schemas/My_another_endpoint_MySchema'
  /api/some_api:
    get:
      operationId: MyEndpointGet
      responses:
        '200':
          content:
            application/json; Elastic-Api-Version=2023-10-31:
              schema:
                $ref: '#/components/schemas/My_endpoint_MySchema'
components:
  schemas:
    My_another_endpoint_MySchema:
      type: number
    My_endpoint_MySchema:
      enum:
        - value1
      type: string
```
  • Loading branch information
maximpn authored and lcawl committed Aug 19, 2024
1 parent 84126f1 commit 01b99e7
Show file tree
Hide file tree
Showing 23 changed files with 1,323 additions and 221 deletions.
50 changes: 4 additions & 46 deletions packages/kbn-openapi-bundler/src/bundler/bundle_document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,35 +9,19 @@
import { isAbsolute } from 'path';
import { RefResolver } from './ref_resolver/ref_resolver';
import { processDocument } from './process_document/process_document';
import { X_CODEGEN_ENABLED, X_INLINE, X_INTERNAL, X_LABELS, X_MODIFY } from './known_custom_props';
import { X_INLINE } from './known_custom_props';
import { isPlainObjectType } from '../utils/is_plain_object_type';
import { ResolvedDocument } from './ref_resolver/resolved_document';
import { ResolvedRef } from './ref_resolver/resolved_ref';
import { createSkipNodeWithInternalPropProcessor } from './process_document/document_processors/skip_node_with_internal_prop';
import { createSkipInternalPathProcessor } from './process_document/document_processors/skip_internal_path';
import { createModifyPartialProcessor } from './process_document/document_processors/modify_partial';
import { createModifyRequiredProcessor } from './process_document/document_processors/modify_required';
import { createRemovePropsProcessor } from './process_document/document_processors/remove_props';
import {
createFlattenFoldedAllOfItemsProcessor,
createMergeNonConflictingAllOfItemsProcessor,
createUnfoldSingleAllOfItemProcessor,
} from './process_document/document_processors/reduce_all_of_items';
import { createIncludeLabelsProcessor } from './process_document/document_processors/include_labels';
import { BundleRefProcessor } from './process_document/document_processors/bundle_refs';
import { RemoveUnusedComponentsProcessor } from './process_document/document_processors/remove_unused_components';
import { insertRefByPointer } from '../utils/insert_by_json_pointer';
import { DocumentNodeProcessor } from './process_document/document_processors/types/document_node_processor';

export class SkipException extends Error {
constructor(public documentPath: string, message: string) {
super(message);
}
}

interface BundleDocumentOptions {
includeLabels?: string[];
}

/**
* Bundles document into one file and performs appropriate document modifications.
*
Expand All @@ -54,7 +38,7 @@ interface BundleDocumentOptions {
*/
export async function bundleDocument(
absoluteDocumentPath: string,
options?: BundleDocumentOptions
processors: Readonly<DocumentNodeProcessor[]> = []
): Promise<ResolvedDocument> {
if (!isAbsolute(absoluteDocumentPath)) {
throw new Error(
Expand All @@ -75,26 +59,11 @@ export async function bundleDocument(
throw new SkipException(resolvedDocument.absolutePath, 'Document has no paths defined');
}

const defaultProcessors = [
createSkipNodeWithInternalPropProcessor(X_INTERNAL),
createSkipInternalPathProcessor('/internal'),
createModifyPartialProcessor(),
createModifyRequiredProcessor(),
createRemovePropsProcessor([X_INLINE, X_MODIFY, X_CODEGEN_ENABLED, X_LABELS]),
createFlattenFoldedAllOfItemsProcessor(),
createMergeNonConflictingAllOfItemsProcessor(),
createUnfoldSingleAllOfItemProcessor(),
];

if (options?.includeLabels) {
defaultProcessors.push(createIncludeLabelsProcessor(options?.includeLabels));
}

const bundleRefsProcessor = new BundleRefProcessor(X_INLINE);
const removeUnusedComponentsProcessor = new RemoveUnusedComponentsProcessor();

await processDocument(resolvedDocument, refResolver, [
...defaultProcessors,
...processors,
bundleRefsProcessor,
removeUnusedComponentsProcessor,
]);
Expand All @@ -111,8 +80,6 @@ export async function bundleDocument(
);
}

injectBundledRefs(resolvedDocument, bundleRefsProcessor.getBundledRefs());

return resolvedDocument;
}

Expand All @@ -127,12 +94,3 @@ function hasPaths(document: MaybeObjectWithPaths): boolean {
Object.keys(document.paths).length > 0
);
}

function injectBundledRefs(
resolvedDocument: ResolvedDocument,
refs: IterableIterator<ResolvedRef>
): void {
for (const ref of refs) {
insertRefByPointer(ref.pointer, ref.refNode, resolvedDocument.document);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export async function mergeDocuments(
// is the simplest way to take initial components into account.
const documentsToMerge = [
{
absolutePath: 'MERGED OpenAPI SPEC',
absolutePath: 'MERGED RESULT',
document: mergedDocument as unknown as ResolvedDocument['document'],
},
...documentsGroup,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import chalk from 'chalk';
import deepEqual from 'fast-deep-equal';
import { OpenAPIV3 } from 'openapi-types';
import { ResolvedDocument } from '../ref_resolver/resolved_document';
import { extractByJsonPointer } from '../../utils/extract_by_json_pointer';
import { extractObjectByJsonPointer } from '../../utils/extract_by_json_pointer';
import { logger } from '../../logger';

const MERGEABLE_COMPONENT_TYPES = [
Expand All @@ -34,7 +34,7 @@ export function mergeSharedComponents(
const mergedTypedComponents = mergeObjects(bundledDocuments, `/components/${componentsType}`);

if (Object.keys(mergedTypedComponents).length === 0) {
// Nothing was merged for this components type, go to the next component type
// Nothing was merged for that components type, go to the next component type
continue;
}

Expand Down Expand Up @@ -63,26 +63,21 @@ function mergeObjects(
const componentToAdd = object[name];
const existingComponent = merged[name];

if (existingComponent) {
// Bundled documents may contain explicit references duplicates. For example
// shared schemas from `@kbn/openapi-common` has `NonEmptyString` which is
// widely used. After bundling references into a document (i.e. making them
// local references) we will have duplicates. This is why we need to check
// for exact match via `deepEqual()` to check whether components match.
if (existingComponent && !deepEqual(componentToAdd, existingComponent)) {
const existingSchemaLocation = componentNameSourceLocationMap.get(name);

if (deepEqual(componentToAdd, existingComponent)) {
logger.warning(
`Found a duplicate component ${chalk.yellow(
`${sourcePointer}/${name}`
)} defined in ${chalk.blue(resolvedDocument.absolutePath)} and in ${chalk.magenta(
existingSchemaLocation
)}.`
);
} else {
throw new Error(
`❌ Unable to merge documents due to conflicts in referenced ${mergedEntityName}. Component ${chalk.yellow(
`${sourcePointer}/${name}`
)} is defined in ${chalk.blue(resolvedDocument.absolutePath)} and in ${chalk.magenta(
existingSchemaLocation
)} but has not matching definitions.`
);
}
throw new Error(
`❌ Unable to merge documents due to conflicts in referenced ${mergedEntityName}. Component ${chalk.yellow(
`${sourcePointer}/${name}`
)} is defined in ${chalk.blue(resolvedDocument.absolutePath)} and in ${chalk.magenta(
existingSchemaLocation
)} but definitions DO NOT match.`
);
}

merged[name] = componentToAdd;
Expand All @@ -98,9 +93,9 @@ function extractObjectToMerge(
sourcePointer: string
): Record<string, unknown> | undefined {
try {
return extractByJsonPointer(resolvedDocument.document, sourcePointer);
return extractObjectByJsonPointer(resolvedDocument.document, sourcePointer);
} catch (e) {
logger.debug(
logger.verbose(
`JSON pointer "${sourcePointer}" is not resolvable in ${resolvedDocument.absolutePath}`
);
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import deepEqual from 'fast-deep-equal';
import chalk from 'chalk';
import { parseRef } from '../../../utils/parse_ref';
import { hasProp } from '../../../utils/has_prop';
import { isChildContext } from '../is_child_context';
import { insertRefByPointer } from '../../../utils/insert_by_json_pointer';
Expand Down Expand Up @@ -59,11 +60,6 @@ export class BundleRefProcessor implements DocumentNodeProcessor {
inlineRef(node, resolvedRef);
} else {
const rootDocument = this.extractRootDocument(context);

if (!rootDocument.components) {
rootDocument.components = {};
}

const ref = this.refs.get(resolvedRef.pointer);

if (ref && !deepEqual(ref.refNode, resolvedRef.refNode)) {
Expand All @@ -77,27 +73,29 @@ export class BundleRefProcessor implements DocumentNodeProcessor {
ref.pointer
)} is defined in ${chalk.blue(ref.absolutePath)} and in ${chalk.magenta(
resolvedRef.absolutePath
)} but has not matching definitions.`
)} but definitions DO NOT match.`
);
}

node.$ref = this.saveComponent(
resolvedRef,
rootDocument.components as Record<string, unknown>
);
// Ref pointer might be modified by previous processors
// resolvedRef.pointer always has the original value
// while node.$ref might have updated
const currentRefPointer = parseRef(node.$ref).pointer;

node.$ref = this.saveComponent(currentRefPointer, resolvedRef.refNode, rootDocument);

this.refs.set(resolvedRef.pointer, resolvedRef);
this.refs.set(currentRefPointer, resolvedRef);
}
}

getBundledRefs(): IterableIterator<ResolvedRef> {
return this.refs.values();
}

private saveComponent(ref: ResolvedRef, components: Record<string, unknown>): string {
insertRefByPointer(ref.pointer, ref.refNode, components);
private saveComponent(pointer: string, refNode: DocumentNode, document: Document): string {
insertRefByPointer(pointer, refNode, document);

return `#${ref.pointer}`;
return `#${pointer}`;
}

private extractParentContext(context: TraverseDocumentContext): TraverseRootDocumentContext {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/*
* 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 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 or the Server
* Side Public License, v 1.
*/

import { createNamespaceComponentsProcessor } from './namespace_components';

describe('namespaceComponentsProcessor', () => {
it.each([
{
sourceValue: 'Something',
originalRef: '#/components/schemas/SomeComponent',
expectedRef: '#/components/schemas/Something_SomeComponent',
},
{
sourceValue: 'Some Domain API (Extra Information)',
originalRef: '#/components/schemas/SomeComponent',
expectedRef: '#/components/schemas/Some_Domain_API_SomeComponent',
},
{
sourceValue: 'Hello, world!',
originalRef: '#/components/schemas/SomeComponent',
expectedRef: '#/components/schemas/Hello_world_SomeComponent',
},
{
sourceValue: 'Something',
originalRef: '../path/to/some.schema.yaml#/components/schemas/SomeComponent',
expectedRef: '../path/to/some.schema.yaml#/components/schemas/Something_SomeComponent',
},
{
sourceValue: 'Some Domain API (Extra Information)',
originalRef: '../path/to/some.schema.yaml#/components/schemas/SomeComponent',
expectedRef: '../path/to/some.schema.yaml#/components/schemas/Some_Domain_API_SomeComponent',
},
{
sourceValue: 'Hello, world!',
originalRef: '../path/to/some.schema.yaml#/components/schemas/SomeComponent',
expectedRef: '../path/to/some.schema.yaml#/components/schemas/Hello_world_SomeComponent',
},
])(
'prefixes reference "$originalRef" with normalized "$sourceValue"',
({ sourceValue, originalRef, expectedRef }) => {
const processor = createNamespaceComponentsProcessor('/info/title');

const document = {
info: {
title: sourceValue,
},
};

processor.onNodeEnter?.(document, {
resolvedDocument: { absolutePath: '', document },
isRootNode: true,
parentNode: document,
parentKey: '',
});

const node = { $ref: originalRef };

processor.onRefNodeLeave?.(
node,
{ pointer: '', refNode: {}, absolutePath: '', document: {} },
{
resolvedDocument: { absolutePath: '', document },
isRootNode: false,
parentNode: document,
parentKey: '',
}
);

expect(node).toMatchObject({
$ref: expectedRef,
});
}
);

it('prefixes security requirements', () => {
const processor = createNamespaceComponentsProcessor('/info/title');

const document = {
info: {
title: 'Something',
},
};

processor.onNodeEnter?.(document, {
resolvedDocument: { absolutePath: '', document },
isRootNode: true,
parentNode: document,
parentKey: '',
});

const node = { security: [{ SomeSecurityRequirement: [] }] };

processor.onNodeLeave?.(node, {
resolvedDocument: { absolutePath: '', document },
isRootNode: false,
parentNode: document,
parentKey: '',
});

// eslint-disable-next-line @typescript-eslint/naming-convention
expect(node).toMatchObject({ security: [{ Something_SomeSecurityRequirement: [] }] });
});

it('prefixes security requirement components', () => {
const processor = createNamespaceComponentsProcessor('/info/title');

const document = {
info: {
title: 'Something',
},
components: {
securitySchemes: {
BasicAuth: {
scheme: 'basic',
type: 'http',
},
},
},
};

processor.onNodeEnter?.(document, {
resolvedDocument: { absolutePath: '', document },
isRootNode: true,
parentNode: document,
parentKey: '',
});

processor.onNodeLeave?.(document, {
resolvedDocument: { absolutePath: '', document },
isRootNode: true,
parentNode: document,
parentKey: '',
});

expect(document.components.securitySchemes).toMatchObject({
// eslint-disable-next-line @typescript-eslint/naming-convention
Something_BasicAuth: {
scheme: 'basic',
type: 'http',
},
});
});
});
Loading

0 comments on commit 01b99e7

Please sign in to comment.