Switch validating to use unevaluatedProperties (#412)

* Re-add `_all` as a valid input for nodes.info metrics Signed-off-by: Thomas Farr <[email protected]> * Correct nodes.info ResponseBase Signed-off-by: Thomas Farr <[email protected]> * Switch to unevaluatedProperties for validating unknown properties Signed-off-by: Thomas Farr <[email protected]> * Spec fixes Signed-off-by: Thomas Farr <[email protected]> * Extra test case Signed-off-by: Thomas Farr <[email protected]> --------- Signed-off-by: Thomas Farr <[email protected]>
opensearch-project · Jul 12, 2024 · 188413c · 188413c
1 parent d5ca873
commit 188413c
Show file tree

Hide file tree

Showing 16 changed files with 122 additions and 69 deletions.
diff --git a/.gitignore b/.gitignore
@@ -17,5 +17,5 @@ _site/
 build/
 
 # coverage output
-coverage/
+/coverage/
 
diff --git a/spec/namespaces/cluster.yaml b/spec/namespaces/cluster.yaml
@@ -514,12 +514,10 @@ components:
             properties:
               persistent:
                 type: object
-                additionalProperties:
-                  type: object
+                additionalProperties: { }
               transient:
                 type: object
-                additionalProperties:
-                  type: object
+                additionalProperties: { }
             description: The settings to be updated. Can be either `transient` or `persistent` (survives cluster restart).
       required: true
     cluster.reroute:
@@ -702,12 +700,10 @@ components:
                 type: boolean
               persistent:
                 type: object
-                additionalProperties:
-                  type: object
+                additionalProperties: { }
               transient:
                 type: object
-                additionalProperties:
-                  type: object
+                additionalProperties: { }
             required:
               - acknowledged
               - persistent

diff --git a/spec/namespaces/indices.yaml b/spec/namespaces/indices.yaml
@@ -1878,7 +1878,7 @@ components:
               template:
                 $ref: '../schemas/indices.put_index_template.yaml#/components/schemas/IndexTemplateMapping'
               data_stream:
-                $ref: '../schemas/indices._common.yaml#/components/schemas/DataStreamVisibility'
+                $ref: '../schemas/indices._common.yaml#/components/schemas/IndexTemplateDataStreamConfiguration'
               priority:
                 description: |-
                   Priority to determine index template precedence when a new data stream or index is created.
@@ -2060,7 +2060,7 @@ components:
               template:
                 $ref: '../schemas/indices.put_index_template.yaml#/components/schemas/IndexTemplateMapping'
               data_stream:
-                $ref: '../schemas/indices._common.yaml#/components/schemas/DataStreamVisibility'
+                $ref: '../schemas/indices._common.yaml#/components/schemas/IndexTemplateDataStreamConfiguration'
               priority:
                 description: |-
                   Priority to determine index template precedence when a new data stream or index is created.
@@ -2092,8 +2092,7 @@ components:
               settings:
                 description: Configuration options for the target index.
                 type: object
-                additionalProperties:
-                  type: object
+                additionalProperties: { }
             description: The configuration for the target index (`settings` and `aliases`)
     indices.update_aliases:
       content:

diff --git a/spec/schemas/_common.query_dsl.yaml b/spec/schemas/_common.query_dsl.yaml
@@ -73,7 +73,7 @@ components:
           additionalProperties:
             anyOf:
               - $ref: '#/components/schemas/MatchQuery'
-              - true
+              - { }
           minProperties: 1
           maxProperties: 1
         match_all:

diff --git a/spec/schemas/cat.health.yaml b/spec/schemas/cat.health.yaml
@@ -49,3 +49,6 @@ components:
         active_shards_percent:
           description: active number of shards in percent
           type: string
+        discovered_cluster_manager:
+          description: cluster manager is discovered or not
+          type: string
diff --git a/spec/schemas/indices._common.yaml b/spec/schemas/indices._common.yaml
@@ -137,6 +137,7 @@ components:
           $ref: '#/components/schemas/IndexingPressure'
         store:
           $ref: '#/components/schemas/Storage'
+      additionalProperties: { }
       description: The index settings to be updated
     SoftDeletes:
       type: object
@@ -980,6 +981,8 @@ components:
         allow_custom_routing:
           description: If true, the data stream supports custom routing.
           type: boolean
+        timestamp_field:
+          $ref: '#/components/schemas/DataStreamTimestampField'
     TemplateMapping:
       type: object
       properties:
@@ -1007,8 +1010,3 @@ components:
         - mappings
         - order
         - settings
-    DataStreamVisibility:
-      type: object
-      properties:
-        hidden:
-          type: boolean
diff --git a/spec/schemas/nodes.info.yaml b/spec/schemas/nodes.info.yaml
@@ -21,8 +21,10 @@ components:
         - aggregations
         - indices
         - search_pipelines
+        - _all
     ResponseBase:
       allOf:
+        - $ref: 'nodes._common.yaml#/components/schemas/NodesResponseBase'
         - type: object
           properties:
             cluster_name:
@@ -31,12 +33,9 @@ components:
               type: object
               additionalProperties:
                 $ref: '#/components/schemas/NodeInfo'
-            _nodes:
-              $ref: 'nodes._common.yaml#/components/schemas/NodesResponseBase'
           required:
             - cluster_name
             - nodes
-            - _nodes
     NodeInfo:
       type: object
       properties:
@@ -328,7 +327,7 @@ components:
         repositories:
           $ref: '#/components/schemas/NodeInfoRepositories'
         discovery:
-          $ref: '#/components/schemas/NodeInfoDiscover'
+          $ref: '#/components/schemas/NodeInfoDiscovery'
         action:
           $ref: '#/components/schemas/NodeInfoAction'
         client:
@@ -365,6 +364,8 @@ components:
           $ref: 'indices._common.yaml#/components/schemas/IndexRouting'
         election:
           $ref: '#/components/schemas/NodeInfoSettingsClusterElection'
+        initial_cluster_manager_nodes:
+          type: string
         initial_master_nodes:
           type: string
         deprecation_indexing:
@@ -438,9 +439,11 @@ components:
           type: string
       required:
         - shard_indexing_pressure_enabled
-    NodeInfoDiscover:
+    NodeInfoDiscovery:
       type: object
       properties:
+        type:
+          type: string
         seed_hosts:
           type: string
     NodeInfoAction:
@@ -694,18 +697,11 @@ components:
         response_processors:
           type: array
           items:
-            $ref: '#/components/schemas/NodeInfoIngestProcessor2'
+            $ref: '#/components/schemas/NodeInfoIngestProcessor'
         request_processors:
           type: array
           items:
-            $ref: '#/components/schemas/NodeInfoIngestProcessor2'
+            $ref: '#/components/schemas/NodeInfoIngestProcessor'
       required:
         - response_processors
-        - request_processors
-    NodeInfoIngestProcessor2:
-      type: object
-      properties:
-        type:
-          type: string
-      required:
-        - type
+        - request_processors
diff --git a/tests/_core/nodes.yaml → tests/nodes/info.yaml b/tests/_core/nodes.yaml → tests/nodes/info.yaml
diff --git a/tools/src/_utils/index.ts b/tools/src/_utils/index.ts
@@ -7,11 +7,17 @@
 * compatible open source license.
 */
 
-import { type OpenAPIV3 } from 'openapi-types'
+import { OpenAPIV3 } from 'openapi-types'
 import { type ValidationError } from 'types'
+import _ from "lodash";
+import { to_json } from "../helpers";
+
+export const HTTP_METHODS: OpenAPIV3.HttpMethods[] = Object.values(OpenAPIV3.HttpMethods)
+export type SchemaObjectType = OpenAPIV3.ArraySchemaObjectType | OpenAPIV3.NonArraySchemaObjectType
+export const SCHEMA_OBJECT_TYPES: SchemaObjectType[] = ['array', 'boolean', 'object', 'number', 'string', 'integer']
 
 export function is_ref<O extends object> (o: MaybeRef<O>): o is OpenAPIV3.ReferenceObject {
-  return typeof (o) === 'object' && '$ref' in o
+  return o != null && typeof o === 'object' && '$ref' in o
 }
 
 export function is_array_schema (schema: OpenAPIV3.SchemaObject): schema is OpenAPIV3.ArraySchemaObject {
@@ -25,6 +31,33 @@ export function is_primitive_schema (schema: OpenAPIV3.SchemaObject): boolean {
     schema.type === 'string'
 }
 
+export function determine_possible_schema_types (doc: OpenAPIV3.Document, schema: MaybeRef<OpenAPIV3.SchemaObject>): SchemaObjectType[] {
+  while (is_ref(schema)) {
+    const key = schema.$ref.split('/').pop()
+    if (key === undefined) throw new Error(`Invalid $ref: '${schema.$ref}'`)
+    const resolved = doc.components?.schemas?.[key]
+    if (resolved === undefined) throw new Error(`Invalid $ref: '${schema.$ref}'`)
+    schema = resolved
+  }
+
+  if (schema?.type !== undefined) return [schema.type]
+
+  const collect_all = (schemas: MaybeRef<OpenAPIV3.SchemaObject>[]): SchemaObjectType[] =>
+    _.uniq(schemas.flatMap(s => determine_possible_schema_types(doc, s)))
+
+  if (schema?.allOf !== undefined) {
+    const types = collect_all(schema.allOf)
+    if (types.length > 1) throw new Error(`allOf schema should resolve to a single type, but was: ${types.join(", ")}`)
+    return types
+  }
+  if (schema?.anyOf !== undefined) return collect_all(schema.anyOf)
+  if (schema?.oneOf !== undefined) return collect_all(schema.oneOf)
+
+  if (schema == null || Object.keys(schema).filter(k => k !== 'description').length == 0) return SCHEMA_OBJECT_TYPES
+
+  throw new Error(`Unable to determine possible types of schema: ${to_json(schema)}`)
+}
+
 export class SpecificationContext {
   private readonly _file: string
   private readonly _location: string[]

diff --git a/tools/src/coverage/CoverageCalculator.ts b/tools/src/coverage/CoverageCalculator.ts
@@ -8,7 +8,8 @@
 */
 
 import { type OpenAPIV3 } from 'openapi-types'
-import { HTTP_METHODS, read_yaml, write_json } from '../helpers'
+import { read_yaml, write_json } from '../helpers'
+import { HTTP_METHODS } from "../_utils";
 
 export default class CoverageCalculator {
   private readonly _cluster_spec: OpenAPIV3.Document

diff --git a/tools/src/helpers.ts b/tools/src/helpers.ts
@@ -11,9 +11,6 @@ import fs from 'fs'
 import path from 'path'
 import YAML from 'yaml'
 import _ from 'lodash'
-import { OpenAPIV3 } from 'openapi-types'
-
-export const HTTP_METHODS: OpenAPIV3.HttpMethods[] = Object.values(OpenAPIV3.HttpMethods)
 
 export function resolve_ref (ref: string, root: Record<string, any>): Record<string, any> | undefined {
   const paths = ref.replace('#/', '').split('/')

diff --git a/tools/src/tester/MergedOpenApiSpec.ts b/tools/src/tester/MergedOpenApiSpec.ts
@@ -7,9 +7,9 @@
 * compatible open source license.
 */
 
-import { type OpenAPIV3 } from 'openapi-types'
+import { OpenAPIV3 } from 'openapi-types'
 import { Logger } from '../Logger'
-import { SpecificationContext } from '../_utils';
+import { determine_possible_schema_types, SpecificationContext } from '../_utils';
 import { SchemaVisitor } from '../_utils/SpecificationVisitor';
 import OpenApiMerger from '../merger/OpenApiMerger';
 
@@ -38,13 +38,27 @@ export default class MergedOpenApiSpec {
   }
 
   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"
-        }
+    const visitor = new SchemaVisitor((ctx, schema) => {
+      // If already has unevaluatedProperties then don't set
+      if ((schema as any).unevaluatedProperties !== undefined) return;
+
+      // Don't apply `unevaluatedProperties` to component schemas as we will apply it at usage points (i.e. $ref's)
+      // Also don't apply to sub-schemas of an allOf as it will conflict with other sub-schemas as we'll apply it to the upper level
+      if (ctx.parent().location === '#/components/schemas' || ctx.parent().key === 'allOf') return;
+
+      const types = determine_possible_schema_types(spec, schema)
+
+      // Don't apply to multi-type or non-object schemas
+      if (types.length > 1 || types[0] !== 'object') return;
+
+      // Don't apply to basic { type: object } schemas
+      if (Object.keys(schema).filter(k => k !== 'type' && k !== 'description').length === 0) return;
+
+      (schema as any).type = 'object';
+      // causes any undeclared field in the response to produce an error
+      (schema as any).unevaluatedProperties = {
+        not: true,
+        errorMessage: "property is not defined in the spec"
       }
     })
 

diff --git a/tools/src/tester/ResultLogger.ts b/tools/src/tester/ResultLogger.ts
@@ -91,21 +91,21 @@ export class ConsoleResultLogger implements ResultLogger {
   #log_evaluation (evaluation: Evaluation, title: string, prefix: number = 0): void {
     const result = ansi.padding(this.#result(evaluation.result), 0, prefix)
 
-    var message = evaluation.message
-
-    if (message !== undefined && message?.length > 128 && !this._verbose) {
-      const message_part = message.split(',')[0]
-      message = message_part === message ? message : message_part + ', ...'
-    }
+    const message = this.#maybe_shorten_error_message(evaluation.message);
 
     if (message !== undefined) {
-      message = ansi.gray(`(${message})`)
-      console.log(`${result} ${title} ${message}`)
+      console.log(`${result} ${title} ${ansi.gray(`(${message})`)}`)
     } else {
       console.log(`${result} ${title}`)
     }
   }
 
+  #maybe_shorten_error_message(message: string | undefined): string | undefined {
+    if (message === undefined || message.length <= 128 || this._verbose) return message
+    const part = message.split(',')[0]
+    return part + (part !== message ? ', ...' : '')
+  }
+
   #result (r: Result): string {
     const text = ansi.padding(r, 7)
     switch (r) {

diff --git a/tools/src/tester/SchemaValidator.ts b/tools/src/tester/SchemaValidator.ts
@@ -30,7 +30,12 @@ export default class SchemaValidator {
     this.logger = logger
     const component_schemas = spec.components?.schemas ?? {}
     const reference_schemas = _.mapKeys(component_schemas, (_, name) => `#/components/schemas/${name}`)
-    this.json_validator = new JsonSchemaValidator(undefined, { reference_schemas, additional_keywords: ADDITIONAL_KEYWORDS, ajv_errors_opts: { singleError: true } })
+    this.json_validator = new JsonSchemaValidator(undefined, {
+      reference_schemas,
+      additional_keywords: ADDITIONAL_KEYWORDS,
+      ajv_errors_opts: { singleError: true },
+      errors_text_opts: { separator: ', ' }
+    })
   }
 
   validate (schema: OpenAPIV3.SchemaObject, data: any): Evaluation {

diff --git a/tools/tests/tester/MergedOpenApiSpec.test.ts b/tools/tests/tester/MergedOpenApiSpec.test.ts
@@ -10,7 +10,7 @@
 import { Logger } from 'Logger'
 import MergedOpenApiSpec from "tester/MergedOpenApiSpec"
 
-describe('additionalProperties', () => {
+describe('unevaluatedProperties', () => {
   const spec = new MergedOpenApiSpec('tools/tests/tester/fixtures/specs/complete', new Logger())
   const responses: any = spec.spec().components?.responses
 
@@ -20,21 +20,26 @@ describe('additionalProperties', () => {
 
   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' })
+    expect(schema.unevaluatedProperties).toEqual({ not: true, errorMessage: 'property is not defined in the spec' })
+  })
+
+  test('is added when no required fields', () => {
+    const schema = responses['info@500'].content['application/json'].schema
+    expect(schema.unevaluatedProperties).toEqual({ not: true, errorMessage: 'property is not defined in the spec' })
+  })
+
+  test('is not added to empty object schema', () => {
+    const schema = responses['info@503'].content['application/json'].schema
+    expect(schema.unevaluatedProperties).toBeUndefined()
   })
 
   test('is not added when true', () => {
     const schema = responses['info@201'].content['application/json'].schema
-    expect(schema.additionalProperties).toEqual(true)
+    expect(schema.unevaluatedProperties).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()
+    expect(schema.unevaluatedProperties).toEqual({ type: 'object' })
   })
 })