From 25c6ec120e6836de85a14a92d71bbadd1f79763a Mon Sep 17 00:00:00 2001 From: dblock Date: Tue, 16 Jul 2024 17:17:52 -0400 Subject: [PATCH] Delete schema elements that don't match target OpenSearch version. Signed-off-by: dblock --- .github/workflows/test-spec.yml | 2 +- CHANGELOG.md | 1 + tools/src/linter/SchemasValidator.ts | 3 +- tools/src/merger/OpenApiMerger.ts | 96 ++++++++++++++----- tools/src/merger/merge.ts | 4 +- tools/src/tester/MergedOpenApiSpec.ts | 8 +- tools/src/tester/test.ts | 3 +- tools/tests/merger/OpenApiMerger.test.ts | 4 +- tools/tests/tester/MergedOpenApiSpec.test.ts | 21 +++- .../specs/complete/namespaces/index.yaml | 16 ++++ 10 files changed, 122 insertions(+), 36 deletions(-) diff --git a/.github/workflows/test-spec.yml b/.github/workflows/test-spec.yml index 7938d925e..b40b2b09c 100644 --- a/.github/workflows/test-spec.yml +++ b/.github/workflows/test-spec.yml @@ -53,4 +53,4 @@ jobs: run: docker-compose up -d - name: Run Tests - run: npm run test:spec -- --opensearch-insecure \ No newline at end of file + run: npm run test:spec -- --opensearch-insecure --version=${{ matrix.entry.version }} \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index c417fbdad..7a3a1d42d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Added missing fields to `/_nodes/stats` ([#415](https://github.com/opensearch-project/opensearch-api-specification/pull/415)) - Added missing metrics options to `/_nodes/stats` ([#422](https://github.com/opensearch-project/opensearch-api-specification/pull/422)) - Added tests against OpenSearch 1.3 ([#424](https://github.com/opensearch-project/opensearch-api-specification/pull/424)) +- Added support for excluding schema elements that were removed before or added after target version ([#428](https://github.com/opensearch-project/opensearch-api-specification/pull/428)) ### Changed diff --git a/tools/src/linter/SchemasValidator.ts b/tools/src/linter/SchemasValidator.ts index 5192595c7..6954d68ab 100644 --- a/tools/src/linter/SchemasValidator.ts +++ b/tools/src/linter/SchemasValidator.ts @@ -32,7 +32,8 @@ export default class SchemasValidator { } validate (): ValidationError[] { - this.spec = new OpenApiMerger(this.root_folder, new Logger(LogLevel.error)).merge().components as Record + const merger = new OpenApiMerger(this.root_folder, undefined, new Logger(LogLevel.error)) + this.spec = merger.merge().spec().components as Record const named_schemas_errors = this.validate_named_schemas() if (named_schemas_errors.length > 0) return named_schemas_errors return [ diff --git a/tools/src/merger/OpenApiMerger.ts b/tools/src/merger/OpenApiMerger.ts index 51907863d..85e51da33 100644 --- a/tools/src/merger/OpenApiMerger.ts +++ b/tools/src/merger/OpenApiMerger.ts @@ -14,20 +14,25 @@ import { read_yaml, write_yaml } from '../helpers' import SupersededOpsGenerator from './SupersededOpsGenerator' import GlobalParamsGenerator from './GlobalParamsGenerator' import { Logger } from '../Logger' +import * as semver from 'semver' // Create a single-file OpenAPI spec from multiple files for OpenAPI validation and programmatic consumption export default class OpenApiMerger { root_folder: string - spec: Record logger: Logger + target_version?: string + + protected _spec: Record + protected _merged: boolean = false paths: Record> = {} // namespace -> path -> path_item_object schemas: Record> = {} // category -> schema -> schema_object - constructor (root_folder: string, logger: Logger = new Logger()) { + constructor (root_folder: string, target_version?: string, logger: Logger = new Logger()) { this.logger = logger this.root_folder = fs.realpathSync(root_folder) - this.spec = { + this.target_version = target_version + this._spec = { openapi: '3.1.0', info: read_yaml(`${this.root_folder}/_info.yaml`, true), paths: {}, @@ -40,17 +45,27 @@ export default class OpenApiMerger { } } - merge (output_path: string = ''): OpenAPIV3.Document { + write_to(output_path: string): OpenApiMerger { + if (!this._merged) { throw "Call merge() first." } + this.logger.info(`Writing ${output_path} ...`) + write_yaml(output_path, this._spec) + return this + } + + merge (): OpenApiMerger { + if (this._merged) { throw "Spec already merged." } this.#merge_schemas() this.#merge_namespaces() this.#sort_spec_keys() this.#generate_global_params() this.#generate_superseded_ops() + this._merged = true + return this + } - this.logger.info(`Writing ${output_path} ...`) - - if (output_path !== '') write_yaml(output_path, this.spec) - return this.spec as OpenAPIV3.Document + spec(): OpenAPIV3.Document { + if (!this._merged) { throw "Call merge() first." } + return this._spec as OpenAPIV3.Document } // Merge files from /namespaces folder. @@ -59,21 +74,50 @@ export default class OpenApiMerger { fs.readdirSync(folder).forEach(file => { this.logger.info(`Merging namespaces in ${folder}/${file} ...`) const spec = read_yaml(`${folder}/${file}`) - this.redirect_refs_in_namespace(spec) - this.spec.paths = { ...this.spec.paths, ...spec.paths } - this.spec.components.parameters = { ...this.spec.components.parameters, ...spec.components.parameters } - this.spec.components.responses = { ...this.spec.components.responses, ...spec.components.responses } - this.spec.components.requestBodies = { ...this.spec.components.requestBodies, ...spec.components.requestBodies } + this.#redirect_refs_in_namespace(spec) + this._spec.paths = { ...this._spec.paths, ...spec.paths } + this._spec.components.parameters = { ...this._spec.components.parameters, ...spec.components.parameters } + this._spec.components.responses = { ...this._spec.components.responses, ...spec.components.responses } + this._spec.components.requestBodies = { ...this._spec.components.requestBodies, ...spec.components.requestBodies } }) + + this.#remove_per_semver(this._spec.paths) + this.#remove_per_semver(this._spec.components.parameters) + this.#remove_per_semver(this._spec.components.responses) + this.#remove_per_semver(this._spec.components.requestBodies) + } + + // Remove any elements that are x-version-added/removed incompatible with the target server version. + #remove_per_semver(obj: any): any { + if (this.target_version === undefined) return + + for (const key in obj) { + if (typeof obj[key] === 'object') { + const x_version_added = obj[key]['x-version-added'] + const x_version_removed = obj[key]['x-version-removed'] + + if (this.target_version !== undefined) { + if (x_version_added !== undefined && !semver.satisfies(this.target_version, `>= ${x_version_added}`)) { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete obj[key] + } else if (x_version_removed!== undefined && !semver.satisfies(this.target_version, `< ${x_version_removed}`)) { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete obj[key] + } else { + this.#remove_per_semver(obj[key]) + } + } + } + } } // Redirect schema references in namespace files to local references in single-file spec. - redirect_refs_in_namespace (obj: any): void { + #redirect_refs_in_namespace (obj: any): void { const ref: string = obj?.$ref if (ref?.startsWith('../schemas/')) { obj.$ref = ref.replace('../schemas/', '#/components/schemas/').replace('.yaml#/components/schemas/', ':') } for (const key in obj) { - if (typeof obj[key] === 'object') { this.redirect_refs_in_namespace(obj[key]) } + if (typeof obj[key] === 'object') { this.#redirect_refs_in_namespace(obj[key]) } } } @@ -92,7 +136,7 @@ export default class OpenApiMerger { Object.entries(this.schemas).forEach(([category, schemas]) => { Object.entries(schemas).forEach(([name, schema_obj]) => { - this.spec.components.schemas[`${category}:${name}`] = schema_obj + this._spec.components.schemas[`${category}:${name}`] = schema_obj }) }) } @@ -115,26 +159,26 @@ export default class OpenApiMerger { // Sort keys in the spec to make it easier to read and compare. #sort_spec_keys (): void { - this.spec.components.schemas = _.fromPairs(Object.entries(this.spec.components.schemas as Document).sort((a, b) => a[0].localeCompare(b[0]))) - this.spec.components.parameters = _.fromPairs(Object.entries(this.spec.components.parameters as Document).sort((a, b) => a[0].localeCompare(b[0]))) - this.spec.components.responses = _.fromPairs(Object.entries(this.spec.components.responses as Document).sort((a, b) => a[0].localeCompare(b[0]))) - this.spec.components.requestBodies = _.fromPairs(Object.entries(this.spec.components.requestBodies as Document).sort((a, b) => a[0].localeCompare(b[0]))) - - this.spec.paths = _.fromPairs(Object.entries(this.spec.paths as Document).sort((a, b) => a[0].localeCompare(b[0]))) - Object.entries(this.spec.paths as Document).forEach(([path, path_item]) => { - this.spec.paths[path] = _.fromPairs(Object.entries(path_item as Document).sort((a, b) => a[0].localeCompare(b[0]))) + this._spec.components.schemas = _.fromPairs(Object.entries(this._spec.components.schemas as Document).sort((a, b) => a[0].localeCompare(b[0]))) + this._spec.components.parameters = _.fromPairs(Object.entries(this._spec.components.parameters as Document).sort((a, b) => a[0].localeCompare(b[0]))) + this._spec.components.responses = _.fromPairs(Object.entries(this._spec.components.responses as Document).sort((a, b) => a[0].localeCompare(b[0]))) + this._spec.components.requestBodies = _.fromPairs(Object.entries(this._spec.components.requestBodies as Document).sort((a, b) => a[0].localeCompare(b[0]))) + + this._spec.paths = _.fromPairs(Object.entries(this._spec.paths as Document).sort((a, b) => a[0].localeCompare(b[0]))) + Object.entries(this._spec.paths as Document).forEach(([path, path_item]) => { + this._spec.paths[path] = _.fromPairs(Object.entries(path_item as Document).sort((a, b) => a[0].localeCompare(b[0]))) }) } // Generate global parameters from _global_params.yaml file. #generate_global_params (): void { const gen = new GlobalParamsGenerator(this.root_folder) - gen.generate(this.spec) + gen.generate(this._spec) } // Generate superseded operations from _superseded_operations.yaml file. #generate_superseded_ops (): void { const gen = new SupersededOpsGenerator(this.root_folder, this.logger) - gen.generate(this.spec) + gen.generate(this._spec) } } diff --git a/tools/src/merger/merge.ts b/tools/src/merger/merge.ts index 50af3360b..404f8e6a5 100644 --- a/tools/src/merger/merge.ts +++ b/tools/src/merger/merge.ts @@ -22,7 +22,7 @@ const command = new Command() const opts = command.opts() const logger = new Logger(opts.verbose ? LogLevel.info : LogLevel.warn) -const merger = new OpenApiMerger(opts.source, logger) +const merger = new OpenApiMerger(opts.source, undefined, logger) logger.log(`Merging ${opts.source} into ${opts.output} ...`) -merger.merge(opts.output) +merger.merge().write_to(opts.output) logger.log('Done.') diff --git a/tools/src/tester/MergedOpenApiSpec.ts b/tools/src/tester/MergedOpenApiSpec.ts index 93a1e25dc..ddc157ece 100644 --- a/tools/src/tester/MergedOpenApiSpec.ts +++ b/tools/src/tester/MergedOpenApiSpec.ts @@ -17,16 +17,20 @@ import OpenApiMerger from '../merger/OpenApiMerger'; export default class MergedOpenApiSpec { logger: Logger file_path: string + target_version?: string + protected _spec: OpenAPIV3.Document | undefined - constructor (spec_path: string, logger: Logger = new Logger()) { + constructor (spec_path: string, target_version?: string, logger: Logger = new Logger()) { this.logger = logger this.file_path = spec_path + this.target_version = target_version } spec (): OpenAPIV3.Document { if (this._spec) return this._spec - const spec = (new OpenApiMerger(this.file_path, this.logger)).merge() + const merger = new OpenApiMerger(this.file_path, this.target_version, this.logger) + const spec = merger.merge().spec() const ctx = new SpecificationContext(this.file_path) this.inject_additional_properties(ctx, spec) this._spec = spec diff --git a/tools/src/tester/test.ts b/tools/src/tester/test.ts index 6fa7b5f68..e69cf8036 100644 --- a/tools/src/tester/test.ts +++ b/tools/src/tester/test.ts @@ -31,6 +31,7 @@ import StoryValidator from "./StoryValidator"; const command = new Command() .description('Run test stories against the OpenSearch spec.') + .addOption(new Option('--version ', 'target schema version').default('2.0')) .addOption(new Option('--spec, --spec-path ', 'path to the root folder of the multi-file spec').default('./spec')) .addOption(new Option('--tests, --tests-path ', 'path to the root folder of the tests').default('./tests')) .addOption( @@ -50,7 +51,7 @@ const command = new Command() const opts = command.opts() const logger = new Logger(opts.verbose ? LogLevel.info : LogLevel.warn) -const spec = new MergedOpenApiSpec(opts.specPath, new Logger(LogLevel.error)) +const spec = new MergedOpenApiSpec(opts.specPath, opts.version, new Logger(LogLevel.error)) const http_client = new OpenSearchHttpClient(get_opensearch_opts_from_cli({ opensearchResponseType: 'arraybuffer', ...opts })) const chapter_reader = new ChapterReader(http_client, logger) const chapter_evaluator = new ChapterEvaluator(new OperationLocator(spec.spec()), chapter_reader, new SchemaValidator(spec.spec(), logger), logger) diff --git a/tools/tests/merger/OpenApiMerger.test.ts b/tools/tests/merger/OpenApiMerger.test.ts index 72eeef042..e0faa8c39 100644 --- a/tools/tests/merger/OpenApiMerger.test.ts +++ b/tools/tests/merger/OpenApiMerger.test.ts @@ -12,8 +12,8 @@ import fs from 'fs' import { Logger, LogLevel } from 'Logger' test('merge()', () => { - const merger = new OpenApiMerger('./tools/tests/merger/fixtures/spec/', new Logger(LogLevel.error)) - merger.merge('./tools/tests/merger/opensearch-openapi.yaml') + const merger = new OpenApiMerger('./tools/tests/merger/fixtures/spec/','2.0', new Logger(LogLevel.error)) + merger.merge().write_to('./tools/tests/merger/opensearch-openapi.yaml') expect(fs.readFileSync('./tools/tests/merger/fixtures/expected.yaml', 'utf8')) .toEqual(fs.readFileSync('./tools/tests/merger/opensearch-openapi.yaml', 'utf8')) fs.unlinkSync('./tools/tests/merger/opensearch-openapi.yaml') diff --git a/tools/tests/tester/MergedOpenApiSpec.test.ts b/tools/tests/tester/MergedOpenApiSpec.test.ts index 2acda940d..985235b23 100644 --- a/tools/tests/tester/MergedOpenApiSpec.test.ts +++ b/tools/tests/tester/MergedOpenApiSpec.test.ts @@ -8,10 +8,11 @@ */ import { Logger } from 'Logger' +import _ from 'lodash' import MergedOpenApiSpec from "tester/MergedOpenApiSpec" describe('unevaluatedProperties', () => { - const spec = new MergedOpenApiSpec('tools/tests/tester/fixtures/specs/complete', new Logger()) + const spec = new MergedOpenApiSpec('tools/tests/tester/fixtures/specs/complete', '2.0', new Logger()) const responses: any = spec.spec().components?.responses test('has an api version', () => { @@ -42,4 +43,22 @@ describe('unevaluatedProperties', () => { const schema = responses['info@404'].content['application/json'].schema expect(schema.unevaluatedProperties).toEqual({ type: 'object' }) }) + + test('has matching 2.0 responses', () => { + expect(_.keys(spec.spec().paths['/index']?.get?.responses)).toEqual([ + '200', '201', '404', '407', '500' + ]) + }) + + test('excludes schema added after 2.0', () => { + expect(spec.spec().paths['/index']?.get?.responses['405']).toBeUndefined() + }) + + test('excludes schema removed before 2.0', () => { + expect(spec.spec().paths['/index']?.get?.responses['406']).toBeUndefined() + }) + + test('excludes schema added/removed in the response type', () => { + expect(responses['info@407']).toBeUndefined() + }) }) diff --git a/tools/tests/tester/fixtures/specs/complete/namespaces/index.yaml b/tools/tests/tester/fixtures/specs/complete/namespaces/index.yaml index 5cf78308e..92a762b00 100644 --- a/tools/tests/tester/fixtures/specs/complete/namespaces/index.yaml +++ b/tools/tests/tester/fixtures/specs/complete/namespaces/index.yaml @@ -14,6 +14,14 @@ paths: $ref: '#/components/responses/info@201' '404': $ref: '#/components/responses/info@404' + '405': + $ref: '#/components/responses/info@405' + x-version-added: 2.99 + '406': + $ref: '#/components/responses/info@406' + x-version-removed: 1.99 + '407': + $ref: '#/components/responses/info@407' '500': $ref: '#/components/responses/info@500' components: @@ -54,6 +62,14 @@ components: - tagline unevaluatedProperties: type: object + info@405: + description: '' + info@406: + description: '' + info@407: + description: '' + x-version-added: 1.99 + x-version-removed: 2.99 info@500: description: '' content: