Merge branch 'add-error-class-to-yaml-ext' into 'master'

Yaml Editor Extension: Handle node not found more gracefully

See merge request gitlab-org/gitlab!78261

app/assets/javascripts/editor/extensions/source_editor_yaml_ext.js

@@ -19,33 +19,6 @@ export class YamlEditorExtension {
     return 'YamlEditor';
   }
 
-  /**
-   * Extends the source editor with capabilities for yaml files.
-   *
-   * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance
-   * @param {YamlEditorExtensionOptions} setupOptions
-   */
-  onSetup(instance, setupOptions = {}) {
-    const { enableComments = false, highlightPath = null, model = null } = setupOptions;
-    this.enableComments = enableComments;
-    this.highlightPath = highlightPath;
-    this.model = model;
-
-    if (model) {
-      this.initFromModel(instance, model);
-    }
-
-    instance.onDidChangeModelContent(() => instance.onUpdate());
-  }
-
-  initFromModel(instance, model) {
-    const doc = new Document(model);
-    if (this.enableComments) {
-      YamlEditorExtension.transformComments(doc);
-    }
-    instance.setValue(doc.toString());
-  }
-
   /**
    * @private
    * This wraps long comments to a maximum line length of 80 chars.
@@ -164,10 +137,10 @@ export class YamlEditorExtension {
     if (!path) throw Error(`No path provided.`);
     const blob = instance.getValue();
     const doc = parseDocument(blob);
-    const pathArray = toPath(path);
+    const pathArray = Array.isArray(path) ? path : toPath(path);
 
     if (!doc.getIn(pathArray)) {
-      throw Error(`The node ${path} could not be found inside the document.`);
+      return [null, null];
     }
 
     const parentNode = doc.getIn(pathArray.slice(0, pathArray.length - 1));
@@ -190,6 +163,33 @@ export class YamlEditorExtension {
     return [startLine, endLine];
   }
 
+  /**
+   * Extends the source editor with capabilities for yaml files.
+   *
+   * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance
+   * @param {YamlEditorExtensionOptions} setupOptions
+   */
+  onSetup(instance, setupOptions = {}) {
+    const { enableComments = false, highlightPath = null, model = null } = setupOptions;
+    this.enableComments = enableComments;
+    this.highlightPath = highlightPath;
+    this.model = model;
+
+    if (model) {
+      this.initFromModel(instance, model);
+    }
+
+    instance.onDidChangeModelContent(() => instance.onUpdate());
+  }
+
+  initFromModel(instance, model) {
+    const doc = new Document(model);
+    if (this.enableComments) {
+      YamlEditorExtension.transformComments(doc);
+    }
+    instance.setValue(doc.toString());
+  }
+
   setDoc(instance, doc) {
     if (this.enableComments) {
       YamlEditorExtension.transformComments(doc);
@@ -202,18 +202,31 @@ export class YamlEditorExtension {
     }
   }
 
-  highlight(instance, path) {
+  highlight(instance, path, keepOnNotFound = false) {
     // IMPORTANT
     // removeHighlight and highlightLines both come from
     // SourceEditorExtension. So it has to be installed prior to this extension
     if (this.highlightPath === path) return;
-    if (!path) {
+
+    if (!path || !path.length) {
       instance.removeHighlights();
-    } else {
-      const res = YamlEditorExtension.locate(instance, path);
-      instance.highlightLines(res);
+      this.highlightPath = null;
+      return;
     }
-    this.highlightPath = path || null;
+
+    const [startLine, endLine] = YamlEditorExtension.locate(instance, path);
+
+    if (startLine === null) {
+      // Path could not be found.
+      if (!keepOnNotFound) {
+        instance.removeHighlights();
+        this.highlightPath = null;
+      }
+      return;
+    }
+
+    instance.highlightLines([startLine, endLine]);
+    this.highlightPath = path;
   }
 
   provides() {
@@ -283,18 +296,23 @@ export class YamlEditorExtension {
        * Add a line highlight style to the node specified by the path.
        *
        * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance
-       * @param {string|null|false} path A path to a node of the Editor's value,
+       * @param {string|(string|number)[]|null|false} path A path to a node
+       * of the Editor's
+       * value,
        * e.g. `"foo.bar[0]"`. If the value is falsy, this will remove all
        * highlights.
+       * @param {boolean} [keepOnNotFound=false] If the passed path cannot
+       * be located, keep the previous highlight state
        */
-      highlight: (instance, path) => this.highlight(instance, path),
+      highlight: (instance, path, keepOnNotFound) => this.highlight(instance, path, keepOnNotFound),
 
       /**
        * Return the line numbers of a certain node identified by `path` within
        * the yaml.
        *
        * @param {module:source_editor_instance~EditorInstance} instance - The Source Editor instance
-       * @param {string} path A path to a node, eg. `foo.bar[0]`
+       * @param {string|(string|number)[]} path A path to a node, eg.
+       * `foo.bar[0]`
        * @returns {number[]} Array following the schema `[firstLine, lastLine]`
        * (both inclusive)
        *

spec/frontend/editor/source_editor_yaml_ext_spec.js

@@ -353,58 +353,78 @@ foo:
   });
 
   describe('highlight', () => {
-    const highlightPathOnSetup = 'abc';
     const value = `foo:
   bar:
     - baz
     - boo
-  abc: def
+abc: def
 `;
     let instance;
     let highlightLinesSpy;
     let removeHighlightsSpy;
 
-    beforeEach(() => {
-      instance = getEditorInstanceWithExtension({ highlightPath: highlightPathOnSetup }, { value });
-      highlightLinesSpy = jest.fn();
-      removeHighlightsSpy = jest.fn();
-      spyOnApi(baseExtension, {
-        highlightLines: highlightLinesSpy,
-        removeHighlights: removeHighlightsSpy,
-      });
-    });
-
     afterEach(() => {
       jest.clearAllMocks();
     });
 
-    it('saves the highlighted path in highlightPath', () => {
-      const path = 'foo.bar';
-      instance.highlight(path);
-      expect(yamlExtension.obj.highlightPath).toEqual(path);
-    });
-
-    it('calls highlightLines with a number of lines', () => {
-      const path = 'foo.bar';
-      instance.highlight(path);
-      expect(highlightLinesSpy).toHaveBeenCalledWith(instance, [2, 4]);
-    });
-
-    it('calls removeHighlights if path is null', () => {
-      instance.highlight(null);
-      expect(removeHighlightsSpy).toHaveBeenCalledWith(instance);
-      expect(highlightLinesSpy).not.toHaveBeenCalled();
-      expect(yamlExtension.obj.highlightPath).toBeNull();
-    });
-
-    it('throws an error if path is invalid and does not change the highlighted path', () => {
-      expect(() => instance.highlight('invalidPath[0]')).toThrow(
-        'The node invalidPath[0] could not be found inside the document.',
-      );
-      expect(yamlExtension.obj.highlightPath).toEqual(highlightPathOnSetup);
-      expect(highlightLinesSpy).not.toHaveBeenCalled();
-      expect(removeHighlightsSpy).not.toHaveBeenCalled();
-    });
+    it.each`
+      highlightPathOnSetup | path              | keepOnNotFound | expectHighlightLinesToBeCalled | withLines    | expectRemoveHighlightsToBeCalled | storedHighlightPath
+      ${null}              | ${undefined}      | ${false}       | ${false}                       | ${undefined} | ${true}                          | ${null}
+      ${'abc'}             | ${'abc'}          | ${undefined}   | ${false}                       | ${undefined} | ${false}                         | ${'abc'}
+      ${null}              | ${null}           | ${false}       | ${false}                       | ${undefined} | ${false}                         | ${null}
+      ${null}              | ${''}             | ${false}       | ${false}                       | ${undefined} | ${true}                          | ${null}
+      ${null}              | ${''}             | ${true}        | ${false}                       | ${undefined} | ${true}                          | ${null}
+      ${'abc'}             | ${''}             | ${false}       | ${false}                       | ${undefined} | ${true}                          | ${null}
+      ${'abc'}             | ${'foo.bar'}      | ${false}       | ${true}                        | ${[2, 4]}    | ${false}                         | ${'foo.bar'}
+      ${'abc'}             | ${['foo', 'bar']} | ${false}       | ${true}                        | ${[2, 4]}    | ${false}                         | ${['foo', 'bar']}
+      ${'abc'}             | ${'invalid'}      | ${true}        | ${false}                       | ${undefined} | ${false}                         | ${'abc'}
+      ${'abc'}             | ${'invalid'}      | ${false}       | ${false}                       | ${undefined} | ${true}                          | ${null}
+      ${'abc'}             | ${'invalid'}      | ${undefined}   | ${false}                       | ${undefined} | ${true}                          | ${null}
+      ${'abc'}             | ${['invalid']}    | ${undefined}   | ${false}                       | ${undefined} | ${true}                          | ${null}
+      ${'abc'}             | ${['invalid']}    | ${true}        | ${false}                       | ${undefined} | ${false}                         | ${'abc'}
+      ${'abc'}             | ${[]}             | ${true}        | ${false}                       | ${undefined} | ${true}                          | ${null}
+      ${'abc'}             | ${[]}             | ${false}       | ${false}                       | ${undefined} | ${true}                          | ${null}
+    `(
+      'returns correct result for highlightPathOnSetup=$highlightPathOnSetup, path=$path' +
+        ' and keepOnNotFound=$keepOnNotFound',
+      ({
+        highlightPathOnSetup,
+        path,
+        keepOnNotFound,
+        expectHighlightLinesToBeCalled,
+        withLines,
+        expectRemoveHighlightsToBeCalled,
+        storedHighlightPath,
+      }) => {
+        instance = getEditorInstanceWithExtension(
+          { highlightPath: highlightPathOnSetup },
+          { value },
+        );
+
+        highlightLinesSpy = jest.fn();
+        removeHighlightsSpy = jest.fn();
+        spyOnApi(baseExtension, {
+          highlightLines: highlightLinesSpy,
+          removeHighlights: removeHighlightsSpy,
+        });
+
+        instance.highlight(path, keepOnNotFound);
+
+        if (expectHighlightLinesToBeCalled) {
+          expect(highlightLinesSpy).toHaveBeenCalledWith(instance, withLines);
+        } else {
+          expect(highlightLinesSpy).not.toHaveBeenCalled();
+        }
+
+        if (expectRemoveHighlightsToBeCalled) {
+          expect(removeHighlightsSpy).toHaveBeenCalled();
+        } else {
+          expect(removeHighlightsSpy).not.toHaveBeenCalled();
+        }
+
+        expect(yamlExtension.obj.highlightPath).toEqual(storedHighlightPath);
+      },
+    );
   });
 
   describe('locate', () => {
@@ -446,10 +466,10 @@ foo:
       expect(instance.locate(path)).toEqual(expected);
     });
 
-    it('throws an error if a path cannot be found inside the yaml', () => {
+    it('returns [null, null] if a path cannot be found inside the yaml', () => {
       const path = 'baz[8]';
       const instance = getEditorInstanceWithExtension(options);
-      expect(() => instance.locate(path)).toThrow();
+      expect(instance.locate(path)).toEqual([null, null]);
     });
 
     it('returns the expected line numbers for a path to an array entry inside the yaml', () => {