chore: refactor new page crud repo methods #38169
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java (1)
271-275: LGTM! Consider caching for performanceThe implementation is clean and follows the established patterns. However, since this is a count query that might be called frequently, consider adding caching to improve performance.
Example cache implementation:
+@Cacheable(value = "pageCount", key = "'deletedAtNull'") public Mono<Long> countByDeletedAtNull() { final BridgeQuery<NewPage> q = Bridge.notExists(NewPage.Fields.deletedAt); return queryBuilder().criteria(q).count(); }
📜 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/CustomNewPageRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/NewPageRepositoryCE.java(0 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/NewPageRepositoryCE.java
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCE.java (2)
48-48: LGTM: Clean implementation of soft delete count
The countByDeletedAtNull() method follows the established pattern for soft deletes.
46-46: Verify security implications of removing AclPermission parameter
The new findByApplicationId method omits the AclPermission parameter present in existing similar methods. This could potentially bypass access control checks.
✅ Verification successful
Let me analyze the search results to verify the security implications of removing the AclPermission parameter.
The search results show that:
-
The new
findByApplicationIdmethod is used in several repository implementations:- CustomNewPageRepositoryCEImpl
- CustomActionCollectionRepositoryCEImpl
- NewActionRepositoryCE
-
The method is primarily used in:
- Test cases
- PolicySolutionCEImpl for updating permissions
- ApplicationSnapshotService for managing snapshots
-
All usages are internal to the repository layer and are wrapped by service layer methods that enforce permissions:
- Service methods add AclPermission checks before calling repository methods
- No direct exposure to API endpoints
- Used within policy management and snapshot functionality that already have security controls
Repository methods can skip ACL checks as service layer handles permissions
The removal of AclPermission parameter from repository method is safe since:
- Service layer methods enforce proper permission checks before accessing data
- Repository layer focuses on data access while security is handled at higher layers
- Follows separation of concerns principle
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of the new method to verify it's only called from secure contexts
ast-grep --pattern 'findByApplicationId($applicationId)'
# Search for existing security checks around the usage
rg -B 5 -A 5 'findByApplicationId\([^,]+\)'
Length of output: 66111
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewPageRepositoryCEImpl.java (1)
265-269: Consider adding AclPermission parameter for consistency
The method lacks access control checks compared to its overloaded version. This could potentially bypass security controls if not used carefully.
Let's verify the usage of this method to ensure it's only called in secure contexts:
Consider one of these approaches:
- Add AclPermission parameter for consistency with other methods
- Document the security implications in method's Javadoc
- Restrict method visibility if it's only meant for internal use
|
|
||
| @Override | ||
| public Mono<Long> countByDeletedAtNull() { | ||
| final BridgeQuery<NewPage> q = Bridge.notExists(NewPage.Fields.deletedAt); |
There was a problem hiding this comment.
@AnaghHegde I hope we have verified this.
## 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 NewPageRepository from the default CrudRepository. ## Automation /ok-to-test tags="@tag.ImportExport" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/12329409662> > Commit: d8ff386 > Workflow: `PR Automation test suite` > Tags: `@tag.ImportExport` > Spec: `` > <hr>Sat, 14 Dec 2024 11:17:37 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 pages by application ID and count non-deleted pages. - **Bug Fixes** - Removed outdated methods from the `NewPageRepositoryCE` interface to streamline functionality. <!-- 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 NewPageRepository 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/12329409662
Commit: d8ff386
Cypress dashboard.
Tags:
@tag.ImportExportSpec:
Sat, 14 Dec 2024 11:42:09 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
NewPageRepositoryCEinterface to streamline functionality.