Map key order (#839)

* Validation for map key order

Adds a new validation that checks if the keys
of a map are in alphabetical order.  Updates the
tests to cover the new validation.

* Add settings for keyOrdering

Adds a setting to control keyOrdering feature.
Tests if the new setting works as expected.
Starts using the new setting to enable the feature.

* add CodeAction to fix map order

Adds a new codeaction that fixes the map order
by rewriting the ast with the alphabetical order
of keys.

* Update docs for keyOrdering

Updates readme and changelog with the information
for the new feature.

* Preserve comments

Adds tests and updates the logic to preserve
comments and spaces.

---------

Co-authored-by: Muthurajan Sivasubramanian <[email protected]>

Author: gorkem

CHANGELOG.md

@@ -1,3 +1,7 @@
+### [UNRELEASED] 
+
+- Add: Enforce alphabetical ordering of keys in mappings and provide codeaction to fix it. 
+
 ### 1.11.0
 - Fix: only the first choice is shown when hovering anyOf-typed properties [#784](https://github.com/redhat-developer/vscode-yaml/issues/784)
 - Fix: Description in the schema root does not get displayed [#809](https://github.com/redhat-developer/vscode-yaml/issues/809)

README.md

@@ -52,6 +52,7 @@ The following settings are supported:
 - `yaml.suggest.parentSkeletonSelectedFirst`: If true, the user must select some parent skeleton first before autocompletion starts to suggest the rest of the properties.\nWhen yaml object is not empty, autocompletion ignores this setting and returns all properties and skeletons.
 - `yaml.style.flowMapping` : Forbids flow style mappings if set to `forbid` 
 - `yaml.style.flowSequence` : Forbids flow style sequences if set to `forbid`
+- `yaml.keyOrdering` : Enforces alphabetical ordering of keys in mappings when set to `true`. Default is `false`
 
 ##### Adding custom tags
 

package.json

@@ -34,6 +34,7 @@
   },
   "dependencies": {
     "ajv": "^8.11.0",
+    "lodash": "4.17.21",
     "request-light": "^0.5.7",
     "vscode-json-languageservice": "4.1.8",
     "vscode-languageserver": "^7.0.0",

src/languageserver/handlers/settingsHandlers.ts

@@ -122,6 +122,7 @@ export class SettingsHandler {
         flowMapping: settings.yaml.style?.flowMapping ?? 'allow',
         flowSequence: settings.yaml.style?.flowSequence ?? 'allow',
       };
+      this.yamlSettings.keyOrdering = settings.yaml.keyOrdering ?? false;
     }
 
     this.yamlSettings.schemaConfigurationSettings = [];
@@ -257,6 +258,7 @@ export class SettingsHandler {
       flowMapping: this.yamlSettings.style?.flowMapping,
       flowSequence: this.yamlSettings.style?.flowSequence,
       yamlVersion: this.yamlSettings.yamlVersion,
+      keyOrdering: this.yamlSettings.keyOrdering,
     };
 
     if (this.yamlSettings.schemaAssociations) {

src/languageservice/services/validation/map-key-order.ts

@@ -0,0 +1,52 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Red Hat. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { TextDocument } from 'vscode-languageserver-textdocument';
+import { Diagnostic, DiagnosticSeverity, Range } from 'vscode-languageserver-types';
+import { SingleYAMLDocument } from '../../parser/yaml-documents';
+import { AdditionalValidator } from './types';
+import { isMap, Pair, visit } from 'yaml';
+
+export class MapKeyOrderValidator implements AdditionalValidator {
+  validate(document: TextDocument, yamlDoc: SingleYAMLDocument): Diagnostic[] {
+    const result = [];
+
+    visit(yamlDoc.internalDocument, (key, node) => {
+      if (isMap(node)) {
+        for (let i = 1; i < node.items.length; i++) {
+          if (compare(node.items[i - 1], node.items[i]) > 0) {
+            const range = createRange(document, node.items[i - 1]);
+            result.push(
+              Diagnostic.create(
+                range,
+                `Wrong ordering of key "${node.items[i - 1].key}" in mapping`,
+                DiagnosticSeverity.Error,
+                'mapKeyOrder'
+              )
+            );
+          }
+        }
+      }
+    });
+
+    return result;
+  }
+}
+
+function createRange(document: TextDocument, node: Pair): Range {
+  const start = node?.srcToken.start[0]?.offset ?? node?.srcToken?.key.offset ?? node?.srcToken?.sep[0]?.offset;
+  const end =
+    node?.srcToken?.value.offset ||
+    node?.srcToken?.sep[0]?.offset ||
+    node?.srcToken?.key.offset ||
+    node?.srcToken.start[node.srcToken.start.length - 1]?.offset;
+  return Range.create(document.positionAt(start), document.positionAt(end));
+}
+
+function compare(thiz: Pair, that: Pair): number {
+  const thatKey = String(that.key);
+  const thisKey = String(thiz.key);
+  return thisKey.localeCompare(thatKey);
+}

src/languageservice/services/yamlCodeActions.ts

@@ -22,9 +22,12 @@ import { LanguageSettings } from '../yamlLanguageService';
 import { YAML_SOURCE } from '../parser/jsonParser07';
 import { getFirstNonWhitespaceCharacterAfterOffset } from '../utils/strings';
 import { matchOffsetToDocument } from '../utils/arrUtils';
-import { isMap, isSeq } from 'yaml';
+import { CST, isMap, isSeq, YAMLMap } from 'yaml';
 import { yamlDocumentsCache } from '../parser/yaml-documents';
 import { FlowStyleRewriter } from '../utils/flow-style-rewriter';
+import { ASTNode } from '../jsonASTTypes';
+import * as _ from 'lodash';
+import { SourceToken } from 'yaml/dist/parse/cst';
 
 interface YamlDiagnosticData {
   schemaUri: string[];
@@ -50,6 +53,7 @@ export class YamlCodeActions {
     result.push(...this.getTabToSpaceConverting(params.context.diagnostics, document));
     result.push(...this.getUnusedAnchorsDelete(params.context.diagnostics, document));
     result.push(...this.getConvertToBlockStyleActions(params.context.diagnostics, document));
+    result.push(...this.getKeyOrderActions(params.context.diagnostics, document));
 
     return result;
   }
@@ -217,10 +221,7 @@ export class YamlCodeActions {
     const results: CodeAction[] = [];
     for (const diagnostic of diagnostics) {
       if (diagnostic.code === 'flowMap' || diagnostic.code === 'flowSeq') {
-        const yamlDocuments = yamlDocumentsCache.getYamlDocument(document);
-        const startOffset = document.offsetAt(diagnostic.range.start);
-        const yamlDoc = matchOffsetToDocument(startOffset, yamlDocuments);
-        const node = yamlDoc.getNodeFromOffset(startOffset);
+        const node = getNodeforDiagnostic(document, diagnostic);
         if (isMap(node.internalNode) || isSeq(node.internalNode)) {
           const blockTypeDescription = isMap(node.internalNode) ? 'map' : 'sequence';
           const rewriter = new FlowStyleRewriter(this.indentation);
@@ -236,6 +237,89 @@ export class YamlCodeActions {
     }
     return results;
   }
+
+  private getKeyOrderActions(diagnostics: Diagnostic[], document: TextDocument): CodeAction[] {
+    const results: CodeAction[] = [];
+    for (const diagnostic of diagnostics) {
+      if (diagnostic?.code === 'mapKeyOrder') {
+        let node = getNodeforDiagnostic(document, diagnostic);
+        while (node && node.type !== 'object') {
+          node = node.parent;
+        }
+        if (node && isMap(node.internalNode)) {
+          const sorted: YAMLMap = _.cloneDeep(node.internalNode);
+          if (
+            (sorted.srcToken.type === 'block-map' || sorted.srcToken.type === 'flow-collection') &&
+            (node.internalNode.srcToken.type === 'block-map' || node.internalNode.srcToken.type === 'flow-collection')
+          ) {
+            sorted.srcToken.items.sort((a, b) => {
+              if (a.key && b.key && CST.isScalar(a.key) && CST.isScalar(b.key)) {
+                return a.key.source.localeCompare(b.key.source);
+              }
+              if (!a.key && b.key) {
+                return -1;
+              }
+              if (a.key && !b.key) {
+                return 1;
+              }
+              if (!a.key && !b.key) {
+                return 0;
+              }
+            });
+
+            for (let i = 0; i < sorted.srcToken.items.length; i++) {
+              const item = sorted.srcToken.items[i];
+              const uItem = node.internalNode.srcToken.items[i];
+              item.start = uItem.start;
+              if (
+                item.value?.type === 'alias' ||
+                item.value?.type === 'scalar' ||
+                item.value?.type === 'single-quoted-scalar' ||
+                item.value?.type === 'double-quoted-scalar'
+              ) {
+                const newLineIndex = item.value?.end?.findIndex((p) => p.type === 'newline') ?? -1;
+                let newLineToken = null;
+                if (uItem.value?.type === 'block-scalar') {
+                  newLineToken = uItem.value?.props?.find((p) => p.type === 'newline');
+                } else if (CST.isScalar(uItem.value)) {
+                  newLineToken = uItem.value?.end?.find((p) => p.type === 'newline');
+                }
+                if (newLineToken && newLineIndex < 0) {
+                  item.value.end = item.value.end ?? [];
+                  item.value.end.push(newLineToken as SourceToken);
+                }
+                if (!newLineToken && newLineIndex > -1) {
+                  item.value.end.splice(newLineIndex, 1);
+                }
+              } else if (item.value?.type === 'block-scalar') {
+                const nwline = item.value.props.find((p) => p.type === 'newline');
+                if (!nwline) {
+                  item.value.props.push({ type: 'newline', indent: 0, offset: item.value.offset, source: '\n' } as SourceToken);
+                }
+              }
+            }
+          }
+          const replaceRange = Range.create(document.positionAt(node.offset), document.positionAt(node.offset + node.length));
+          results.push(
+            CodeAction.create(
+              'Fix key order for this map',
+              createWorkspaceEdit(document.uri, [TextEdit.replace(replaceRange, CST.stringify(sorted.srcToken))]),
+              CodeActionKind.QuickFix
+            )
+          );
+        }
+      }
+    }
+    return results;
+  }
+}
+
+function getNodeforDiagnostic(document: TextDocument, diagnostic: Diagnostic): ASTNode {
+  const yamlDocuments = yamlDocumentsCache.getYamlDocument(document);
+  const startOffset = document.offsetAt(diagnostic.range.start);
+  const yamlDoc = matchOffsetToDocument(startOffset, yamlDocuments);
+  const node = yamlDoc.getNodeFromOffset(startOffset);
+  return node;
 }
 
 function createWorkspaceEdit(uri: string, edits: TextEdit[]): WorkspaceEdit {

src/languageservice/services/yamlValidation.ts

@@ -20,6 +20,7 @@ import { Telemetry } from '../telemetry';
 import { AdditionalValidator } from './validation/types';
 import { UnusedAnchorsValidator } from './validation/unused-anchors';
 import { YAMLStyleValidator } from './validation/yaml-style';
+import { MapKeyOrderValidator } from './validation/map-key-order';
 
 /**
  * Convert a YAMLDocDiagnostic to a language server Diagnostic
@@ -64,6 +65,9 @@ export class YAMLValidation {
       if (settings.flowMapping === 'forbid' || settings.flowSequence === 'forbid') {
         this.validators.push(new YAMLStyleValidator(settings));
       }
+      if (settings.keyOrdering) {
+        this.validators.push(new MapKeyOrderValidator());
+      }
     }
     this.validators.push(new UnusedAnchorsValidator());
   }

src/languageservice/yamlLanguageService.ts

@@ -115,6 +115,10 @@ export interface LanguageSettings {
    * Control the use of flow sequences. Default is allow.
    */
   flowSequence?: 'allow' | 'forbid';
+  /**
+   * If set enforce alphabetical ordering of keys in mappings.
+   */
+  keyOrdering?: boolean;
 }
 
 export interface WorkspaceContextService {

src/yamlSettings.ts

@@ -29,6 +29,7 @@ export interface Settings {
       flowMapping: 'allow' | 'forbid';
       flowSequence: 'allow' | 'forbid';
     };
+    keyOrdering: boolean;
     maxItemsComputed: number;
     yamlVersion: YamlVersion;
   };
@@ -86,6 +87,7 @@ export class SettingsState {
     flowMapping: 'allow' | 'forbid';
     flowSequence: 'allow' | 'forbid';
   };
+  keyOrdering = true;
   maxItemsComputed = 5000;
 
   // File validation helpers

test/settingsHandlers.test.ts

@@ -110,6 +110,37 @@ describe('Settings Handlers Tests', () => {
     });
   });
 
+  describe('Settings for key ordering should ', () => {
+    it(' reflect to the settings ', async () => {
+      const settingsHandler = new SettingsHandler(
+        connection,
+        (languageService as unknown) as LanguageService,
+        settingsState,
+        (validationHandler as unknown) as ValidationHandler,
+        {} as Telemetry
+      );
+      workspaceStub.getConfiguration.resolves([{ keyOrdering: true }, {}, {}, {}, {}]);
+
+      await settingsHandler.pullConfiguration();
+      expect(settingsState.keyOrdering).to.exist;
+      expect(settingsState.keyOrdering).to.be.true;
+    });
+    it(' reflect default values if no settings given', async () => {
+      const settingsHandler = new SettingsHandler(
+        connection,
+        (languageService as unknown) as LanguageService,
+        settingsState,
+        (validationHandler as unknown) as ValidationHandler,
+        {} as Telemetry
+      );
+      workspaceStub.getConfiguration.resolves([{}, {}, {}, {}, {}]);
+
+      await settingsHandler.pullConfiguration();
+      expect(settingsState.style).to.exist;
+      expect(settingsState.keyOrdering).to.be.false;
+    });
+  });
+
   describe('Settings for file associations should ', () => {
     it('reflect to settings state', async () => {
       const settingsHandler = new SettingsHandler(

test/utils/serviceSetup.ts

@@ -71,4 +71,8 @@ export class ServiceSetup {
     this.languageSettings.flowSequence = sequence;
     return this;
   }
+  withKeyOrdering(order = true): ServiceSetup {
+    this.languageSettings.keyOrdering = order;
+    return this;
+  }
 }