chore: Remove findById signature with Optional#34302
Conversation
WalkthroughThe changes across multiple files focus on refining the method signatures by removing the usage of Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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/services/ce/WorkspaceServiceCEImpl.java (1)
[!TIP]
Codebase VerificationThe search results indicate that the method
findByIdwithAclPermissionis still present in the codebase, including in theWorkspaceServiceCEImplclass. This confirms that the method signature change was not consistently applied.
- Files to be updated:
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.javaPlease ensure that the method signature change is applied consistently throughout the codebase.
Analysis chain
Line range hint
193-193: The updated method signature forfindByIdimproves readability and aligns with the codebase's handling ofnullvalues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to `findById` in the WorkspaceServiceCEImpl class have been updated across the codebase. # Test: Search for the method usage with the old signature. Expect: No occurrences. rg --type java $'findById\([^,]+,\s*Optional<'Length of output: 47
Script:
#!/bin/bash # Description: Verify that all calls to `findById` in the WorkspaceServiceCEImpl class have been updated across the codebase. # Test: Search for the method usage with the new signature. Expect: Occurrences of the new method signature. rg --type java $'findById\\([^,]+,\\s*AclPermission\\)'Length of output: 53
Script:
#!/bin/bash # Description: Verify all method signatures of `findById` in the codebase. # Test: Search for all occurrences of `findById` method signatures. Expect: Occurrences of various signatures. rg --type java $'findById\\('Length of output: 76916
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java (2 hunks)
Additional context used
Learnings (6)
Common learnings
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCE.java (1)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java (2)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java:29-29 Timestamp: 2024-06-03T11:52:27.242Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (2)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java:29-29 Timestamp: 2024-06-03T11:52:27.242Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java (1)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (1)
User: sharat87 PR: appsmithorg/appsmith#33602 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java:462-473 Timestamp: 2024-06-03T11:56:24.442Z Learning: The codebase has mechanisms to gracefully handle `null` values, making the use of `Optional` for `AclPermission` unnecessary in certain contexts. This is leveraged by replacing `Optional.empty()` with `null` in method signatures where `null` is already handled gracefully.
Additional comments not posted (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCE.java (1)
Line range hint
17-17: The updated method signature forfindByIdimproves readability and aligns with the codebase's handling ofnullvalues.Verification successful
The method signature for
findByIdin theWorkspaceServiceCEinterface is already usingAclPermissionas expected. This aligns with the changes mentioned in the review comment, and no issues were found regarding the method signature.
WorkspaceServiceCE.javacontains the method signature forfindByIdusingAclPermission.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to `findById` in the WorkspaceServiceCE interface have been updated across the codebase. # Test: Search for the method usage with the old signature. Expect: No occurrences. rg --type java $'findById\([^,]+,\s*Optional<'Length of output: 47
Script:
#!/bin/bash # Description: Verify that the new method signature for `findById` in the WorkspaceServiceCE interface is used correctly across the codebase. # Test: Search for the method usage with the new signature. Expect: Occurrences with the new signature. rg --type java 'findById\([^,]+,\s*AclPermission\)'Length of output: 53
Script:
#!/bin/bash # Description: Verify the `findById` method signature within the `WorkspaceServiceCE` interface. # Test: Search for the method definition within the `WorkspaceServiceCE` interface. Expect: The method definition with the new signature. ast-grep --lang java --pattern 'interface WorkspaceServiceCE { $_ Mono<Workspace> findById($_, AclPermission $_); $_ }'Length of output: 127
Script:
#!/bin/bash # Description: Display the content of the `WorkspaceServiceCE` interface file to verify its method signatures. # Test: Output the content of the `WorkspaceServiceCE` interface file. fd 'WorkspaceServiceCE.java' -x catLength of output: 2480
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java (1)
11-11: The updated method signature forfindByIdimproves readability and aligns with the codebase's handling ofnullvalues.Verification successful
The verification confirms that the
findByIdmethod signature has been updated to useAclPermissionacross various files in the codebase.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/CurlImporterServiceTest.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/PermissionGroupRepositoryCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.javaThis indicates that the changes have been consistently applied throughout the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to `findById` in the AppsmithRepository interface have been updated across the codebase. # Test: Search for the method usage with the old signature. Expect: No occurrences. rg --type java $'findById\([^,]+,\s*Optional<'Length of output: 47
Script:
#!/bin/bash # Description: Verify that all calls to `findById` in the AppsmithRepository interface have been updated across the codebase. # Test: Search for the method usage with the new signature. Expect: Occurrences of the new method signature. rg --type java $'findById\([^,]+,\s*AclPermission'Length of output: 3823
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java (1)
Line range hint
104-104: The updated method signature forfindByIdimproves readability and aligns with the codebase's handling ofnullvalues.Verification successful
The updated method signature for
findByIdwithAclPermissionis used consistently across the codebase, as evidenced by the search results. There are no instances of the old signature (findByIdwithOptional), confirming that the codebase has been updated correctly.
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/CurlImporterServiceTest.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/WorkspaceServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/PermissionGroupRepositoryCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCE.java
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to `findById` in the BaseAppsmithRepositoryCEImpl class have been updated across the codebase. # Test: Search for the method usage with the old signature. Expect: No occurrences. rg --type java $'findById\([^,]+,\s*Optional<'Length of output: 47
Script:
#!/bin/bash # Description: Verify that the updated method signature for `findById` is used consistently across the codebase. # Test: Search for the method usage with the updated signature. Expect: Multiple occurrences. rg --type java $'findById\([^,]+,\s*AclPermission'Length of output: 3823
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (4)
143-143: The method signature and the call tocreateExhave been updated to useAclPermissiondirectly instead ofOptional<AclPermission>. This aligns with the PR's objective to simplify method signatures by removing unnecessary usage ofOptional. Good to see consistency in implementation across different methods.
149-149: The methodcreateWithoutPermissionsnow passesnulldirectly tocreateEx, reflecting the removal ofOptional. This change is consistent with the learning that the codebase handlesnullvalues gracefully. This simplifies the code by avoiding unnecessary wrapping and unwrapping ofOptional.
265-265: The methodgenerateAndSetDatasourcePoliciesnow acceptsAclPermissiondirectly. This is a good improvement, as it simplifies the code by removing the unnecessary use ofOptional. The change should make the codebase cleaner and more straightforward.
152-152: The updatedcreateExmethod now directly acceptsAclPermissioninstead ofOptional<AclPermission>. This change simplifies the method signature and avoids the need for null checks associated withOptional. Ensure that all callers of this method are updated to passAclPermissiondirectly ornullwhere appropriate.Verification successful
The
rgoutput shows that the calls tocreateExin theDatasourceServiceCEImplclass are consistent with the new method signature. Specifically, the calls are made with either a validAclPermissionornull, which aligns with the updated method.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java: return createEx(datasource, workspacePermission.getDatasourceCreatePermission());app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java: return createEx(datasource, null);app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java: private Mono<Datasource> createEx(@NotNull Datasource datasource, AclPermission permission);These lines confirm that the method calls have been updated to match the new signature.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `createEx` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type java $'createEx'Length of output: 5869
3 participants
Follow up to #34281.
The
.findByIdmethod is now removed.No conflicts, but needs extra changes in this PR for build to pass on EE.
/test sanity
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9563607766
Commit: 02f6031
Cypress dashboard.
Tags:
@tag.SanitySummary by CodeRabbit
Optionalfor permissions, simplifying method signatures and improving readability.