chore: Deprecate .findById signature with Optional parameter
#34281
Conversation
WalkthroughThe primary objective of these changes was to eliminate the use 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 (
|
sharat87
changed the title
.findById signature with Optional parameter.findById signature with Optional parameter
github-actions
bot
added
the
skip-changelog
Failed server tests
|
sharat87
changed the title
.findById signature with Optional parameter.findById signature with Optional parameter
sharat87
requested review from
nidhi-nair,
sondermanish and
a team
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (18)
- app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/importable/NewPageImportableServiceCEImpl.java (1 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 (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CurlImporterServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (6 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java (4 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/PartialImportServiceTest.java (3 hunks)
Files not reviewed due to errors (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (no review received)
Files skipped from review due to trivial changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/BaseAppsmithRepositoryCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CurlImporterServiceCEImpl.java
Additional context used
Learnings (9)
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/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/services/ce/ApplicationPageServiceCE.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/actioncollections/base/ActionCollectionServiceCE.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/newactions/base/NewActionServiceCE.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/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.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/actioncollections/base/ActionCollectionServiceCEImpl.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/services/ce/ApplicationPageServiceCEImpl.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/newactions/base/NewActionServiceCEImpl.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 (14)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCE.java (1)
52-57: The updated signature ofdeleteUnpublishedPagecorrectly replacesOptionalparameters withAclPermission. Ensure all calls to this method are updated to reflect this change.app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCE.java (1)
49-50: The updated signature ofdeleteUnpublishedActionCollectioncorrectly replacesOptionalparameters withAclPermission. Ensure all calls to this method are updated to reflect this change.app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCE.java (1)
74-74: The addition ofarchiveByIdWithoutPermissionprovides flexibility in handling permissions. Ensure this method is used appropriately in contexts where permission checks are not required.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (2)
51-51: The method signature change fromOptional<AclPermission>toAclPermissionaligns with the PR's objectives to simplify method signatures and avoid unnecessary use ofOptional. Good job on enhancing clarity.
87-87: ReplacingOptional<AclPermission>withAclPermissionin the method signature ofdeleteUnpublishedActionis a positive change, enhancing the method's clarity and consistency with other service methods.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java (1)
259-259: ReplacingOptional.empty()withnullin thefindByIdmethod call simplifies the interface and leverages the codebase's existing null handling mechanisms. This change aligns well with the PR's objectives.app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/PartialImportServiceTest.java (1)
285-285: The replacement ofOptional.empty()withnullin thefindByIdmethod calls across multiple test methods improves consistency and aligns with the PR's objectives to simplify method signatures.Also applies to: 403-403, 484-484
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (1)
101-101: Refactoring to removeOptionalparameters is well executed here by directly passingnullwhereOptional.empty()was previously used.app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/base/NewPageServiceCEImpl.java (1)
519-519: The refactoring to removeOptionalparameters from thearchiveByIdWithoutPermissionandarchiveByIdmethods aligns with the PR's objectives. This simplifies the method signatures and avoids potential confusion.Also applies to: 525-525, 533-533
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java (1)
377-377: The change fromOptionaltonullaligns with the PR's objectives to simplify method signatures and the codebase's capability to handlenullvalues effectively.app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/importable/NewPageImportableServiceCEImpl.java (1)
362-365: The methoddeleteUnpublishedPagehas been modified to use directnullvalues instead ofOptional.empty(). This aligns with the PR's objective to simplify method signatures by removingOptionalparameters. However, ensure that the methoddeleteUnpublishedPageandarchiveByIdWithoutPermissionare robust againstnullvalues to prevent potentialNullPointerExceptions.app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (2)
Line range hint
355-369: The methoddeleteUnpublishedActionCollectionis well-structured with clear error handling and logging. The integration of permission checks and analytics is effective, supporting the overall functionality of the method.
345-345: The methoddeleteWithoutPermissionUnpublishedActionCollectionnow correctly delegates todeleteUnpublishedActionCollectionwith anullfor permissions, aligning with the PR's objective to simplify signatures.Verification successful
We need to verify the reference to
deleteWithoutPermissionUnpublishedActionCollectioninNewActionImportableServiceCEImpl.javaand ensure that the method declaration inActionCollectionServiceCE.javamatches the new signature. Let's examine the relevant code in these files to confirm the compatibility.
The verification confirms that the method
deleteWithoutPermissionUnpublishedActionCollectioninNewActionImportableServiceCEImpl.javacorrectly uses the new signature. Additionally, the method declaration inActionCollectionServiceCE.javamatches the new signature.
NewActionImportableServiceCEImpl.java: The method call todeleteWithoutPermissionUnpublishedActionCollectioncorrectly passes a singlecollectionIdargument.ActionCollectionServiceCE.java: The interface declaration fordeleteWithoutPermissionUnpublishedActionCollectionmatches the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `deleteWithoutPermissionUnpublishedActionCollection` handle the new signature correctly. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type java $'deleteWithoutPermissionUnpublishedActionCollection'Length of output: 722
Script:
#!/bin/bash # Verify the reference in NewActionImportableServiceCEImpl.java rg --context 5 --type java $'deleteWithoutPermissionUnpublishedActionCollection' app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java # Verify the method declaration in ActionCollectionServiceCE.java rg --context 5 --type java $'deleteWithoutPermissionUnpublishedActionCollection' app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCE.javaLength of output: 1779
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java (1)
910-915: Refactor to removeOptionalparameters in method signatures aligns with PR objectives.This change simplifies the method signature by directly using
AclPermissioninstead ofOptional<AclPermission>, reducing unnecessary complexity and aligning with Java best practices for handling permissions.Also applies to: 936-939
app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java
Show resolved
Hide resolved
app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java
Show resolved
Hide resolved
app/server/appsmith-server/src/test/java/com/appsmith/server/services/NewPageServiceTest.java
Show resolved
Hide resolved
...ppsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
Show resolved
Hide resolved
...erver/appsmith-server/src/main/java/com/appsmith/server/repositories/AppsmithRepository.java
Show resolved
Hide resolved
.../src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java
Show resolved
Hide resolved
Follow up to #34281. The `.findById` method is now removed. No conflicts, but needs extra changes in [this PR](appsmithorg/appsmith-ee#4482) for build to pass on EE. **/test sanity** <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9563607766> > Commit: 02f6031 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9563607766&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Updated methods to use direct values instead of `Optional` for permissions, simplifying method signatures and improving readability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This is work on getting to remove the
.findByIdsignature withOptional<>arguments. This signature doesn't add any value, encourages confusing multi-signature service methods (check the diff here), and is causing unnecessary problems inpgbranch with generating*Cakeclasses.This PR doesn't get rid of this entirely, just one part. A follow-up PR will be opened after this is merged.
Nothing new, nothing fixed. Only a refactor.
No conflicts to EE, but needs extra changes, in this PR to be merged for the build to pass.
/test sanity
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9549476417
Commit: c1e45f2
Cypress dashboard.
Tags:
@tag.SanitySummary by CodeRabbit
Refactor
Optionalfor permission parameters across various services. Permissions are now passed directly.findByIdmethod that usesOptional<AclPermission>to improve code clarity and maintainability.Chores
Optional.empty()and replaced withnullin method calls.Optionalin multiple files.