chore: refactor the crud method to custom repo class #38138
Conversation
WalkthroughThe changes introduce new methods to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
AnaghHegde
requested review from
abhvsn
and removed request for
nidhi-nair and
sondermanish
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomDatasourceRepositoryCEImpl.java (1)
47-51: Consider adding batch size limit for large ID lists.While the implementation is correct, consider adding a limit check for the size of the ID list to prevent potential performance issues with very large lists.
@Override public Flux<Datasource> findByIdIn(List<String> ids) { + if (ids.size() > 1000) { + log.warn("Large batch size in findByIdIn: {}", ids.size()); + } final BridgeQuery<Datasource> q = Bridge.in(Datasource.Fields.id, ids); return queryBuilder().criteria(q).all(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomDatasourceRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomDatasourceRepositoryCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/DatasourceRepositoryCE.java(1 hunks)
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/DatasourceRepositoryCE.java (1)
7-7: LGTM! Clean interface design with proper separation of concerns.
The interface is now more focused with the policy-related method, while other CRUD operations are moved to the custom repository interface.
Also applies to: 12-14
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomDatasourceRepositoryCE.java (2)
17-19: LGTM! Clear method signature for workspace-specific queries.
The method properly handles permission checks through the AclPermission parameter.
21-23: LGTM! Batch retrieval method properly defined.
The method signature for batch retrieval by IDs is clear and follows repository pattern best practices.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomDatasourceRepositoryCEImpl.java (2)
35-39: LGTM! Clean implementation using Bridge pattern.
The query construction is clear and follows the established pattern.
53-57: LGTM! Efficient query for counting active datasources.
The implementation correctly uses isNull check for counting non-deleted datasources.
...h-server/src/main/java/com/appsmith/server/repositories/ce/CustomDatasourceRepositoryCE.java
Outdated
Show resolved
Hide resolved
| @@ -28,4 +31,28 @@ public Mono<Datasource> findByNameAndWorkspaceId(String name, String workspaceId | |||
| .permission(aclPermission) | |||
| .one(); | |||
| } | |||
|
|
|||
| @Override | |||
| public Flux<Datasource> findByWorkspaceId(String workspaceId, AclPermission aclPermission) { | |||
There was a problem hiding this comment.
This is not present in DatasourceRepoisitoryCE, are we adding this as a new methods?
There was a problem hiding this comment.
This is a duplicate since the method already exists in custom implementation.
...rver/src/main/java/com/appsmith/server/repositories/ce/CustomDatasourceRepositoryCEImpl.java
Outdated
Show resolved
Hide resolved
|
|
||
| Flux<Datasource> findByIdIn(List<String> ids); | ||
|
|
||
| Mono<Long> countByDeletedAtNull(); |
There was a problem hiding this comment.
Are we not adding Flux<Datasource> findAllByWorkspaceId(String workspaceId); or this is not used in CE and EE?
There was a problem hiding this comment.
This was not used anywhere in the code base. Hence removing this.
AnaghHegde
added
ok-to-test
## Description As part of transaction support in PG, we are moving from using the jpa methods for database operations. This PR is refactoring the code to use custom repository class for DatasourceRepository from the default CrudRepository. ## Automation /ok-to-test tags="@tag.Datasource" ### 🔍 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/12388978242> > Commit: df7dc42 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12388978242&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource` > Spec: > <hr>Wed, 18 Dec 2024 08:59:01 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 - **New Features** - Introduced methods to retrieve `Datasource` objects by IDs and count non-deleted `Datasource` instances. - **Bug Fixes** - Removed outdated method signatures from the `DatasourceRepositoryCE` interface to streamline functionality. - **Documentation** - Added `@Repository` annotation to the `DatasourceRepositoryCE` interface for clarity on its role within the application. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Description As part of transaction support in PG, we are moving from using the jpa methods for database operations. This PR is refactoring the code to use custom repository class for DatasourceRepository from the default CrudRepository. ## Automation /ok-to-test tags="@tag.Datasource" ### 🔍 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/12388978242> > Commit: df7dc42 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12388978242&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource` > Spec: > <hr>Wed, 18 Dec 2024 08:59:01 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 - **New Features** - Introduced methods to retrieve `Datasource` objects by IDs and count non-deleted `Datasource` instances. - **Bug Fixes** - Removed outdated method signatures from the `DatasourceRepositoryCE` interface to streamline functionality. - **Documentation** - Added `@Repository` annotation to the `DatasourceRepositoryCE` interface for clarity on its role within the application. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
As part of transaction support in PG, we are moving from using the jpa methods for database operations. This PR is refactoring the code to use custom repository class for DatasourceRepository from the default CrudRepository.
Automation
/ok-to-test tags="@tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12388978242
Commit: df7dc42
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Wed, 18 Dec 2024 08:59:01 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Datasourceobjects by IDs and count non-deletedDatasourceinstances.DatasourceRepositoryCEinterface to streamline functionality.@Repositoryannotation to theDatasourceRepositoryCEinterface for clarity on its role within the application.