Path Tree compaction refactor (#13415)

* Bones of a just-in-time compaction pathTree

* wooo got compaction going in sub-ms times

* PR cleanup

* Path compaction tests

* lint fix to equal instead of .ok()

* Name prop specifically being equality checked

ui/app/components/variable-paths.js

@@ -2,7 +2,7 @@
 import Component from '@glimmer/component';
 import { action } from '@ember/object';
 import { inject as service } from '@ember/service';
-
+import compactPath from '../utils/compact-path';
 export default class VariablePathsComponent extends Component {
   @service router;
 
@@ -10,8 +10,8 @@ export default class VariablePathsComponent extends Component {
    * @returns {Array<Object.<string, Object>>}
    */
   get folders() {
-    return Object.entries(this.args.branch.children).map(([name, data]) => {
-      return { name, data };
+    return Object.entries(this.args.branch.children).map(([name]) => {
+      return compactPath(this.args.branch.children[name], name);
     });
   }
 

ui/app/controllers/variables/variable/index.js

@@ -14,8 +14,8 @@ export default class VariablesVariableIndexController extends Controller {
     try {
       yield this.model.deleteRecord();
       yield this.model.save();
-      if (this.model.folderPath) {
-        this.router.transitionTo('variables.path', this.model.folderPath);
+      if (this.model.parentFolderPath) {
+        this.router.transitionTo('variables.path', this.model.parentFolderPath);
       } else {
         this.router.transitionTo('variables');
       }

ui/app/models/variable.js

@@ -6,7 +6,6 @@ import classic from 'ember-classic-decorator';
 // eslint-disable-next-line no-unused-vars
 import MutableArray from '@ember/array/mutable';
 import { trimPath } from '../helpers/trim-path';
-import { pathToObject } from '../utils/path-tree';
 
 /**
  * @typedef KeyValue
@@ -57,8 +56,10 @@ export default class VariableModel extends Model {
   @attr('string') namespace;
 
   @computed('path')
-  get folderPath() {
-    return pathToObject(this.path).path;
+  get parentFolderPath() {
+    const split = this.path.split('/');
+    const [, ...folderPath] = [split.pop(), ...split];
+    return folderPath.join('/');
   }
 
   /**

ui/app/routes/variables/path.js

@@ -1,5 +1,4 @@
 import Route from '@ember/routing/route';
-
 export default class VariablesPathRoute extends Route {
   model({ absolutePath }) {
     const treeAtPath =

ui/app/utils/compact-path.js

@@ -0,0 +1,18 @@
+/**
+ * Takes a branch created by our path-tree, and if it has only a single directory as descendent and no files, compacts it down to its terminal folder (the first descendent folder with either files or branching directories)
+ * Uses tail recursion
+ * @param {import("./path-tree").NestedPathTreeNode} branch
+ * @returns
+ */
+export default function compactPath(branch, name = '') {
+  let { children, files } = branch;
+  if (children && Object.keys(children).length === 1 && !files.length) {
+    const [key] = Object.keys(children);
+    const child = children[key];
+    return compactPath(child, `${name}/${key}`);
+  }
+  return {
+    name,
+    data: branch,
+  };
+}

ui/app/utils/path-tree.js

@@ -8,52 +8,25 @@ import { trimPath } from '../helpers/trim-path';
 
 //#region Types
 /**
- * @typedef {Object} VariablePathObject
+ * @typedef {Object} VariableFile
  * @property {string} path - the folder path containing our "file", relative to parent
  * @property {string} name - the secure variable "file" name
- * @property {string} [absoluteFilePath] - the folder path containing our "file", absolute
- * @property {string} [absolutePath] - the folder path containing our "file", absolute
+ * @property {string} absoluteFilePath - the folder path containing our "file", absolute
+ * @property {VariableModel} variable - the secure variable itself
  */
 
 /**
- * @typedef {Object.<string, Object>} NestedPathTreeNode
+ * @typedef {Object} VariableFolder
+ * @property {Array<VariableFile>} files
+ * @property {NestedPathTreeNode} children
+ * @property {string} absolutePath - the folder path containing our "file", absolute
+ */
+
+/**
+ * @typedef {Object.<string, VariableFolder>} NestedPathTreeNode
  */
 //#endregion Types
 
-/**
- * Turns a file path into an object with file and path properties.
- * @param {string} path - the file path
- * @return {VariablePathObject}
- */
-export function pathToObject(path) {
-  const split = path.split('/');
-  const [name, ...folderPath] = [split.pop(), ...split];
-  return {
-    name,
-    absoluteFilePath: path,
-    path: folderPath.join('/'),
-  };
-}
-
-/**
- * Compacts an object of path:file key-values so any same-common-ancestor paths are collapsed into a single path.
- * @param {NestedPathTreeNode} vars
- * @returns {void}}
- */
-function COMPACT_EMPTY_DIRS(vars) {
-  Object.keys(vars).map((pathString) => {
-    const encompasser = Object.keys(vars).find(
-      (ps) => ps !== pathString && pathString.startsWith(ps)
-    );
-    if (encompasser) {
-      vars[encompasser].children[pathString.replace(encompasser, '')] =
-        vars[pathString];
-      delete vars[pathString];
-      COMPACT_EMPTY_DIRS(vars[encompasser].children);
-    }
-  });
-}
-
 /**
  * @returns {NestedPathTreeNode}
  */
@@ -63,43 +36,45 @@ export default class PathTree {
    */
   constructor(variables) {
     this.variables = variables;
-    this.paths = this.generatePaths();
+    this.paths = this.generatePaths(variables);
   }
 
   /**
-   * Takes our variables array and groups them by common path
+   * @type {VariableFolder}
+   */
+  root = { children: {}, files: [], absolutePath: '' };
+
+  /**
+   * Takes our variables array and creates a tree of paths
+   * @param {MutableArray<VariableModel>} variables
    * @returns {NestedPathTreeNode}
    */
-  generatePaths = () => {
-    const paths = this.variables
-      .map((variable) => trimPath([variable.path]))
-      .map(pathToObject)
-      .reduce(
-        (acc, cur) => {
-          const { name, absoluteFilePath } = cur;
-          if (cur.path) {
-            acc.root.children[cur.path]
-              ? acc.root.children[cur.path].files.push({
-                  name,
-                  absoluteFilePath,
-                })
-              : (acc.root.children[cur.path] = {
-                  files: [{ name, absoluteFilePath }],
-                  children: {},
-                });
-            acc.root.children[cur.path].absolutePath = cur.path;
-          } else {
-            acc.root.files
-              ? acc.root.files.push({ name, absoluteFilePath })
-              : (acc.root.files = [{ name, absoluteFilePath }]);
+  generatePaths = (variables) => {
+    variables.forEach((variable) => {
+      const path = trimPath([variable.path]).split('/');
+      path.reduce((acc, segment, index, arr) => {
+        if (index === arr.length - 1) {
+          // If it's a file (end of the segment array)
+          acc.files.push({
+            name: segment,
+            absoluteFilePath: path.join('/'),
+            path: arr.slice(0, index + 1).join('/'),
+            variable,
+          });
+        } else {
+          // Otherwise, it's a folder
+          if (!acc.children[segment]) {
+            acc.children[segment] = {
+              children: {},
+              files: [],
+              absolutePath: trimPath([`${acc.absolutePath || ''}/${segment}`]),
+            };
           }
-          return acc;
-        },
-        { root: { files: [], children: {}, absolutePath: '' } }
-      );
-
-    COMPACT_EMPTY_DIRS(paths.root.children);
-    return paths;
+        }
+        return acc.children[segment];
+      }, this.root);
+    });
+    return { root: this.root };
   };
 
   /**

ui/tests/unit/utils/compact-path-test.js

@@ -0,0 +1,44 @@
+import compactPath from 'nomad-ui/utils/compact-path';
+import pathTree from 'nomad-ui/utils/path-tree';
+import { module, test } from 'qunit';
+
+const PATHSTRINGS = [
+  { path: 'a/b/c/d/e/foo0' },
+  { path: 'a/b/c/d/e/bar1' },
+  { path: 'a/b/c/d/e/baz2' },
+  { path: 'a/b/c/d/e/z/z/z/z/z/z/z/z/z/z/foo3' },
+  { path: 'z/y/x/dalmation/index' },
+  { path: 'z/y/x/doberman/index' },
+  { path: 'z/y/x/dachshund/index' },
+  { path: 'z/y/x/dachshund/poppy' },
+];
+
+module('Unit | Utility | compact-path', function () {
+  test('it compacts empty folders correctly', function (assert) {
+    const tree = new pathTree(PATHSTRINGS);
+    assert.ok(
+      'a' in tree.paths.root.children,
+      'root.a exists in the path tree despite having no files and only a single path'
+    );
+    assert.equal(
+      compactPath(tree.root.children['a'], 'a').name,
+      'a/b/c/d/e',
+      'but root.a is displayed compacted down to /e from its root level folder'
+    );
+    assert.equal(
+      compactPath(tree.findPath('z/y'), 'y').name,
+      'y/x',
+      'Path z/y is compacted to y/x, since it has a single child'
+    );
+    assert.equal(
+      compactPath(tree.findPath('z/y/x'), 'x').name,
+      'x',
+      'Path z/y/x is uncompacted, since it has multiple children'
+    );
+    assert.equal(
+      compactPath(tree.findPath('a/b/c/d/e/z'), 'z').name,
+      'z/z/z/z/z/z/z/z/z/z',
+      'Long path is recursively compacted'
+    );
+  });
+});

ui/tests/unit/utils/path-tree-test.js

@@ -32,27 +32,6 @@ module('Unit | Utility | path-tree', function () {
     );
   });
 
-  test('it compacts empty folders correctly', function (assert) {
-    const tree = new pathTree(PATHSTRINGS);
-    assert.ok(
-      'a' in tree.paths.root.children,
-      'root.a is uncompacted since it contains a file (b)'
-    );
-    assert.notOk(
-      'foo' in tree.paths.root.children,
-      'root.foo does not exist since it contains no files'
-    );
-    assert.ok(
-      'foo/bar' in tree.paths.root.children,
-      'root.foo/bar is compacted since the only child from foo is bar'
-    );
-    assert.equal(
-      tree.paths.root.children['foo/bar'].files.length,
-      3,
-      'A compacted directory contains all terminal files'
-    );
-  });
-
   test('it allows for node-based search and traversal', function (assert) {
     const tree = new pathTree(PATHSTRINGS);
     assert.deepEqual(
@@ -60,10 +39,10 @@ module('Unit | Utility | path-tree', function () {
       tree.findPath(''),
       'Returns tree root on default findPath'
     );
-    assert.notOk(
+    assert.ok(
       tree.findPath('foo'),
-      'No path found at the first part of a concatenated folder'
-    ); // TODO: but maybe we want this to work eventually, so if this test fails because you add mid-tree traversal? Great!
+      'Path found at the first part of a concatenated folder'
+    );
     assert.ok(
       tree.findPath('foo/bar'),
       'Finds a path at the concatenated folder path'