Fail on missing properties. (#342)

* Fail on missing properties.

Signed-off-by: dblock <[email protected]>

CHANGELOG.md

@@ -21,6 +21,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Added support to read outputs from requests in tests([#324](https://github.com/opensearch-project/opensearch-api-specification/pull/324))
 - Added `eslint-plugin-eslint-comments` ([#333](https://github.com/opensearch-project/opensearch-api-specification/pull/333))
 - Added `distribution` field to `OpenSearchVersionInfo` ([#336](https://github.com/opensearch-project/opensearch-api-specification/pull/336)) 
+- Added `created_time` and `last_updated_time` to `ml.get_model_group@200` ([#342](https://github.com/opensearch-project/opensearch-api-specification/pull/342))
 
 ### Changed
 

package.json

@@ -36,6 +36,7 @@
     "@eslint/js": "^9.1.1",
     "@types/jest": "^29.5.12",
     "@typescript-eslint/eslint-plugin": "^6.21.0",
+    "ajv-errors": "^3.0.0",
     "eslint": "^8.57.0",
     "eslint-config-standard-with-typescript": "^43.0.1",
     "eslint-plugin-eslint-comments": "^3.2.0",

spec/namespaces/ml.yaml

@@ -177,6 +177,12 @@ components:
               access:
                 type: string
                 description: The model group access.
+              created_time:
+                type: integer
+                format: int64
+              last_updated_time:
+                type: integer
+                format: int64
             required:
               - name
               - latest_version

tools/src/helpers.ts

@@ -93,8 +93,12 @@ export function write_yaml (file_path: string, content: any): void {
     }))
 }
 
+export function to_json(content: any, replacer?: (this: any, key: string, value: any) => any): string {
+  return JSON.stringify(content, replacer, 2)
+}
+
 export function write_json (file_path: string, content: any, replacer?: (this: any, key: string, value: any) => any): void {
-  write_text(file_path, JSON.stringify(content, replacer, 2))
+  write_text(file_path, to_json(content, replacer))
 }
 
 export async function sleep (ms: number): Promise<void> {

tools/src/linter/SchemaRefsValidator.ts

@@ -28,7 +28,7 @@ export default class SchemaRefsValidator {
 
   #find_refs_in_namespaces_folder (): void {
     const search = (obj: any): void => {
-      const ref: string = obj.$ref ?? ''
+      const ref: string = obj?.$ref ?? ''
       if (ref !== '') {
         const file = ref.split('#')[0].replace('../', '')
         const name = ref.split('/').pop() ?? ''

tools/src/linter/components/base/FileValidator.ts

@@ -10,7 +10,7 @@
 import ValidatorBase from './ValidatorBase'
 import { type ValidationError } from 'types'
 import { type OpenAPIV3 } from 'openapi-types'
-import { read_yaml } from '../../../helpers'
+import { read_yaml, to_json } from '../../../helpers'
 import AJV from 'ajv'
 import addFormats from 'ajv-formats'
 
@@ -65,7 +65,7 @@ export default class FileValidator extends ValidatorBase {
     addFormats(ajv)
     const validator = ajv.compile(schema)
     if (!validator(this.spec())) {
-      return this.error(`File content does not match JSON schema found in '${json_schema_path}':\n ${JSON.stringify(validator.errors, null, 2)}`)
+      return this.error(`File content does not match JSON schema found in '${json_schema_path}':\n ${to_json(validator.errors)}`)
     }
   }
 }

tools/src/merger/OpenApiMerger.ts

@@ -85,7 +85,9 @@ export default class OpenApiMerger {
       const spec = read_yaml(`${folder}/${file}`)
       const category = file.split('.yaml')[0]
       this.redirect_refs_in_schema(category, spec)
-      this.schemas[category] = spec.components.schemas as Record<string, OpenAPIV3.SchemaObject>
+      if (spec.components?.schemas !== undefined) {
+        this.schemas[category] = spec.components?.schemas
+      }
     })
 
     Object.entries(this.schemas).forEach(([category, schemas]) => {
@@ -97,7 +99,7 @@ export default class OpenApiMerger {
 
   // Redirect schema references in schema files to local references in single-file spec.
   redirect_refs_in_schema (category: string, obj: any): void {
-    const ref: string = obj.$ref ?? ''
+    const ref: string = obj?.$ref ?? ''
     if (ref !== '') {
       if (ref.startsWith('#/components/schemas')) { obj.$ref = `#/components/schemas/${category}:${ref.split('/').pop()}` } else {
         const other_category = ref.match(/(.*)\.yaml/)?.[1] ?? ''

tools/src/tester/ChapterReader.ts

@@ -10,31 +10,41 @@
 import { type ChapterRequest, type ActualResponse, type Parameter } from './types/story.types'
 import { type OpenSearchHttpClient } from '../OpenSearchHttpClient'
 import { type StoryOutputs } from './StoryOutputs'
+import { Logger } from 'Logger'
+import { to_json } from '../helpers'
 
 // A lightweight client for testing the API
 export default class ChapterReader {
   private readonly _client: OpenSearchHttpClient
+  private readonly logger: Logger
 
-  constructor (client: OpenSearchHttpClient) {
+  constructor (client: OpenSearchHttpClient, logger: Logger) {
     this._client = client
+    this.logger = logger
   }
 
   async read (chapter: ChapterRequest, story_outputs: StoryOutputs): Promise<ActualResponse> {
     const response: Record<string, any> = {}
     const resolved_params = story_outputs.resolve_params(chapter.parameters ?? {})
     const [url_path, params] = this.#parse_url(chapter.path, resolved_params)
     const request_data = chapter.request_body?.payload !== undefined ? story_outputs.resolve_value(chapter.request_body.payload) : undefined
+    this.logger.info(`=> ${chapter.method} ${url_path} (${to_json(params)}) | ${to_json(request_data)}`)
     await this._client.request({
       url: url_path,
       method: chapter.method,
       params,
       data: request_data
     }).then(r => {
+      this.logger.info(`<= ${r.status} (${r.headers['content-type']}) | ${to_json(r.data)}`)
       response.status = r.status
       response.content_type = r.headers['content-type'].split(';')[0]
       response.payload = r.data
     }).catch(e => {
-      if (e.response == null) throw e
+      if (e.response == null) {
+        this.logger.info(`<= ERROR: ${e}`)
+        throw e
+      }
+      this.logger.info(`<= ${e.response.status} (${e.response.headers['content-type']}) | ${to_json(e.response.data)}`)
       response.status = e.response.status
       response.content_type = e.response.headers['content-type'].split(';')[0]
       response.payload = e.response.data?.error

tools/src/tester/MergedOpenApiSpec.ts

@@ -0,0 +1,49 @@
+/*
+* Copyright OpenSearch Contributors
+* SPDX-License-Identifier: Apache-2.0
+*
+* The OpenSearch Contributors require contributions made to
+* this file be licensed under the Apache-2.0 license or a
+* compatible open source license.
+*/
+
+import { type OpenAPIV3 } from 'openapi-types'
+import { Logger } from '../Logger'
+import { SpecificationContext } from '../linter/utils';
+import { SchemaVisitor } from '../linter/utils/SpecificationVisitor';
+import OpenApiMerger from '../merger/OpenApiMerger';
+
+// An augmented spec with additionalProperties: false.
+export default class MergedOpenApiSpec {
+  logger: Logger
+  file_path: string
+  protected _spec: OpenAPIV3.Document | undefined
+
+  constructor (spec_path: string, logger: Logger = new Logger()) {
+    this.logger = logger
+    this.file_path = spec_path
+  }
+
+  spec (): OpenAPIV3.Document {
+    if (this._spec) return this._spec
+    const spec = (new OpenApiMerger(this.file_path, this.logger)).merge()
+    const ctx = new SpecificationContext(this.file_path)
+    this.inject_additional_properties(ctx, spec)
+    this._spec = spec
+    return this._spec
+  }
+
+  private inject_additional_properties(ctx: SpecificationContext, spec: OpenAPIV3.Document): void {
+    const visitor = new SchemaVisitor((_ctx, schema: any) => {
+      if (schema.required !== undefined && schema.properties !== undefined && schema.additionalProperties === undefined) {
+        // causes any undeclared field in the response to produce an error
+        schema.additionalProperties = {
+          not: true,
+          errorMessage: "property is not defined in the spec"
+        }
+      }
+    })
+
+    visitor.visit_specification(ctx, spec)
+  }
+}

tools/src/tester/ResultLogger.ts

@@ -84,11 +84,6 @@ export class ConsoleResultLogger implements ResultLogger {
     const result = ansi.padding(this.#result(evaluation.result), 0, prefix)
     const message = evaluation.message != null ? `${ansi.gray('(' + evaluation.message + ')')}` : ''
     console.log(`${result} ${title} ${message}`)
-    if (evaluation.error != null && this._verbose) {
-      console.log('-'.repeat(100))
-      console.error(evaluation.error)
-      console.log('-'.repeat(100))
-    }
   }
 
   #result (r: Result): string {

tools/src/tester/SchemaValidator.ts

@@ -8,15 +8,22 @@
 */
 
 import AJV from 'ajv'
+import ajv_errors from 'ajv-errors'
 import addFormats from 'ajv-formats'
 import { type OpenAPIV3 } from 'openapi-types'
 import { type Evaluation, Result } from './types/eval.types'
+import { Logger } from 'Logger'
+import { to_json } from '../helpers'
 
 export default class SchemaValidator {
   private readonly ajv: AJV
-  constructor (spec: OpenAPIV3.Document) {
+  private readonly logger: Logger
+
+  constructor (spec: OpenAPIV3.Document, logger: Logger) {
+    this.logger = logger
     this.ajv = new AJV({ allErrors: true, strict: true })
     addFormats(this.ajv)
+    ajv_errors(this.ajv, { singleError: true })
     this.ajv.addKeyword('discriminator')
     const schemas = spec.components?.schemas ?? {}
     for (const key in schemas) this.ajv.addSchema(schemas[key], `#/components/schemas/${key}`)
@@ -25,6 +32,10 @@ export default class SchemaValidator {
   validate (schema: OpenAPIV3.SchemaObject, data: any): Evaluation {
     const validate = this.ajv.compile(schema)
     const valid = validate(data)
+    if (! valid) {
+      this.logger.info(`# ${to_json(schema)}`)
+      this.logger.info(`* ${to_json(data)}`)
+    }
     return {
       result: valid ? Result.PASSED : Result.FAILED,
       message: valid ? undefined : this.ajv.errorsText(validate.errors)

tools/src/tester/StoryEvaluator.ts

@@ -152,7 +152,7 @@ export default class StoryEvaluator {
     StoryEvaluator.#extract_request_body_variables(chapter.request_body?.payload ?? {}, variables)
     for (const { chapter_id, output_name } of variables) {
       if (!story_outputs.has_chapter(chapter_id)) {
-        return  StoryEvaluator.#failed_evaluation(title, `Chapter makes reference to non existent chapter "${chapter_id}`)
+        return StoryEvaluator.#failed_evaluation(title, `Chapter makes reference to non existent chapter "${chapter_id}`)
       }
       if (!story_outputs.has_output_value(chapter_id, output_name)) {
         return StoryEvaluator.#failed_evaluation(title, `Chapter makes reference to non existent output "${output_name}" in chapter "${chapter_id}"`)
@@ -202,5 +202,4 @@ export default class StoryEvaluator {
   static #failed_evaluation(title: string, message: string): ChapterEvaluation {
     return { title, overall: { result: Result.FAILED, message } }
   }
-
 }

tools/src/tester/test.ts

@@ -7,7 +7,6 @@
 * compatible open source license.
 */
 
-import OpenApiMerger from '../merger/OpenApiMerger'
 import { LogLevel, Logger } from '../Logger'
 import TestRunner from './TestRunner'
 import { Command, Option } from '@commander-js/extra-typings'
@@ -26,6 +25,7 @@ import StoryEvaluator from './StoryEvaluator'
 import { ConsoleResultLogger } from './ResultLogger'
 import * as process from 'node:process'
 import SupplementalChapterEvaluator from './SupplementalChapterEvaluator'
+import MergedOpenApiSpec from './MergedOpenApiSpec'
 
 const command = new Command()
   .description('Run test stories against the OpenSearch spec.')
@@ -48,11 +48,10 @@ const command = new Command()
 const opts = command.opts()
 const logger = new Logger(opts.verbose ? LogLevel.info : LogLevel.warn)
 
-const spec = (new OpenApiMerger(opts.specPath, logger)).merge()
-
+const spec = (new MergedOpenApiSpec(opts.specPath, logger)).spec()
 const http_client = new OpenSearchHttpClient(get_opensearch_opts_from_cli(opts))
-const chapter_reader = new ChapterReader(http_client)
-const chapter_evaluator = new ChapterEvaluator(new OperationLocator(spec), chapter_reader, new SchemaValidator(spec))
+const chapter_reader = new ChapterReader(http_client, logger)
+const chapter_evaluator = new ChapterEvaluator(new OperationLocator(spec), chapter_reader, new SchemaValidator(spec, logger))
 const supplemental_chapter_evaluator = new SupplementalChapterEvaluator(chapter_reader)
 const story_evaluator = new StoryEvaluator(chapter_evaluator, supplemental_chapter_evaluator)
 const result_logger = new ConsoleResultLogger(opts.tabWidth, opts.verbose)

tools/tests/tester/MergedOpenApiSpec.test.ts

@@ -0,0 +1,36 @@
+/*
+* Copyright OpenSearch Contributors
+* SPDX-License-Identifier: Apache-2.0
+*
+* The OpenSearch Contributors require contributions made to
+* this file be licensed under the Apache-2.0 license or a
+* compatible open source license.
+*/
+
+import { Logger } from 'Logger'
+import MergedOpenApiSpec from "tester/MergedOpenApiSpec"
+
+describe('additionalProperties', () => {
+  const spec = new MergedOpenApiSpec('tools/tests/tester/fixtures/specs/complete', new Logger())
+  const responses: any = spec.spec().components?.responses
+
+  test('is added with required fields', () => {
+    const schema = responses['info@200'].content['application/json'].schema
+    expect(schema.additionalProperties).toEqual({ not: true, errorMessage: 'property is not defined in the spec' })
+  })
+
+  test('is not added when true', () => {
+    const schema = responses['info@201'].content['application/json'].schema
+    expect(schema.additionalProperties).toEqual(true)
+  })
+
+  test('is not added when object', () => {
+    const schema = responses['info@404'].content['application/json'].schema
+    expect(schema.additionalProperties).toEqual({ type: 'object' })
+  })
+
+  test('is not added unless required is present', () => {
+    const schema = responses['info@500'].content['application/json'].schema
+    expect(schema.additionalProperties).toBeUndefined()
+  })
+})

tools/tests/tester/fixtures/specs/complete/_global_parameters.yaml

@@ -0,0 +1,4 @@
+openapi: 3.1.0
+info:
+  title: ''
+  version: ''

tools/tests/tester/fixtures/specs/complete/_info.yaml

@@ -0,0 +1,5 @@
+$schema: should-be-ignored
+
+title: OpenSearch API
+description: OpenSearch API
+version: 1.0.0