perf: bulk action import caching#41807
Conversation
WalkthroughThis PR introduces bulk action validation before import with datasource/plugin caching and public policy coordination. The service interface now declares ChangesAction Import Validation & Persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![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 (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (2)
426-427: 💤 Low valueInaccurate comment -
freshActionListis for insertions only.The comment states "inserted or updated in bulk" but
freshActionListcontains only new actions for insertion. Update the comment to reflect this.📝 Proposed fix
- // Add it to actions list that'll be inserted or updated in bulk + // Add it to actions list that'll be inserted in bulk freshActionList.add(updatedAction);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java` around lines 426 - 427, The comment above freshActionList incorrectly says "inserted or updated in bulk"; update it to accurately state that freshActionList contains only newly created actions to be inserted in bulk (not updates), e.g., clarify near the freshActionList.add(updatedAction) line that this list is for new/insert-only actions so future readers know updates are handled elsewhere.
386-388: 💤 Low valueRedundant method call - use already-extracted
editActionDTO.Line 361 already extracts
editActionDTO = actionToImport.getUnpublishedAction(). UseeditActionDTO.getDatasource()instead of callingactionToImport.getUnpublishedAction().getDatasource()again.♻️ Proposed refactor
- Datasource datasource = actionToImport - .getUnpublishedAction() - .getDatasource(); + Datasource datasource = editActionDTO.getDatasource();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java` around lines 386 - 388, The code redundantly calls actionToImport.getUnpublishedAction().getDatasource() even though editActionDTO = actionToImport.getUnpublishedAction() was already extracted; replace the redundant call by using editActionDTO.getDatasource() when assigning the Datasource (e.g., change the Datasource datasource assignment to use editActionDTO.getDatasource()), ensuring any references to the datasource in the surrounding logic use that variable and remove the extra getUnpublishedAction() invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java`:
- Around line 327-329: The constraint messages from
validator.validate(actionConfig) are being added to invalids but
editActionDTO.isValid is never updated; modify the blocks that call
validator.validate(actionConfig) (e.g., where actionConfig is checked) so that
when validator.validate(...) returns any constraint violations you both add
their messages to invalids and set editActionDTO.setIsValid(false) (or the
appropriate setter) to mark the action invalid; apply the same change to the
other occurrence around the method handling lines similar to 585-588 so
malformed configurations are not treated as valid.
- Around line 344-355: The code currently reuses the DTO-local datasource when
editActionDTO.getDatasource().getId() != null, causing incomplete persisted
state to be used later; change that branch to resolve the persisted datasource
first (e.g., call datasourceService.findById(datasource.getId()) or similar
lookup) and then continue: set
newAction.setWorkspaceId(persistedDatasource.getWorkspaceId()) and pass the
persistedDatasource into collectDatasourcePolicyUpdateForPublicAction(newAction,
persistedDatasource, validationContext) so workspace propagation, jsonPathKeys
extraction, and updateDatasourcePoliciesForPublicActions() operate on the true
persisted object rather than the DTO.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java`:
- Around line 356-357: Remove the unnecessary synchronized wrappers around the
lists: replace Collections.synchronizedList(new ArrayList<>()) for
freshActionList and existingNewActionList with plain ArrayList instances (e.g.,
new ArrayList<>()) in NewActionImportableServiceCEImpl where these lists are
populated inside the sequential flatMap and later used in the then(Mono.defer())
step; this removes needless locking overhead while preserving behavior because
the reactive pipeline processes elements sequentially by default.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java`:
- Around line 426-427: The comment above freshActionList incorrectly says
"inserted or updated in bulk"; update it to accurately state that
freshActionList contains only newly created actions to be inserted in bulk (not
updates), e.g., clarify near the freshActionList.add(updatedAction) line that
this list is for new/insert-only actions so future readers know updates are
handled elsewhere.
- Around line 386-388: The code redundantly calls
actionToImport.getUnpublishedAction().getDatasource() even though editActionDTO
= actionToImport.getUnpublishedAction() was already extracted; replace the
redundant call by using editActionDTO.getDatasource() when assigning the
Datasource (e.g., change the Datasource datasource assignment to use
editActionDTO.getDatasource()), ensuring any references to the datasource in the
surrounding logic use that variable and remove the extra getUnpublishedAction()
invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8e97d22-b489-4ab1-a620-e3f0408ebbd8
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/NewActionServiceTest.java
| if (actionConfig != null) { | ||
| validator.validate(actionConfig).stream().forEach(x -> invalids.add(x.getMessage())); | ||
| } |
There was a problem hiding this comment.
Mark the action invalid when ActionConfiguration bean validation fails.
These blocks add constraint messages to invalids, but editActionDTO.isValid stays true. That lets malformed configurations flow through both the import and regular validation paths as “valid” actions.
Possible fix
- if (actionConfig != null) {
- validator.validate(actionConfig).stream().forEach(x -> invalids.add(x.getMessage()));
- }
+ if (actionConfig != null) {
+ var violations = validator.validate(actionConfig);
+ if (!violations.isEmpty()) {
+ editActionDTO.setIsValid(false);
+ violations.forEach(x -> invalids.add(x.getMessage()));
+ }
+ }Also applies to: 585-588
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java`
around lines 327 - 329, The constraint messages from
validator.validate(actionConfig) are being added to invalids but
editActionDTO.isValid is never updated; modify the blocks that call
validator.validate(actionConfig) (e.g., where actionConfig is checked) so that
when validator.validate(...) returns any constraint violations you both add
their messages to invalids and set editActionDTO.setIsValid(false) (or the
appropriate setter) to mark the action invalid; apply the same change to the
other occurrence around the method handling lines similar to 585-588 so
malformed configurations are not treated as valid.
There was a problem hiding this comment.
@coderabbitai, I'm not sure that we do make the config invalid after validationContrstraints fail, I can take this up later, but can you check validateAction method in the same file, basically this is a copy of that
There was a problem hiding this comment.
@sondermanish — confirmed. The validateAction(NewAction, boolean isDryOps) method has the exact same pattern:
ActionConfiguration actionConfig = editActionDTO.getActionConfiguration();
if (actionConfig != null) {
validator.validate(actionConfig).stream().forEach(x -> invalids.add(x.getMessage()));
}No setIsValid(false) there either, so validateActionBeforeImport faithfully mirrors the pre-existing behavior. The gap exists in both paths.
Since you want to address this later, would you like me to open a follow-up GitHub issue to track the fix for both validateAction and validateActionBeforeImport?
🧠 Learnings used
Learnt from: subrata71
Repo: appsmithorg/appsmith PR: 41684
File: app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java:93-97
Timestamp: 2026-04-07T19:40:13.566Z
Learning: For SSRF mitigations involving SSH/Git URL handling, use a dual-validation strategy: validate the target hostname before initiating any network connection (e.g., via a URL/host validator like `validateGitSshUrl`), and then validate the resolved remote IP address after the TCP handshake/connection is established (e.g., in a session factory or connection accept path like `accept()`). Do not rely on a single pre-connection hostname check alone, since DNS rebinding can change the destination between validation and connect. If the underlying library (e.g., JGit) does not allow injecting a pre-resolved `InetAddress`, implement the post-connection IP validation as the second gate to complete the mitigation.
| Mono<Datasource> datasourceMono = Mono.just(editActionDTO.getDatasource()); | ||
| if (editActionDTO.getPluginType() != PluginType.JS) { | ||
| if (editActionDTO.getDatasource().getId() == null) { | ||
| datasourceMono = datasourceMono.flatMap(datasourceService::validateDatasource); | ||
| } else { | ||
| datasourceMono = datasourceMono | ||
| .map(datasource -> { | ||
| newAction.setWorkspaceId(datasource.getWorkspaceId()); | ||
| return datasource; | ||
| }) | ||
| .flatMap(datasource -> collectDatasourcePolicyUpdateForPublicAction( | ||
| newAction, datasource, validationContext)); |
There was a problem hiding this comment.
Resolve datasource IDs to the persisted datasource before reusing them.
When datasource.id is present, this branch keeps using the DTO-local datasource object. The later import flow uses that same object for workspace propagation, datasource-key extraction, and public-policy saves, so a reference-shaped datasource can end up producing incomplete jsonPathKeys or, worse, get partially written back in updateDatasourcePoliciesForPublicActions().
Possible fix
+ private Mono<Datasource> getDatasourceById(
+ String datasourceId, BulkActionValidationContext validationContext) {
+ return validationContext.datasourceById.computeIfAbsent(
+ datasourceId,
+ id -> datasourceService
+ .findById(id)
+ .switchIfEmpty(Mono.error(
+ new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.DATASOURCE, id)))
+ .cache());
+ }
+
...
- } else {
- datasourceMono = datasourceMono
- .map(datasource -> {
- newAction.setWorkspaceId(datasource.getWorkspaceId());
- return datasource;
- })
- .flatMap(datasource -> collectDatasourcePolicyUpdateForPublicAction(
- newAction, datasource, validationContext));
+ } else {
+ datasourceMono = getDatasourceById(
+ editActionDTO.getDatasource().getId(), validationContext)
+ .map(datasource -> {
+ newAction.setWorkspaceId(datasource.getWorkspaceId());
+ return datasource;
+ })
+ .flatMap(datasource -> collectDatasourcePolicyUpdateForPublicAction(
+ newAction, datasource, validationContext));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java`
around lines 344 - 355, The code currently reuses the DTO-local datasource when
editActionDTO.getDatasource().getId() != null, causing incomplete persisted
state to be used later; change that branch to resolve the persisted datasource
first (e.g., call datasourceService.findById(datasource.getId()) or similar
lookup) and then continue: set
newAction.setWorkspaceId(persistedDatasource.getWorkspaceId()) and pass the
persistedDatasource into collectDatasourcePolicyUpdateForPublicAction(newAction,
persistedDatasource, validationContext) so workspace propagation, jsonPathKeys
extraction, and updateDatasourcePoliciesForPublicActions() operate on the true
persisted object rather than the DTO.
There was a problem hiding this comment.
@coderabbitai if you take a look in newActionimportableserviceCEImpl you would find that we already sanitise the actions from DB stored while importing datasource
There was a problem hiding this comment.
🧩 Analysis chain
Let me check NewActionImportableServiceCEImpl to see the datasource sanitization during import.
🏁 Script executed:
#!/bin/bash
# Find and display the relevant datasource sanitization logic in NewActionImportableServiceCEImpl
fd "NewActionImportableServiceCEImpl.java" --exec cat {}
Length of output: 35921
@sondermanish You're right. Looking at updateDatasourceInAction() in NewActionImportableServiceCEImpl:
if (StringUtils.hasText(newAction.getUnpublishedAction().getDatasource().getId())) {
final String datasourceId = newAction.getUnpublishedAction().getDatasource().getId();
Map<String, Datasource> datasourceMap = mappedImportableResourcesDTO.getIdToDatasourceMap();
if (datasourceMap.containsKey(datasourceId)) {
datasource = datasourceMap.get(datasourceId);
}
datasource.setIsAutoGenerated(false);
newAction.setWorkspaceId(datasource.getWorkspaceId());
}
newAction.getUnpublishedAction().setDatasource(datasource);This resolves the full persisted datasource from idToDatasourceMap and sets it on the action before validateActionsBeforeImport is ever called. My concern about the DTO-local stub was incorrect — the datasource is already fully hydrated at that point. I'll retract the comment.
( ˘ ³˘)♥ 🐇
✏️ Learnings added
Learnt from: sondermanish
Repo: appsmithorg/appsmith PR: 41807
File: app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java:344-355
Timestamp: 2026-05-14T07:42:54.629Z
Learning: In `NewActionImportableServiceCEImpl` (appsmithorg/appsmith), `updateDatasourceInAction()` is called for each action before `validateActionsBeforeImport()`. It resolves the full persisted datasource from `mappedImportableResourcesDTO.getIdToDatasourceMap()` using the datasource ID, sets `workspaceId` on the action, and assigns the full datasource object onto the action DTO. As a result, by the time `validateActionsBeforeImport` / `validateActionBeforeImport` runs in `NewActionServiceCEImpl`, the action already carries the complete persisted datasource — not a reference-shaped stub. Concerns about incomplete `jsonPathKeys` or partial writes due to DTO-local datasource reuse in this flow are not valid.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: subrata71
Repo: appsmithorg/appsmith PR: 41684
File: app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java:93-97
Timestamp: 2026-04-07T19:40:13.566Z
Learning: For SSRF mitigations involving SSH/Git URL handling, use a dual-validation strategy: validate the target hostname before initiating any network connection (e.g., via a URL/host validator like `validateGitSshUrl`), and then validate the resolved remote IP address after the TCP handshake/connection is established (e.g., in a session factory or connection accept path like `accept()`). Do not rely on a single pre-connection hostname check alone, since DNS rebinding can change the destination between validation and connect. If the underlying library (e.g., JGit) does not allow injecting a pre-resolved `InetAddress`, implement the post-connection IP validation as the second gate to complete the mitigation.
| List<NewAction> freshActionList = Collections.synchronizedList(new ArrayList<>()); | ||
| List<NewAction> existingNewActionList = Collections.synchronizedList(new ArrayList<>()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for flatMap with concurrency parameter in similar import services
rg -nP --type=java -C3 'flatMap\([^,]+,\s*\d+\)' --glob '*Importable*.java'
# Check for any documentation or comments about concurrency in action import
rg -nP --type=java -C2 'concurrent|parallel|thread-safe' app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/Repository: appsmithorg/appsmith
Length of output: 1105
🏁 Script executed:
# Find the method containing lines 356-357 and check how freshActionList and existingNewActionList are used
rg -nP --type=java 'freshActionList|existingNewActionList' app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java -A2 -B2
# Also check if this method is called with concurrency concerns
rg -nP --type=java 'NewActionImportableServiceCEImpl' --glob '*.java' -l | head -5Repository: appsmithorg/appsmith
Length of output: 2229
🏁 Script executed:
# Read the method containing the synchronized lists (approximately lines 356-450)
sed -n '356,450p' app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.javaRepository: appsmithorg/appsmith
Length of output: 6666
Remove unnecessary synchronization; the pipeline executes sequentially.
The flatMap operator without an explicit concurrency parameter processes elements sequentially (concurrency=1 by default). Items are added to freshActionList and existingNewActionList only from within the sequential flatMap closure, then passed downstream via then(Mono.defer()) after all processing completes. Synchronized lists add locking overhead without preventing any actual concurrent access. In reactive streams, avoid synchronized collections as locks can block scheduler threads and degrade performance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java`
around lines 356 - 357, Remove the unnecessary synchronized wrappers around the
lists: replace Collections.synchronizedList(new ArrayList<>()) for
freshActionList and existingNewActionList with plain ArrayList instances (e.g.,
new ArrayList<>()) in NewActionImportableServiceCEImpl where these lists are
populated inside the sequential flatMap and later used in the then(Mono.defer())
step; this removes needless locking overhead while preserving behavior because
the reactive pipeline processes elements sequentially by default.
subrata71
left a comment
subrata71
left a comment
There was a problem hiding this comment.
The changes look directionally correct. If we have a way to measure the performance improvement then it could have been great.
| Mono<NewAction> validateAction(NewAction newAction, boolean isDryOps); | ||
|
|
||
| Mono<Tuple2<List<NewAction>, List<NewAction>>> validateActionsBeforeImport( | ||
| String artifactId, List<NewAction> freshActions, List<NewAction> existingActions); |
There was a problem hiding this comment.
Nit: What does freshActions mean here? Does it refer only to the new actions from the incoming list that will be inserted, or does it refer to the entire incoming actions list?
2 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
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.ImportExport, @tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/25814789299
Commit: 1fc6461
Cypress dashboard.
Tags:
@tag.ImportExport, @tag.GitSpec:
Wed, 13 May 2026 17:37:04 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Tests