chore: removed redundant page DB calls from js action creation flow #37668
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Integrations Pod General
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12003842608. |
![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
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java (1)
1183-1183: LGTM! Consider documenting the new parameter.The addition of the null parameter aligns with the PR objective to optimize page fetch time. However, the purpose of this parameter could be clearer.
Consider adding a comment explaining the purpose of the null parameter in the context of page fetch optimization.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/clonepage/ActionClonePageServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java(1 hunks)
🔇 Additional comments (9)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (5)
341-342: Correct permission assignment based on action type.
345-350: Efficient reuse of newPage to eliminate redundant database calls.
354-356: Overloaded createAction method enhances performance by accepting newPage.
360-363: Optimizing method calls by passing newPage.
Line range hint 420-438: Streamlined page retrieval in validateAndGenerateActionDomainBasedOnContext.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCE.java (2)
5-5: LGTM!
Clean import addition following the existing import organization pattern.
27-28: Verify implementation of the new parameter across all service implementations
The interface change aligns with the optimization goal of reducing redundant page fetches. However, ensure that implementing classes are making effective use of the newPage parameter rather than passing null, which would defeat the optimization purpose.
✅ Verification successful
The previous pattern didn't work well with ast-grep, so I'm using ripgrep with more specific patterns to:
- Find actual implementations of the createAction method with the new parameter
- Find classes that implement or extend LayoutActionService
This will help us verify how the new parameter is being used across implementations.
Now let's examine the actual implementation to see how the newPage parameter is being used in both the CE implementation and the main implementation.
Let's try a simpler pattern to see the method implementations and how they use the newPage parameter.
Implementation correctly utilizes the newPage parameter
The implementation in LayoutActionServiceCEImpl properly uses the newPage parameter in the validateAndGenerateActionDomainBasedOnContext call, and there's no evidence of null being passed. The parameter flows through the service hierarchy as intended, supporting the optimization goal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations passing null for the newPage parameter
ast-grep --pattern 'createAction($_, $_, $_, null)'
# Find all implementations of this interface
ast-grep --pattern 'class $_ implements $$$LayoutActionServiceCE'
Length of output: 121
Script:
#!/bin/bash
# Search for implementations of createAction method
rg -U "createAction\(.*?\bActionDTO\b.*?\bAppsmithEventContext\b.*?\bBoolean\b.*?\bNewPage\b.*?\)" -g "*.java" -A 5
# Find all classes that implement or extend LayoutActionService
rg "class\s+\w+(?:CE|Impl)?\s+(?:implements|extends)\s+.*?LayoutActionService.*?" -g "*.java"
Length of output: 594
Script:
#!/bin/bash
# Look for the implementation of createAction in LayoutActionServiceCEImpl
rg -U "public\s+Mono<ActionDTO>\s+createAction\([^)]+\)\s*\{[\s\S]+?(?=\n\s*\})" app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
# Also check if LayoutActionServiceImpl overrides the method
rg -U "public\s+Mono<ActionDTO>\s+createAction\([^)]+\)\s*\{[\s\S]+?(?=\n\s*\})" app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutActionServiceImpl.java
Length of output: 1042
Script:
#!/bin/bash
# Look for the implementation of createAction in LayoutActionServiceCEImpl
rg "createAction.*\(" -A 20 app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
# Check if LayoutActionServiceImpl has any createAction implementation
rg "createAction.*\(" -A 20 app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutActionServiceImpl.java
Length of output: 3040
app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (2)
Line range hint 1-624: Implementation follows reactive patterns effectively.
The forking implementation properly handles async operations, maintains transaction consistency, and has good error handling. The new parameter addition is well-integrated into the existing reactive chain.
317-318: 🛠️ Refactor suggestion
Verify the null parameter usage in createAction.
The new parameter is currently set to null, which aligns with the backward compatibility pattern but may not achieve the intended optimization of reducing redundant page DB calls.
Consider passing the already fetched page object instead of null to avoid redundant DB calls, as per the PR objective.
✅ Verification successful
Let me gather more information about the createAction method implementation.
Let me try a different search pattern to find the method definition.
Let me check the interface definition to understand the expected parameter type.
The null parameter for NewPage is correct in this context
The createAction method in LayoutActionServiceCE accepts NewPage as its fourth parameter, which is optional (can be null) based on the implementation pattern seen across the codebase. In the application forking flow, passing null is appropriate since we're not creating the action in the context of a new page.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other createAction calls that might need updating
ast-grep --pattern 'layoutActionService.createAction($_, $_, $_, $_)'
# Check the implementation of createAction
ast-grep --pattern 'createAction(ActionDTO $_, AppsmithEventContext $_, Boolean $_, $_)'
Length of output: 2107
Script:
#!/bin/bash
# Search for the createAction method definition in the LayoutActionService
ast-grep --pattern 'createAction(ActionDTO $_, AppsmithEventContext $_, Boolean $_, Page $_)'
# Search for any references to this method in interfaces or parent classes
rg "createAction.*Page\s+page\)" -A 5
Length of output: 135
Script:
#!/bin/bash
# Search for createAction method in LayoutActionService with broader pattern
rg "createAction.*ActionDTO.*Boolean" -A 10
# Also check for the interface definition
fd -e java LayoutActionService
Length of output: 2340
Script:
#!/bin/bash
# Check the interface definition for createAction
cat app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCE.java
# Also check NewPage usage
rg "class NewPage" -A 5
Length of output: 8380
| @@ -54,7 +54,7 @@ public Mono<Void> cloneEntities(ClonePageMetaDTO clonePageMetaDTO) { | |||
| // Indicates that source of action creation is clone page action | |||
| cloneActionDTO.setSource(ActionCreationSourceTypeEnum.CLONE_PAGE); | |||
| copyNestedNonNullProperties(actionDTO, cloneActionDTO); | |||
| return layoutActionService.createAction(cloneActionDTO, eventContext, isJsAction); | |||
| return layoutActionService.createAction(cloneActionDTO, eventContext, isJsAction, null); | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Pass clonePageMetaDTO.getClonedPageDTO() instead of null
The ClonePageMetaDTO contains the cloned page via getClonedPageDTO() and we can see it's already being used in the same method to set the page ID. Pass this page object to optimize the DB calls as intended.
-return layoutActionService.createAction(cloneActionDTO, eventContext, isJsAction, null);
+return layoutActionService.createAction(cloneActionDTO, eventContext, isJsAction, clonePageMetaDTO.getClonedPageDTO());🔗 Analysis chain
Consider passing the actual page object instead of null
While the infrastructure change to accept the page parameter is in place, passing null here doesn't achieve the optimization goal of reducing redundant page fetches. The page object should be available from clonePageMetaDTO.getClonedPageDTO().
Let's verify if we can access the page object:
Consider this approach if the page object is available:
-return layoutActionService.createAction(cloneActionDTO, eventContext, isJsAction, null);
+return layoutActionService.createAction(cloneActionDTO, eventContext, isJsAction, clonePageMetaDTO.getClonedPageDTO());🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ClonedPageDTO contains the complete page object
ast-grep --pattern 'class ClonePageMetaDTO {
$$$
getClonedPageDTO() {
$$$
}
$$$
}'
Length of output: 96
Script:
#!/bin/bash
# Let's try a different approach to find the ClonePageMetaDTO class
rg -l "ClonePageMetaDTO"
# Then check the ActionClonePageServiceCEImpl class to understand the context
rg -A 5 "createAction.*cloneActionDTO.*eventContext.*isJsAction"
Length of output: 1553
Script:
#!/bin/bash
# Let's check the ClonePageMetaDTO class definition
rg -A 10 "class ClonePageMetaDTO" app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ClonePageMetaDTO.java
# And check how clonePageMetaDTO is used in the ActionClonePageServiceCEImpl
rg -B 5 -A 5 "clonePageMetaDTO" app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/clonepage/ActionClonePageServiceCEImpl.java
Length of output: 1715
|
Deploy-Preview-URL: https://ce-37668.dp.appsmith.com |
There was a problem hiding this comment.
@sneha122 can we please add a unit test to assert the removal of redundant page DB calls. Other changes look good to me.
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/CreateActionMetaDTO.java (2)
15-17: Add Javadoc for fields and consider nullable annotations.Consider adding:
- Field documentation to improve maintainability
@Nullableor@NotNullannotations to make null-safety explicitpublic class CreateActionMetaDTO { + /** Indicates if this is a JavaScript action */ + @Nullable Boolean isJsAction; + /** Context of the Appsmith event triggering this action creation */ + @Nullable AppsmithEventContext eventContext; + /** Associated page information */ + @Nullable NewPage newPage; }
1-18: Consider reordering fields for better readability.The field order could be improved by placing the most important/frequently used fields first. Consider moving
newPagebeforeeventContextsince it's central to the optimization goal of reducing page fetches.public class CreateActionMetaDTO { Boolean isJsAction; - AppsmithEventContext eventContext; NewPage newPage; + AppsmithEventContext eventContext; }app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (2)
38-39: Remove unused importThe
javax.swing.*import is not used in this server-side implementation and should be removed.-import javax.swing.*;
438-446: Consider enhancing error handling in the pageMono chainWhile the optimization is good, consider extracting the error handling into a separate method for better reusability and clarity.
- Mono<NewPage> pageMono = newPage != null - ? Mono.just(newPage) - : newPageService - .findById(action.getPageId(), aclPermission) - .name(GET_PAGE_BY_ID) - .tap(Micrometer.observation(observationRegistry)) - .switchIfEmpty(Mono.error(new AppsmithException( - AppsmithError.NO_RESOURCE_FOUND, FieldName.PAGE, action.getPageId()))) - .cache(); + Mono<NewPage> pageMono = newPage != null + ? Mono.just(newPage) + : fetchPageWithErrorHandling(action.getPageId(), aclPermission); + + private Mono<NewPage> fetchPageWithErrorHandling(String pageId, AclPermission permission) { + return newPageService + .findById(pageId, permission) + .name(GET_PAGE_BY_ID) + .tap(Micrometer.observation(observationRegistry)) + .switchIfEmpty(Mono.error(new AppsmithException( + AppsmithError.NO_RESOURCE_FOUND, FieldName.PAGE, pageId))) + .cache(); + }app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (1)
313-322: Consider adding validation for CreateActionMetaDTO fields.While the refactoring to use CreateActionMetaDTO is good for reducing redundant page fetches, consider adding null checks for the DTO fields to ensure robustness.
+ if (createActionMetaDTO.getEventContext() == null) { + createActionMetaDTO.setEventContext( + new AppsmithEventContext(AppsmithEventContextType.CLONE_PAGE) + ); + } return layoutActionService.createAction(actionDTO, createActionMetaDTO);app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (1)
930-977: LGTM: Well-structured test for page fetch optimization.The test effectively verifies that the page is fetched only once during action creation, aligning with the PR's objective to optimize database calls. The test uses Mockito verification to ensure createAction is called exactly once with the correct parameters.
A minor suggestion to improve test readability:
- public void testUpdateUnpublishedActionCollection_createSingleAction_fetchesPageFromDBOnlyOnce() { + public void testUpdateUnpublishedActionCollection_whenCreatingSingleAction_shouldFetchPageFromDBOnlyOnce() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/CreateActionMetaDTO.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/clonepage/ActionClonePageServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/clonepage/ActionClonePageServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCE.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/CreateActionMetaDTO.java (1)
10-18: LGTM! Well-structured DTO with appropriate Lombok annotations.
The class structure is clean and follows best practices with proper use of Lombok annotations for reducing boilerplate code.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (2)
357-363: LGTM: Well-structured method refactoring
The new method signature effectively implements the optimization by allowing page object reuse through CreateActionMetaDTO.
Line range hint 367-424: LGTM: Clean implementation of optimized action creation
The refactored implementation maintains all existing functionality while reducing redundant page fetches through the new DTO structure.
app/server/appsmith-server/src/main/java/com/appsmith/server/fork/internal/ApplicationForkingServiceCEImpl.java (1)
26-26: LGTM!
Clean import addition for the new DTO class.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (1)
95-95: LGTM: Appropriate use of @SpyBean for verification.
The change from @Autowired to @SpyBean is correct as it allows verifying the interaction with layoutActionService in the new test.
Failed server tests
|
Description
During action creation flow, page is fetched twice (making two independent DB calls) and taking significant time, this PR makes only 1 page DB call and passes the page object to subsequent calls, instead of fetching page again.
Fixes #37567
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.JS, @tag.Datasource"
🔍 Cypress test results
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12027675952
Commit: 2f2d957
Cypress dashboard.
Tags: @tag.JS, @tag.Datasource
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Tue, 26 Nov 2024 09:37:59 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
executeOnLoadproperty during action creation in cloning contexts.Tests