chore: perf optimisation for js action creation phase 1 #37391
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
Failed server tests
|
1 similar comment
Failed server tests
|
github-actions
bot
added
Integrations Pod General
Failed server tests
|
… to retain the previous logic we had
Failed server tests
|
nidhi-nair
reviewed
Nov 15, 2024
...mith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java
Outdated
Show resolved
Hide resolved
![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/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (1)
870-928: LGTM! Consider extracting common test setup.The test effectively validates handling of multiple actions with duplicate names. Consider extracting the common setup code (action collection creation, mock setup) into a helper method to improve maintainability.
+ private ActionCollectionDTO createTestActionCollection(String name, String actionName) { + ActionCollectionDTO actionCollectionDTO = new ActionCollectionDTO(); + actionCollectionDTO.setName(name); + actionCollectionDTO.setPageId(testPage.getId()); + actionCollectionDTO.setApplicationId(testApp.getId()); + actionCollectionDTO.setWorkspaceId(workspaceId); + actionCollectionDTO.setPluginId(datasource.getPluginId()); + actionCollectionDTO.setVariables(List.of(new JSValue("test", "String", "test", true))); + actionCollectionDTO.setBody("collectionBody"); + actionCollectionDTO.setPluginType(PluginType.JS); + + ActionDTO action = new ActionDTO(); + action.setName(actionName); + action.setActionConfiguration(new ActionConfiguration()); + action.getActionConfiguration().setBody("initial body"); + action.getActionConfiguration().setIsValid(false); + actionCollectionDTO.setActions(List.of(action)); + + return actionCollectionDTO; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java(3 hunks)
🔇 Additional comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (1)
439-446: Verify client-side validation for JS action names
While bypassing server-side validation for JS actions improves performance, we need to ensure that client-side validation is robust.
Consider adding debug logs before the conditional check:
+ log.debug("Validating action name. Action type: {}, Name: {}", isJsAction ? "JS" : "Non-JS", name);
if (!isJsAction) {
return refactoringService
.isNameAllowed(page.getId(), contextType, layout.getId(), name)
.name(IS_NAME_ALLOWED)
.tap(Micrometer.observation(observationRegistry));
}
+ log.debug("Skipping name validation for JS action: {}", name);
return Mono.just(true);app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (2)
318-325: LGTM! Efficient approach for duplicate name validation.
The stream-based implementation efficiently validates unique action names in memory before any DB operations.
346-358: Consider adding transaction boundary for concurrent action creation.
While the duplicate name check works for single requests, there's a potential race condition when multiple requests attempt to create actions with the same name simultaneously. Consider wrapping the name check and action creation in a transaction.
Let's verify if there are any existing transaction boundaries:
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (1)
810-868: LGTM! Well-structured test for duplicate action name validation.
The test effectively validates the error handling when attempting to create a new action with the same name as an existing one.
sneha122
added
ok-to-test
6 tasks
github-actions bot
pushed a commit
to Zeral-Zhang/appsmith
that referenced
this pull request
Nov 20, 2024
…37391) ## Description [During analysis of action creation flow metrics](appsmithorg#37151 (comment)), we observed that RefactoringService.isNameAllowed is taking 80-90% of the total JS object action time. This PR optimises this part in a way that for any jsobject action, instead of fetching all actions from DB and comparing it to see if current action name is allowed, we simply do that check in memory where for current action collection, if any action names are being duplicated, we throw the error. We could make this change easily because recently we merged a [PR](appsmithorg#36958) which removes the actions with duplicate name from client payload whenever Js object update API is called, with this change, we can guarantee that for any JS object update call, all actions inside it will always have unique names. This PR makes the similar check on backend where if any action has duplicate name within collection, we throw an error and don't store that action in the DB. We may need to consider following test case in both before and after implementation of this approach. This can be covered during PR testing: What happens if the client sends multiple requests to add a new function in an existing collection. That is, as a result of the debounce logic, if the server receives 2 consecutive requests with a populated collection but without actionId associated to either request. Relevant thread: https://theappsmith.slack.com/archives/C040LHZN03V/p1731571364933089 Fixes appsmithorg#37365 _or_ Fixes `Issue URL` > [!WARNING] > _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" ### 🔍 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/11911295324> > Commit: d5c75ed > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11911295324&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS` > Spec: > <hr>Tue, 19 Nov 2024 11:14:16 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced validation to prevent the creation of actions with duplicate names in action collections. - Simplified handling of JavaScript actions, allowing them to bypass certain validation checks. - **Bug Fixes** - Improved error handling during action updates and collection modifications to ensure better logging and management of failures. - **Tests** - Added tests to verify that duplicate action names trigger appropriate error messages, enhancing the robustness of the action collection feature. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“[email protected]”>
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
During analysis of action creation flow metrics, we observed that RefactoringService.isNameAllowed is taking 80-90% of the total JS object action time. This PR optimises this part in a way that for any jsobject action, instead of fetching all actions from DB and comparing it
to see if current action name is allowed, we simply do that check in memory where for current action collection, if any action names are being duplicated, we throw the error.
We could make this change easily because recently we merged a PR which removes the actions with duplicate name from client payload whenever Js object update API is called, with this change, we can guarantee that for any JS object update call, all actions inside it will always have unique names. This PR makes the similar check on backend where if any action has duplicate name within collection, we throw an error and don't store that action in the DB.
We may need to consider following test case in both before and after implementation of this approach. This can be covered during PR testing:
What happens if the client sends multiple requests to add a new function in an existing collection. That is, as a result of the debounce logic, if the server receives 2 consecutive requests with a populated collection but without actionId associated to either request.
Relevant thread: https://theappsmith.slack.com/archives/C040LHZN03V/p1731571364933089
Fixes #37365
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"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11911295324
Commit: d5c75ed
Cypress dashboard.
Tags:
@tag.JSSpec:
Tue, 19 Nov 2024 11:14:16 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests