Validate order of superseded operations. (#326)

* Validate order of superseded operations. Signed-off-by: dblock <[email protected]> * Correct order of superseded operations. Signed-off-by: dblock <[email protected]> * Moved helpers into src. Signed-off-by: dblock <[email protected]> * Added CHANGELOG entry. Signed-off-by: dblock <[email protected]> * Everything is relative to src. Signed-off-by: dblock <[email protected]> * Fix badge. Signed-off-by: dblock <[email protected]> --------- Signed-off-by: dblock <[email protected]>
opensearch-project · Jun 9, 2024 · 362c90c · 362c90c
1 parent e6597c0
commit 362c90c
Show file tree

Hide file tree

Showing 28 changed files with 166 additions and 49 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,7 +17,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Added `npm run test:unit` and `test:integ` ([#320](https://github.com/opensearch-project/opensearch-api-specification/pull/320))
 - Added code coverage to tools' tests ([#323](https://github.com/opensearch-project/opensearch-api-specification/pull/323))
 - Added a YAML linter ([#312](https://github.com/opensearch-project/opensearch-api-specification/pull/312))
-
+- Added linter to validate order of spec operations ([#325](https://github.com/opensearch-project/opensearch-api-specification/pull/326)) ([#326](https://github.com/opensearch-project/opensearch-api-specification/pull/326))
+
 ### Changed
 
 - Replaced Smithy with a native OpenAPI spec ([#189](https://github.com/opensearch-project/opensearch-api-specification/issues/189))

diff --git a/README.md b/README.md
@@ -5,7 +5,7 @@
 [![Code Covergage](https://codecov.io/github/opensearch-project/opensearch-api-specification/graph/badge.svg?token=TO9YMAKSHH)](https://codecov.io/github/opensearch-project/opensearch-api-specification)
 [![Test Tools (Unit)](https://github.com/opensearch-project/opensearch-api-specification/actions/workflows/test-tools-unit.yml/badge.svg)](https://github.com/opensearch-project/opensearch-api-specification/actions/workflows/test-tools-unit.yml)
 [![Test Tools (Integration)](https://github.com/opensearch-project/opensearch-api-specification/actions/workflows/test-tools-integ.yml/badge.svg)](https://github.com/opensearch-project/opensearch-api-specification/actions/workflows/test-tools-integ.yml)
-[![Test Spec](https://github.com/opensearch-project/opensearch-api-specification/actions/workflows/test.yml/badge.svg)](https://github.com/opensearch-project/opensearch-api-specification/actions/workflows/test.yml)
+[![Test Spec](https://github.com/opensearch-project/opensearch-api-specification/actions/workflows/test-spec.yml/badge.svg)](https://github.com/opensearch-project/opensearch-api-specification/actions/workflows/test-spec.yml)
 [![Validate Spec](https://github.com/opensearch-project/opensearch-api-specification/actions/workflows/validate-spec.yml/badge.svg)](https://github.com/opensearch-project/opensearch-api-specification/actions/workflows/validate-spec.yml)
 
 - [OpenSearch API Specification](#opensearch-api-specification)

diff --git a/spec/_superseded_operations.yaml b/spec/_superseded_operations.yaml
@@ -49,9 +49,9 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml
   superseded_by: /_plugins/_alerting/monitors/{monitorID}
   operations:
     - GET
+    - HEAD
     - PUT
     - DELETE
-    - HEAD
 /_opendistro/_alerting/monitors/{monitorID}/_acknowledge/alerts:
   superseded_by: /_plugins/_alerting/monitors/{monitorID}/_acknowledge/alerts
   operations:
@@ -107,9 +107,9 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml
   superseded_by: /_plugins/_anomaly_detection/detectors/{detectorID}
   operations:
     - GET
+    - HEAD
     - PUT
     - DELETE
-    - HEAD
 /_opendistro/_anomaly_detection/detectors/{detectorID}/_preview:
   superseded_by: /_plugins/_anomaly_detection/detectors/{detectorID}/_preview
   operations:
@@ -200,9 +200,9 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml
   superseded_by: /_plugins/_ism/policies/{policyID}
   operations:
     - GET
+    - HEAD
     - PUT
     - DELETE
-    - HEAD
 /_opendistro/_ism/remove:
   superseded_by: /_plugins/_ism/remove
   operations:
@@ -353,9 +353,9 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml
   superseded_by: /_plugins/_rollup/jobs/{rollupID}
   operations:
     - GET
+    - HEAD
     - PUT
     - DELETE
-    - HEAD
 /_opendistro/_rollup/jobs/{rollupID}/_explain:
   superseded_by: /_plugins/_rollup/jobs/{rollupID}/_explain
   operations:
@@ -420,8 +420,8 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml
   superseded_by: /_plugins/_security/api/cache
   operations:
     - GET
-    - PUT
     - POST
+    - PUT
     - DELETE
 /_opendistro/_security/api/internalusers/:
   superseded_by: /_plugins/_security/api/internalusers/
@@ -433,8 +433,8 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml
   operations:
     - GET
     - PUT
-    - DELETE
     - PATCH
+    - DELETE
 /_opendistro/_security/api/internalusers/{name}/authtoken:
   superseded_by: /_plugins/_security/api/internalusers/{name}/authtoken
   operations:
@@ -457,8 +457,8 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml
   operations:
     - GET
     - PUT
-    - DELETE
     - PATCH
+    - DELETE
 /_opendistro/_security/api/rolesmapping/:
   superseded_by: /_plugins/_security/api/rolesmapping/
   operations:
@@ -469,8 +469,8 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml
   operations:
     - GET
     - PUT
-    - DELETE
     - PATCH
+    - DELETE
 /_opendistro/_security/api/securityconfig:
   superseded_by: /_plugins/_security/api/securityconfig
   operations:
@@ -519,8 +519,8 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml
   operations:
     - GET
     - PUT
-    - DELETE
     - PATCH
+    - DELETE
 /_opendistro/_security/api/user:
   superseded_by: /_plugins/_security/api/user
   operations:

diff --git a/tools/src/OpenSearchHttpClient.ts b/tools/src/OpenSearchHttpClient.ts
@@ -10,7 +10,7 @@
 import { Option } from '@commander-js/extra-typings'
 import axios, { type AxiosInstance, type AxiosRequestConfig, type AxiosResponse } from 'axios'
 import * as https from 'node:https'
-import { sleep } from '../helpers'
+import { sleep } from './helpers'
 
 const DEFAULT_URL = 'https://localhost:9200'
 const DEFAULT_USER = 'admin'

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

diff --git a/tools/src/dump-cluster-spec/dump-cluster-spec.ts b/tools/src/dump-cluster-spec/dump-cluster-spec.ts
@@ -10,7 +10,7 @@
 import { Command, Option } from '@commander-js/extra-typings'
 import { resolve } from 'path'
 import * as process from 'node:process'
-import { write_yaml } from '../../helpers'
+import { write_yaml } from '../helpers'
 import {
   get_opensearch_opts_from_cli,
   OPENSEARCH_INSECURE_OPTION,

diff --git a/tools/helpers.ts → tools/src/helpers.ts b/tools/helpers.ts → tools/src/helpers.ts
@@ -56,6 +56,18 @@ export function sort_by_keys (obj: Record<string, any>, priorities: string[] = [
   })
 }
 
+export function sort_array_by_keys (values: any[], priorities: string[] = []): string[] {
+  const orders = _.fromPairs(priorities.map((k, i) => [k, i + 1]))
+  return _.clone(values).sort((a, b) => {
+    const order_a = orders[a]
+    const order_b = orders[b]
+    if (order_a != null && order_b != null) return order_a - order_b
+    if (order_a != null) return 1
+    if (order_b != null) return -1
+    return a.localeCompare(b)
+  })
+}
+
 export function ensure_parent_dir (file_path: string): void {
   fs.mkdirSync(path.dirname(file_path), { recursive: true })
 }

diff --git a/tools/src/linter/components/NamespaceFile.ts b/tools/src/linter/components/NamespaceFile.ts
@@ -12,7 +12,7 @@ import { type OperationSpec, type ValidationError } from 'types'
 import OperationGroup from './OperationGroup'
 import _ from 'lodash'
 import Operation from './Operation'
-import { resolve_ref, sort_by_keys } from '../../../helpers'
+import { resolve_ref, sort_by_keys } from '../../helpers'
 import FileValidator from './base/FileValidator'
 
 const HTTP_METHODS = ['get', 'head', 'post', 'put', 'patch', 'delete', 'options', 'trace']

diff --git a/tools/src/linter/components/SupersededOperationsFile.ts b/tools/src/linter/components/SupersededOperationsFile.ts
@@ -7,8 +7,40 @@
 * compatible open source license.
 */
 
+import _ from 'lodash'
+import { sort_array_by_keys } from '../../helpers'
 import FileValidator from './base/FileValidator'
+import { type ValidationError } from 'types'
+import { type OpenAPIV3 } from 'openapi-types'
+
+const HTTP_METHODS = ['GET', 'HEAD', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS', 'TRACE']
 
 export default class SupersededOperationsFile extends FileValidator {
   has_json_schema = true
+  protected _superseded_ops: OpenAPIV3.Document | undefined
+
+  validate (): ValidationError[] {
+    const schema_validations = super.validate()
+    if (schema_validations.length > 0) return schema_validations
+    return this.validate_order_of_operations()
+  }
+
+  superseded_ops (): OpenAPIV3.Document {
+    if (this._superseded_ops) return this._superseded_ops
+    this._superseded_ops = this.spec()
+    delete (this._superseded_ops as any).$schema
+    return this._superseded_ops
+  }
+
+  validate_order_of_operations (): ValidationError[] {
+    return _.entries(this.superseded_ops()).map(([path, p]) => {
+      const current_keys = p.operations
+      const sorted_keys = sort_array_by_keys(p.operations as string[], HTTP_METHODS)
+      if(!_.isEqual(current_keys, sorted_keys)) {
+        return this.error(
+          `Operations must be sorted. Expected ${_.join(sorted_keys, ', ')}.`,
+          path)
+      }
+    }).filter((e) => e) as ValidationError[]
+  }
 }
diff --git a/tools/src/linter/components/base/FileValidator.ts b/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 } from '../../../helpers'
 import AJV from 'ajv'
 import addFormats from 'ajv-formats'
 

diff --git a/tools/src/merger/GlobalParamsGenerator.ts b/tools/src/merger/GlobalParamsGenerator.ts
@@ -9,7 +9,7 @@
 
 import { type OpenAPIV3 } from 'openapi-types'
 import _ from 'lodash'
-import { read_yaml } from '../../helpers'
+import { read_yaml } from '../helpers'
 
 export default class GlobalParamsGenerator {
   global_params: Record<string, OpenAPIV3.ParameterObject>

diff --git a/tools/src/merger/OpenApiMerger.ts b/tools/src/merger/OpenApiMerger.ts
@@ -10,7 +10,7 @@
 import { type OpenAPIV3 } from 'openapi-types'
 import fs from 'fs'
 import _ from 'lodash'
-import { read_yaml, write_yaml } from '../../helpers'
+import { read_yaml, write_yaml } from '../helpers'
 import SupersededOpsGenerator from './SupersededOpsGenerator'
 import GlobalParamsGenerator from './GlobalParamsGenerator'
 import { Logger } from '../Logger'

diff --git a/tools/src/merger/OpenDistro.ts b/tools/src/merger/OpenDistro.ts
@@ -8,7 +8,7 @@
 */
 
 import { type HttpVerb, type OperationPath, type SupersededOperationMap } from 'types'
-import { read_yaml, write_yaml } from '../../helpers'
+import { read_yaml, write_yaml } from '../helpers'
 
 // One-time script to generate _superseded_operations.yaml file for OpenDistro
 // Keeping this for now in case we need to update the file in the near future. Can be removed after a few months.

diff --git a/tools/src/merger/SupersededOpsGenerator.ts b/tools/src/merger/SupersededOpsGenerator.ts
@@ -9,7 +9,7 @@
 
 import { type OperationSpec, type SupersededOperationMap } from 'types'
 import _ from 'lodash'
-import { read_yaml } from '../../helpers'
+import { read_yaml } from '../helpers'
 import { type Logger } from '../Logger'
 
 export default class SupersededOpsGenerator {

diff --git a/tools/src/tester/OperationLocator.ts b/tools/src/tester/OperationLocator.ts
@@ -8,7 +8,7 @@
 */
 
 import { type OpenAPIV3 } from 'openapi-types'
-import { resolve_ref } from '../../helpers'
+import { resolve_ref } from '../helpers'
 import { type Chapter } from './types/story.types'
 import { type ParsedOperation } from './types/spec.types'
 import _ from 'lodash'

diff --git a/tools/src/tester/TestRunner.ts b/tools/src/tester/TestRunner.ts
@@ -11,7 +11,7 @@ import type StoryEvaluator from './StoryEvaluator'
 import { type StoryFile } from './StoryEvaluator'
 import fs from 'fs'
 import { type Story } from './types/story.types'
-import { read_yaml } from '../../helpers'
+import { read_yaml } from '../helpers'
 import { Result, type StoryEvaluation } from './types/eval.types'
 import { type ResultLogger } from './ResultLogger'
 import { basename, resolve } from 'path'

diff --git a/tools/src/tester/_generate_story_types.ts b/tools/src/tester/_generate_story_types.ts
@@ -9,7 +9,7 @@
 
 import * as js2ts from 'json-schema-to-typescript'
 import fs from 'fs'
-import { read_yaml } from '../../helpers'
+import { read_yaml } from '../helpers'
 
 const schema = read_yaml('json_schemas/test_story.schema.yaml')
 void js2ts.compile(schema, 'Story',

diff --git a/tools/tests/helpers.test.ts b/tools/tests/helpers.test.ts
@@ -0,0 +1,28 @@
+/*
+* 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 { sort_array_by_keys } from '../src/helpers'
+
+describe('helpers', () => {
+  describe('sort_array_by_keys', () => {
+    test('sorts arrays of string', () => {
+        expect(sort_array_by_keys([])).toEqual([])
+        expect(sort_array_by_keys(['GET', 'POST'], ['GET', 'POST'])).toEqual(['GET', 'POST'])
+        expect(sort_array_by_keys(['GET', 'POST'], ['POST', 'GET'])).toEqual(['POST', 'GET'])
+        expect(sort_array_by_keys(['GET', 'POST'], ['POST', 'GET', 'DELETE'])).toEqual(['POST', 'GET'])
+        expect(sort_array_by_keys(['DELETE', 'POST', 'GET'], ['POST', 'GET', 'DELETE'])).toEqual(['POST', 'GET', 'DELETE'])
+      })
+
+      test('does not modify the original object', () => {
+        const arr = ['GET', 'POST'] 
+        expect(sort_array_by_keys(arr, ['POST', 'GET'])).toEqual(['POST', 'GET'])
+        expect(arr).toEqual(['GET', 'POST'] )
+      })    
+  })
+})
diff --git a/tools/tests/linter/SchemasValidator.test.ts b/tools/tests/linter/SchemasValidator.test.ts
@@ -8,7 +8,7 @@
 */
 
 import { Logger } from 'Logger'
-import SchemasValidator from '../../src/linter/SchemasValidator'
+import SchemasValidator from 'linter/SchemasValidator'
 
 test('validate() - named_schemas', () => {
   const validator = new SchemasValidator('./tools/tests/linter/fixtures/schemas_validator/named_schemas', new Logger())

diff --git a/tools/tests/linter/SupersededOperationsFile.test.ts b/tools/tests/linter/SupersededOperationsFile.test.ts
@@ -9,12 +9,45 @@
 
 import SupersededOperationsFile from 'linter/components/SupersededOperationsFile'
 
-test('validate()', () => {
-  const validator = new SupersededOperationsFile('./tools/tests/linter/fixtures/_superseded_operations.yaml')
-  expect(validator.validate()).toEqual([
-    {
-      file: 'fixtures/_superseded_operations.yaml',
-      message: "File content does not match JSON schema found in './json_schemas/_superseded_operations.schema.yaml':\n [\n  {\n    \"instancePath\": \"/~1hello~1world/operations/1\",\n    \"schemaPath\": \"#/patternProperties/%5E~1/properties/operations/items/enum\",\n    \"keyword\": \"enum\",\n    \"params\": {\n      \"allowedValues\": [\n        \"GET\",\n        \"POST\",\n        \"PUT\",\n        \"DELETE\",\n        \"HEAD\",\n        \"OPTIONS\",\n        \"PATCH\"\n      ]\n    },\n    \"message\": \"must be equal to one of the allowed values\"\n  }\n]"
-    }
-  ])
+describe('validate()', () => {
+  test('invalid schema', () => {
+    const validator = new SupersededOperationsFile('./tools/tests/linter/fixtures/superseded_operations/invalid_schema.yaml')
+    expect(validator.validate()).toEqual([
+      {
+        file: 'superseded_operations/invalid_schema.yaml',
+        message: "File content does not match JSON schema found in './json_schemas/_superseded_operations.schema.yaml':\n " +
+          JSON.stringify([
+              {
+                "instancePath": "/~1hello~1world/operations/1",
+                "schemaPath": "#/patternProperties/%5E~1/properties/operations/items/enum",
+                "keyword": "enum",
+                "params": {
+                  "allowedValues": [
+                    "GET",
+                    "POST",
+                    "PUT",
+                    "DELETE",
+                    "HEAD",
+                    "OPTIONS",
+                    "PATCH"
+                  ]
+                },
+                "message": "must be equal to one of the allowed values"
+              }
+        ], null, 2),
+      },
+    ])
+  })
+
+  test('incorrect order of operations', () => {
+    const validator = new SupersededOperationsFile('./tools/tests/linter/fixtures/superseded_operations/incorrect_order_of_operations.yaml')
+    expect(validator.validate()).toEqual([
+      {
+        file: 'superseded_operations/incorrect_order_of_operations.yaml',
+        location: '/world/hello',
+        message: "Operations must be sorted. Expected GET, HEAD, POST, PUT, PATCH, DELETE."
+      },
+    ])
+  })
 })
+
diff --git a/tools/tests/linter/fixtures/superseded_operations/incorrect_order_of_operations.yaml b/tools/tests/linter/fixtures/superseded_operations/incorrect_order_of_operations.yaml
@@ -0,0 +1,11 @@
+$schema: ./json_schemas/_superseded_operations.schema.yaml
+
+/world/hello:
+  superseded_by: /world/goodbye
+  operations:
+    - PATCH
+    - GET
+    - DELETE
+    - HEAD
+    - POST
+    - PUT
diff --git a/...nter/fixtures/_superseded_operations.yaml → ...superseded_operations/invalid_schema.yaml b/...nter/fixtures/_superseded_operations.yaml → ...superseded_operations/invalid_schema.yaml
@@ -4,4 +4,4 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml
   superseded_by: /goodbye/world
   operations:
     - GET
-    - CLEAN
+    - CLEAN
-Original file line number
+Diff line change
@@ Expand Up / @@ -4,4 +4,4 @@ $schema: ./json_schemas/_superseded_operations.schema.yaml @@
       superseded_by: /goodbye/world
       operations:
         - GET
-        - CLEAN
+        - CLEAN