Merge pull request #932 from concord-consortium/186201910-formula-att…

…r-delete

Improve error handling when an attribute used by formula is deleted

v3/src/models/data/formula-fn-registry.ts

@@ -1,7 +1,7 @@
 import { create, all, mean, median, mad, max, min, sum, random, pickRandom, MathNode, ConstantNode } from 'mathjs'
 import { FormulaMathJsScope } from './formula-mathjs-scope'
 import {
-  DisplayNameMap, FValue, CODAPMathjsFunctionRegistry, ILookupDependency, isConstantStringNode
+  DisplayNameMap, FValue, CODAPMathjsFunctionRegistry, ILookupDependency, isConstantStringNode, rmCanonicalPrefix
 } from './formula-types'
 import type { IDataSet } from './data-set'
 
@@ -132,8 +132,8 @@ export const fnRegistry = {
       validArgs[1].value = displayNameMap.dataSet[dataSetName]?.attribute[attrName]
     },
     evaluateRaw: (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => {
-      const dataSetId = evaluateNode(args[0], scope)
-      const attrId = evaluateNode(args[1], scope)
+      const dataSetId = rmCanonicalPrefix(evaluateNode(args[0], scope))
+      const attrId = rmCanonicalPrefix(evaluateNode(args[1], scope))
       const zeroBasedIndex = evaluateNode(args[2], scope) - 1
       return scope.getDataSet(dataSetId)?.getValueAtIndex(zeroBasedIndex, attrId) || UNDEF_RESULT
     }
@@ -170,9 +170,9 @@ export const fnRegistry = {
       validArgs[2].value = displayNameMap.dataSet[dataSetName]?.attribute[keyAttrName]
     },
     evaluateRaw: (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => {
-      const dataSetId = evaluateNode(args[0], scope)
-      const attrId = evaluateNode(args[1], scope)
-      const keyAttrId = evaluateNode(args[2], scope)
+      const dataSetId = rmCanonicalPrefix(evaluateNode(args[0], scope))
+      const attrId = rmCanonicalPrefix(evaluateNode(args[1], scope))
+      const keyAttrId = rmCanonicalPrefix(evaluateNode(args[2], scope))
       const keyAttrValue = evaluateNode(args[3], scope)
 
       const dataSet: IDataSet | undefined = scope.getDataSet(dataSetId)

v3/src/models/data/formula-manager.ts

@@ -11,7 +11,7 @@ import {
 } from "./formula-utils"
 import {
   DisplayNameMap, IFormulaDependency, GLOBAL_VALUE, LOCAL_ATTR, ILocalAttributeDependency, IGlobalValueDependency,
-  ILookupDependency, NO_PARENT_KEY, FValue, CASE_INDEX_FAKE_ATTR_ID
+  ILookupDependency, NO_PARENT_KEY, FValue, CASE_INDEX_FAKE_ATTR_ID, CANONICAL_NAME
 } from "./formula-types"
 import { math } from "./formula-fn-registry"
 import { IDataSet } from "./data-set"
@@ -303,12 +303,6 @@ export class FormulaManager {
     })
   }
 
-  updateDisplayFormulas() {
-    this.formulaMetadata.forEach(({ formula }) => {
-      formula.updateDisplayFormula()
-    })
-  }
-
   unregisterFormula(formulaId: string) {
     const formulaMetadata = this.formulaMetadata.get(formulaId)
     if (formulaMetadata) {
@@ -372,10 +366,11 @@ export class FormulaManager {
       dataSet: {}
     }
 
-    const mapAttributeNames = (dataSet: IDataSet, prefix: string, _useSafeSymbolNames: boolean) => {
+    const mapAttributeNames = (dataSet: IDataSet, localPrefix: string, _useSafeSymbolNames: boolean) => {
       const result: Record<string, string> = {}
       dataSet.attributes.forEach(attr => {
-        result[_useSafeSymbolNames ? safeSymbolName(attr.name) : attr.name] = `${prefix}${attr.id}`
+        const key = _useSafeSymbolNames ? safeSymbolName(attr.name) : attr.name
+        result[key] = `${CANONICAL_NAME}${localPrefix}${attr.id}`
       })
       return result
     }
@@ -384,18 +379,18 @@ export class FormulaManager {
       ...mapAttributeNames(localDataSet, LOCAL_ATTR, useSafeSymbolNames),
       // caseIndex is a special name supported by formulas. It essentially behaves like a local data set attribute
       // that returns the current, 1-based index of the case in its collection group.
-      caseIndex: `${LOCAL_ATTR}${CASE_INDEX_FAKE_ATTR_ID}`
+      caseIndex: `${CANONICAL_NAME}${LOCAL_ATTR}${CASE_INDEX_FAKE_ATTR_ID}`
     }
 
     this.globalValueManager?.globals.forEach(global => {
       const key = useSafeSymbolNames ? safeSymbolName(global.name) : global.name
-      displayNameMap.localNames[key] = `${GLOBAL_VALUE}${global.id}`
+      displayNameMap.localNames[key] = `${CANONICAL_NAME}${GLOBAL_VALUE}${global.id}`
     })
 
     this.dataSets.forEach(dataSet => {
       if (dataSet.name) {
         displayNameMap.dataSet[dataSet.name] = {
-          id: dataSet.id,
+          id: `${CANONICAL_NAME}${dataSet.id}`,
           // No prefix is necessary for external attributes. They always need to be resolved manually by custom
           // mathjs functions (like "lookupByIndex"). Also, it's never necessary to use safe names, as these names
           // are string constants, not a symbols, so MathJS will not care about special characters there.
@@ -432,7 +427,19 @@ export class FormulaManager {
       () => dataSet.attrNameMap,
       () => {
         this.unregisterDeletedFormulas()
-        this.updateDisplayFormulas()
+        this.formulaMetadata.forEach(({ formula }) => {
+          formula.updateDisplayFormula()
+          // Note that when attribute is removed or renamed, this can also affect the formula's canonical form.
+          // 1. Attribute is removed - its ID needs to be removed from the canonical form and the formula should be
+          //    recalculated (usually to show the error about undefined symbol).
+          // 2. Attribute is renamed - if the previous display form had undefined symbols, they might now be resolved
+          //    to the renamed attribute. This means that the canonical form needs to be updated.
+          const oldCanonical = formula.canonical
+          formula.updateCanonicalFormula()
+          if (oldCanonical !== formula.canonical) {
+            this.recalculateFormula(formula.id)
+          }
+        })
       },
       {
         equals: comparer.structural,

v3/src/models/data/formula-mathjs-scope.ts

@@ -1,4 +1,6 @@
-import { FValue, CASE_INDEX_FAKE_ATTR_ID, GLOBAL_VALUE, LOCAL_ATTR, NO_PARENT_KEY } from "./formula-types"
+import {
+  FValue, CASE_INDEX_FAKE_ATTR_ID, GLOBAL_VALUE, LOCAL_ATTR, NO_PARENT_KEY, CANONICAL_NAME
+} from "./formula-types"
 import type { IGlobalValueManager } from "../global/global-value-manager"
 import type { IDataSet } from "./data-set"
 import type { IValueType } from "./attribute"
@@ -52,7 +54,7 @@ export class FormulaMathJsScope {
       // Make sure that all the caching and case processing is done lazily, only for attributes that are actually
       // referenced by the formula.
       let cachedGroup: Record<string, IValueType[]>
-      Object.defineProperty(this.dataStorage, `${LOCAL_ATTR}${attrId}`, {
+      Object.defineProperty(this.dataStorage, `${CANONICAL_NAME}${LOCAL_ATTR}${attrId}`, {
         get: () => {
           if (!this.isAggregate) {
             return this.getLocalValue(this.caseId, attrId)
@@ -76,7 +78,7 @@ export class FormulaMathJsScope {
     })
     // Global value symbols.
     context.globalValueManager?.globals.forEach(global => {
-      Object.defineProperty(this.dataStorage, `${GLOBAL_VALUE}${global.id}`, {
+      Object.defineProperty(this.dataStorage, `${CANONICAL_NAME}${GLOBAL_VALUE}${global.id}`, {
         get: () => {
           return global.value
         }

v3/src/models/data/formula-types.test.ts

@@ -0,0 +1,31 @@
+import { CANONICAL_NAME, isCanonicalName, rmCanonicalPrefix } from "./formula-types"
+
+describe("isCanonicalName", () => {
+  it("returns true if the name starts with the canonical name", () => {
+    expect(isCanonicalName(`${CANONICAL_NAME}TEST`)).toBe(true)
+  })
+  it("returns false if the name does not start with the canonical name", () => {
+    expect(isCanonicalName("FOO_BAR")).toBe(false)
+  })
+  it("returns false if the name is undefined or unexpected type", () => {
+    expect(isCanonicalName(undefined)).toBe(false)
+    expect(isCanonicalName(1)).toBe(false)
+    expect(isCanonicalName({})).toBe(false)
+    expect(isCanonicalName([])).toBe(false)
+  })
+})
+
+describe("rmCanonicalPrefix", () => {
+  it("removes the canonical prefix from the name", () => {
+    expect(rmCanonicalPrefix(`${CANONICAL_NAME}TEST`)).toBe("TEST")
+  })
+  it("returns the original name if it does not start with the canonical name", () => {
+    expect(rmCanonicalPrefix("FOO_BAR")).toBe("FOO_BAR")
+  })
+  it("returns the if the name is undefined or unexpected type", () => {
+    expect(rmCanonicalPrefix(undefined)).toEqual(undefined)
+    expect(rmCanonicalPrefix(1)).toEqual(1)
+    expect(rmCanonicalPrefix({})).toEqual({})
+    expect(rmCanonicalPrefix([])).toEqual([])
+  })
+})

v3/src/models/data/formula-types.ts

@@ -1,6 +1,7 @@
 import { ConstantNode, MathNode, SymbolNode, isConstantNode, isFunctionNode, isSymbolNode } from "mathjs"
 import type { FormulaMathJsScope } from "./formula-mathjs-scope"
 
+export const CANONICAL_NAME = "__CANONICAL_NAME__"
 export const GLOBAL_VALUE = "GLOBAL_VALUE_"
 export const LOCAL_ATTR = "LOCAL_ATTR_"
 export const CASE_INDEX_FAKE_ATTR_ID = "CASE_INDEX"
@@ -15,6 +16,10 @@ export const isConstantStringNode = (node: MathNode): node is ConstantNode<strin
 export const isNonFunctionSymbolNode = (node: MathNode, parent: MathNode): node is SymbolNode =>
   isSymbolNode(node) && (!isFunctionNode(parent) || parent.fn !== node)
 
+export const isCanonicalName = (name: any): name is string => !!name?.startsWith?.(CANONICAL_NAME)
+
+export const rmCanonicalPrefix = (name: any) => isCanonicalName(name) ? name.substring(CANONICAL_NAME.length) : name
+
 export type DisplayNameMap = {
   localNames: Record<string, string>
   dataSet: Record<string, {

v3/src/models/data/formula-utils.test.ts

@@ -1,74 +1,95 @@
 import { ICase } from "./data-set-types"
-import { DisplayNameMap, ILocalAttributeDependency, ILookupDependency } from "./formula-types"
+import { CANONICAL_NAME, DisplayNameMap, GLOBAL_VALUE, ILocalAttributeDependency, ILookupDependency, LOCAL_ATTR } from "./formula-types"
 import {
   safeSymbolName, customizeDisplayFormula, reverseDisplayNameMap, canonicalToDisplay, makeDisplayNamesSafe,
   displayToCanonical, unescapeBacktickString, escapeBacktickString, safeSymbolNameFromDisplayFormula,
-  getLocalAttrCasesToRecalculate, getLookupCasesToRecalculate, isAttrDefined
+  getLocalAttrCasesToRecalculate, getLookupCasesToRecalculate, isAttrDefined, parseBasicCanonicalName
 } from "./formula-utils"
 
 const displayNameMapExample: DisplayNameMap = {
   "localNames": {
-    "LifeSpan": "LOCAL_ATTR_ATTR_LIFE_SPAN",
-    "Order": "LOCAL_ATTR_ATTR_ORDER",
-    "caseIndex": "LOCAL_ATTR_CASE_INDEX",
-    "v1": "GLOBAL_VALUE_GLOB_V1"
+    "LifeSpan": "__CANONICAL_NAME__LOCAL_ATTR_ATTR_LIFE_SPAN",
+    "Order": "__CANONICAL_NAME__LOCAL_ATTR_ATTR_ORDER",
+    "caseIndex": "__CANONICAL_NAME__LOCAL_ATTR_CASE_INDEX",
+    "v1": "__CANONICAL_NAME__GLOBAL_VALUE_GLOB_V1"
   },
   "dataSet": {
     "Mammals": {
-      "id": "DATA_MAMMALS",
+      "id": "__CANONICAL_NAME__DATA_MAMMALS",
       "attribute": {
-        "LifeSpan": "ATTR_LIFE_SPAN",
-        "Order": "ATTR_ORDER",
+        "LifeSpan": "__CANONICAL_NAME__ATTR_LIFE_SPAN",
+        "Order": "__CANONICAL_NAME__ATTR_ORDER",
       }
     },
     "Roller Coaster": {
-      "id": "DATA_ROLLER_COASTER",
+      "id": "__CANONICAL_NAME__DATA_ROLLER_COASTER",
       "attribute": {
         // \, ', and " are added to test characters escaping
-        "Park\"": "ATTR_PARK",
-        "Top\\Speed'": "ATTR_TOP_SPEED",
+        "Park\"": "__CANONICAL_NAME__ATTR_PARK",
+        "Top\\Speed'": "__CANONICAL_NAME__ATTR_TOP_SPEED",
       }
     }
   }
 }
 
+describe("parseBasicCanonicalName", () => {
+  it("returns undefined if the name is not a canonical name", () => {
+    const name = "FOO_BAR"
+    const result = parseBasicCanonicalName(name)
+    expect(result).toBeUndefined()
+  })
+  it("returns a local attribute dependency if the name starts with the local attribute prefix", () => {
+    const name = `${CANONICAL_NAME}${LOCAL_ATTR}foo`
+    const result = parseBasicCanonicalName(name)
+    expect(result).toEqual({ type: "localAttribute", attrId: "foo" })
+  })
+  it("returns a global value dependency if the name starts with the global value prefix", () => {
+    const name = `${CANONICAL_NAME}${GLOBAL_VALUE}bar`
+    const result = parseBasicCanonicalName(name)
+    expect(result).toEqual({ type: "globalValue", globalId: "bar" })
+  })
+})
+
 describe("displayToCanonical", () => {
   it("converts display formula to canonical formula", () => {
     expect(displayToCanonical(
       "mean(LifeSpan) * v1", displayNameMapExample
-    )).toEqual("mean(LOCAL_ATTR_ATTR_LIFE_SPAN) * GLOBAL_VALUE_GLOB_V1")
+    )).toEqual("mean(__CANONICAL_NAME__LOCAL_ATTR_ATTR_LIFE_SPAN) * __CANONICAL_NAME__GLOBAL_VALUE_GLOB_V1")
   })
   describe("when function name or constant is equal to attribute name", () => {
     const displayMap: DisplayNameMap = {
       localNames: {
-        mean: "LOCAL_ATTR_ATTR_MEAN",
+        mean: "__CANONICAL_NAME__LOCAL_ATTR_ATTR_MEAN",
       },
       dataSet: {}
     }
     it("still converts display formula to canonical formula correctly", () => {
       expect(displayToCanonical(
         "mean(mean) + 'mean'", displayMap
-      )).toEqual('mean(LOCAL_ATTR_ATTR_MEAN) + "mean"')
+      )).toEqual('mean(__CANONICAL_NAME__LOCAL_ATTR_ATTR_MEAN) + "mean"')
     })
   })
   describe("when attribute name includes special characters", () => {
     const testDisplayMap: DisplayNameMap = {
       localNames: {
-        [safeSymbolName("mean`attribute\\🙃")]: "LOCAL_ATTR_ATTR_MEAN",
+        [safeSymbolName("mean`attribute\\🙃")]: "__CANONICAL_NAME__LOCAL_ATTR_ATTR_MEAN",
       },
       dataSet: {}
     }
     it("works as long as it's enclosed in backticks", () => {
       expect(displayToCanonical(
         "mean(`mean\\`attribute\\\\🙃`) + 'mean'", testDisplayMap
-      )).toEqual('mean(LOCAL_ATTR_ATTR_MEAN) + "mean"')
+      )).toEqual('mean(__CANONICAL_NAME__LOCAL_ATTR_ATTR_MEAN) + "mean"')
     })
   })
   describe("when attribute name is provided as string constant (e.g. lookup functions)", () => {
     it("is still converted correctly and names with special characters are NOT enclosed in backticks", () => {
       expect(displayToCanonical(
         'lookupByKey("Roller Coaster", "Park\\"", "Top\\\\Speed\'", Order) * 2', displayNameMapExample
-      )).toEqual('lookupByKey("DATA_ROLLER_COASTER", "ATTR_PARK", "ATTR_TOP_SPEED", LOCAL_ATTR_ATTR_ORDER) * 2')
+      )).toEqual(
+        'lookupByKey("__CANONICAL_NAME__DATA_ROLLER_COASTER", "__CANONICAL_NAME__ATTR_PARK", ' +
+        '"__CANONICAL_NAME__ATTR_TOP_SPEED", __CANONICAL_NAME__LOCAL_ATTR_ATTR_ORDER) * 2'
+      )
     })
   })
 })
@@ -143,63 +164,72 @@ describe("customizeDisplayFormula", () => {
 describe("reverseDisplayNameMap", () => {
   it("reverses the display name map", () => {
     expect(reverseDisplayNameMap(displayNameMapExample)).toEqual({
-      LOCAL_ATTR_ATTR_LIFE_SPAN: "LifeSpan",
-      LOCAL_ATTR_ATTR_ORDER: "Order",
-      LOCAL_ATTR_CASE_INDEX: "caseIndex",
-      GLOBAL_VALUE_GLOB_V1: "v1",
-      DATA_MAMMALS: "Mammals",
-      ATTR_LIFE_SPAN: "LifeSpan",
-      ATTR_ORDER: "Order",
-      DATA_ROLLER_COASTER: "Roller Coaster",
-      ATTR_PARK: "Park\"",
-      ATTR_TOP_SPEED: "Top\\Speed'",
+      __CANONICAL_NAME__LOCAL_ATTR_ATTR_LIFE_SPAN: "LifeSpan",
+      __CANONICAL_NAME__LOCAL_ATTR_ATTR_ORDER: "Order",
+      __CANONICAL_NAME__LOCAL_ATTR_CASE_INDEX: "caseIndex",
+      __CANONICAL_NAME__GLOBAL_VALUE_GLOB_V1: "v1",
+      __CANONICAL_NAME__DATA_MAMMALS: "Mammals",
+      __CANONICAL_NAME__ATTR_LIFE_SPAN: "LifeSpan",
+      __CANONICAL_NAME__ATTR_ORDER: "Order",
+      __CANONICAL_NAME__DATA_ROLLER_COASTER: "Roller Coaster",
+      __CANONICAL_NAME__ATTR_PARK: "Park\"",
+      __CANONICAL_NAME__ATTR_TOP_SPEED: "Top\\Speed'",
     })
   })
 })
 
 describe("canonicalToDisplay", () => {
   it("converts canonical formula to display formula maintaining whitespace characters", () => {
     expect(canonicalToDisplay(
-      "mean(LOCAL_ATTR_ATTR_LIFE_SPAN) + GLOBAL_VALUE_GLOB_V1",
+      "mean(__CANONICAL_NAME__LOCAL_ATTR_ATTR_LIFE_SPAN) +__CANONICAL_NAME__GLOBAL_VALUE_GLOB_V1",
       "mean (\nLifeSpan\n) + v1 ", reverseDisplayNameMap(displayNameMapExample)
     )).toEqual("mean (\nLifeSpan\n) + v1 ")
     expect(canonicalToDisplay(
-      "mean(LOCAL_ATTR_ATTR_LIFE_SPAN) + LOCAL_ATTR_ATTR_ORDER * GLOBAL_VALUE_GLOB_V1",
+      "mean(__CANONICAL_NAME__LOCAL_ATTR_ATTR_LIFE_SPAN) + __CANONICAL_NAME__LOCAL_ATTR_ATTR_ORDER" +
+      " * __CANONICAL_NAME__GLOBAL_VALUE_GLOB_V1",
       "mean (\nOldLifeSpan\n) + OldOrder * OldV1", reverseDisplayNameMap(displayNameMapExample)
     )).toEqual("mean (\nLifeSpan\n) + Order * v1")
   })
+  it("throws an error if canonical formula contains unresolved canonical names", () => {
+    expect(() => canonicalToDisplay(
+      "mean(__CANONICAL_NAME__LOCAL_ATTR_ATTR_REMOVED_ATTRIBUTE)",
+      "mean(RemovedAttribute)", reverseDisplayNameMap(displayNameMapExample)
+    )).toThrow("canonicalToDisplay: canonical name not found in canonicalNameMap")
+  })
+
   describe("when function name or constant is equal to attribute name", () => {
     const displayMap: DisplayNameMap = {
       localNames: {
-        NewMeanAttr: "LOCAL_ATTR_ATTR_MEAN",
+        NewMeanAttr: "__CANONICAL_NAME__LOCAL_ATTR_ATTR_MEAN",
       },
       dataSet: {}
     }
     it("still converts canonical formula to display formula correctly", () => {
       expect(canonicalToDisplay(
-        "mean(LOCAL_ATTR_ATTR_MEAN) + 'mean'",
+        "mean(__CANONICAL_NAME__LOCAL_ATTR_ATTR_MEAN) + 'mean'",
         "mean ( mean ) + 'mean'", reverseDisplayNameMap(displayMap)
       )).toEqual("mean ( NewMeanAttr ) + 'mean'")
     })
   })
   describe("when attribute name includes special characters", () => {
     const testDisplayMap: DisplayNameMap = {
       localNames: {
-        "new\\mean`attribute 🙃": "LOCAL_ATTR_ATTR_MEAN",
+        "new\\mean`attribute 🙃": "__CANONICAL_NAME__LOCAL_ATTR_ATTR_MEAN",
       },
       dataSet: {}
     }
     it("is enclosed in backticks and special characters are escaped", () => {
       expect(canonicalToDisplay(
-        "mean(LOCAL_ATTR_ATTR_MEAN) + 'mean'",
+        "mean(__CANONICAL_NAME__LOCAL_ATTR_ATTR_MEAN) + 'mean'",
         "mean ( mean ) + 'mean'", reverseDisplayNameMap(testDisplayMap)
       )).toEqual("mean ( `new\\\\mean\\`attribute 🙃` ) + 'mean'")
     })
   })
   describe("when attribute name is provided as string constant (e.g. lookup functions)", () => {
     it("is still converted correctly and names with special characters are NOT enclosed in backticks", () => {
       expect(canonicalToDisplay(
-        'lookupByKey("DATA_ROLLER_COASTER", "ATTR_PARK", "ATTR_TOP_SPEED", LOCAL_ATTR_ATTR_ORDER) * 2',
+        'lookupByKey("__CANONICAL_NAME__DATA_ROLLER_COASTER", "__CANONICAL_NAME__ATTR_PARK",' +
+        ' "__CANONICAL_NAME__ATTR_TOP_SPEED", __CANONICAL_NAME__LOCAL_ATTR_ATTR_ORDER) * 2',
         'lookupByKey("Old Roller Coaster", "Old Park", "Old Top Speed", OldOrder) * 2',
         reverseDisplayNameMap(displayNameMapExample)
       )).toEqual('lookupByKey("Roller Coaster", "Park\\"", "Top\\\\Speed\'", Order) * 2')