From 395fb93344f289145fd1773bdcef3ead92f2c1ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Fri, 29 Nov 2019 15:23:27 +0100 Subject: [PATCH 1/3] feat: validate mapping keys --- src/__tests__/parseWithPointers.spec.ts | 163 ++++++++++++++++++++++++ src/parse.ts | 2 +- src/parseWithPointers.ts | 120 +++++++++-------- 3 files changed, 234 insertions(+), 51 deletions(-) diff --git a/src/__tests__/parseWithPointers.spec.ts b/src/__tests__/parseWithPointers.spec.ts index 42f9085..1e6a445 100644 --- a/src/__tests__/parseWithPointers.spec.ts +++ b/src/__tests__/parseWithPointers.spec.ts @@ -514,4 +514,167 @@ european-cities: &cities expect(result.diagnostics).toEqual([]); }); }); + + 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: 'YAMLException', + 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: 'YAMLException', + 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: 'YAMLException', + 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: 'YAMLException', + message: 'mapping key must be a string scalar', + path: [], + range: { + end: { + character: 3, + line: 0, + }, + start: { + character: 0, + line: 0, + }, + }, + severity: DiagnosticSeverity.Error, + }, + { + code: 'YAMLException', + message: 'mapping key must be a string scalar', + path: [], + range: { + end: { + character: 8, + line: 1, + }, + start: { + character: 0, + line: 1, + }, + }, + severity: DiagnosticSeverity.Error, + }, + { + code: 'YAMLException', + 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([]); + }); + }); + }); }); 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 96e8b43..e519626 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)); @@ -68,8 +58,9 @@ export const parseWithPointers = (value: string, options?: IParseOptions): Ya export const walkAST = ( node: YAMLNode | null, - options?: IParseOptions, - duplicatedMappingKeys?: YAMLNode[], + options: Optional, + lineMap: number[], + diagnostics: IDiagnostic[], ): unknown => { if (node) { switch (node.kind) { @@ -78,19 +69,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; - if (handleDuplicates && (!handleMergeKeys || key !== SpecialMappingKeys.MergeKey)) { - if (seenKeys.includes(mapping.key.value)) { - if (options !== void 0 && options.json === false) { + const key = String(getScalarValue(mapping.key)); + + 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); @@ -99,16 +93,16 @@ export const walkAST = ( // https://yaml.org/type/merge.html merge keys, not a part of YAML spec if (handleMergeKeys && key === SpecialMappingKeys.MergeKey) { - Object.assign(container, reduceMergeKeys(walkAST(mapping.value, options, duplicatedMappingKeys))); + Object.assign(container, reduceMergeKeys(walkAST(mapping.value, options, lineMap, diagnostics))); } else { - container[mapping.key.value] = walkAST(mapping.value, options, duplicatedMappingKeys); + container[mapping.key.value] = walkAST(mapping.value, options, lineMap, diagnostics); } } 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: { @@ -116,7 +110,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; @@ -233,32 +227,27 @@ 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], - }, +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], }, - severity: DiagnosticSeverity.Error, - }); - } - - return validations; -}; + end: { + line: endLine, + character: endLine === 0 ? node.endPosition : node.endPosition - lineMap[endLine - 1], + }, + }, + }; +} const reduceMergeKeys = (items: unknown): object | null => { if (Array.isArray(items)) { @@ -268,3 +257,34 @@ const reduceMergeKeys = (items: unknown): object | null => { return typeof items !== 'object' || items === null ? null : Object(items); }; + +function validateMappingKey( + mapping: YAMLMapping, + lineMap: number[], + diagnostics: IDiagnostic[], + yamlMode: boolean, +): boolean { + if (mapping.key.kind !== Kind.SCALAR) { + if (!yamlMode) { + diagnostics.push(createYAMLException(mapping.key, lineMap, 'mapping key must be a string scalar')); + } + + // 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( + createYAMLException( + mapping.key, + lineMap, + `mapping key must be a string scalar rather than ${mapping.key.valueObject === null ? 'null' : type}`, + ), + ); + } + } + + return true; +} From 77444dc78864e04fa760aae78461205cb1ebceb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Mon, 2 Dec 2019 15:23:41 +0100 Subject: [PATCH 2/3] fix: better diagnostics --- src/__tests__/parseWithPointers.spec.ts | 118 ++++++++++++++++++++++-- src/parseWithPointers.ts | 83 +++++++++-------- 2 files changed, 155 insertions(+), 46 deletions(-) diff --git a/src/__tests__/parseWithPointers.spec.ts b/src/__tests__/parseWithPointers.spec.ts index 1e6a445..920f3cd 100644 --- a/src/__tests__/parseWithPointers.spec.ts +++ b/src/__tests__/parseWithPointers.spec.ts @@ -558,7 +558,7 @@ european-cities: &cities expect(diagnostics).toEqual([ { - code: 'YAMLException', + code: 'YAMLIncompatibleValue', message: 'mapping key must be a string scalar rather than number', path: ['responses', '400'], range: { @@ -574,7 +574,7 @@ european-cities: &cities severity: DiagnosticSeverity.Error, }, { - code: 'YAMLException', + code: 'YAMLIncompatibleValue', message: 'mapping key must be a string scalar rather than boolean', path: ['responses', 'true'], range: { @@ -590,7 +590,7 @@ european-cities: &cities severity: DiagnosticSeverity.Error, }, { - code: 'YAMLException', + code: 'YAMLIncompatibleValue', message: 'mapping key must be a string scalar rather than null', path: ['responses', 'null'], range: { @@ -613,7 +613,7 @@ european-cities: &cities expect(diagnostics).toEqual([ { - code: 'YAMLException', + code: 'YAMLIncompatibleValue', message: 'mapping key must be a string scalar', path: [], range: { @@ -629,7 +629,7 @@ european-cities: &cities severity: DiagnosticSeverity.Error, }, { - code: 'YAMLException', + code: 'YAMLIncompatibleValue', message: 'mapping key must be a string scalar', path: [], range: { @@ -645,7 +645,7 @@ european-cities: &cities severity: DiagnosticSeverity.Error, }, { - code: 'YAMLException', + code: 'YAMLIncompatibleValue', message: 'mapping key must be a string scalar rather than number', path: ['2'], range: { @@ -664,16 +664,114 @@ european-cities: &cities }); describe('when json mode is disabled', () => { - it('does not warn about non-string scalar mapping keys', () => { + it('still warns about non-string scalar mapping keys', () => { const { diagnostics } = parseWithPointers(responses, { json: false }); - expect(diagnostics).toEqual([]); + 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.Hint, + }, + { + 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.Hint, + }, + { + 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.Hint, + }, + ]); }); - it('does not warn about complex mapping keys', () => { + it('still warns about complex mapping keys', () => { const { diagnostics } = parseWithPointers(complex, { json: false }); - expect(diagnostics).toStrictEqual([]); + 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.Hint, + }, + { + code: 'YAMLIncompatibleValue', + message: 'mapping key must be a string scalar', + path: [], + range: { + end: { + character: 8, + line: 1, + }, + start: { + character: 0, + line: 1, + }, + }, + severity: DiagnosticSeverity.Hint, + }, + { + 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.Hint, + }, + ]); }); }); }); diff --git a/src/parseWithPointers.ts b/src/parseWithPointers.ts index e519626..70dbef8 100644 --- a/src/parseWithPointers.ts +++ b/src/parseWithPointers.ts @@ -227,28 +227,6 @@ const transformErrors = (errors: YAMLException[], lineMap: number[]): IDiagnosti return validations; }; -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], - }, - }, - }; -} - const reduceMergeKeys = (items: unknown): object | null => { if (Array.isArray(items)) { // reduceRight is on purpose here! We need to respect the order - the key cannot be overridden.. @@ -265,26 +243,59 @@ function validateMappingKey( yamlMode: boolean, ): boolean { if (mapping.key.kind !== Kind.SCALAR) { - if (!yamlMode) { - diagnostics.push(createYAMLException(mapping.key, lineMap, 'mapping key must be a string scalar')); - } + 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( - createYAMLException( - mapping.key, - lineMap, - `mapping key must be a string scalar rather than ${mapping.key.valueObject === null ? 'null' : type}`, - ), - ); - } + 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], + }, + }, + }; +} From 13825bb4aa3586f4f7029b2d0706f97172cd6f8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Thu, 5 Dec 2019 00:46:01 +0100 Subject: [PATCH 3/3] revert: diagnostics are exclusive to json mode --- src/__tests__/parseWithPointers.spec.ts | 106 +----------------------- src/parseWithPointers.ts | 30 ++++--- 2 files changed, 21 insertions(+), 115 deletions(-) diff --git a/src/__tests__/parseWithPointers.spec.ts b/src/__tests__/parseWithPointers.spec.ts index 920f3cd..8b77a55 100644 --- a/src/__tests__/parseWithPointers.spec.ts +++ b/src/__tests__/parseWithPointers.spec.ts @@ -664,114 +664,16 @@ european-cities: &cities }); describe('when json mode is disabled', () => { - it('still warns about non-string scalar mapping keys', () => { + it('does not warn about non-string scalar mapping keys', () => { const { diagnostics } = parseWithPointers(responses, { json: false }); - 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.Hint, - }, - { - 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.Hint, - }, - { - 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.Hint, - }, - ]); + expect(diagnostics).toEqual([]); }); - it('still warns about complex mapping keys', () => { + it('does not warn about complex mapping keys', () => { const { diagnostics } = parseWithPointers(complex, { json: false }); - 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.Hint, - }, - { - code: 'YAMLIncompatibleValue', - message: 'mapping key must be a string scalar', - path: [], - range: { - end: { - character: 8, - line: 1, - }, - start: { - character: 0, - line: 1, - }, - }, - severity: DiagnosticSeverity.Hint, - }, - { - 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.Hint, - }, - ]); + expect(diagnostics).toStrictEqual([]); }); }); }); diff --git a/src/parseWithPointers.ts b/src/parseWithPointers.ts index 70dbef8..62db379 100644 --- a/src/parseWithPointers.ts +++ b/src/parseWithPointers.ts @@ -243,24 +243,28 @@ function validateMappingKey( yamlMode: boolean, ): boolean { if (mapping.key.kind !== Kind.SCALAR) { - diagnostics.push( - createYAMLIncompatibilityException(mapping.key, lineMap, 'mapping key must be a string scalar', yamlMode), - ); + 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; } - 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, - ), - ); + 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;