chore: add postSaveHook #39306
chore: add postSaveHook #39306
Conversation
WalkthroughThis pull request introduces several enhancements centered on datasource management. A new transient field, Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DSStorageService as DatasourceStorageServiceCEImpl
participant Repo as DatasourceRepository
participant PluginSvc as PluginService
participant ExecutorHelper as PluginExecutorHelper
participant Executor as PluginExecutor
DSStorageService->>Repo: Save DatasourceStorage
DSStorageService->>DSStorageService: executePostSaveActions(datasourceStorage)
DSStorageService->>PluginSvc: Retrieve Plugin
DSStorageService->>ExecutorHelper: Get PluginExecutor
ExecutorHelper->>Executor: Call postSaveHook(datasourceStorage)
Executor-->>DSStorageService: Return Mono<DatasourceStorage>
DSStorageService-->>Repo: Return saved & processed DatasourceStorage
sequenceDiagram
autonumber
participant DSService as DatasourceServiceCEImpl
participant Tenant as TenantService
participant Config as ConfigService
participant DSRepo as DatasourceRepository
DSService->>Tenant: Fetch default tenantId
DSService->>Config: Fetch instanceId
Tenant-->>DSService: tenantId
Config-->>DSService: instanceId
DSService->>DSRepo: Save DatasourceStorage with tenant & instance IDs
DSRepo-->>DSService: Return saved DatasourceStorage
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
![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
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (2)
126-128: Constructor may be getting crowded
AddingTenantServiceandConfigServiceis valid, yet the constructor is quite large. Consider extracting some logic into dedicated service classes/factories to keep the constructor more manageable.
239-274: Flow for creating datasource storage with tenant and instance
The zipped flow correctly mergesdatasourceMonowithtenantIdAndInstanceIdMonoCached. However, watch out for:
- Error handling: If tenant/instance fetch fails, does the user receive clear feedback?
- Transient fields:
tenantId/instanceIdare transient. Ensure usage is clarified so new contributors know these fields are not persisted.- Complex method: This method is fairly long. Consider extracting the inner callbacks into smaller methods to improve readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceStorage.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceImpl.java (1)
29-70:❓ Verification inconclusive
LGTM! Clean constructor extension.
The addition of
TenantServiceandConfigServicefollows Spring's dependency injection pattern and is properly passed to the parent class.Let's verify the parent class implementation:
🏁 Script executed:
#!/bin/bash # Check if the parent class DatasourceServiceCEImpl has matching constructor parameters ast-grep --pattern 'class DatasourceServiceCEImpl { $$$ DatasourceServiceCEImpl($$$, TenantService $_, ConfigService $_) { $$$ } $$$ }'Length of output: 149
Constructor changes approved—but please manually verify the parent class signature.
The changes in
DatasourceServiceImplcorrectly addTenantServiceandConfigServicefollowing our dependency injection pattern and are properly passed to the superclass. However, our automated search for the constructor in the parent class (DatasourceServiceCEImpl) didn’t produce a match, so please double-check that its constructor supports these new parameters to ensure consistency at runtime.app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (2)
31-36: Imports look consistent with usage
BothConfigServiceandTenantServiceare referenced in the updated code. These additions are valid and appear necessary for multi-tenant handling.
193-198: Ensure.cache()usage is intentional
.cache()will indefinitely store and reuse the same tenant/instance data for every subscriber, including any errors. Verify that the tenant ID and instance ID remain constant once resolved and that caching errors is the desired behavior.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java (1)
188-193: LGTM!The
postSaveHookmethod follows the established pattern of hook methods in the interface, with proper documentation and default implementation.app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java (1)
186-194: LGTM!The
executePostSaveActionsmethod follows the established pattern of theexecutePreSaveActionsmethod, with proper error handling and plugin executor retrieval.
| @Transient | ||
| String tenantId; | ||
|
|
||
| @Transient | ||
| String instanceId; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update prepareTransientFields to include new fields.
The new transient fields tenantId and instanceId should be initialized in the prepareTransientFields method to maintain consistency with other transient fields.
Apply this diff to update the method:
public void prepareTransientFields(Datasource datasource) {
this.datasourceId = datasource.getId();
this.name = datasource.getName();
this.pluginId = datasource.getPluginId();
this.pluginName = datasource.getPluginName();
this.workspaceId = datasource.getWorkspaceId();
this.templateName = datasource.getTemplateName();
this.isAutoGenerated = datasource.getIsAutoGenerated();
this.isRecentlyCreated = datasource.getIsRecentlyCreated();
this.isTemplate = datasource.getIsTemplate();
this.isMock = datasource.getIsMock();
+ this.tenantId = datasource.getTenantId();
+ this.instanceId = datasource.getInstanceId();
if (datasource.getInvalids() != null) {
this.invalids.addAll(datasource.getInvalids());
}
this.gitSyncId = datasource.getGitSyncId();
}📝 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.
| @Transient | |
| String tenantId; | |
| @Transient | |
| String instanceId; | |
| public void prepareTransientFields(Datasource datasource) { | |
| this.datasourceId = datasource.getId(); | |
| this.name = datasource.getName(); | |
| this.pluginId = datasource.getPluginId(); | |
| this.pluginName = datasource.getPluginName(); | |
| this.workspaceId = datasource.getWorkspaceId(); | |
| this.templateName = datasource.getTemplateName(); | |
| this.isAutoGenerated = datasource.getIsAutoGenerated(); | |
| this.isRecentlyCreated = datasource.getIsRecentlyCreated(); | |
| this.isTemplate = datasource.getIsTemplate(); | |
| this.isMock = datasource.getIsMock(); | |
| this.tenantId = datasource.getTenantId(); | |
| this.instanceId = datasource.getInstanceId(); | |
| if (datasource.getInvalids() != null) { | |
| this.invalids.addAll(datasource.getInvalids()); | |
| } | |
| this.gitSyncId = datasource.getGitSyncId(); | |
| } |
| return repository | ||
| .save(unsavedDatasourceStorage) | ||
| .then(this.executePostSaveActions(unsavedDatasourceStorage)) | ||
| .thenReturn(unsavedDatasourceStorage); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider transaction boundary for save operation.
The save operation and post-save actions should be executed within a transaction to ensure atomicity. If the post-save action fails, the save should be rolled back.
Consider using Spring's @Transactional annotation or a transaction template to wrap both operations:
-return repository
- .save(unsavedDatasourceStorage)
- .then(this.executePostSaveActions(unsavedDatasourceStorage))
- .thenReturn(unsavedDatasourceStorage);
+return transactionTemplate.execute(status -> {
+ return repository
+ .save(unsavedDatasourceStorage)
+ .then(this.executePostSaveActions(unsavedDatasourceStorage))
+ .thenReturn(unsavedDatasourceStorage);
+});📝 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.
| return repository | |
| .save(unsavedDatasourceStorage) | |
| .then(this.executePostSaveActions(unsavedDatasourceStorage)) | |
| .thenReturn(unsavedDatasourceStorage); | |
| return transactionTemplate.execute(status -> { | |
| return repository | |
| .save(unsavedDatasourceStorage) | |
| .then(this.executePostSaveActions(unsavedDatasourceStorage)) | |
| .thenReturn(unsavedDatasourceStorage); | |
| }); |
| @Transient | ||
| String tenantId; | ||
|
|
||
| @Transient | ||
| String instanceId; |
There was a problem hiding this comment.
I'm not sure if DatasourceStorage should have these fields configured in the POJO. I understand that we need the fields instanceId and tenantId in our operations. But having these fields in the DatasourceStorage object seems awkward as it's violating the boundaries of our system architecture.
What do you think of instead introducing a map of parameters that can store this metadata that is parsed by the plugin itself? Would that be more extendable? Or do you think it'll lead to confusion and lack of clarity in the code?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (1)
266-278: Add logging for metadata operations.The new method for setting metadata would benefit from debug logging to aid troubleshooting.
private Mono<DatasourceStorage> setAdditionalMetadataInDatasourceStorage(DatasourceStorage datasourceStorage) { + log.debug("Setting metadata for datasource storage: {}", datasourceStorage.getId()); Mono<String> tenantIdMono = tenantService.getDefaultTenantId(); Mono<String> instanceIdMono = configService.getInstanceId(); Map<String, Object> metadata = new HashMap<>(); return tenantIdMono.zipWith(instanceIdMono).map(tuple -> { metadata.put(TENANT_ID, tuple.getT1()); metadata.put(INSTANCE_ID, tuple.getT2()); datasourceStorage.setMetadata(metadata); + log.debug("Metadata set successfully for datasource storage: {}", datasourceStorage.getId()); return datasourceStorage; }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceStorage.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceStorage.java
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1)
200-200: LGTM! Addition aligns with tenant-specific datasource management.The new constant follows the established naming conventions and is appropriately placed near related constants.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (2)
100-101: LGTM! New service dependencies added.The addition of TenantService and ConfigService as dependencies follows good dependency injection practices.
235-257: Verify error handling in the metadata setting chain.The new flatMap chain for setting metadata should handle potential errors from tenant/config service calls.
Consider adding error handling:
return datasourceMono.flatMap(savedDatasource -> this.organiseDatasourceStorages(savedDatasource) .flatMap(datasourceStorageX -> setAdditionalMetadataInDatasourceStorage(datasourceStorageX) + .onErrorResume(error -> { + log.error("Error setting metadata in datasource storage", error); + return Mono.just(datasourceStorageX); + }) .flatMap(datasourceStorage -> {
Description
Important
Add a post save hook on datasources.
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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13386879786
Commit: 260ad93
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 18 Feb 2025 09:36:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
TENANT_IDadded for consistent field name referencing.