feat: validate mapping keys

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

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 = <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;

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 = <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));
@@ -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) {
@@ -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,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;
@@ -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],
+      },
+    },
+  };
+}