Improve logging and generator output. (#322)

* Make superseded by warning message more useful.

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

* Display log level as a string.

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

* Added command line tool tests for merger and linter.

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

---------

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

DEVELOPER_GUIDE.md

@@ -380,7 +380,7 @@ npm run test
 
 Specify the test path to run tests for one of the tools:
 ```bash
-npm run test -- tools/tests/tester/
+npm run jest -- tools/tests/linter/lint.test.ts 
 ```
 
 The test suite contains unit tests and integration tests. Integration tests, such as [these](tools/tests/tester/integ/), require a local instance of OpenSearch and are placed into a folder named `integ`. Unit tests are run in parallel and integration tests are run sequentially using `--runInBand`. You can run unit tests with `npm run test:unit` separately from integration tests with `npm run test:integ`.

package.json

@@ -12,6 +12,7 @@
     "lint--fix": "eslint . --fix",
     "merge": "ts-node tools/src/merger/merge.ts",
     "test": "npm run test:unit && npm run test:integ",
+    "jest": "jest",
     "test:unit": "jest --testMatch='**/*.test.ts' --testPathIgnorePatterns=/integ/",
     "test:integ": "jest --testMatch='**/integ/*.test.ts' --runInBand",
     "test:spec": "ts-node tools/src/tester/start.ts"

tools/src/Logger.ts

@@ -16,10 +16,20 @@ export enum LogLevel {
 export class Logger {
   level: LogLevel
 
-  constructor (level: LogLevel) {
+  static messages = {
+    [LogLevel.error]: 'ERROR',
+    [LogLevel.warn]: 'WARNING',
+    [LogLevel.info]: 'INFO'
+  }
+
+  constructor (level: LogLevel = LogLevel.warn) {
     this.level = level
   }
 
+  log (message: string): void {
+    console.log(message)
+  }
+
   info (message: string): void {
     this.#log(LogLevel.info, message)
   }
@@ -34,9 +44,7 @@ export class Logger {
 
   #log (level: LogLevel, message: string): void {
     if (level > this.level) return
-    const output = `[${level}] ${message}`
-    if (level === LogLevel.error) console.error(output)
-    if (level === LogLevel.warn) console.warn(output)
-    if (level === LogLevel.info) console.info(output)
+    const output = `[${Logger.messages[level]}] ${message}`
+    console.log(output)
   }
 }

tools/src/linter/SchemasValidator.ts

@@ -11,7 +11,7 @@ import AJV from 'ajv'
 import addFormats from 'ajv-formats'
 import OpenApiMerger from '../merger/OpenApiMerger'
 import { type ValidationError } from '../types'
-import { LogLevel } from '../Logger'
+import { type Logger } from '../Logger'
 
 const IGNORED_ERROR_PREFIXES = [
   'can\'t resolve reference', // errors in referenced schemas will also cause reference errors
@@ -26,19 +26,21 @@ const ADDITIONAL_KEYWORDS = [
 ]
 
 export default class SchemasValidator {
+  logger: Logger
   root_folder: string
   spec: Record<string, any> = {}
   ajv: AJV
 
-  constructor (root_folder: string) {
+  constructor (root_folder: string, logger: Logger) {
+    this.logger = logger
     this.root_folder = root_folder
     this.ajv = new AJV({ strict: true, discriminator: true })
     addFormats(this.ajv)
     for (const keyword of ADDITIONAL_KEYWORDS) this.ajv.addKeyword(keyword)
   }
 
   validate (): ValidationError[] {
-    this.spec = new OpenApiMerger(this.root_folder, LogLevel.error).merge().components as Record<string, any>
+    this.spec = new OpenApiMerger(this.root_folder, this.logger).merge().components as Record<string, any>
     const named_schemas_errors = this.validate_named_schemas()
     if (named_schemas_errors.length > 0) return named_schemas_errors
     return [

tools/src/linter/SpecValidator.ts

@@ -15,8 +15,10 @@ import SupersededOperationsFile from './components/SupersededOperationsFile'
 import InfoFile from './components/InfoFile'
 import InlineObjectSchemaValidator from './InlineObjectSchemaValidator'
 import SchemasValidator from './SchemasValidator'
+import { type Logger } from '../Logger'
 
 export default class SpecValidator {
+  logger: Logger
   superseded_ops_file: SupersededOperationsFile
   info_file: InfoFile
   namespaces_folder: NamespacesFolder
@@ -25,12 +27,13 @@ export default class SpecValidator {
   schema_refs_validator: SchemaRefsValidator
   inline_object_schema_validator: InlineObjectSchemaValidator
 
-  constructor (root_folder: string) {
+  constructor (root_folder: string, logger: Logger) {
+    this.logger = logger
     this.superseded_ops_file = new SupersededOperationsFile(`${root_folder}/_superseded_operations.yaml`)
     this.info_file = new InfoFile(`${root_folder}/_info.yaml`)
     this.namespaces_folder = new NamespacesFolder(`${root_folder}/namespaces`)
     this.schemas_folder = new SchemasFolder(`${root_folder}/schemas`)
-    this.schemas_validator = new SchemasValidator(root_folder)
+    this.schemas_validator = new SchemasValidator(root_folder, logger)
     this.schema_refs_validator = new SchemaRefsValidator(this.namespaces_folder, this.schemas_folder)
     this.inline_object_schema_validator = new InlineObjectSchemaValidator(this.namespaces_folder, this.schemas_folder)
   }

tools/src/linter/lint.ts

@@ -10,24 +10,27 @@
 import { Command, Option } from '@commander-js/extra-typings'
 import SpecValidator from './SpecValidator'
 import { resolve } from 'path'
+import { Logger, LogLevel } from '../Logger'
 
 const command = new Command()
   .description('Validate the OpenSearch multi-file spec.')
   .addOption(new Option('-s, --source <path>', 'path to the root folder of the multi-file spec').default(resolve(__dirname, '../../../spec')))
+  .addOption(new Option('--verbose', 'whether to print the full stack trace of errors').default(false))
   .allowExcessArguments(false)
   .parse()
 
 const opts = command.opts()
-console.log(`Validating ${opts.source} ...`)
-const validator = new SpecValidator(opts.source)
+const logger = new Logger(opts.verbose ? LogLevel.info : LogLevel.warn)
+logger.log(`Validating ${opts.source} ...`)
+const validator = new SpecValidator(opts.source, logger)
 const errors = validator.validate()
 
 if (errors.length === 0) {
-  console.log('No errors found.')
+  logger.log('No errors found.')
   process.exit(0)
 } else {
-  console.log('Errors found:\n')
+  logger.error('Errors found:\n')
   errors.forEach(e => { console.error(e) })
-  console.log('\nTotal errors:', errors.length)
+  logger.error(`\nTotal errors:${errors.length}`)
   process.exit(1)
 }

tools/src/merger/OpenApiMerger.ts

@@ -13,7 +13,7 @@ import _ from 'lodash'
 import { read_yaml, write_yaml } from '../../helpers'
 import SupersededOpsGenerator from './SupersededOpsGenerator'
 import GlobalParamsGenerator from './GlobalParamsGenerator'
-import { Logger, LogLevel } from '../Logger'
+import { Logger } from '../Logger'
 
 // Create a single-file OpenAPI spec from multiple files for OpenAPI validation and programmatic consumption
 export default class OpenApiMerger {
@@ -24,8 +24,8 @@ export default class OpenApiMerger {
   paths: Record<string, Record<string, OpenAPIV3.PathItemObject>> = {} // namespace -> path -> path_item_object
   schemas: Record<string, Record<string, OpenAPIV3.SchemaObject>> = {} // category -> schema -> schema_object
 
-  constructor (root_folder: string, log_level: LogLevel = LogLevel.warn) {
-    this.logger = new Logger(log_level)
+  constructor (root_folder: string, logger: Logger = new Logger()) {
+    this.logger = logger
     this.root_folder = fs.realpathSync(root_folder)
     this.spec = {
       openapi: '3.1.0',
@@ -47,6 +47,8 @@ export default class OpenApiMerger {
     this.#generate_global_params()
     this.#generate_superseded_ops()
 
+    this.logger.info(`Writing ${output_path} ...`)
+
     if (output_path !== '') write_yaml(output_path, this.spec)
     return this.spec as OpenAPIV3.Document
   }
@@ -55,6 +57,7 @@ export default class OpenApiMerger {
   #merge_namespaces (): void {
     const folder = `${this.root_folder}/namespaces`
     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 }
@@ -78,6 +81,7 @@ export default class OpenApiMerger {
   #merge_schemas (): void {
     const folder = `${this.root_folder}/schemas`
     fs.readdirSync(folder).forEach(file => {
+      this.logger.info(`Merging schema ${folder}/${file} ...`)
       const spec = read_yaml(`${folder}/${file}`)
       const category = file.split('.yaml')[0]
       this.redirect_refs_in_schema(category, spec)

tools/src/merger/SupersededOpsGenerator.ts

@@ -10,14 +10,14 @@
 import { type OperationSpec, type SupersededOperationMap } from 'types'
 import _ from 'lodash'
 import { read_yaml } from '../../helpers'
-import { Logger, LogLevel } from '../Logger'
+import { type Logger } from '../Logger'
 
 export default class SupersededOpsGenerator {
   logger: Logger
   superseded_ops: SupersededOperationMap
 
-  constructor (root_path: string, logger: Logger | undefined) {
-    this.logger = logger ?? new Logger(LogLevel.warn)
+  constructor (root_path: string, logger: Logger) {
+    this.logger = logger
     const file_path = root_path + '/_superseded_operations.yaml'
     this.superseded_ops = read_yaml(file_path)
     delete this.superseded_ops.$schema
@@ -30,7 +30,7 @@ export default class SupersededOpsGenerator {
       const superseded_path = this.copy_params(superseded_by, path)
       const path_entry = _.entries(spec.paths as Document).find(([path, _]) => regex.test(path))
       if (path_entry != null) spec.paths[superseded_path] = this.path_object(path_entry[1], operation_keys)
-      else this.logger.warn(`Path not found: ${superseded_by}`)
+      else this.logger.warn(`${path} is superseded by a path that does not exist: ${superseded_by}`)
     }
   }
 

tools/src/merger/merge.ts

@@ -10,16 +10,19 @@
 import { Command, Option } from '@commander-js/extra-typings'
 import OpenApiMerger from './OpenApiMerger'
 import { resolve } from 'path'
+import { Logger, LogLevel } from '../Logger'
 
 const command = new Command()
   .description('Merges the multi-file OpenSearch spec into a single file for programmatic use.')
   .addOption(new Option('-s, --source <path>', 'path to the root folder of the multi-file spec').default(resolve(__dirname, '../../../spec')))
   .addOption(new Option('-o, --output <path>', 'output file name').default(resolve(__dirname, '../../../build/opensearch-openapi.yaml')))
+  .addOption(new Option('--verbose', 'show merge details').default(false))
   .allowExcessArguments(false)
   .parse()
 
 const opts = command.opts()
-const merger = new OpenApiMerger(opts.source)
-console.log(`Merging ${opts.source} into ${opts.output} ...`)
+const logger = new Logger(opts.verbose ? LogLevel.info : LogLevel.warn)
+const merger = new OpenApiMerger(opts.source, logger)
+logger.log(`Merging ${opts.source} into ${opts.output} ...`)
 merger.merge(opts.output)
-console.log('Done.')
+logger.log('Done.')

tools/src/tester/start.ts

@@ -8,7 +8,7 @@
 */
 
 import OpenApiMerger from '../merger/OpenApiMerger'
-import { LogLevel } from '../Logger'
+import { LogLevel, Logger } from '../Logger'
 import TestRunner from './TestRunner'
 import { Command, Option } from '@commander-js/extra-typings'
 import {
@@ -45,8 +45,9 @@ const command = new Command()
   .parse()
 
 const opts = command.opts()
+const logger = new Logger(opts.verbose ? LogLevel.info : LogLevel.warn)
 
-const spec = (new OpenApiMerger(opts.specPath, LogLevel.error)).merge()
+const spec = (new OpenApiMerger(opts.specPath, logger)).merge()
 
 const http_client = new OpenSearchHttpClient(get_opensearch_opts_from_cli(opts))
 const chapter_reader = new ChapterReader(http_client)

tools/tests/Logger.test.ts

@@ -0,0 +1,102 @@
+/*
+* 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, LogLevel } from '../src/Logger'
+
+describe('Logger', () => {
+  let log: jest.Mock
+
+  beforeEach(() => {
+    log = console.log = jest.fn()
+  })
+
+  afterEach(() => {
+    log.mockClear()
+  })
+
+  describe('at default log level', () => {
+    const logger = new Logger()
+
+    test('sets log level at warn', () => {
+      expect(logger.level).toEqual(LogLevel.warn)
+    })
+
+    test('does not log info level messages', () => {
+      logger.log('log')
+      logger.error('error')
+      logger.warn('warn')
+      logger.info('info')
+      expect(log.mock.calls).toEqual([
+        ['log'],
+        ['[ERROR] error'],
+        ['[WARNING] warn']
+      ])
+    })
+  })
+
+  describe('at info log level', () => {
+    const logger = new Logger(LogLevel.info)
+
+    test('sets log level at info', () => {
+      expect(logger.level).toEqual(LogLevel.info)
+    })
+
+    test('logs all messages', () => {
+      logger.log('log')
+      logger.error('error')
+      logger.warn('warn')
+      logger.info('info')
+      expect(log.mock.calls).toEqual([
+        ['log'],
+        ['[ERROR] error'],
+        ['[WARNING] warn'],
+        ['[INFO] info']
+      ])
+    })
+  })
+
+  describe('at warn log level', () => {
+    const logger = new Logger(LogLevel.warn)
+
+    test('sets log level at warn', () => {
+      expect(logger.level).toEqual(LogLevel.warn)
+    })
+
+    test('does not log info messages', () => {
+      logger.log('log')
+      logger.error('error')
+      logger.warn('warn')
+      logger.info('info')
+      expect(log.mock.calls).toEqual([
+        ['log'],
+        ['[ERROR] error'],
+        ['[WARNING] warn']
+      ])
+    })
+  })
+
+  describe('at error log level', () => {
+    const logger = new Logger(LogLevel.error)
+
+    test('sets log level at error', () => {
+      expect(logger.level).toEqual(LogLevel.error)
+    })
+
+    test('does not log warn and info messages', () => {
+      logger.log('log')
+      logger.error('error')
+      logger.warn('warn')
+      logger.info('info')
+      expect(log.mock.calls).toEqual([
+        ['log'],
+        ['[ERROR] error']
+      ])
+    })
+  })
+})

tools/tests/linter/SchemasValidator.test.ts

@@ -7,10 +7,11 @@
 * compatible open source license.
 */
 
+import { Logger } from 'Logger'
 import SchemasValidator from '../../src/linter/SchemasValidator'
 
 test('validate() - named_schemas', () => {
-  const validator = new SchemasValidator('./tools/tests/linter/fixtures/schemas_validator/named_schemas')
+  const validator = new SchemasValidator('./tools/tests/linter/fixtures/schemas_validator/named_schemas', new Logger())
   expect(validator.validate()).toEqual([
     {
       file: 'schemas/actions.yaml',
@@ -26,7 +27,7 @@ test('validate() - named_schemas', () => {
 })
 
 test('validate() - anonymous_schemas', () => {
-  const validator = new SchemasValidator('./tools/tests/linter/fixtures/schemas_validator/anonymous_schemas')
+  const validator = new SchemasValidator('./tools/tests/linter/fixtures/schemas_validator/anonymous_schemas', new Logger())
   expect(validator.validate()).toEqual([
     {
       file: '_global_parameters.yaml',

tools/tests/linter/SpecValidator.test.ts

@@ -7,10 +7,11 @@
 * compatible open source license.
 */
 
+import { Logger } from 'Logger'
 import SpecValidator from 'linter/SpecValidator'
 
 test('validate()', () => {
-  const validator = new SpecValidator('./tools/tests/linter/fixtures/empty')
+  const validator = new SpecValidator('./tools/tests/linter/fixtures/empty', new Logger())
   validator.namespaces_folder.validate = jest.fn().mockReturnValue([{ file: 'namespaces/', message: 'namespace error' }])
   validator.schemas_folder.validate = jest.fn().mockReturnValue([{ file: 'schemas/', message: 'schema error' }])
   validator.schema_refs_validator.validate = jest.fn().mockReturnValue([{ file: 'schema_refs', message: 'schema refs error' }])