chore: move metadata calculation to datasource storage#39657
Conversation
WalkthroughThis pull request removes dependencies on Changes
Sequence Diagram(s)sequenceDiagram
participant DS as DatasourceStorageServiceCEImpl
participant Org as OrganizationService
participant Config as ConfigService
participant Repo as DatasourceStorageRepository
DS->>Org: Request organization ID
DS->>Config: Request instance ID
Org-->>DS: Return organization ID
Config-->>DS: Return instance ID
DS->>Repo: Save DatasourceStorage with metadata
Suggested labels
Suggested reviewers
Poem
✨ 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
nsarupr
added
ok-to-test
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: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java (1)
280-293: Added method to set metadata from organization and instance information.This new method uses the injected services to fetch organization and instance IDs asynchronously and combines them into a metadata map. The Reactor operators are used correctly and the method maintains proper error handling through the Mono chain.
The comment on line 287 mentions changing to ORGANIZATION_ID once available in datasource storage. Consider creating a ticket to track this future improvement.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCECompatibleImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceImpl.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (11)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceImpl.java (2)
7-8: Added necessary imports for new service dependencies.These imports correspond to the new service dependencies being added to the constructor.
22-32: Updated constructor with new dependencies.The constructor has been properly updated to include ConfigService and OrganizationService parameters, and the super constructor is correctly invoked with these new parameters. This change aligns with the PR objective of moving metadata calculation to datasource storage.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCECompatibleImpl.java (2)
7-8: Added necessary imports for new service dependencies.These imports correspond to the new service dependencies being added to the constructor.
21-31: Updated constructor with new dependencies.The constructor has been properly updated to include ConfigService and OrganizationService parameters, and the super constructor is correctly invoked with these new parameters. This change maintains consistency with the implementation changes in DatasourceStorageServiceImpl.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java (4)
7-8: Added imports for new service dependencies and constants.The imports for ConfigService, OrganizationService, and constants for field names are appropriately added to support the new metadata calculation functionality.
Also applies to: 20-21, 35-36
48-49: Added service fields for metadata handling.These private final fields are correctly added to hold the injected service instances necessary for metadata calculation.
56-65: Updated constructor with new dependencies.The constructor properly initializes the ConfigService and OrganizationService fields with the injected parameters, maintaining the class's initialization logic.
266-270: Updated post-save flow with metadata setting.The execution flow now includes setting additional metadata before executing post-save actions. This implementation correctly preserves the return value chain and maintains the method's contract.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (2)
121-121: Removed ConfigService and OrganizationService from constructor parameters.The constructor properly removes the now unnecessary dependencies that have been moved to the DatasourceStorageService classes. This aligns with the PR objective of simplifying the constructor and removing metadata handling from this class.
227-247: Simplified datasource storage creation logic.The nested logic for handling datasource storage has been streamlined by removing the metadata setting that was previously done here. The code now directly checks if the ID is populated without additional metadata processing, making the code cleaner and more focused on its core responsibility.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceImpl.java (1)
27-64: Constructor simplified with focused dependenciesThe removal of
OrganizationServiceandConfigServicefrom the constructor aligns with the PR objective of moving metadata calculation to datasource storage. This change properly separates concerns and reduces unnecessary dependencies in this service implementation.
Failed server tests
|
…9657) ## 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 Number` _or_ Fixes `Issue URL` > [!WARNING] > _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 <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13782216026> > Commit: b835526 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13782216026&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Tue, 11 Mar 2025 07:40:11 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined and optimized the data source configuration process by simplifying how associated metadata is handled. - Improved the underlying service interactions to enhance system performance and maintainability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Nilesh Sarupriya <20905988+nsarupr@users.noreply.github.com>
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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13782216026
Commit: b835526
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 11 Mar 2025 07:40:11 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit