Merge pull request #1371 from concord-consortium/187950451-mathjs-update

Update MathJS to v12.4.3 and handle the new approach to scope/partitioned map

v3/package.json

@@ -191,7 +191,7 @@
     "iframe-phone": "^1.3.1",
     "leaflet": "^1.9.4",
     "lodash": "^4.17.21",
-    "mathjs": "12.3.1",
+    "mathjs": "12.4.3",
     "mime": "^4.0.4",
     "mobx": "^6.13.0",
     "mobx-react-lite": "^4.0.7",

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

@@ -129,14 +129,6 @@ export class FormulaMathJsScope {
     throw new Error("It's not allowed to clear values in the formula scope.")
   }
 
-  // MathJS requests a separate subscope for every parsed and evaluated function call.
-  createSubScope () {
-    // In regular MathJS use case, a subscope is used to create a new scope for a function call. It needs to ensure
-    // that variables from subscope cannot overwrite variables from the parent scope. However, since we don't allow
-    // any variables to be set in the formula scope, we can reuse the same scope instance.
-    return this
-  }
-
   // --- Custom functions used by our formulas or formula manager --
 
   setExtraScope(extraScope?: Map<string, FValue>) {

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

@@ -41,7 +41,14 @@ export type EvaluateFunc = (...args: FValue[]) => FValue
 
 export type EvaluateFuncWithAggregateContextSupport = (...args: (FValueOrArray)[]) => FValueOrArray
 
-export type EvaluateRawFunc = (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => FValueOrArray
+// MathJS v12.3.2 introduced the concept of PartitionedMap, replacing the previous concepts of scopes and sub-scopes.
+// The parent scope is still available as the `a` property of the PartitionedMap.
+// For more information, see: https://github.com/josdejong/mathjs/pull/3150
+export type MathJSPartitionedMap = { a: CurrentScope, b: Map<string, any>}
+
+export type CurrentScope = MathJSPartitionedMap | FormulaMathJsScope
+
+export type EvaluateRawFunc = (args: MathNode[], mathjs: any, currentScope: CurrentScope) => FValueOrArray
 
 export type CaseList = ICase[] | "ALL_CASES"
 

v3/src/models/formula/functions/aggregate-functions.test.ts

@@ -151,25 +151,26 @@ describe("cachedAggregateFnFactory", () => {
       setCached: (key: string, val: any) => cache.set(key, val),
       getCached: (key: string) => cache.get(key)
     } as any as FormulaMathJsScope
+    const currentScope = { a: scope, b: new Map() }
 
-    expect(cachedFn([ parse("1"), parse("2") ], null, scope)).toEqual(123)
+    expect(cachedFn([ parse("1"), parse("2") ], null, currentScope)).toEqual(123)
     expect(fn).toHaveBeenCalledTimes(1)
 
-    expect(cachedFn([ parse("1"), parse("2") ], null, scope)).toEqual(123)
+    expect(cachedFn([ parse("1"), parse("2") ], null, currentScope)).toEqual(123)
     expect(fn).toHaveBeenCalledTimes(1) // Same arguments as in the previous call, cache should be used.
 
-    expect(cachedFn([ parse("1"), parse("2"), parse("3") ], null, scope)).toEqual(123)
+    expect(cachedFn([ parse("1"), parse("2"), parse("3") ], null, currentScope)).toEqual(123)
     expect(fn).toHaveBeenCalledTimes(2) // Different arguments, so cache should not be used.
 
-    expect(cachedFn([ parse("1"), parse("2"), parse("3") ], null, scope)).toEqual(123)
+    expect(cachedFn([ parse("1"), parse("2"), parse("3") ], null, currentScope)).toEqual(123)
     expect(fn).toHaveBeenCalledTimes(2) // Same arguments as in the previous call, cache should be used.
 
     // Update scope.getCaseAggregateGroupId
     ;(scope.getCaseAggregateGroupId as jest.Mock).mockImplementation(() => "newTestGroup")
-    expect(cachedFn([ parse("1"), parse("2"), parse("3") ], null, scope)).toEqual(123)
+    expect(cachedFn([ parse("1"), parse("2"), parse("3") ], null, currentScope)).toEqual(123)
     expect(fn).toHaveBeenCalledTimes(3) // New caseAggregateGroupId, so cache should not be used.
 
-    expect(cachedFn([ parse("1"), parse("2"), parse("3") ], null, scope)).toEqual(123)
+    expect(cachedFn([ parse("1"), parse("2"), parse("3") ], null, currentScope)).toEqual(123)
     expect(fn).toHaveBeenCalledTimes(3) // Same arguments and caseAggregateGroupId, cache should be used.
   })
 })

v3/src/models/formula/functions/aggregate-functions.ts

@@ -1,18 +1,18 @@
 import { MathNode, mad, max, mean, median, min, sum } from "mathjs"
-import { FormulaMathJsScope } from "../formula-mathjs-scope"
-import { FValue, FValueOrArray } from "../formula-types"
-import { UNDEF_RESULT, evaluateNode, isNumber, isValueTruthy } from "./function-utils"
+import { CurrentScope, FValue, FValueOrArray } from "../formula-types"
+import { UNDEF_RESULT, evaluateNode, getRootScope, isNumber, isValueTruthy } from "./function-utils"
 
 // Almost every aggregate function can be cached in the same way.
 export const cachedAggregateFnFactory =
-(fnName: string, fn: (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => FValueOrArray) => {
-  return (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => {
+(fnName: string, fn: (args: MathNode[], mathjs: any, currentScope: CurrentScope) => FValueOrArray) => {
+  return (args: MathNode[], mathjs: any, currentScope: CurrentScope) => {
+    const scope = getRootScope(currentScope)
     const cacheKey = `${fnName}(${args.toString()})-${scope.getCaseAggregateGroupId()}`
     const cachedValue = scope.getCached(cacheKey)
     if (cachedValue !== undefined) {
       return cachedValue
     }
-    const result = fn(args, mathjs, scope)
+    const result = fn(args, mathjs, currentScope)
     scope.setCached(cacheKey, result)
     return result
   }
@@ -21,7 +21,8 @@ export const cachedAggregateFnFactory =
 // Note that aggregate functions like mean, max, min, etc., all have exactly the same signature and implementation.
 // The only difference is the final math operation applies to the expression results.
 const aggregateFnWithFilterFactory = (fn: (values: number[]) => FValue) => {
-  return (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => {
+  return (args: MathNode[], mathjs: any, currentScope: CurrentScope) => {
+    const scope = getRootScope(currentScope)
     const [ expression, filter ] = args
     let expressionValues = evaluateNode(expression, scope)
     if (!Array.isArray(expressionValues)) {
@@ -96,7 +97,8 @@ export const aggregateFunctions = {
     // arguments, the default caching method would calculate incorrect cache key. Hence, caching is implemented directly
     // in the function body.
     cachedEvaluateFactory: undefined,
-    evaluateRaw: (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => {
+    evaluateRaw: (args: MathNode[], mathjs: any, currentScope: CurrentScope) => {
+      const scope = getRootScope(currentScope)
       const [expression, filter] = args
       if (!expression) {
         // Special case: count() without arguments returns number of children cases. Note that this cannot be cached

v3/src/models/formula/functions/function-utils.test.ts

@@ -1,5 +1,7 @@
-import { isValueTruthy, equal, evaluateNode } from "./function-utils"
+import { isValueTruthy, equal, evaluateNode, isPartitionedMap, isMap, getRootScope } from "./function-utils"
 import { parse } from "mathjs"
+import { CurrentScope } from "../formula-types"
+import { FormulaMathJsScope } from "../formula-mathjs-scope"
 
 describe("isValueTruthy", () => {
   it("should return true for truthy values", () => {
@@ -42,3 +44,39 @@ describe("evaluateNode", () => {
     expect(evaluateNode(node)).toBe(3)
   })
 })
+
+describe("isPartitionedMap", () => {
+  it("should return true for partitioned map", () => {
+    const map = { a: {}, b: new Map() }
+    expect(isPartitionedMap(map)).toBe(true)
+  })
+
+  it("should return false for non-partitioned map", () => {
+    const map = new Map()
+    expect(isPartitionedMap(map)).toBe(false)
+  })
+})
+
+describe("isMap", () => {
+  it("should return true for map", () => {
+    const map = new Map()
+    expect(isMap(map)).toBe(true)
+  })
+
+  it("should return false for non-map", () => {
+    const map = {}
+    expect(isMap(map)).toBe(false)
+  })
+})
+
+describe("getRootScope", () => {
+  it("should return root scope for partitioned map", () => {
+    const scope = { a: {}, b: new Map() }
+    expect(getRootScope(scope as any as CurrentScope)).toBe(scope.a)
+  })
+
+  it("should return scope for non-partitioned map", () => {
+    const scope = {} as any as FormulaMathJsScope
+    expect(getRootScope(scope)).toBe(scope)
+  })
+})

v3/src/models/formula/functions/function-utils.ts

@@ -1,6 +1,7 @@
 import { MathNode } from "mathjs"
 import { FormulaMathJsScope } from "../formula-mathjs-scope"
 import { isValueNonEmpty } from "../../../utilities/math-utils"
+import { CurrentScope, MathJSPartitionedMap } from "../formula-types"
 export { isNumber, isValueNonEmpty } from "../../../utilities/math-utils"
 
 export const UNDEF_RESULT = ""
@@ -33,3 +34,23 @@ equal = (a: any, b: any): boolean => {
 export const evaluateNode = (node: MathNode, scope?: FormulaMathJsScope) => {
   return node.compile().evaluate(scope)
 }
+
+// This function is used to get the root scope (an instance of our FormulaMathJSScope) within custom MathJS functions.
+// When the formula expression is executed, the initially passed scope can be wrapped in MathJS's PartitionedMap, which
+// is used to store temporary values. This function retrieves the root scope from the PartitionedMap.
+// This approach has been recommended by the MathJS maintainer. For more details, see:
+// https://github.com/josdejong/mathjs/pull/3150#issuecomment-2248101774
+export const getRootScope = (currentScope: CurrentScope): FormulaMathJsScope => {
+  return isPartitionedMap(currentScope) ? getRootScope(currentScope.a) : currentScope
+}
+
+export const isPartitionedMap = (map: any): map is MathJSPartitionedMap => {
+  // We could also check isMap(map.a), but that makes tests and mocking more difficult.
+  // The current check is probably specific enough to be safe.
+  return map && typeof map.a === "object" && isMap(map.b)
+}
+
+export const isMap = (map: any) => {
+  const requiredMapMethods = ['get', 'set', 'has', 'delete', 'entries']
+  return map && typeof map === 'object' && requiredMapMethods.every(method => typeof map[method] === 'function')
+}

v3/src/models/formula/functions/lookup-functions.ts

@@ -1,8 +1,7 @@
 import { ConstantNode, MathNode } from "mathjs"
-import { FormulaMathJsScope } from "../formula-mathjs-scope"
-import { DisplayNameMap, FValue, ILookupDependency } from "../formula-types"
+import { CurrentScope, DisplayNameMap, FValue, ILookupDependency } from "../formula-types"
 import { rmCanonicalPrefix } from "../utils/name-mapping-utils"
-import { UNDEF_RESULT, equal, evaluateNode } from "./function-utils"
+import { UNDEF_RESULT, equal, evaluateNode, getRootScope } from "./function-utils"
 import { isConstantStringNode } from "../utils/mathjs-utils"
 import { t } from "../../../utilities/translation/translate"
 import type { IDataSet } from "../../data/data-set"
@@ -32,7 +31,8 @@ export const lookupFunctions = {
         attrNameArg.value = displayNameMap.dataSet[dataSetName]?.attribute[attrName] || attrName
       }
     },
-    evaluateRaw: (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => {
+    evaluateRaw: (args: MathNode[], mathjs: any, currentScope: CurrentScope) => {
+      const scope = getRootScope(currentScope)
       const functionName = "lookupByIndex"
       const numOfReqArgs = lookupFunctions.lookupByIndex.numOfRequiredArguments
       if (args.length !== numOfReqArgs) {
@@ -91,7 +91,8 @@ export const lookupFunctions = {
         keyAttrNameArg.value = displayNameMap.dataSet[dataSetName]?.attribute[keyAttrName] || keyAttrName
       }
     },
-    evaluateRaw: (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => {
+    evaluateRaw: (args: MathNode[], mathjs: any, currentScope: CurrentScope) => {
+      const scope = getRootScope(currentScope)
       const functionName = "lookupByKey"
       const numOfReqArgs = lookupFunctions.lookupByKey.numOfRequiredArguments
       if (args.length !== numOfReqArgs) {

v3/src/models/formula/functions/math.test.ts

@@ -1,9 +1,8 @@
 import { MathNode, SymbolNode, parse } from "mathjs"
-import { FormulaMathJsScope } from "../formula-mathjs-scope"
 import {
   evaluateRawWithAggregateContext, evaluateRawWithDefaultArg, evaluateToEvaluateRaw, evaluateWithAggregateContextSupport
 } from "./math"
-import { FValue, FValueOrArray } from "../formula-types"
+import { FValue, FValueOrArray, MathJSPartitionedMap } from "../formula-types"
 
 describe("evaluateRawWithAggregateContext", () => {
   it("should call provided function within withAggregateContext", () => {
@@ -17,11 +16,12 @@ describe("evaluateRawWithAggregateContext", () => {
         scope.aggregateContext = false
       }
     }
+    const currentScope = { a: scope, b: new Map() } as any as MathJSPartitionedMap
     let result = false
     const mockFn = jest.fn(() => { result = scope.aggregateContext; return "" })
 
-    evaluateRawWithAggregateContext(mockFn)(args, mathjs, scope as any as FormulaMathJsScope)
-    expect(mockFn).toHaveBeenCalledWith(args, mathjs, scope)
+    evaluateRawWithAggregateContext(mockFn)(args, mathjs, currentScope)
+    expect(mockFn).toHaveBeenCalledWith(args, mathjs, currentScope)
     expect(result).toBeTruthy()
   })
 })
@@ -33,11 +33,12 @@ describe("evaluateRawWithDefaultArg", () => {
     const scope = {
       defaultArgumentNode: { name: "default" }
     }
+    const currentScope = { a: scope, b: new Map() } as any as MathJSPartitionedMap
     const mockFn = jest.fn((_args: MathNode[]) => (_args[0] as SymbolNode).name)
 
     const numOfReqArgs = 1
-    const res = evaluateRawWithDefaultArg(mockFn, numOfReqArgs)(args, mathjs, scope as any as FormulaMathJsScope)
-    expect(mockFn).toHaveBeenCalledWith([scope.defaultArgumentNode], mathjs, scope)
+    const res = evaluateRawWithDefaultArg(mockFn, numOfReqArgs)(args, mathjs, currentScope)
+    expect(mockFn).toHaveBeenCalledWith([scope.defaultArgumentNode], mathjs, currentScope)
     expect(res).toEqual("default")
   })
 
@@ -47,11 +48,12 @@ describe("evaluateRawWithDefaultArg", () => {
     const scope = {
       defaultArgumentNode: { name: "default" }
     }
+    const currentScope = { a: scope, b: new Map() } as any as MathJSPartitionedMap
     const mockFn = jest.fn((_args: MathNode[]) => (_args[0] as SymbolNode).name)
 
     const res =
-      evaluateRawWithDefaultArg(mockFn, 1)(args as any as MathNode[], mathjs, scope as any as FormulaMathJsScope)
-    expect(mockFn).toHaveBeenCalledWith(args, mathjs, scope)
+      evaluateRawWithDefaultArg(mockFn, 1)(args as any as MathNode[], mathjs, currentScope)
+    expect(mockFn).toHaveBeenCalledWith(args, mathjs, currentScope)
     expect(res).toEqual("provided")
   })
 })
@@ -61,9 +63,10 @@ describe("evaluateToEvaluateRaw", () => {
     const args = [ parse("1"), parse("2") ]
     const mathjs = {}
     const scope = {}
+    const currentScope = { a: scope, b: new Map() } as any as MathJSPartitionedMap
     const mockFn = jest.fn((a: FValueOrArray, b: FValueOrArray) => Number(a) + Number(b))
 
-    const res = evaluateToEvaluateRaw(mockFn)(args as any as MathNode[], mathjs, scope as any as FormulaMathJsScope)
+    const res = evaluateToEvaluateRaw(mockFn)(args as any as MathNode[], mathjs, currentScope)
     expect(mockFn).toHaveBeenCalledWith(1, 2)
     expect(res).toEqual(3)
   })

v3/src/models/formula/functions/math.ts

@@ -1,10 +1,9 @@
 import { create, all, MathNode } from 'mathjs'
-import { FormulaMathJsScope } from '../formula-mathjs-scope'
 import {
-  CODAPMathjsFunctionRegistry, EvaluateFunc, EvaluateFuncWithAggregateContextSupport, EvaluateRawFunc, FValue,
-  FValueOrArray
+  CODAPMathjsFunctionRegistry, CurrentScope, EvaluateFunc, EvaluateFuncWithAggregateContextSupport, EvaluateRawFunc,
+  FValue, FValueOrArray
 } from '../formula-types'
-import { evaluateNode } from './function-utils'
+import { evaluateNode, getRootScope } from './function-utils'
 import { arithmeticFunctions } from './arithmetic-functions'
 import { dateFunctions } from './date-functions'
 import { stringFunctions } from './string-functions'
@@ -18,23 +17,26 @@ export const math = create(all)
 
 // Each aggregate function needs to be evaluated with `withAggregateContext` method.
 export const evaluateRawWithAggregateContext = (fn: EvaluateRawFunc): EvaluateRawFunc => {
-  return (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => {
+  return (args: MathNode[], mathjs: any, currentScope: CurrentScope) => {
+    const scope = getRootScope(currentScope)
     // withAggregateContext returns result of the callback function
-    return scope.withAggregateContext(() => fn(args, mathjs, scope))
+    return scope.withAggregateContext(() => fn(args, mathjs, currentScope))
   }
 }
 
 export const evaluateRawWithDefaultArg = (fn: EvaluateRawFunc, numOfRequiredArguments: number): EvaluateRawFunc => {
-  return (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => {
+  return (args: MathNode[], mathjs: any, currentScope: CurrentScope) => {
+    const scope = getRootScope(currentScope)
     if (scope.defaultArgumentNode && args.length < numOfRequiredArguments) {
-      return fn([...args, scope.defaultArgumentNode], mathjs, scope)
+      return fn([...args, scope.defaultArgumentNode], mathjs, currentScope)
     }
-    return fn(args, mathjs, scope)
+    return fn(args, mathjs, currentScope)
   }
 }
 
 export const evaluateToEvaluateRaw = (fn: EvaluateFuncWithAggregateContextSupport): EvaluateRawFunc => {
-  return (args: MathNode[], mathjs: any, scope: FormulaMathJsScope) => {
+  return (args: MathNode[], mathjs: any, currentScope: CurrentScope) => {
+    const scope = getRootScope(currentScope)
     return fn(...(args.map(arg => evaluateNode(arg, scope))))
   }
 }