fix: moved js filter to mongoDB #35826
Conversation
WalkthroughThe changes introduce a new method for retrieving actions based on application ID and specific plugin types across multiple classes, enhancing the granularity of action retrieval. The existing methods have been updated to accommodate filtering by plugin types, improving the service's overall functionality. Additionally, new criteria-building logic has been added to facilitate these enhanced queries, while maintaining backward compatibility for existing methods. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NewActionServiceCE
participant CustomNewActionRepositoryCEImpl
Client->>NewActionServiceCE: findAllByApplicationIdAndPluginType(applicationId, pluginTypes)
NewActionServiceCE->>CustomNewActionRepositoryCEImpl: findByApplicationIdAndPluginType(applicationId, pluginTypes)
CustomNewActionRepositoryCEImpl->>CustomNewActionRepositoryCEImpl: getCriterionForFindByApplicationIdAndPluginType(applicationId, pluginTypes)
CustomNewActionRepositoryCEImpl-->>NewActionServiceCE: return actions
NewActionServiceCE-->>Client: return actions
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 Documentation and Community
|
github-actions
bot
added
Integrations Pod General
|
@sneha122 Could we please add or modify any test cases relevant to the new logic we’ve implemented? Additionally, please confirm if no new test cases are necessary in this context. |
@sagar-qa007 This wont require any test cases as the functioning would be same only performance would be improved. |
...ppsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
Show resolved
Hide resolved
sneha122
added
ok-to-test
sneha122
requested review from
NilanshBansal
and removed request for
nidhi-nair and
sondermanish
![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, codebase verification and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (1)
215-222: Consider enhancing query criteria construction.The
getCriterionForFindByApplicationIdAndPluginTypemethod is well-structured, but consider adding comments to explain the logic for including actions with specific plugin types or no associated plugin type. This will improve readability and maintainability.protected BridgeQuery<NewAction> getCriterionForFindByApplicationIdAndPluginType( String applicationId, List<String> pluginTypes) { final BridgeQuery<NewAction> q = getCriterionForFindByApplicationId(applicationId); + // Include actions with specific plugin types or those with no associated plugin type q.and(Bridge.or( Bridge.in(NewAction.Fields.pluginType, pluginTypes), Bridge.isNull(NewAction.Fields.pluginType))); return q; }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1 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/repositories/ce/CustomNewActionRepositoryCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (4 hunks)
Additional comments not posted (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java (1)
40-41: Great addition offindByApplicationIdAndPluginTypemethod!This new method enhances the repository's querying capabilities by allowing filtering based on application ID and plugin types. It's a smart move to streamline the filtering process directly in MongoDB.
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1)
81-82: Well done on addingfindAllByApplicationIdAndPluginType!This method significantly enhances the service's ability to filter actions by plugin types, which is a great step towards optimizing performance by leveraging MongoDB's capabilities.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (1)
205-213: Excellent implementation offindByApplicationIdAndPluginType!This method effectively utilizes the new query criteria to filter actions by application ID and plugin types, aligning perfectly with the performance optimization goals.
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (3)
Line range hint
737-758: Great job on enhancing the filtering logic!The modifications to include filtering by plugin types and handling view mode are well-implemented. The use of reactive streams is appropriate for this context.
764-784: Well done on maintaining backward compatibility!The overloaded method effectively adds new functionality for filtering by published state while preserving existing behavior. The use of reactive streams is consistent and efficient.
Line range hint
817-835: Excellent integration of the new filtering method!The use of
findAllByApplicationIdAndPluginTypeto fetch actions and filter out JS plugin types aligns well with the PR's performance objectives.However, ensure that the filtering logic correctly excludes JS plugin types in all relevant scenarios.
Run the following script to verify the filtering logic:
Verification successful
Filtering Logic Verified: JS Plugin Types Excluded Correctly
The
getActionsForViewModemethod inNewActionServiceCEImpl.javacorrectly implements the filtering logic to exclude JS plugin types. This aligns with the performance objectives and ensures that only the desired plugin types are processed.
- File:
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java- Method:
getActionsForViewMode- Logic: Excludes
PluginType.JSusing a filter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that JS plugin types are correctly excluded in the filtering logic. # Test: Search for the filtering logic in the codebase. Expect: Correct exclusion of JS plugin types. rg --type java -A 5 $'filter(newAction -> !PluginType.JS.equals(newAction.getPluginType()))'Length of output: 93
Script:
#!/bin/bash # Description: Use ast-grep to find and verify the implementation of getActionsForViewMode method. # Search for the getActionsForViewMode method implementation in the codebase. ast-grep --lang java --pattern $'class $_ { $$$ List<Action> getActionsForViewMode($_) { $$$ } $$$ }'Length of output: 114
Script:
#!/bin/bash # Description: Use rg to search for the getActionsForViewMode method by name in the codebase. # Search for the getActionsForViewMode method in the codebase. rg --type java 'getActionsForViewMode'Length of output: 1700
Script:
#!/bin/bash # Description: Extract the implementation of getActionsForViewMode from NewActionServiceCEImpl.java to verify the filtering logic. # Extract lines from the implementation of getActionsForViewMode in NewActionServiceCEImpl.java rg --type java -A 20 'public Flux<ActionViewDTO> getActionsForViewMode' app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.javaLength of output: 1208
...ppsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...ppsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- 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/repositories/ce/CustomNewActionRepositoryCEImpl.java (4 hunks)
Additional comments not posted (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (2)
215-223: Well-structured query criteria!The
getCriterionForFindByApplicationIdAndPluginTypemethod efficiently constructs query criteria for filtering actions by application ID and excluded plugin types.This method enhances the flexibility and granularity of query operations.
205-213: Great addition! Verify the method usage.The
findByApplicationIdAndPluginTypemethod is well-implemented to enhance query capabilities by filtering based on plugin types.Ensure that this method is correctly integrated and used throughout the codebase.
Run the following script to verify the method usage:
Verification successful
Method usage verified and correctly integrated!
The
findByApplicationIdAndPluginTypemethod is properly implemented and used within the codebase. It is declared in the interface and utilized in theNewActionServiceCEImplclass, ensuring it adheres to the expected contract and functionality.
- Location of Usage:
NewActionServiceCEImpl.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all method calls to `findByApplicationIdAndPluginType`. # Test: Search for the method usage. Expect: Correct integration and usage. rg --type java -A 5 $'findByApplicationIdAndPluginType'Length of output: 3162
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (4)
757-763: Verify the filtering logic infindAllByApplicationIdAndViewMode.Ensure that the filtering logic correctly excludes unpublished actions when the view mode is true. This aligns with the intended functionality of the method.
Run the following script to verify the filtering logic:
797-805: Verify the exclusion logic for JS plugin types ingetActionsForViewMode.Ensure that the exclusion logic for JS plugin types is consistent with the rest of the codebase and effectively filters out these types.
Run the following script to verify the exclusion logic:
Verification successful
Exclusion Logic for JS Plugin Types Verified
The exclusion logic for JS plugin types is consistently implemented across the codebase. The use of
Bridge.notInensures that actions with the JS plugin type are effectively filtered out, aligning with the intended functionality.
CustomNewActionRepositoryCEImpl.java: UsesBridge.notInto exclude plugin types.NewActionServiceCEImpl.java: Implements exclusion logic as expected.This confirms that the exclusion logic is correctly applied.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the exclusion logic for JS plugin types. # Test: Search for the exclusion logic implementation. Expect: Consistent exclusion of JS plugin types. rg --type java 'excludedPluginTypes'Length of output: 1626
1091-1102: Verify the filtering logic for published actions infilterAction.Ensure that the filtering logic for published actions is consistently applied across the codebase, particularly when the view mode is true.
Run the following script to verify the filtering logic:
737-748: Verify the exclusion logic infindAllByApplicationIdAndPluginType.Ensure that the repository method
findByApplicationIdAndPluginTypecorrectly implements the exclusion logic for the specified plugin types. This is crucial for the performance improvements described in the PR.Run the following script to verify the repository method implementation:
Verification successful
Exclusion logic in
findByApplicationIdAndPluginTypeis correctly implemented.The method successfully excludes specified plugin types by using a combination of
Bridge.notInandBridge.isNullconditions. This ensures that actions with excluded plugin types or null plugin types are filtered out as expected.
- Implementation is found in
CustomNewActionRepositoryCEImpl.java.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the exclusion logic in the repository method. # Test: Search for the repository method implementation. Expect: Correct exclusion logic. rg --type java 'findByApplicationIdAndPluginType'Length of output: 597
Script:
#!/bin/bash # Description: Extract the implementation of the repository method to verify exclusion logic. # Extract the implementation of `findByApplicationIdAndPluginType` from the relevant file. ast-grep --lang java --pattern 'public Flux<NewAction> findByApplicationIdAndPluginType($_, $_, $_, $_) { $$$ }' app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.javaLength of output: 730
Script:
#!/bin/bash # Description: Extract the implementation of the repository method to verify exclusion logic. # Use a simplified pattern to extract the method implementation. ast-grep --lang java --pattern 'findByApplicationIdAndPluginType($_, $_, $_, $_)' app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java -A 20Length of output: 203
Script:
#!/bin/bash # Description: Use rg to search for the method implementation with more context. # Search for the method implementation and include additional context lines. rg 'findByApplicationIdAndPluginType' app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java -A 30Length of output: 1598
Description
This PR adds an improvement to the actions part of the consolidated view API, where by it moves the filtering that we do to check if the action of pluginType JSObject has been moved to MongoDB. This way the mongoDB query that we do contains pluginTypes to include and we won't have to do additional filtering at the code layer, there by saving some time that is spend on further filtering and sanitising each action.
Here are some of the observations for before and after making this change:
Case 1:
Before (Total consolidated view api duration ~ 1.72s)
Actions span totally took ~ 1.12s
After (Total consolidated view api duration ~ 1.72s)
Actions span totally took ~ 566ms
Case 2:
Before (Total consolidated view api duration ~ 2.66s)
Actions span totally took ~ 1.67s
After (Total consolidated view api duration ~ 2.88s)
Actions span totally took ~ 935ms
Note: These numbers are obtained by testing the app with many actions and js objects [Xolair App]
Fixes #
Issue Numberor
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.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10522148136
Commit: f0cf9b2
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Fri, 23 Aug 2024 08:34:57 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Chores