chore: add dsl version validation for app computation cache#40301
chore: add dsl version validation for app computation cache#40301
Conversation
WalkthroughThis change introduces stricter validation and explicit handling of a new Changes
Sequence Diagram(s)sequenceDiagram
participant S as Saga (evaluateTreeSaga)
participant Sel as Selector (getCurrentPageDSLVersion)
participant W as Worker (evalWorker)
S->>Sel: Select DSL version from state
Sel-->>S: Return dslVersion
S->>W: Send evaluation request with dslVersion in cacheProps
W-->>S: Return evaluation result
sequenceDiagram
participant E as DataTreeEvaluator
participant C as AppComputationCache
participant L as Logger
E->>C: fetchOrCompute(allKeys, cacheProps)
alt Cache fetch success
C-->>E: Return cached or computed allKeys
else Cache fetch error
C-->>L: Log error, throw exception
E->>E: Catch error, record CACHE_ERROR, compute allKeys directly
end
Possibly related PRs
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 18, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
…om:appsmithorg/appsmith into chore/app-computation-cache-error-handling
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/14514190262. |
|
Deploy-Preview-URL: https://ce-40301.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/14519646998. |
|
Deploy-Preview-URL: https://ce-40301.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/14529328623. |
|
Deploy-Preview-URL: https://ce-40301.dp.appsmith.com |
| cacheProps, | ||
| cacheName: EComputationCacheName.DEPENDENCY_MAP, | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
So the changes over here is if we are unable to retrieve, we would get a null in the dependencyMapCache and subseqeunetly reconstruct the dependencyMap. Is my understanding correct @diljit.
There was a problem hiding this comment.
If the cache is invalid or not present the appcomputation cache will send back null. The catch block is added so the can capture the errors thrown by the app computation cache.
| const fallbackResult = await computeFn(); | ||
|
|
||
| return fallbackResult; | ||
| loglevel.error(error); |
There was a problem hiding this comment.
If it can't retrieve it, it can recompute the result like what the code was earlier doing.
There was a problem hiding this comment.
I have moved the fallback logic to where it is consumed. We need to throw the error here so that we can catch it where it is used. This would help us then send the error to the main catch block in eval tree and then eventually send it back to the main thread.
app/client/src/workers/common/AppComputationCache/AppComputationCache.test.ts
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/14533303535. |
|
Deploy-Preview-URL: https://ce-40301.dp.appsmith.com |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
app/client/src/workers/common/AppComputationCache/types.ts (1)
12-15: ClarifydslVersionsemantics & promote immutability
ICacheProps.dslVersionisnumber | nullwhiletimestampisstring | undefined.
Usingnullfor missing butundefinedfor other optional fields introduces a
tri‑state (undefinedvsnullvs valid). Prefer one convention – e.g. make it
number | undefinedand treat “not supplied” uniformly.Consider marking all fields in
IValidatedCachePropsasreadonlyto prevent
accidental mutation after validation.These tweaks tighten typing without altering runtime behaviour.
Also applies to: 17-24
app/client/src/workers/common/DependencyMap/index.ts (2)
66-82: Bubble cache‑layer failures when dependency map is mandatoryThe new try‑catch ensures the app continues even when the cache layer throws, but
silently discarding the cache means we might repeatedly recompute the dependency
map, adding latency. At minimum, consider logging aterrorlevel (or re‑throw
whenprocess.env.NODE_ENV !== "production") so the issue surfaces quickly.Extracting the cache‑access pattern into a helper (e.g.
safeGetCachedResult)
would also remove the duplicated try‑catch here and inDataTreeEvaluator.
121-134: Symmetric error handling forcacheComputationResult
cacheComputationResultis wrapped in a try‑catch exactly like the read path—
great. You already push aCACHE_ERROR, but unlike the read branch you do not
log the original error. Adding alog.error(or Sentry breadcrumb) will help
diagnose write‑side failures that otherwise stay silent.app/client/src/workers/common/AppComputationCache/AppComputationCache.test.ts (1)
542-563: Test is in the wrong describe‑blockThe “should not cache result when dsl version is invalid” test lives inside the
fetchOrComputesuite but directly callscacheComputationResult. Moving it to
the neighbouringcacheComputationResultdescribekeeps the test hierarchy
intuitive.No functional change – purely organisational.
app/client/src/workers/common/AppComputationCache/index.ts (4)
61-64: Consolidate mode validation with the configuration source
isAppModeValidduplicates knowledge already stored inappModeConfig. Instead of maintaining two separate allow‑lists, reuse the config to remove duplication and the risk of them diverging.isAppModeValid(appMode: unknown) { - return appMode === APP_MODE.PUBLISHED || appMode === APP_MODE.EDIT; + return Object.values(this.appModeConfig) + .flat() + .includes(appMode as APP_MODE); }
69-74: Timestamp validation is overly strict and brittleHard‑coding an ISO‑8601 regex blocks perfectly valid date strings that include a timezone offset (e.g.
2024‑04‑18T10:35:00+05:30). Consider deferring to the built‑inDate.parse()for broader coverage and lower maintenance cost.-const isoStringRegex = /^(\d{4})-(\d{2})-(\d{2})T(\d{2}):(\d{2}):(\d{2})\.(\d{3})Z$/; -... -return isoStringRegex.test(timestamp); +return !Number.isNaN(Date.parse(timestamp));
76-78: Permit DSL version 0 or document the assumption
dslVersion > 0silently rejects a legitimate0.
If0is an impossible sentinel, add a clarifying comment. Otherwise use>= 0to avoid edge‑case cache misses.
312-315: Early‑exit before scanning keys whentimestampis missingInside the filter you short‑circuit on
!cacheProps.timestamp, but the loop still iterates over every key first. Bail out earlier to avoid unnecessary work on large caches:-// Get invalid cache keys -const invalidCacheKeys = cacheKeys.filter((key) => { - const keyParts = key.split(AppComputationCache.CACHE_KEY_DELIMITER); - ... - if (!cacheProps.timestamp) { - return false; - } +if (!cacheProps.timestamp) return; + +const invalidCacheKeys = cacheKeys.filter((key) => { + const keyParts = key.split(AppComputationCache.CACHE_KEY_DELIMITER); ... });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/client/src/ce/selectors/entitiesSelector.ts(1 hunks)app/client/src/sagas/EvalErrorHandler.ts(1 hunks)app/client/src/sagas/EvaluationsSaga.test.ts(7 hunks)app/client/src/sagas/EvaluationsSaga.ts(3 hunks)app/client/src/utils/DynamicBindingUtils.ts(1 hunks)app/client/src/workers/Evaluation/__tests__/evaluation.test.ts(1 hunks)app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts(1 hunks)app/client/src/workers/common/AppComputationCache/AppComputationCache.test.ts(18 hunks)app/client/src/workers/common/AppComputationCache/index.ts(13 hunks)app/client/src/workers/common/AppComputationCache/types.ts(1 hunks)app/client/src/workers/common/DataTreeEvaluator/dataTreeEvaluator.test.ts(3 hunks)app/client/src/workers/common/DataTreeEvaluator/index.ts(1 hunks)app/client/src/workers/common/DependencyMap/index.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
app/client/src/sagas/EvaluationsSaga.test.ts (1)
app/client/src/ce/selectors/entitiesSelector.ts (1)
getCurrentPageDSLVersion(1808-1810)
app/client/src/sagas/EvalErrorHandler.ts (2)
app/client/src/instrumentation/sendFaroErrors.ts (1)
captureException(5-19)app/client/src/utils/Analytics/sentry.ts (1)
captureException(19-21)
app/client/src/ce/selectors/entitiesSelector.ts (1)
app/client/src/ce/reducers/index.tsx (1)
AppState(98-191)
app/client/src/sagas/EvaluationsSaga.ts (1)
app/client/src/ce/selectors/entitiesSelector.ts (1)
getCurrentPageDSLVersion(1808-1810)
app/client/src/workers/common/DataTreeEvaluator/index.ts (2)
app/client/src/workers/common/AppComputationCache/index.ts (1)
appComputationCache(348-348)app/client/src/ce/workers/Evaluation/evaluationUtils.ts (1)
getAllPaths(545-568)
app/client/src/workers/common/AppComputationCache/index.ts (2)
app/client/src/utils/helpers.tsx (1)
isString(556-558)app/client/src/workers/common/AppComputationCache/types.ts (2)
ICacheProps(8-15)IValidatedCacheProps(17-24)
🪛 Biome (1.9.4)
app/client/src/workers/common/AppComputationCache/index.ts
[error] 11-11: Do not shadow the global "isFinite" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (16)
app/client/src/utils/DynamicBindingUtils.ts (1)
164-164: Adding new CACHE_ERROR type to support cache validation enhancementsThis addition of a new error type will be used for error handling in cache operations across various modules, allowing consistent handling of cache-related errors.
app/client/src/ce/selectors/entitiesSelector.ts (1)
1808-1810: Added selector for retrieving page DSL version for cache validationThis new selector extracts the DSL version from the canvas widget state, which will be used to ensure cache validity in the evaluation process. Good implementation of a simple and focused selector.
app/client/src/workers/Evaluation/__tests__/evaluation.test.ts (1)
591-591: Updated test to include dslVersion parameter in setupFirstTreeThe test has been properly updated to provide the dslVersion in the options object, aligning with the changes in cache validation requirements.
app/client/src/workers/common/DataTreeEvaluator/dataTreeEvaluator.test.ts (3)
293-293: Added dslVersion parameter to test setupThe test setup has been updated to include the dslVersion parameter, ensuring proper test coverage for the enhanced cache validation mechanism.
395-395: Added dslVersion parameter to test setupConsistent update to include the dslVersion parameter in another test block, maintaining proper test coverage.
459-459: Added dslVersion parameter to test setupAdded the dslVersion parameter to complete the necessary test updates for the array accessor dependency handling tests, ensuring all test cases correctly include the new required parameter.
app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts (1)
203-203: Add DSL version to test setupIncluding
dslVersionin the test context aligns with the new validation requirements for app computation cache.app/client/src/sagas/EvaluationsSaga.test.ts (4)
15-18: Import getCurrentPageDSLVersion selectorProper inclusion of the DSL version selector for testing the cache validation feature.
65-65: Mock DSL version selector for testCorrectly mocks the DSL version selector to return a fixed value for test predictability.
74-74: Add DSL version to cacheProps in expected worker requestVerifies that the evaluation worker receives the DSL version in its cache properties.
113-113: Include DSL version in remaining test casesEnsures consistent DSL version verification across all relevant test scenarios.
Also applies to: 122-122, 170-170, 179-179
app/client/src/sagas/EvalErrorHandler.ts (1)
330-334: Add dedicated error handler for cache errorsProperly handles cache errors with specific logging and error tracking. This is essential for diagnosing issues with the new DSL version cache validation.
app/client/src/sagas/EvaluationsSaga.ts (3)
80-80: Import getCurrentPageDSLVersion selectorAdds the necessary selector to retrieve the current page's DSL version.
273-273: Select current page DSL version from stateRetrieves the DSL version from Redux state for inclusion in cache validation.
284-284: Include DSL version in cache propertiesPasses the DSL version to the evaluation worker as part of cache validation properties.
app/client/src/workers/common/AppComputationCache/index.ts (1)
340-345: ExposeresetInstanceonly in test environmentsResetting the singleton at runtime outside of tests can break cache consistency. Guard the method behind
process.env.NODE_ENV === "test"or move it to a dedicated test helper to avoid accidental misuse.
| try { | ||
| this.allKeys = await appComputationCache.fetchOrCompute({ | ||
| cacheProps, | ||
| cacheName: EComputationCacheName.ALL_KEYS, | ||
| computeFn: () => getAllPaths(unEvalTreeWithStrigifiedJSFunctions), | ||
| }); | ||
| } catch (error) { | ||
| this.errors.push({ | ||
| type: EvalErrorTypes.CACHE_ERROR, | ||
| message: (error as Error).message, | ||
| stack: (error as Error).stack, | ||
| }); | ||
|
|
||
| this.allKeys = getAllPaths(unEvalTreeWithStrigifiedJSFunctions); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid running the costly getAllPaths twice on cache‑failure
Inside the catch block you recompute this.allKeys with a direct call to getAllPaths, even though fetchOrCompute will already have executed the very same computeFn once before propagating the error.
That means the traversal work is done twice when the cache layer is unhealthy.
A small refactor can keep the first computation result and still record the error:
-} catch (error) {
- this.errors.push({
- type: EvalErrorTypes.CACHE_ERROR,
- message: (error as Error).message,
- stack: (error as Error).stack,
- });
-
- this.allKeys = getAllPaths(unEvalTreeWithStrigifiedJSFunctions);
-}
+} catch (error: unknown) {
+ this.errors.push({
+ type: EvalErrorTypes.CACHE_ERROR,
+ message: (error as Error).message,
+ stack: (error as Error).stack,
+ });
+ /* `fetchOrCompute` already executed computeFn; reuse the fallback value if it
+ was returned, otherwise compute locally. */
+ this.allKeys =
+ (error as any)?.result ??
+ getAllPaths(unEvalTreeWithStrigifiedJSFunctions);
+}This keeps error logging intact while saving one full tree walk.
Committable suggestion skipped: line range outside the PR's diff.
| import { isFinite, isNumber, isString } from "lodash"; | ||
|
|
There was a problem hiding this comment.
Rename isFinite import to avoid shadowing the global API
Importing isFinite from lodash shadows the global isFinite function. This triggers lint warnings (noShadowRestrictedNames) and is easy to confuse when reading the code.
-import { isFinite, isNumber, isString } from "lodash";
+import { isFinite as _isFinite, isNumber, isString } from "lodash";…and update the two call‑sites (isDSLVersionValid) to use _isFinite.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { isFinite, isNumber, isString } from "lodash"; | |
| // app/client/src/workers/common/AppComputationCache/index.ts | |
| -import { isFinite, isNumber, isString } from "lodash"; | |
| +import { isFinite as _isFinite, isNumber, isString } from "lodash"; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Do not shadow the global "isFinite" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
| /** | ||
| * Checks if the cached value is valid | ||
| * @returns - A boolean indicating whether the cached value is valid | ||
| */ | ||
| isCacheValid<T>( | ||
| cachedValue: ICachedData<T> | null, | ||
| cacheProps: IValidatedCacheProps, | ||
| ): cachedValue is ICachedData<T> { | ||
| if (isNull(cachedValue)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!cachedValue.dslVersion) { | ||
| return false; | ||
| } | ||
|
|
||
| return cachedValue.dslVersion === cacheProps.dslVersion; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
isCacheValid ignores other invalidating props
The method only checks dslVersion. If any of the validated properties (appId, pageId, timestamp, etc.) ever change without altering the key (e.g. a future refactor), stale data could leak through. At minimum compare cachedValue.dslVersion and guard against a null/undefined mismatch explicitly:
-return cachedValue.dslVersion === cacheProps.dslVersion;
+return (
+ cachedValue.dslVersion !== undefined &&
+ cachedValue.dslVersion === cacheProps.dslVersion
+);Long‑term, consider encoding all props that affect validity into the key itself to remove this class of errors entirely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Checks if the cached value is valid | |
| * @returns - A boolean indicating whether the cached value is valid | |
| */ | |
| isCacheValid<T>( | |
| cachedValue: ICachedData<T> | null, | |
| cacheProps: IValidatedCacheProps, | |
| ): cachedValue is ICachedData<T> { | |
| if (isNull(cachedValue)) { | |
| return false; | |
| } | |
| if (!cachedValue.dslVersion) { | |
| return false; | |
| } | |
| return cachedValue.dslVersion === cacheProps.dslVersion; | |
| } | |
| /** | |
| * Checks if the cached value is valid | |
| * @returns - A boolean indicating whether the cached value is valid | |
| */ | |
| isCacheValid<T>( | |
| cachedValue: ICachedData<T> | null, | |
| cacheProps: IValidatedCacheProps, | |
| ): cachedValue is ICachedData<T> { | |
| if (isNull(cachedValue)) { | |
| return false; | |
| } | |
| if (!cachedValue.dslVersion) { | |
| return false; | |
| } | |
| return ( | |
| cachedValue.dslVersion !== undefined && | |
| cachedValue.dslVersion === cacheProps.dslVersion | |
| ); | |
| } |
2 participants
Description
This PR adds dsl version to the app computation cache. If there is a mismatch in dsl version the cache is updated with the new value.
Change also includes error handling and reporting for cases when there are exceptions while fetching the cache.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14533292018
Commit: fadb0c5
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 18 Apr 2025 11:07:11 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor