chore: simplify datasource import logic with flipped conditionals #41507
Conversation
WalkthroughRefactored datasource import workflow from per-entity streaming to bulk operations per workspace. Introduced DatasourceStorageService dependency replacing SequenceService. Added bulk save methods for datasources and storages. Centralized datasource lookups via in-memory maps instead of direct value-stream retrieval. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java:
- Around line 450-464: The generateUniqueName method currently appends suffixes
like " #1", " #2" which mismatches SequenceService; change it to start suffixing
at 2 and append with a space only (e.g., " 2", " 3") and remove the '#' so the
loop builds candidateName as baseName + " " + suffix with suffix starting at 2
and incrementing until a unique name is found; update references to
generateUniqueName accordingly if any tests expect the SequenceService naming
convention.
In
@app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java:
- Around line 30-31: datasourceNameToIdMap and idToDatasourceMap are declared as
ConcurrentHashMap but are populated sequentially inside the reactive .map()
chain in DatasourceImportableServiceCEImpl (no parallel scheduler), while
pluginMap remains a plain HashMap; either remove unnecessary concurrency or
document why it’s needed. Change datasourceNameToIdMap and idToDatasourceMap to
HashMap if there is no concurrent access, or add a short comment above their
declarations explaining the specific concurrent-access scenario that requires
ConcurrentHashMap; also make pluginMap’s type consistent with the chosen
decision.
🧹 Nitpick comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/importable/CustomJSLibImportableServiceCEImpl.java (1)
74-77: ConsiderdoOnErrorfor simple logging.Since you're just logging and rethrowing,
doOnErroris more idiomatic for side-effect-only error handling:♻️ Suggested simplification
.elapsed() .doOnNext(objects -> log.debug("time to import custom JSLibs: {}", objects.getT1())) - .then() - .onErrorResume(e -> { - log.error("Error importing custom JSLibs", e); - return Mono.error(e); - }); + .then() + .doOnError(e -> log.error("Error importing custom JSLibs", e));app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (4)
92-95: Consider memory impact for large workspaces.Collecting all workspace datasources into a List upfront could be memory-intensive for workspaces with a very large number of datasources. However, this is likely acceptable for typical use cases and enables the bulk operation pattern.
253-262: Clarify the datasource reuse logic.This code reuses an existing datasource (matched by name + pluginId) but adds it to
createdDatasources, which later gets bulk-saved. While this works (the save operation is safe and ensures mappings are updated), the logic is not immediately clear. Consider adding a comment explaining why an existing datasource is treated as "created" or refactoring to make the intent more explicit.
352-355: Document why datasourceConfiguration is nullified.Setting
datasourceConfigurationandpluginIdtonullappears intentional (likely to preserve user-configured database settings during git sync), but this critical decision lacks explanation. Adding a comment would clarify the reasoning for future maintainers.
368-373: Name collision handling is subtle but correct.The name update logic relies on
findExistingDatasourceto ensure the target name is either free or belongs to the same datasource. While this is correct, the safety mechanism is not immediately obvious here. Consider adding a brief comment referencing the upstream validation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/base/CustomJSLibServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/base/CustomJSLibServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/importable/CustomJSLibImportableServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImplTest.java
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 40462
File: app/client/src/instrumentation/index.ts:0-0
Timestamp: 2025-04-29T09:12:14.552Z
Learning: Only comment on files that are directly related to the PR's objectives, even if other files appear in the diff. For PR #40462, the focus is on the import override feature for artifacts, not on instrumentation or telemetry files.
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
📚 Learning: 2024-10-08T15:32:34.115Z
Learnt from: nidhi-nair
Repo: appsmithorg/appsmith PR: 29372
File: app/server/appsmith-server/src/main/java/com/appsmith/server/applications/jslibs/ApplicationJsLibServiceCEImpl.java:51-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `updateJsLibsInContext` method in the `ApplicationJsLibServiceCEImpl` class was mistakenly referred to as a new method in a previous comment. It is important to accurately assess the context of changes in a pull request to avoid misrepresenting the nature of the code modifications.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/base/CustomJSLibServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/importable/CustomJSLibImportableServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/base/CustomJSLibServiceCE.java
📚 Learning: 2024-10-08T15:32:39.374Z
Learnt from: sharat87
Repo: appsmithorg/appsmith PR: 34471
File: app/client/src/ce/api/JSActionAPI.tsx:143-161
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The `updateJSCollectionActionRefactor` method in `JSActionAPI.tsx` includes a check to ensure `action.datasource` is not null before manipulating its properties.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java
📚 Learning: 2024-10-08T15:32:34.114Z
Learnt from: sondermanish
Repo: appsmithorg/appsmith PR: 36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-10-08T15:32:34.114Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/importable/CustomJSLibImportableServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
📚 Learning: 2025-02-18T15:18:20.927Z
Learnt from: sharat87
Repo: appsmithorg/appsmith PR: 39337
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:154-160
Timestamp: 2025-02-18T15:18:20.927Z
Learning: Specific JSON parsing error handling using try-catch blocks isn't needed in the ImportServiceCEImpl's extractArtifactExchangeJson method as Mono.fromCallable provides adequate error handling.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/importable/CustomJSLibImportableServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
📚 Learning: 2025-03-06T12:23:01.075Z
Learnt from: abhvsn
Repo: appsmithorg/appsmith PR: 39594
File: app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.java:179-268
Timestamp: 2025-03-06T12:23:01.075Z
Learning: When using Reactor's `flatMap` operator with shared mutable state like ArrayList, synchronization is necessary because `flatMap` may process items concurrently, and operations like ArrayList.add() are not atomic across multiple threads.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.java
📚 Learning: 2025-01-30T10:47:35.449Z
Learnt from: ApekshaBhosale
Repo: appsmithorg/appsmith PR: 38925
File: app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java:0-0
Timestamp: 2025-01-30T10:47:35.449Z
Learning: In reactive programming with Spring WebFlux, avoid using `block()` as it can lead to thread pool exhaustion. Instead, maintain the reactive chain using operators like `flatMap()` and handle empty cases with `defaultIfEmpty()`. When adding tags or metrics, ensure they are applied within the reactive chain.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (14)
app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/base/CustomJSLibServiceCEImpl.java (1)
177-183: LGTM!Clean implementation with appropriate null/empty guard. Delegating to
repository.saveAllis the standard approach for bulk persistence.app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/base/CustomJSLibServiceCE.java (1)
44-51: LGTM!Well-documented interface method. The Javadoc clearly describes the purpose and usage context.
app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/importable/CustomJSLibImportableServiceCEImpl.java (1)
50-70: LGTM!The dry-ops pattern with a local map for collecting libs to save is clean. The
bulkSavemethod already handles null/empty input gracefully (returnsFlux.empty()), so the flow is safe even whenjsLibsDryOpsMap.get(DBOpsType.SAVE)returns null.app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceImpl.java (1)
14-19: LGTM! Dependency swap looks correct.The constructor has been cleanly updated to inject
DatasourceStorageServiceinstead ofSequenceService, aligning with the bulk import refactoring.app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.java (1)
23-23: LGTM! Standard bulk operation API.The
saveAllmethod addition follows the expected reactive pattern for batch persistence.app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java (1)
92-99: LGTM! Clean bulk save implementation.The implementation correctly handles the empty case and delegates to the repository layer. The reactive chain is properly maintained.
app/server/appsmith-server/src/test/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImplTest.java (1)
37-46: LGTM! Test dependency updated correctly.The test properly mocks the new
DatasourceStorageServicedependency and passes it to the constructor, maintaining test coverage for the refactored implementation.app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (5)
52-59: LGTM! Constructor updated for bulk operations.The constructor now injects
DatasourceStorageServiceinstead ofSequenceService, enabling the bulk import workflow. The constantsUPDATEandCREATEadd clarity to operation categorization.
121-331: Complex but well-structured import logic.The
importDatasourcesmethod is quite long (210 lines) but the inline documentation and logical flow (validate → categorize → process → bulk save) make it reasonably clear. The helper methods (processUpdateDatasource,processCreateDatasource, etc.) effectively decompose the complexity.
469-488: LGTM! Clean bulk save implementations.Both
performBulkSaveandperformBulkSaveStoragesproperly handle empty inputs, include helpful logging, and correctly delegate to their respective service layers.
498-526: LGTM! Robust collision detection.The
findExistingDatasourcemethod correctly handles the critical edge case where a gitSyncId match exists but the target name is already taken by a different datasource. Returningnullin this scenario prevents name collisions and ensures the datasource is processed via the CREATE path instead.
534-575: LGTM! Authentication rehydration logic is sound.The
updateAuthenticationDTOmethod correctly handles all authentication types with appropriate null checks. The OAuth2 handling properly reconstructs the authentication response, and the bearer token logic safely handles null cases.app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java (1)
22-22: LGTM - Required import for concurrent map fields.The import is necessary for the ConcurrentHashMap usage in the fields below.
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (1)
613-621: No issues to address. The code is null-safe. TheidToDatasourceMapis initialized at the field level asnew ConcurrentHashMap<>(), and Lombok's@Dataannotation generates a simple getter that returns the non-null field.ConcurrentHashMapdoes not allow null values—any attempt to insert null raisesNullPointerException. The caller already null-checks thedatasourceparameter before invoking this method (line 357), so the method safely handles both cases: map lookup hit or fallback to the parameter.Likely an incorrect or invalid review comment.
| private String generateUniqueName(String baseName, Set<String> takenNames) { | ||
| if (!takenNames.contains(baseName)) { | ||
| return baseName; | ||
| } | ||
|
|
||
| // Generate suffix: " #1", " #2", etc. (matching SequenceService convention) | ||
| int suffix = 1; | ||
| String candidateName; | ||
| do { | ||
| candidateName = baseName + " #" + suffix; | ||
| suffix++; | ||
| } while (takenNames.contains(candidateName)); | ||
|
|
||
| return candidateName; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the SequenceService class
fd SequenceService --type fRepository: appsmithorg/appsmith
Length of output: 445
🏁 Script executed:
# Search for any references to unique suffix or duplicate name handling
rg "getUniqueSuffixForDuplicateNameEntity|generateUniqueSuffix|SequenceService" --type java -A 3 -B 3Repository: appsmithorg/appsmith
Length of output: 12254
🏁 Script executed:
# Also search for the format pattern " #" to see how it's used elsewhere
rg -E '"[^"]*#[^"]*"' --type java | grep -i name | head -20Repository: appsmithorg/appsmith
Length of output: 460
🏁 Script executed:
cat app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/SequenceServiceCEImpl.javaRepository: appsmithorg/appsmith
Length of output: 1511
🏁 Script executed:
cat -n app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java | sed -n '445,470p'Repository: appsmithorg/appsmith
Length of output: 1107
The suffix format does not match SequenceService convention.
The implementation generates " #1", " #2" suffixes, but SequenceService actually produces " 2", " 3" (without the "#" and starting from 2). This mismatch will cause inconsistency with existing naming patterns and should be corrected to match SequenceService's actual behavior.
🤖 Prompt for AI Agents
In
@app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
around lines 450 - 464, The generateUniqueName method currently appends suffixes
like " #1", " #2" which mismatches SequenceService; change it to start suffixing
at 2 and append with a space only (e.g., " 2", " 3") and remove the '#' so the
loop builds candidateName as baseName + " " + suffix with suffix starting at 2
and incrementing until a unique name is found; update references to
generateUniqueName accordingly if any tests expect the SequenceService naming
convention.
| Map<String, String> datasourceNameToIdMap = new ConcurrentHashMap<>(); | ||
| Map<String, Datasource> idToDatasourceMap = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and examine the DTO file
find . -path "*MappedImportableResourcesCE_DTO.java" -type fRepository: appsmithorg/appsmith
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Get the content of the DTO file to understand context
cat "app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java"Repository: appsmithorg/appsmith
Length of output: 3691
🏁 Script executed:
#!/bin/bash
# Search for concurrent access patterns to these maps
echo "=== Searching for parallel/concurrent datasource map operations ==="
rg -n -C5 --type=java 'idToDatasourceMap\.(put|putAll|compute|get)' app/server/
echo -e "\n=== Searching for datasourceNameToIdMap operations ==="
rg -n -C5 --type=java 'datasourceNameToIdMap\.(put|putAll|compute)' app/server/
echo -e "\n=== Checking for parallel streams with these maps ==="
rg -n -C3 --type=java '(flatMap|parallel|Schedulers|subscribeOn).*datasource' app/server/Repository: appsmithorg/appsmith
Length of output: 50377
🏁 Script executed:
#!/bin/bash
# Search for where MappedImportableResourcesCE_DTO is instantiated and used
echo "=== Finding MappedImportableResourcesCE_DTO usage ==="
rg -n 'MappedImportableResourcesCE_DTO\|new MappedImportableResourcesCE' --type=java app/server/ | head -20
echo -e "\n=== Searching for import flow operations ==="
rg -n -C5 'idToDatasourceMap\|datasourceNameToIdMap' --type=java app/server/ | head -50Repository: appsmithorg/appsmith
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Search for DatasourceImportableService to understand the import flow context
echo "=== Examining DatasourceImportableServiceCEImpl.java around map operations ==="
sed -n '290,320p' "app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java"
echo -e "\n=== Check if maps are used in parallel streams or with subscribeOn ==="
rg -n 'flatMapMany|parallel\(' --type=java app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/ -A 5 -B 5
echo -e "\n=== Search for MappedImportableResourcesCE_DTO instantiation and usage ==="
rg -n 'new MappedImportableResourcesCE_DTO\|MappedImportableResourcesCE_DTO ' --type=java app/server/appsmith-server/src/main/java/ | head -30Repository: appsmithorg/appsmith
Length of output: 6657
🏁 Script executed:
#!/bin/bash
# Check if the maps are accessed from multiple threads/schedulers
echo "=== Check context of map writes in import flow ==="
sed -n '290,330p' "app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java"
echo -e "\n=== Look for parallel operations on the Flux/Mono chain ==="
rg -n 'performBulkSave\|\.map\|\.flatMap\|parallel' --type=java app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java -A 2 -B 2 | head -50Repository: appsmithorg/appsmith
Length of output: 2969
🏁 Script executed:
#!/bin/bash
# Search for where the DTO maps are actually read/written concurrently
echo "=== All lines with datasourceNameToIdMap and idToDatasourceMap in entire codebase ==="
rg -n 'datasourceNameToIdMap|idToDatasourceMap' --type=java app/server/appsmith-server/src/main/java/ -B 3 -A 3Repository: appsmithorg/appsmith
Length of output: 11571
No evidence of concurrent map access. Reconsider ConcurrentHashMap usage or add documentation.
The datasource maps are populated sequentially within a .map() operator in the reactive chain (line 302 of DatasourceImportableServiceCEImpl.java). flatMapMany() and collectList() are sequential operations; there's no parallel() or thread pool scheduler like subscribeOn(Schedulers.parallel()) that would introduce concurrent writes.
Two remaining concerns:
-
Inconsistent map types:
pluginMap(line 29) remainsHashMapdespite similar usage patterns in the import flow. If concurrency isn't required, these should all beHashMap. -
Missing documentation: Unlike the synchronized lists (lines 67-72), there's no comment explaining why these two maps require
ConcurrentHashMap. If concurrent access is legitimately needed, document it; otherwise, revert toHashMapfor better performance.
🤖 Prompt for AI Agents
In
@app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java
around lines 30 - 31, datasourceNameToIdMap and idToDatasourceMap are declared
as ConcurrentHashMap but are populated sequentially inside the reactive .map()
chain in DatasourceImportableServiceCEImpl (no parallel scheduler), while
pluginMap remains a plain HashMap; either remove unnecessary concurrency or
document why it’s needed. Change datasourceNameToIdMap and idToDatasourceMap to
HashMap if there is no concurrent access, or add a short comment above their
declarations explaining the specific concurrent-access scenario that requires
ConcurrentHashMap; also make pluginMap’s type consistent with the chosen
decision.
There was a problem hiding this comment.
Can you please validate this call out?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
🧩 Analysis chain
@subrata71, let me thoroughly validate this concern by examining the concurrent access patterns in the codebase.
🏁 Script executed:
#!/bin/bash
# Check for any parallel schedulers or concurrent access in the import flow
echo "=== Checking for subscribeOn/publishOn schedulers in import services ==="
rg -n 'subscribeOn|publishOn|parallel\(\)' --type=java app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/ -C 3
echo -e "\n=== Checking where MappedImportableResourcesDTO is passed between services ==="
rg -n 'MappedImportableResourcesDTO|mappedImportableResourcesDTO' --type=java app/server/appsmith-server/src/main/java/com/appsmith/server/ -A 2 -B 2 | grep -E '(flatMap|zip|merge|concat)' | head -30
echo -e "\n=== Check if maps are accessed in NewActionImportableServiceCEImpl (which was also modified) ==="
sed -n '1,100p' app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java | grep -E '(idToDatasourceMap|datasourceNameToIdMap)' -A 3 -B 3
Length of output: 4195
🏁 Script executed:
#!/bin/bash
# Check for any concurrent writes or reads to these specific maps
echo "=== Finding all write operations to idToDatasourceMap ==="
rg -n '\.put\(.*idToDatasourceMap|idToDatasourceMap\.put' --type=java app/server/appsmith-server/src/main/java/ -B 5 -A 2
echo -e "\n=== Finding all write operations to datasourceNameToIdMap ==="
rg -n '\.put\(.*datasourceNameToIdMap|datasourceNameToIdMap\.put' --type=java app/server/appsmith-server/src/main/java/ -B 5 -A 2
Length of output: 3418
🏁 Script executed:
#!/bin/bash
# Check if the DTO is used in any parallel/concurrent context elsewhere
echo "=== Check for zip/merge operations that might access DTO concurrently ==="
rg -n -C 5 'Flux\.zip|Mono\.zip|Flux\.merge|parallel' --type=java app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
echo -e "\n=== Check the broader import orchestration that calls datasource import ==="
rg -n 'importDatasources\(' --type=java app/server/appsmith-server/src/main/java/ -B 3 -A 3 | head -40
Length of output: 2566
@subrata71, I've validated the call-out and it appears to be correct. Here's what I found:
Evidence Supporting the Call-out
-
Sequential write operations: Both maps are populated in a single location (lines 303, 308) within a
.map()operator of a sequential reactive chain:performBulkSave(allDatasourcesToBeSaved).map(datasource -> { idToDatasourceMap.put(datasource.getId(), datasource); datasourceNameToIdMap.put(originalName, datasource.getId()); }) -
No concurrent schedulers: No
subscribeOn(),publishOn(), orparallel()operations were found in the datasource import flow that would introduce multi-threaded access. -
Linear flow: The import orchestration is purely sequential:
workspaceDatasourcesMono.flatMap()→importDatasources()→performBulkSave()→.map()(populate maps)
-
Read access is sequential:
idToDatasourceMapis read inNewActionImportableServiceCEImpl, but only after the datasource import completes, as part of the sequential import chain.
Issues Identified
- Unnecessary concurrency: No evidence of concurrent access to these maps
- Inconsistency:
pluginMap(line 29) remainsHashMapdespite similar usage patterns - Missing documentation: Unlike the synchronized lists (lines 67-72), there's no comment explaining why
ConcurrentHashMapis needed
Questions
- Is there a specific concurrent access scenario I'm missing (e.g., future bulk operations mentioned in the PR)?
- Should we revert to
HashMapfor consistency and performance, or is there defensive programming for upcoming changes?
If there's no concrete concurrent access requirement, I'd recommend reverting to HashMap for all three maps (pluginMap, datasourceNameToIdMap, idToDatasourceMap) for consistency.
🧬 Code Graph Analysis Results
<no_relevant_code_snippets>
</no_relevant_code_snippets>
Filename: app/client/src/entities/Datasource/index.ts
Line range 154-162:
export interface Datasource extends BaseDatasource {
id: string;
// key in the map representation of environment id of type string
datasourceStorages: Record<string, DatasourceStorage>;
success?: boolean;
isMock?: boolean;
invalids?: string[];
messages?: string[];
}
Filename: app/client/src/entities/Datasource/index.ts
Line range 164-172:
export interface DatasourceStorage {
datasourceId: string;
environmentId: string;
datasourceConfiguration: DatasourceConfiguration;
isValid: boolean;
structure?: DatasourceStructure;
isConfigured?: boolean;
toastMessage?: string;
}
🧠 Learnings used
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 40462
File: app/client/src/instrumentation/index.ts:0-0
Timestamp: 2025-04-29T09:12:14.552Z
Learning: Only comment on files that are directly related to the PR's objectives, even if other files appear in the diff. For PR #40462, the focus is on the import override feature for artifacts, not on instrumentation or telemetry files.
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
2 participants
Description
This PR refactors the datasource import logic in
DatasourceImportableServiceCEImplfor better readability and prepares for future bulk operations.Fixes https://linear.app/appsmith/issue/APP-14812/fix-datasource-import-flow-for-published-entities-dry-operation-and
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Changes
importDatasourcesfor better readability (handle creation case first, then update)findExistingDatasourcehelper method for cleaner gitSyncId matching logicdatasourcesToCreateanddatasourcesToUpdatefor future bulk operationsexistingDatasourceFluxtoallWorkspaceDatasourcesFromDBFluxfor clarityimportDatasourcesV2method as draft for future bulk operationsTesting
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/20855454082
Commit: d2da8fa
Cypress dashboard.
Tags: @tag.Git
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Fri, 09 Jan 2026 14:47:11 UTC