From 2712bee8ef672127ade17c3d6d17a9771383f532 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Mon, 9 Dec 2019 09:08:32 +0100 Subject: [PATCH] feat: validate mapping keys (#33) * feat: validate mapping keys * fix: better diagnostics * revert: diagnostics are exclusive to json mode --- src/__tests__/parseWithPointers.spec.ts | 163 ++++++++++++++++++++++++ src/parse.ts | 2 +- src/parseWithPointers.ts | 139 ++++++++++++-------- 3 files changed, 251 insertions(+), 53 deletions(-) diff --git a/src/__tests__/parseWithPointers.spec.ts b/src/__tests__/parseWithPointers.spec.ts index 55e87a5..5022241 100644 --- a/src/__tests__/parseWithPointers.spec.ts +++ b/src/__tests__/parseWithPointers.spec.ts @@ -515,6 +515,169 @@ european-cities: &cities }); }); + describe('invalid (not JSON-ish) mapping keys', () => { + const complex = `[2]: test +{2:null}: false +2: test`; + + const responses = `responses: + "200": {} + 400: {} + true: false + null: 2`; + + it('always excludes any complex types', () => { + const { data: yamlData } = parseWithPointers(complex, { json: false }); + const { data: jsonData } = parseWithPointers(complex); + + expect(yamlData).toStrictEqual({ + '2': 'test', + }); + + expect(yamlData).toStrictEqual(jsonData); + }); + + it('always includes all scalar mapping keys', () => { + const { data: yamlData } = parseWithPointers(responses, { json: false }); + const { data: jsonData } = parseWithPointers(responses); + + expect(yamlData).toStrictEqual({ + responses: { + '200': {}, + '400': {}, + null: 2, + true: false, + }, + }); + + expect(yamlData).toStrictEqual(jsonData); + }); + + it('warns about non-string scalar mapping keys', () => { + const { diagnostics } = parseWithPointers(responses); + + expect(diagnostics).toEqual([ + { + code: 'YAMLIncompatibleValue', + message: 'mapping key must be a string scalar rather than number', + path: ['responses', '400'], + range: { + end: { + character: 5, + line: 2, + }, + start: { + character: 2, + line: 2, + }, + }, + severity: DiagnosticSeverity.Error, + }, + { + code: 'YAMLIncompatibleValue', + message: 'mapping key must be a string scalar rather than boolean', + path: ['responses', 'true'], + range: { + end: { + character: 6, + line: 3, + }, + start: { + character: 2, + line: 3, + }, + }, + severity: DiagnosticSeverity.Error, + }, + { + code: 'YAMLIncompatibleValue', + message: 'mapping key must be a string scalar rather than null', + path: ['responses', 'null'], + range: { + end: { + character: 6, + line: 4, + }, + start: { + character: 2, + line: 4, + }, + }, + severity: DiagnosticSeverity.Error, + }, + ]); + }); + + it('warns about complex mapping keys', () => { + const { diagnostics } = parseWithPointers(complex); + + expect(diagnostics).toEqual([ + { + code: 'YAMLIncompatibleValue', + message: 'mapping key must be a string scalar', + path: [], + range: { + end: { + character: 3, + line: 0, + }, + start: { + character: 0, + line: 0, + }, + }, + severity: DiagnosticSeverity.Error, + }, + { + code: 'YAMLIncompatibleValue', + message: 'mapping key must be a string scalar', + path: [], + range: { + end: { + character: 8, + line: 1, + }, + start: { + character: 0, + line: 1, + }, + }, + severity: DiagnosticSeverity.Error, + }, + { + code: 'YAMLIncompatibleValue', + message: 'mapping key must be a string scalar rather than number', + path: ['2'], + range: { + end: { + character: 1, + line: 2, + }, + start: { + character: 0, + line: 2, + }, + }, + severity: DiagnosticSeverity.Error, + }, + ]); + }); + + describe('when json mode is disabled', () => { + it('does not warn about non-string scalar mapping keys', () => { + const { diagnostics } = parseWithPointers(responses, { json: false }); + + expect(diagnostics).toEqual([]); + }); + + it('does not warn about complex mapping keys', () => { + const { diagnostics } = parseWithPointers(complex, { json: false }); + + expect(diagnostics).toStrictEqual([]); + }); + }); + }); + describe('keys order', () => { it('does not retain the order of keys by default', () => { const { data } = parseWithPointers(`foo: true diff --git a/src/parse.ts b/src/parse.ts index f142a1a..2a806ac 100644 --- a/src/parse.ts +++ b/src/parse.ts @@ -2,4 +2,4 @@ import { load as loadAST } from '@stoplight/yaml-ast-parser'; import { walkAST } from './parseWithPointers'; import { YAMLNode } from './types'; -export const parse = (value: string): T => walkAST(loadAST(value) as YAMLNode) as T; +export const parse = (value: string): T => walkAST(loadAST(value) as YAMLNode, void 0, [], []) as T; diff --git a/src/parseWithPointers.ts b/src/parseWithPointers.ts index f1196d6..af5d296 100644 --- a/src/parseWithPointers.ts +++ b/src/parseWithPointers.ts @@ -1,4 +1,4 @@ -import { DiagnosticSeverity, IDiagnostic } from '@stoplight/types'; +import { DiagnosticSeverity, IDiagnostic, Optional } from '@stoplight/types'; import { determineScalarType, load as loadAST, @@ -39,17 +39,7 @@ export const parseWithPointers = (value: string, options?: IParseOptions): Ya if (!ast) return parsed; - const duplicatedMappingKeys: YAMLNode[] = []; - - parsed.data = walkAST( - ast, - options, - options !== undefined && options.ignoreDuplicateKeys === false ? duplicatedMappingKeys : undefined, - ) as T; - - if (duplicatedMappingKeys.length > 0) { - parsed.diagnostics.push(...transformDuplicatedMappingKeys(duplicatedMappingKeys, lineMap)); - } + parsed.data = walkAST(ast, options, lineMap, parsed.diagnostics) as T; if (ast.errors) { parsed.diagnostics.push(...transformErrors(ast.errors, lineMap)); @@ -70,8 +60,9 @@ const KEYS = Symbol('object_keys'); export const walkAST = ( node: YAMLNode | null, - options?: IParseOptions, - duplicatedMappingKeys?: YAMLNode[], + options: Optional, + lineMap: number[], + diagnostics: IDiagnostic[], ): unknown => { if (node) { switch (node.kind) { @@ -81,19 +72,22 @@ export const walkAST = ( // note, we don't handle null aka '~' keys on purpose const seenKeys: string[] = []; const handleMergeKeys = options !== void 0 && options.mergeKeys === true; - const handleDuplicates = (options !== void 0 && options.json === false) || duplicatedMappingKeys !== void 0; + const yamlMode = options !== void 0 && options.json === false; + const handleDuplicates = options !== void 0 && options.ignoreDuplicateKeys === false; for (const mapping of node.mappings) { - const key = mapping.key.value; + if (!validateMappingKey(mapping, lineMap, diagnostics, yamlMode)) continue; + + const key = String(getScalarValue(mapping.key)); - if (handleDuplicates && (!handleMergeKeys || key !== SpecialMappingKeys.MergeKey)) { - if (seenKeys.includes(mapping.key.value)) { - if (options !== void 0 && options.json === false) { + if ((yamlMode || handleDuplicates) && (!handleMergeKeys || key !== SpecialMappingKeys.MergeKey)) { + if (seenKeys.includes(key)) { + if (yamlMode) { throw new Error('Duplicate YAML mapping key encountered'); } - if (duplicatedMappingKeys !== void 0) { - duplicatedMappingKeys.push(mapping.key); + if (handleDuplicates) { + diagnostics.push(createYAMLException(mapping.key, lineMap, 'duplicate key')); } } else { seenKeys.push(key); @@ -102,7 +96,7 @@ export const walkAST = ( // https://yaml.org/type/merge.html merge keys, not a part of YAML spec if (handleMergeKeys && key === SpecialMappingKeys.MergeKey) { - const reduced = reduceMergeKeys(walkAST(mapping.value, options, duplicatedMappingKeys), preserveKeyOrder); + const reduced = reduceMergeKeys(walkAST(mapping.value, options, lineMap, diagnostics), preserveKeyOrder); if (preserveKeyOrder && reduced !== null) { for (const reducedKey of Object.keys(reduced)) { pushKey(container, reducedKey); @@ -111,7 +105,7 @@ export const walkAST = ( Object.assign(container, reduced); } else { - container[key] = walkAST(mapping.value, options, duplicatedMappingKeys); + container[key] = walkAST(mapping.value, options, lineMap, diagnostics); if (preserveKeyOrder) { pushKey(container, key); @@ -126,7 +120,7 @@ export const walkAST = ( return container; } case Kind.SEQ: - return node.items.map(item => walkAST(item, options, duplicatedMappingKeys)); + return node.items.map(item => walkAST(item, options, lineMap, diagnostics)); case Kind.SCALAR: return getScalarValue(node); case Kind.ANCHOR_REF: { @@ -134,7 +128,7 @@ export const walkAST = ( node.value = dereferenceAnchor(node.value, node.referencesAnchor)!; } - return node.value && walkAST(node.value, options, duplicatedMappingKeys); + return node.value && walkAST(node.value, options, lineMap, diagnostics); } default: return null; @@ -251,33 +245,6 @@ const transformErrors = (errors: YAMLException[], lineMap: number[]): IDiagnosti return validations; }; -const transformDuplicatedMappingKeys = (nodes: YAMLNode[], lineMap: number[]): IDiagnostic[] => { - const validations: IDiagnostic[] = []; - for (const node of nodes) { - const startLine = lineForPosition(node.startPosition, lineMap); - const endLine = lineForPosition(node.endPosition, lineMap); - - validations.push({ - code: 'YAMLException', - message: 'duplicate key', - path: buildJsonPath(node), - range: { - start: { - line: startLine, - character: startLine === 0 ? node.startPosition : node.startPosition - lineMap[startLine - 1], - }, - end: { - line: endLine, - character: endLine === 0 ? node.endPosition : node.endPosition - lineMap[endLine - 1], - }, - }, - severity: DiagnosticSeverity.Error, - }); - } - - return validations; -}; - const reduceMergeKeys = (items: unknown, preserveKeyOrder: boolean): object | null => { if (Array.isArray(items)) { // reduceRight is on purpose here! We need to respect the order - the key cannot be overridden @@ -340,3 +307,71 @@ function pushKey(container: object, key: string) { deleteKey(container, key); container[KEYS].push(key); } + +function validateMappingKey( + mapping: YAMLMapping, + lineMap: number[], + diagnostics: IDiagnostic[], + yamlMode: boolean, +): boolean { + if (mapping.key.kind !== Kind.SCALAR) { + if (!yamlMode) { + diagnostics.push( + createYAMLIncompatibilityException(mapping.key, lineMap, 'mapping key must be a string scalar', yamlMode), + ); + } + + // no exception is thrown, yet the mapping is excluded regardless of mode, as we cannot represent the value anyway + return false; + } + + if (!yamlMode) { + const type = typeof getScalarValue(mapping.key); + if (type !== 'string') { + diagnostics.push( + createYAMLIncompatibilityException( + mapping.key, + lineMap, + `mapping key must be a string scalar rather than ${mapping.key.valueObject === null ? 'null' : type}`, + yamlMode, + ), + ); + } + } + + return true; +} + +function createYAMLIncompatibilityException( + node: YAMLNode, + lineMap: number[], + message: string, + yamlMode: boolean, +): IDiagnostic { + const exception = createYAMLException(node, lineMap, message); + exception.code = 'YAMLIncompatibleValue'; + exception.severity = yamlMode ? DiagnosticSeverity.Hint : DiagnosticSeverity.Error; + return exception; +} + +function createYAMLException(node: YAMLNode, lineMap: number[], message: string): IDiagnostic { + const startLine = lineForPosition(node.startPosition, lineMap); + const endLine = lineForPosition(node.endPosition, lineMap); + + return { + code: 'YAMLException', + message, + severity: DiagnosticSeverity.Error, + path: buildJsonPath(node), + range: { + start: { + line: startLine, + character: startLine === 0 ? node.startPosition : node.startPosition - lineMap[startLine - 1], + }, + end: { + line: endLine, + character: endLine === 0 ? node.endPosition : node.endPosition - lineMap[endLine - 1], + }, + }, + }; +}