chore: ensure consistency of ContextNode keys and add new tests (#6254

)

* refactor: a bit more verbose code for traversing tree nodes

To make it a bit easier to read and debug.

* chore: add JSDocs for `ContextNode` to explain its semantics

* test: add test for the current semantics of `getByContext`

* test: add test to ensure internal state and invariants of `TreeCache`

* fix: prevent unwanted key mutations in `ContextNode`

Use a copy of a context key array when creating a new node.

Otherwise, the parent node will be modified and while child node creation,
and will get the same key.

It doesn't seem to be an actual bug,
but such keys look very confusing in the debugger.

core/src/cache.ts

@@ -25,7 +25,13 @@ interface CacheEntry {
 
 type CacheEntries = Map<string, CacheEntry>
 
-interface ContextNode {
+/**
+ * Represents a node (either a non-leaf or a leaf) node of the context tree.
+ *
+ *  - A non-leaf node can have only children and has no entries.
+ *  - A leaf node has only entries and cannot have any children.
+ */
+export interface ContextNode {
   key: CacheContext
   children: { [contextPart: string]: ContextNode }
   entries: Set<string>
@@ -103,7 +109,7 @@ export class TreeCache {
       entry.value = value
     }
 
-    contexts.forEach((c) => (entry!.contexts[stringifyKey(c)] = c))
+    contexts.forEach((c) => (entry.contexts[stringifyKey(c)] = c))
 
     for (const context of Object.values(contexts)) {
       let node = this.contextTree
@@ -121,10 +127,13 @@ export class TreeCache {
       for (const part of context) {
         contextKey.push(part)
 
-        if (node.children[part]) {
-          node = node.children[part]
+        let child = node.children[part]
+        if (child) {
+          node = child
         } else {
-          node = node.children[part] = makeContextNode(contextKey)
+          child = makeContextNode([...contextKey])
+          node.children[part] = child
+          node = child
         }
       }
 

core/test/unit/src/cache.ts

@@ -6,6 +6,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
+import type { ContextNode } from "../../../src/cache.js"
 import { BoundedCache, TreeCache } from "../../../src/cache.js"
 import { expect } from "chai"
 import { expectError } from "../../helpers.js"
@@ -41,6 +42,61 @@ describe("TreeCache", () => {
     expect(cache.get(log, key)).to.equal(value)
   })
 
+  it("ContextNode should have consistent node states and hierarchical context keys", () => {
+    const key = ["my-key"]
+    const value = "my-value"
+    const parentContext = ["context"]
+    const contextA = [...parentContext, "a"]
+    const contextB = [...parentContext, "b"]
+
+    cache.set(log, key, value, contextA, contextB)
+
+    const contextTreeRoot = cache["contextTree"] as ContextNode
+    expect(contextTreeRoot.key).to.be.empty
+    // non-leaf nodes contain only children
+    expect(contextTreeRoot.children).to.be.not.empty
+    // non-leaf nodes do not contain any entries
+    expect(contextTreeRoot.entries).to.be.empty
+
+    const parentContextNode = contextTreeRoot.children["context"]
+    expect(parentContextNode.key).to.eql(parentContext)
+    // non-leaf nodes contain only children
+    expect(parentContextNode.children).to.be.not.empty
+    // non-leaf nodes do not contain any entries
+    expect(parentContextNode.entries).to.be.empty
+
+    const expectedEntries = new Set([JSON.stringify(key)])
+
+    const contextNodeA = parentContextNode.children["a"]
+    expect(contextNodeA.key).to.eql(contextA)
+    // leaf nodes do not contain any children
+    expect(contextNodeA.children).to.be.empty
+    // leaf nodes contain only entries
+    expect(contextNodeA.entries).to.eql(expectedEntries)
+
+    const contextNodeB = parentContextNode.children["b"]
+    expect(contextNodeB.key).to.eql(contextB)
+    // leaf nodes do not contain any children
+    expect(contextNodeB.children).to.be.empty
+    // leaf nodes contain only entries
+    expect(contextNodeB.entries).to.eql(expectedEntries)
+  })
+
+  describe("getByContext", () => {
+    it("should NOT return anything for incomplete (partial) context", () => {
+      const key = ["my-key"]
+      const value = "my-value"
+      const parentContext = ["context"]
+      const contextA = [...parentContext, "a"]
+      const contextB = [...parentContext, "b"]
+
+      cache.set(log, key, value, contextA, contextB)
+
+      // parent context references a "non-leaf" node that never contains any entries
+      expect(mapToPairs(cache.getByContext(parentContext))).to.eql([])
+    })
+  })
+
   describe("set", () => {
     it("should accept multiple contexts", () => {
       const key = ["my-key"]