Skip to content

Commit

Permalink
feat: validate mapping keys (#33)
Browse files Browse the repository at this point in the history 
* feat: validate mapping keys

* fix: better diagnostics

* revert: diagnostics are exclusive to json mode
  • Loading branch information
@P0lip
P0lip authored Dec 9, 2019
1 parent d7797f3 commit 2712bee
Show file tree
Hide file tree
Showing 3 changed files with 251 additions and 53 deletions.
163 changes: 163 additions & 0 deletions src/__tests__/parseWithPointers.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/parse.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@ import { load as loadAST } from '@stoplight/yaml-ast-parser';
import { walkAST } from './parseWithPointers';
import { YAMLNode } from './types';

export const parse = <T>(value: string): T => walkAST(loadAST(value) as YAMLNode) as T;
export const parse = <T>(value: string): T => walkAST(loadAST(value) as YAMLNode, void 0, [], []) as T;
139 changes: 87 additions & 52 deletions src/parseWithPointers.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { DiagnosticSeverity, IDiagnostic } from '@stoplight/types';
import { DiagnosticSeverity, IDiagnostic, Optional } from '@stoplight/types';
import {
determineScalarType,
load as loadAST,
Expand Down Expand Up @@ -39,17 +39,7 @@ export const parseWithPointers = <T>(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));
Expand All @@ -70,8 +60,9 @@ const KEYS = Symbol('object_keys');

export const walkAST = (
node: YAMLNode | null,
options?: IParseOptions,
duplicatedMappingKeys?: YAMLNode[],
options: Optional<IParseOptions>,
lineMap: number[],
diagnostics: IDiagnostic[],
): unknown => {
if (node) {
switch (node.kind) {
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -126,15 +120,15 @@ 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: {
if (isObject(node.value) && isCircularAnchorRef(node)) {
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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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],
},
},
};
}

0 comments on commit 2712bee

Please sign in to comment.