Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a linter for numeric types #800

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Added `GET`, `POST /_plugins/_ml/tasks/_search`, `GET /_plugins/_ml/tools`, `tools/{tool_name}` ([#797](https://github.com/opensearch-project/opensearch-api-specification/pull/797))
- Added `POST /_plugins/_ml/agents/{agent_id}/_execute`, `GET /_plugins/_ml/agents/{agent_id}`, `GET`, `POST /_plugins/_ml/agents/_search` ([#798](https://github.com/opensearch-project/opensearch-api-specification/pull/798))
- Added a warning for test file names that don't match the API being tested ([#793](https://github.com/opensearch-project/opensearch-api-specification/pull/793))
- Added a linter rule for numeric types ([#800](https://github.com/opensearch-project/opensearch-api-specification/pull/800))
- Added `time` field to the `GetStats` schema in `_common.yml` ([#803](https://github.com/opensearch-project/opensearch-api-specification/pull/803))
- Added version for `POST /_plugins/_ml/_train/{algorithm_name}`, `_predict/{algorithm_name}/{model_id}`, and `_train_predict/{algorithm_name}` ([#763](https://github.com/opensearch-project/opensearch-api-specification/pull/763))
- Added `POST _plugins/_security/api/internalusers/{username}` response `201` ([#810](https://github.com/opensearch-project/opensearch-api-specification/pull/810))
Expand Down
50 changes: 34 additions & 16 deletions tools/src/linter/SchemaVisitingValidator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
SpecificationContext
} from '../_utils'
import { type OpenAPIV3 } from 'openapi-types'
import { namespace_file } from '../../tests/linter/factories/namespace_file'

Check failure on line 23 in tools/src/linter/SchemaVisitingValidator.ts

View workflow job for this annotation

GitHub Actions / lint

'namespace_file' is defined but never used

export default class SchemaVisitingValidator {
private readonly _namespaces_folder: NamespacesFolder
Expand Down Expand Up @@ -52,6 +53,33 @@
return errors
}

validateFile(filePath: string): ValidationError[] {

Check failure on line 56 in tools/src/linter/SchemaVisitingValidator.ts

View workflow job for this annotation

GitHub Actions / lint

Class Method name `validateFile` must match one of the following formats: snake_case

Check failure on line 56 in tools/src/linter/SchemaVisitingValidator.ts

View workflow job for this annotation

GitHub Actions / lint

Parameter name `filePath` must match one of the following formats: snake_case, UPPER_CASE
const errors: ValidationError[] = []
const visitor = this.createVisitor(errors)

Check failure on line 59 in tools/src/linter/SchemaVisitingValidator.ts

View workflow job for this annotation

GitHub Actions / lint

Trailing spaces not allowed
const targetFile = [...this._namespaces_folder.files, ...this._schemas_folder.files].find(f => f.file === filePath)

Check failure on line 60 in tools/src/linter/SchemaVisitingValidator.ts

View workflow job for this annotation

GitHub Actions / lint

Variable name `targetFile` must match one of the following formats: snake_case, UPPER_CASE

if (!targetFile) {
return [{ message: `File not found in namespaces/schemas: ${filePath}`, file: filePath }]
}

visitor.visit_specification(new SpecificationContext(targetFile.file), targetFile.spec())

return errors
}

private createVisitor(errors: ValidationError[]): SchemaVisitor {
const validating_functions = [
this.#validate_numeric_schema.bind(this)
]

return new SchemaVisitor((ctx, schema) => {
for (const f of validating_functions) {
f(ctx, schema, errors)
}
})
}

#validate_inline_object_schema (ctx: SpecificationContext, schema: MaybeRef<OpenAPIV3.SchemaObject>, errors: ValidationError[]): void {
if (is_ref(schema) || schema.type !== 'object' || schema.properties === undefined) {
return
Expand All @@ -73,26 +101,16 @@
}

if (schema.type === 'number') {
if (schema.format === undefined || SCHEMA_NUMBER_FORMATS.includes(schema.format)) {
return
}

if (SCHEMA_INTEGER_FORMATS.includes(schema.format)) {
errors.push(ctx.error(`schema of type 'number' with format '${schema.format}' should instead be of type 'integer'`))
} else {
errors.push(ctx.error(`schema of type 'number' with format '${schema.format}' is invalid, expected one of: ${SCHEMA_NUMBER_FORMATS.join(', ')}`))
if (schema.format == null || !schema.format) {
errors.push(ctx.error(`Schema of type 'number' must specify a valid format. Allowed formats: ${SCHEMA_NUMBER_FORMATS.join(', ')}`));
return;
}
}

if (schema.type === 'integer') {
if (schema.format === undefined || SCHEMA_INTEGER_FORMATS.includes(schema.format)) {
return
}

if (SCHEMA_NUMBER_FORMATS.includes(schema.format)) {
errors.push(ctx.error(`schema of type 'integer' with format '${schema.format}' should instead be of type 'number'`))
} else {
errors.push(ctx.error(`schema of type 'integer' with format '${schema.format}' is invalid, expected one of: ${SCHEMA_INTEGER_FORMATS.join(', ')}`))
if (schema.format == null || !schema.format) {
errors.push(ctx.error(`Schema of type 'integer' must specify a valid format. Allowed formats: ${SCHEMA_INTEGER_FORMATS.join(', ')}`));
return;
}
}
}
Expand Down
44 changes: 43 additions & 1 deletion tools/src/linter/SpecValidator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import SchemaVisitingValidator from './SchemaVisitingValidator'
import SchemasValidator from './SchemasValidator'
import { type Logger } from '../Logger'
import { execSync } from 'child_process'
import { relative, resolve } from 'path'

export default class SpecValidator {
logger: Logger
Expand All @@ -26,8 +28,9 @@
schemas_validator: SchemasValidator
schema_refs_validator: SchemaRefsValidator
inline_object_schema_validator: SchemaVisitingValidator
changedOnly: boolean

Check failure on line 31 in tools/src/linter/SpecValidator.ts

View workflow job for this annotation

GitHub Actions / lint

Class Property name `changedOnly` must match one of the following formats: snake_case

constructor (root_folder: string, logger: Logger) {
constructor (root_folder: string, logger: Logger, changedOnly: boolean) {

Check failure on line 33 in tools/src/linter/SpecValidator.ts

View workflow job for this annotation

GitHub Actions / lint

Parameter name `changedOnly` must match one of the following formats: snake_case, UPPER_CASE
this.logger = logger
this.superseded_ops_file = new SupersededOperationsFile(`${root_folder}/_superseded_operations.yaml`)
this.info_file = new InfoFile(`${root_folder}/_info.yaml`)
Expand All @@ -36,9 +39,48 @@
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 SchemaVisitingValidator(this.namespaces_folder, this.schemas_folder)
this.changedOnly = changedOnly
}

private getChangedFiles(): string[] {
try {
const output = execSync('git diff --name-only origin/main', { encoding: 'utf-8' })
return output.split('\n').filter(file => file.endsWith('.yml') || file.endsWith('.yaml'))
} catch (error) {
this.logger.error('Error:' + error)

Check failure on line 50 in tools/src/linter/SpecValidator.ts

View workflow job for this annotation

GitHub Actions / lint

Invalid operand for a '+' operation. Operands must each be a number or string, allowing a string + any of: `any`, `boolean`, `null`, `RegExp`, `undefined`. Got `unknown`
return []
}
}

validateChangedFiles(): ValidationError[] {

Check failure on line 55 in tools/src/linter/SpecValidator.ts

View workflow job for this annotation

GitHub Actions / lint

Class Method name `validateChangedFiles` must match one of the following formats: snake_case
const changedFiles = this.getChangedFiles()

Check failure on line 56 in tools/src/linter/SpecValidator.ts

View workflow job for this annotation

GitHub Actions / lint

Variable name `changedFiles` must match one of the following formats: snake_case, UPPER_CASE

if (changedFiles.length === 0) {
this.logger.log('No valid files to check')
return []
}

this.logger.log(`Checking diff files:\n${changedFiles.join('\n')}`)
let errors: ValidationError[] = []

for (const file of changedFiles) {
let normalizedFile = file

if (file.startsWith('spec/')) {
normalizedFile = relative(resolve('spec'), resolve(file))
} else if (file.startsWith('schemas/')) {
normalizedFile = relative(resolve('schemas'), resolve(file))
}

errors = errors.concat(this.inline_object_schema_validator.validateFile(normalizedFile))
}

return errors
}

validate (): ValidationError[] {
if (this.changedOnly) return this.validateChangedFiles()

const component_errors = [
...this.namespaces_folder.validate(),
...this.schemas_folder.validate()
Expand Down
3 changes: 2 additions & 1 deletion tools/src/linter/lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@ 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))
.addOption(new Option('--changed-only', 'whether to only validate changed files').default(false))
.allowExcessArguments(false)
.parse()

const opts = command.opts()
const logger = new Logger(opts.verbose ? LogLevel.info : LogLevel.warn)
logger.log(`Validating ${opts.source} ...`)
const validator = new SpecValidator(opts.source, logger)
const validator = new SpecValidator(opts.source, logger, opts.changedOnly)
const errors = validator.validate()

if (errors.length === 0) {
Expand Down
2 changes: 1 addition & 1 deletion tools/src/prepare-for-vale/KeepDescriptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,4 @@ export default class KeepDescriptions {
return ' ' + p1 + spaces
})
}
}
}
Loading