chore: Rename base-getById where appropriate
#33253
Conversation
github-actions
bot
added
the
skip-changelog
sharat87
changed the title
getById where appropriate
Failed server tests
|
sharat87
added
ok-to-test
sharat87
added
ok-to-test
sharat87
requested review from
nidhi-nair,
sondermanish and
a team
as code owners
WalkthroughWalkthroughThe recent update introduces a new method, Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (22)
Files not reviewed due to errors (1)
Files skipped from review due to trivial changes (1)
Additional comments not posted (33)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java (1)
[!TIP]
Codebase VerificationSeveral files in the codebase still use the old function signature.
The
getByIdmethod inApplicationServiceCEImplhas been updated to include a permission check. However, there are multiple instances in test files that still use the old signature without the permission parameter.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/git/GitServiceCETest.javaPlease update these instances to match the new method signature.
Analysis chain
Line range hint
218-218: LGTM! Ensure all function calls match the new signature.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `applicationService.getById` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type java $'applicationService.getById'Length of output: 2186
Script:
#!/bin/bash # Description: Verify the implementation of `applicationService.getById` in the main service file. # Test: Search for the function definition. Expect: The new signature with permission check. rg --type java --pcre2 'public.*getById\(.*\)' --context 5Length of output: 7477
Script:
#!/bin/bash # Description: Verify the full implementation of `getById` in `ApplicationServiceCEImpl`. # Test: Extract the full method implementation. Expect: The new signature with permission check. rg --type java --pcre2 'public Mono<Application> getById\(String id\)' --context 10Length of output: 3259
| Mono<User> userData = userDataService | ||
| .getForCurrentUser() | ||
| .flatMap(userData1 -> userService.getByIdWithoutPermissionCheck(userData1.getUserId())); |
There was a problem hiding this comment.
Tip
Codebase Verification
Several files in the codebase still use the old function signature for getByIdWithoutPermissionCheck.
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/CrudService.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/BaseService.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/refactors/applications/RefactoringServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/UserDataServiceTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/CurlImporterServiceTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/TenantServiceCETest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ApplicationPageServiceTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/PageServiceTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCEImplTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCETest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java
Please update these instances to match the new function signature.
Analysis chain
LGTM! Ensure all function calls match the new signature.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `getByIdWithoutPermissionCheck` match the new signature.
# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type java $'getByIdWithoutPermissionCheck'
Length of output: 6212
Script:
#!/bin/bash
# Description: Verify all function calls to `getByIdWithoutPermissionCheck` match the new signature.
# Extract the function definitions and their usage context to verify the signature.
rg --type java -A 3 -B 3 'getByIdWithoutPermissionCheck'
Length of output: 34365
The
getByIdmethod inCrudService/BaseServicegets an item from the DB without checking for permissions. But a few services (ApplicationandWorkspace) have overridden this method to run the query with permission check. This gives the false impression that this method has a permission check for all services, which is not the case.So we're renaming the base method to
getByIdWithoutPermissionCheck, and make the overridden versions asgetById. This should make it a lot more obvious where we're querying with permissions and where we're ignoring them, and make an informed choice of when what is needed.Review tips
getByIddoes a permission check. The newgetByIdWithoutPermissionCheckdoesn't do a permission check.getByIdfor the application and workspace service were using permission check, we need to ensure that:getByIdon application service and workspace service, are untouched.getByIdon any other service are changed togetByIdWithoutPermissionCheck. Any remaining call togetByIdwould throw a compile error since we removed it fromBaseServiceanyway.EE PR at https://github.com/appsmithorg/appsmith-ee/pull/4136.
/ok-to-test tags="@tag.All"
Tip
馃煝 馃煝 馃煝 All cypress tests have passed! 馃帀 馃帀 馃帀
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9111401344
Commit: 96df2fe
Cypress dashboard url: Click here!