Skip to content

Commit

Permalink
chore: removed sanitise action functionality from actions span (#35884)
Browse files Browse the repository at this point in the history
## Description
This PR improves performance of actions in consolidated view API by:
1. Removing `sanitiseAction` method that was getting called after
fetching actions from DB
2. Moving the publishedAction filtering to the DB layer

Reasoning:
`sanitiseAction` was added earlier in the
[PR](https://github.com/appsmithorg/appsmith/pull/13263/files) because
of the [issue](#11927).
The issue mentioned for some reason JSObject actions were getting
corrupted and causing it to lose pluginId and pluginType values, since
we did not know as to why the JsObject actions were going in bad state,
we added a bandaid fix which would add pluginId and pluginType if these
actions did not have them.

RCA for above issue can be found here:
https://www.notion.so/appsmith/Data-inconsistencies-w-r-t-JS-Objects-c58f124fe20e4415b5c0e180b423b0be

For our consolidated view api -> actions span, we do not need JSObject
actions, we only need DB actions and APIs. Since we are not fetching
JSObject actions from DB, sanitisation won't be required.

As for the second part, where filtering is moved to DB layer. Earlier we
used to fetch all actions from DB and on server we would check if the
viewMode is true, then filter out published actions. This filtering has
now been moved to DB layer to avoid any further operations on service
layer.


Fixes #35857 
_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.Datasource"

### 🔍 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/10573445222>
> Commit: 2e4f36d
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10573445222&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Datasource`
> Spec:
> <hr>Tue, 27 Aug 2024 10:26:49 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit


- **New Features**
- Enhanced action retrieval to focus exclusively on published actions,
improving clarity and performance.

- **Bug Fixes**
- Removed unnecessary filtering conditions, broadening the set of
actions available in view mode.

- **Refactor**
- Renamed methods for better alignment with their purpose, specifically
around fetching published actions.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: “sneha122” <“[email protected]”>
  • Loading branch information
sneha122 and “sneha122” committed Aug 27, 2024
1 parent cbd7ccc commit 9da8c49
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,10 @@ public class ActionSpanCE {
public static final String GET_UNPUBLISHED_ACTION = APPSMITH_SPAN_PREFIX + "get.action.unpublished";
public static final String GET_VIEW_MODE_ACTION = APPSMITH_SPAN_PREFIX + "get.action.viewmode";
public static final String GET_ACTION_REPOSITORY_CALL = APPSMITH_SPAN_PREFIX + "get.action.repository.call";
public static final String VIEW_MODE_FILTER_ACTION = ACTIONS_VIEW_MODE_PREFIX + "filter";
public static final String VIEW_MODE_SANITISE_ACTION = ACTIONS_VIEW_MODE_PREFIX + "sanitise";
public static final String VIEW_MODE_INITIAL_ACTION = ACTIONS_VIEW_MODE_PREFIX + "initial";
public static final String VIEW_MODE_FINAL_ACTION = ACTIONS_VIEW_MODE_PREFIX + "final";
public static final String VIEW_MODE_SET_PLUGIN_ID_AND_TYPE_JS = ACTIONS_VIEW_MODE_PREFIX + "set_js";
public static final String VIEW_MODE_SET_PLUGIN_ID_AND_TYPE_ACTION = ACTIONS_VIEW_MODE_PREFIX + "set_action";
public static final String VIEW_MODE_FETCH_PLUGIN_FROM_DB = ACTIONS_VIEW_MODE_PREFIX + "plugindb";
public static final String VIEW_MODE_FETCH_ACTIONS_FROM_DB = ACTIONS_VIEW_MODE_PREFIX + "fetchactions";
public static final String VIEW_MODE_FETCH_ACTIONS_FROM_DB_QUERY = ACTIONS_VIEW_MODE_PREFIX + "actionsdb";
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,6 @@
import static com.appsmith.external.constants.spans.ActionSpan.GET_ACTION_REPOSITORY_CALL;
import static com.appsmith.external.constants.spans.ce.ActionSpanCE.VIEW_MODE_FETCH_ACTIONS_FROM_DB;
import static com.appsmith.external.constants.spans.ce.ActionSpanCE.VIEW_MODE_FETCH_PLUGIN_FROM_DB;
import static com.appsmith.external.constants.spans.ce.ActionSpanCE.VIEW_MODE_FILTER_ACTION;
import static com.appsmith.external.constants.spans.ce.ActionSpanCE.VIEW_MODE_FINAL_ACTION;
import static com.appsmith.external.constants.spans.ce.ActionSpanCE.VIEW_MODE_INITIAL_ACTION;
import static com.appsmith.external.constants.spans.ce.ActionSpanCE.VIEW_MODE_SANITISE_ACTION;
import static com.appsmith.external.constants.spans.ce.ActionSpanCE.VIEW_MODE_SET_PLUGIN_ID_AND_TYPE_ACTION;
import static com.appsmith.external.constants.spans.ce.ActionSpanCE.VIEW_MODE_SET_PLUGIN_ID_AND_TYPE_JS;
import static com.appsmith.external.helpers.AppsmithBeanUtils.copyNestedNonNullProperties;
Expand Down Expand Up @@ -741,15 +737,8 @@ public Flux<NewAction> findAllByApplicationIdAndPluginType(
Sort sort,
List<String> excludedPluginTypes) {
return repository
.findByApplicationIdAndPluginType(applicationId, excludedPluginTypes, permission, sort)
.findPublishedActionsByApplicationIdAndPluginType(applicationId, excludedPluginTypes, permission, sort)
.name(VIEW_MODE_FETCH_ACTIONS_FROM_DB)
.tap(Micrometer.observation(observationRegistry))
// In case of view mode being true, filter out all the actions which haven't been published
.flatMap(action -> this.filterAction(action, viewMode))
.name(VIEW_MODE_FILTER_ACTION)
.tap(Micrometer.observation(observationRegistry))
.flatMap(this::sanitizeAction)
.name(VIEW_MODE_SANITISE_ACTION)
.tap(Micrometer.observation(observationRegistry));
}

Expand Down Expand Up @@ -800,12 +789,7 @@ public Flux<ActionViewDTO> getActionsForViewMode(String applicationId) {
// No need to sort the results
return findAllByApplicationIdAndPluginType(
applicationId, true, actionPermission.getExecutePermission(), null, excludedPluginTypes)
.name(VIEW_MODE_INITIAL_ACTION)
.tap(Micrometer.observation(observationRegistry))
.filter(newAction -> !PluginType.JS.equals(newAction.getPluginType()))
.map(action -> generateActionViewDTO(action, action.getPublishedAction(), true))
.name(VIEW_MODE_FINAL_ACTION)
.tap(Micrometer.observation(observationRegistry));
.map(action -> generateActionViewDTO(action, action.getPublishedAction(), true));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Flux<NewAction> findAllActionsByNameAndPageIdsAndViewMode(

Flux<NewAction> findByApplicationId(String applicationId, AclPermission aclPermission, Sort sort);

Flux<NewAction> findByApplicationIdAndPluginType(
Flux<NewAction> findPublishedActionsByApplicationIdAndPluginType(
String applicationId, List<String> pluginTypes, AclPermission aclPermission, Sort sort);

Flux<NewAction> findByApplicationId(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,22 +203,25 @@ protected BridgeQuery<NewAction> getCriterionForFindByApplicationId(String appli
}

@Override
public Flux<NewAction> findByApplicationIdAndPluginType(
public Flux<NewAction> findPublishedActionsByApplicationIdAndPluginType(
String applicationId, List<String> excludedPluginTypes, AclPermission aclPermission, Sort sort) {
return queryBuilder()
.criteria(getCriterionForFindByApplicationIdAndPluginType(applicationId, excludedPluginTypes))
.criteria(getCriterionForFindPublishedActionsByApplicationIdAndPluginType(
applicationId, excludedPluginTypes))
.permission(aclPermission)
.sort(sort)
.all();
}

protected BridgeQuery<NewAction> getCriterionForFindByApplicationIdAndPluginType(
protected BridgeQuery<NewAction> getCriterionForFindPublishedActionsByApplicationIdAndPluginType(
String applicationId, List<String> excludedPluginTypes) {
final BridgeQuery<NewAction> q = getCriterionForFindByApplicationId(applicationId);
q.and(Bridge.or(
Bridge.notIn(NewAction.Fields.pluginType, excludedPluginTypes),
Bridge.isNull(NewAction.Fields.pluginType)));

q.isNotNull(NewAction.Fields.publishedAction_pageId);

return q;
}

Expand Down

0 comments on commit 9da8c49

Please sign in to comment.