Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (4)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (4)
The StreamSupport import is correctly used for converting Iterable to List in the findAllByIdIn method.
The method provides a simplified version for querying by applicationId without permission checks, consistent with the repository's pattern.
The method now correctly uses the 'id' field and safely converts Iterable to List using StreamSupport.
Fix inverted logic in deletedAt check The method name Apply this fix: -public Mono<Long> countByDeletedAtNull() {
- return queryBuilder()
- .criteria(Bridge.exists(NewAction.Fields.deletedAt))
- .count();
-}
+public Mono<Long> countByDeletedAtNull() {
+ return queryBuilder()
+ .criteria(Bridge.isNull(NewAction.Fields.deletedAt))
+ .count();
+}
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: 3
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (1)
494-499: Consider consolidating duplicate methodsThis method duplicates the existing
findByApplicationId(String, AclPermission)with only the permission check removed. Consider using the existing method with an Optional permission parameter for better code maintainability.-public Flux<NewAction> findByApplicationId(String applicationId) { - return queryBuilder() - .criteria(Bridge.equal(NewAction.Fields.applicationId, applicationId)) - .all(); -} +public Flux<NewAction> findByApplicationId(String applicationId) { + return findByApplicationId(applicationId, null); +}app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java (1)
9-9: LGTM! Well-structured repository interface enhancements.The new methods complement the existing functionality while adding optimized paths for bulk operations. The cursor batch size of 10000 is appropriate for MongoDB operations at this scale.
Consider documenting these performance-optimized methods in the interface to help other developers choose the most efficient method for their use case.
Also applies to: 85-91
📜 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/CustomNewActionRepositoryCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/NewActionRepositoryCE.java(0 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/NewActionRepositoryCE.java
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java (2)
91-91: LGTM! Clean and follows naming conventions.
The method name clearly describes its purpose and follows Spring Data naming conventions.
85-86: Verify security implications of permission-less query.
The new findByApplicationId overload doesn't include ACL permissions. Ensure this is intentionally designed for internal/system use only.
✅ Verification successful
Let me analyze the usage patterns and security context with these broader searches.
Permission-less query is used safely in internal system operations
The permission-less findByApplicationId(String) method is correctly implemented and used appropriately:
- It's used internally by system services like
ApplicationSnapshotService,PolicySolution, and import/export operations - Other public-facing methods use the secured overloads that require
AclPermission - The implementation in
CustomNewActionRepositoryCEImplshows it's a basic query without any sensitive data exposure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages to verify security context
rg -A 5 "findByApplicationId\(String\)" --type java
Length of output: 53
Script:
#!/bin/bash
# Search for usages with broader pattern to catch all variants
rg -A 5 "findByApplicationId" --type java
# Search for class definition to understand context
ast-grep --pattern 'interface $_ {
$$$
findByApplicationId($$$) {
$$$
}
$$$
}'
# Look for similar methods with security checks
rg -A 5 "findByApplication.*Permission" --type java
Length of output: 65724
...erver/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java
Show resolved
Hide resolved
...erver/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java
Show resolved
Hide resolved
...th-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public Flux<NewAction> findAllByIdIn(Iterable<String> ids) { | ||
| return queryBuilder() | ||
| .criteria(Bridge.in(NewAction.Fields.applicationId, IteratorUtils.toList((Iterator) ids))) |
There was a problem hiding this comment.
Should it be NewAction.Fields.id?
...th-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java
Outdated
Show resolved
Hide resolved
| Flux<NewAction> findByApplicationId(String applicationId); | ||
|
|
||
| // @Meta(cursorBatchSize = 10000) | ||
| // TODO Implement cursor with batch size |
There was a problem hiding this comment.
Let's use the .all() method to implement the batching, which will make sure all the DB calls by default will be batched.
AnaghHegde
added
ok-to-test
…new-action-crud-repo-refactor
## 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 DatasourceStorageRepository 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/12406578294> > Commit: c7efa63 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12406578294&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.ImportExport` > Spec: > <hr>Thu, 19 Dec 2024 05:27:22 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 ## Summary by CodeRabbit - **New Features** - Introduced new methods for retrieving and counting `NewAction` entities based on application ID and IDs collection. - **Bug Fixes** - Removed outdated methods from the `NewActionRepositoryCE` interface to streamline functionality. - **Refactor** - Enhanced repository capabilities for managing `NewAction` entities while maintaining backward compatibility. <!-- 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 DatasourceStorageRepository 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/12406578294> > Commit: c7efa63 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12406578294&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.ImportExport` > Spec: > <hr>Thu, 19 Dec 2024 05:27:22 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 ## Summary by CodeRabbit - **New Features** - Introduced new methods for retrieving and counting `NewAction` entities based on application ID and IDs collection. - **Bug Fixes** - Removed outdated methods from the `NewActionRepositoryCE` interface to streamline functionality. - **Refactor** - Enhanced repository capabilities for managing `NewAction` entities while maintaining backward compatibility. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
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 DatasourceStorageRepository 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/12406578294
Commit: c7efa63
Cypress dashboard.
Tags:
@tag.ImportExportSpec:
Thu, 19 Dec 2024 05:27:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
NewActionentities based on application ID and IDs collection.Bug Fixes
NewActionRepositoryCEinterface to streamline functionality.Refactor
NewActionentities while maintaining backward compatibility.