chore: refactor the crud method to custom repo class #38137
Conversation
WalkthroughThe changes introduce a new method Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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: 0
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCE.java (2)
45-46: Add Javadoc to clarify method relationships.Consider adding documentation to explain:
- The relationship between this method and its overloads
- When to use this version vs. the permission-aware versions
- Any security implications
+/** + * Finds ActionCollections by applicationId without explicit permission checks. + * Note: Prefer using permission-aware overloads unless specifically needed. + * + * @param applicationId the ID of the application + * @return Flux of ActionCollections + */ Flux<ActionCollection> findByApplicationId(String applicationId);
Line range hint
13-46: Consider reordering methods for better readability.Group related methods together:
- All findByApplicationId variants
- All findByPageId variants
- Other specialized finders
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java (1)
Line range hint
20-29: Add proper deprecation documentation.The deprecated method should include Javadoc with
@deprecatedtag explaining the reason for deprecation and suggesting the migration path to the new method.+ /** + * @deprecated Use {@link #findByApplicationId(String)} instead. + * This method will be removed in a future release. + */ @Override @Deprecated public Flux<ActionCollection> findByApplicationId(String applicationId, AclPermission aclPermission, Sort sort) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java(1 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCE.java (1)
46-46:
Verify permission handling in the new method implementation.
The new method bypasses the explicit permission check present in other similar methods. Ensure this is intentional and doesn't create security vulnerabilities.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java (1)
172-176: Verify security implications of removing permission checks.
The new method doesn't include permission checks that were present in the deprecated version. Please confirm if this is intentional and doesn't introduce security vulnerabilities.
✅ Verification successful
Let me analyze the permission check patterns in the codebase with these broader searches to verify the security implications.
Permission check removal is intentional and safe
The implementation is correct as this method serves as a basic repository query without permission checks, while the interface provides other methods with permission checks for secured operations:
findByApplicationId(String, AclPermission, Sort)- deprecated but with permissionsfindByApplicationId(String, Optional<AclPermission>, Optional<Sort>)- flexible permission checks- The new method is meant for internal repository operations where permissions are handled at the service layer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for permission checks in similar repository methods
ast-grep --pattern 'public Flux<$_> find$_($_) {
$$$
.permission($_)
$$$
}'
Length of output: 83
Script:
#!/bin/bash
# Search for similar repository methods with permission checks using a broader pattern
rg -U "public\s+Flux<.*?>\s+find.*?\(.*?\).*?\{[\s\S]*?permission.*?\}" --type java
# Search for the deprecated version of this method
rg -U "findByApplicationId.*?deprecated" --type java -A 10
# Look for permission checks in the base class or interface
fd "CustomActionCollectionRepository" --type f --exec rg -l "permission|PermissionGroup" {}
Length of output: 482
Script:
#!/bin/bash
# Let's examine the content of these repository files to understand the permission handling
cat ./app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCE.java
# Also check the implementation file
cat ./app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java
# Search for any service layer that might be using this repository method
rg -U "findByApplicationId.*?ActionCollection" --type java -A 5
Length of output: 10369
Failed server tests
|
AnaghHegde
deleted the
chore/refactor-crud-to-custom-repository-action-collection
branch
## 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.ImportExport" ### 🔍 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/12304558211> > Commit: d85d3ae > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12304558211&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.ImportExport` > Spec: > <hr>Thu, 12 Dec 2024 21:15:06 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 a new method to retrieve action collections by application ID without requiring additional parameters. - **Bug Fixes** - Deprecated the previous method for fetching action collections that required more parameters, streamlining the querying process. - **Documentation** - Updated method signatures to reflect the new querying capabilities. <!-- 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.ImportExport"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12304558211
Commit: d85d3ae
Cypress dashboard.
Tags:
@tag.ImportExportSpec:
Thu, 12 Dec 2024 21:15:06 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation