Merge pull request #911 from concord-consortium/186152786-fn-name-att…

…r-name

Fix handling of scenario when attribute name is the same as function name (e.g. "mean(mean)")

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

@@ -332,7 +332,7 @@ export class FormulaManager {
     })
   }
 
-  getDisplayNameMapForFormula(formulaId: string) {
+  getDisplayNameMapForFormula(formulaId: string, makeSymbolNamesSafe = true) {
     const { dataSet: localDataSet } = this.getFormulaContext(formulaId)
 
     const displayNameMap: DisplayNameMap = {
@@ -343,7 +343,7 @@ export class FormulaManager {
     const mapAttributeNames = (dataSet: IDataSet, prefix: string) => {
       const result: Record<string, string> = {}
       dataSet.attributes.forEach(attr => {
-        result[safeSymbolName(attr.name)] = `${prefix}${attr.id}`
+        result[makeSymbolNamesSafe ? safeSymbolName(attr.name) : attr.name] = `${prefix}${attr.id}`
       })
       return result
     }

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

@@ -1,6 +1,6 @@
 import { DisplayNameMap } from "./formula-types"
 import {
-  safeSymbolName, customizeFormula, reverseDisplayNameMap, canonicalToDisplay, makeNamesSafe
+  safeSymbolName, customizeFormula, reverseDisplayNameMap, canonicalToDisplay, makeNamesSafe, displayToCanonical
 } from "./formula-utils"
 
 const displayNameMapExample: DisplayNameMap = {
@@ -28,6 +28,47 @@ const displayNameMapExample: DisplayNameMap = {
   }
 }
 
+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")
+  })
+  describe("when function name or constant is equal to attribute name", () => {
+    const displayMap: DisplayNameMap = {
+      localNames: {
+        mean: "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"')
+    })
+  })
+  describe("when attribute name includes special characters", () => {
+    const testDisplayMap: DisplayNameMap = {
+      localNames: {
+        [safeSymbolName("mean attribute 🙃")]: "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"')
+    })
+  })
+  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')
+    })
+  })
+})
+
 describe("safeSymbolName", () => {
   it("converts strings that are not parsable by Mathjs to valid symbol names", () => {
     expect(safeSymbolName("Valid_Name_Should_Not_Be_Changed")).toEqual("Valid_Name_Should_Not_Be_Changed")

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

@@ -109,10 +109,10 @@ export const ifSelfReference = (dependency?: IFormulaDependency, formulaAttribut
 
 // Function replaces all the symbol names typed by user (display names) with the symbol canonical names that
 // can be resolved by formula context and do not rely on user-based display names.
-export const canonicalizeExpression = (displayExpression: string, displayNameMap: DisplayNameMap) => {
+export const displayToCanonical = (displayExpression: string, displayNameMap: DisplayNameMap) => {
   const formulaTree = parse(customizeFormula(makeNamesSafe(displayExpression)))
-  const visitNode = (node: MathNode) => {
-    if (isSymbolNode(node)) {
+  const visitNode = (node: MathNode, path: string, parent: MathNode) => {
+    if (isNonFunctionSymbolNode(node, parent)) {
       const canonicalName = generateCanonicalSymbolName(node.name, displayNameMap)
       if (canonicalName) {
         node.name = canonicalName
@@ -171,7 +171,7 @@ export const getFormulaDependencies = (formulaCanonical: string, formulaAttribut
     }
     const isDescendantOfAggregateFunc = !!node.isDescendantOfAggregateFunc
     const isSelfReferenceAllowed = !!node.isSelfReferenceAllowed
-    if (isSymbolNode(node)) {
+    if (isNonFunctionSymbolNode(node, parent)) {
       const dependency = parseCanonicalSymbolName(node.name)
       if (dependency?.type === "localAttribute" && isDescendantOfAggregateFunc) {
         dependency.aggregate = true

v3/src/models/data/formula.ts

@@ -1,7 +1,7 @@
 import { Instance, types } from "mobx-state-tree"
 import { parse } from "mathjs"
 import { typedId } from "../../utilities/js-utils"
-import { canonicalizeExpression, customizeFormula, isRandomFunctionPresent } from "./formula-utils"
+import { displayToCanonical, customizeFormula, isRandomFunctionPresent } from "./formula-utils"
 import { getFormulaManager } from "../tiles/tile-environment"
 
 export const Formula = types.model("Formula", {
@@ -20,7 +20,7 @@ export const Formula = types.model("Formula", {
       return ""
     }
     const displayNameMap = this.formulaManager.getDisplayNameMapForFormula(self.id)
-    return canonicalizeExpression(self.display, displayNameMap)
+    return displayToCanonical(self.display, displayNameMap)
   },
   get empty() {
     return self.display.length === 0