Skip to content

Commit

Permalink
fix: Reduce RTS calls in the updateLayout flow (appsmithorg#37127)
Browse files Browse the repository at this point in the history 
## Description

In this PR, we update the code to send all the bindings to RTS together
instead of calling it for each property binding. It was observed that
the calls were earlier around 40k for the test application.

This change leads to optimisation of performance for 
- updateLayout call ( when updating anything in the DSL ) 
- query/API action update 
- JS action update

Fixes appsmithorg#37055



## Tests on DP 

Deploy-Preview-URL: https://ce-37127.dp.appsmith.com/

- [x] Multiple widgets with the same bindings impacting the sequence of
onPageLoadActions
- onPageLoadActions update as expected when
   - [x] DSL is updated
   - [x] Query is updated
   - [x] JS Object is updated

EE PR - https://github.com/appsmithorg/appsmith-ee/pull/5642 

## Automation

/ok-to-test tags="@tag.All"

### 🔍 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/12082854290>
> Commit: 07b68d4
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12082854290&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.All`
> Spec:
> <hr>Fri, 29 Nov 2024 11:44:31 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

## Summary by CodeRabbit

- **New Features**
- Enhanced retrieval of possible entity references and their
relationships with a new method.
- Added new test cases to validate action execution on page load and
dynamic binding handling.

- **Bug Fixes**
	- Improved error handling and validation for executable references.
	- Enhanced assertions to ensure correct execution of layout actions.

- **Tests**
- Added a test to validate layout actions with multiple widgets
referencing the same action.
	- Refactored existing tests for better readability and maintainability.
- Expanded test coverage for layout updates and dynamic binding
scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: “sneha122” <“[email protected]”>
  • Loading branch information
Rishabh Rathod and “sneha122” authored Nov 29, 2024
1 parent fbc4e8a commit 155ea6a
Show file tree
Hide file tree
Showing 4 changed files with 313 additions and 124 deletions.
1 change: 0 additions & 1 deletion appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OnLoadSpanCE.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ public class OnLoadSpanCE {
APPSMITH_SPAN_PREFIX + "getAllExecutablesByCreatorIdFlux";
public static final String EXECUTABLE_NAME_TO_EXECUTABLE_MAP =
APPSMITH_SPAN_PREFIX + "executableNameToExecutableMap";
public static final String EXECUTABLE_IN_CREATOR_CONTEXT = APPSMITH_SPAN_PREFIX + "executablesInCreatorContext";
public static final String ADD_DIRECTLY_REFERENCED_EXECUTABLES_TO_GRAPH =
APPSMITH_SPAN_PREFIX + "addDirectlyReferencedExecutablesToGraph";
public static final String UPDATE_EXECUTABLE_SELF_REFERENCING_PATHS =
Expand Down
140 changes: 113 additions & 27 deletions ...server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -524,9 +524,8 @@ private Mono<Set<EntityDependencyNode>> getPossibleEntityReferences(
Set<EntityDependencyNode> bindingsInDsl) {
// We want to be finding both type of references
final int entityTypes = EXECUTABLE_ENTITY_REFERENCES | WIDGET_ENTITY_REFERENCES;

return executableNameToExecutableMono
.zipWith(getPossibleEntityParentsMap(bindings, entityTypes, evalVersion))
.zipWith(getPossibleEntityParentsMap(new ArrayList<>(bindings), entityTypes, evalVersion))
.map(tuple -> {
Map<String, Executable> executableMap = tuple.getT1();
// For each binding, here we receive a set of possible references to global entities
Expand Down Expand Up @@ -584,6 +583,79 @@ private Mono<Set<EntityDependencyNode>> getPossibleEntityReferences(
});
}

private Mono<Map<String, Set<EntityDependencyNode>>> getPossibleEntityReferencesMap(
Mono<Map<String, Executable>> executableNameToExecutableMono,
List<String> bindings,
int evalVersion,
Set<EntityDependencyNode> bindingsInDsl) {
// We want to be finding both type of references
final int entityTypes = EXECUTABLE_ENTITY_REFERENCES | WIDGET_ENTITY_REFERENCES;

return executableNameToExecutableMono
.zipWith(getPossibleEntityParentsMap(bindings, entityTypes, evalVersion))
.map(tuple -> {
Map<String, Executable> executableMap = tuple.getT1();
// For each binding, here we receive a set of possible references to global entities
// At this point we're guaranteed that these references are made to possible variables,
// but we do not know if those entities exist in the global namespace yet
Map<String, Set<EntityDependencyNode>> bindingToPossibleParentMap = tuple.getT2();

Map<String, Set<EntityDependencyNode>> possibleEntitiesReferencesToBindingMap = new HashMap<>();

// From these references, we will try to validate executable references at this point
// Each identified node is already annotated with the expected type of entity we need to search for
bindingToPossibleParentMap.entrySet().stream().forEach(entry -> {
Set<EntityDependencyNode> bindingsWithExecutableReference = new HashSet<>();
String binding = entry.getKey();
Set<EntityDependencyNode> possibleEntitiesReferences = new HashSet<>();
entry.getValue().stream().forEach(possibleParent -> {
// For each possible reference node, check if the reference was to an executable
Executable executable = executableMap.get(possibleParent.getValidEntityName());

if (executable != null) {
// If it was, and had been identified as the same type of executable as what exists in
// this app,
if (possibleParent
.getEntityReferenceType()
.equals(executable.getEntityReferenceType())) {
// Copy over some data from the identified executable, this ensures that we do not
// have
// to query the DB again later
possibleParent.setExecutable(executable);
bindingsWithExecutableReference.add(possibleParent);
// Only if this is not a direct JS function call,
// add it to a possible on page load executable call.
// This discards the following type:
// {{ JSObject1.func() }}
if (!TRUE.equals(possibleParent.getIsFunctionCall())) {
possibleEntitiesReferences.add(possibleParent);
}
// We're ignoring any reference that was identified as a widget but actually matched
// an executable
// We wouldn't have discarded JS collection names here, but this is just an
// optimization, so it's fine
}
} else {
// If the reference node was identified as a widget, directly add it as a possible
// reference
// Because we are not doing any validations for widget references at this point
if (EntityReferenceType.WIDGET.equals(possibleParent.getEntityReferenceType())) {
possibleEntitiesReferences.add(possibleParent);
}
}

possibleEntitiesReferencesToBindingMap.put(binding, possibleEntitiesReferences);
});

if (!bindingsWithExecutableReference.isEmpty() && bindingsInDsl != null) {
bindingsInDsl.addAll(bindingsWithExecutableReference);
}
});

return possibleEntitiesReferencesToBindingMap;
});
}

/**
* This method is an abstraction that queries the ast service for possible global references as string values,
* and then uses the mustache helper utility to classify these global references into possible types of EntityDependencyNodes
Expand All @@ -594,9 +666,9 @@ private Mono<Set<EntityDependencyNode>> getPossibleEntityReferences(
* @return A mono of a map of each of the provided binding values to the possible set of EntityDependencyNodes found in the binding
*/
private Mono<Map<String, Set<EntityDependencyNode>>> getPossibleEntityParentsMap(
Set<String> bindings, int types, int evalVersion) {
List<String> bindings, int types, int evalVersion) {
Flux<Tuple2<String, Set<String>>> findingToReferencesFlux =
astService.getPossibleReferencesFromDynamicBinding(new ArrayList<>(bindings), evalVersion);
astService.getPossibleReferencesFromDynamicBinding(bindings, evalVersion);
return MustacheHelper.getPossibleEntityParentsMap(findingToReferencesFlux, types);
}

Expand Down Expand Up @@ -627,31 +699,45 @@ private Mono<Set<ExecutableDependencyEdge>> addDirectlyReferencedExecutablesToGr
Mono<Map<String, Executable>> executableNameToExecutableMapMono,
Set<EntityDependencyNode> executableBindingsInDslRef,
int evalVersion) {
return Flux.fromIterable(widgetDynamicBindingsMap.entrySet())
.flatMap(entry -> {
String widgetName = entry.getKey();
// For each widget in the DSL that has a dynamic binding,
// we define an entity dependency node beforehand
// This will be a leaf node in the DAG that is constructed for on page load dependencies
EntityDependencyNode widgetDependencyNode =
new EntityDependencyNode(EntityReferenceType.WIDGET, widgetName, widgetName, null, null);
Set<String> bindingsInWidget = entry.getValue();
return getPossibleEntityReferences(
executableNameToExecutableMapMono,
bindingsInWidget,
evalVersion,
executableBindingsInDslRef)
.flatMapMany(Flux::fromIterable)
// Add dependencies of the executables found in the DSL in the graph
// We are ignoring the widget references at this point
// TODO: Possible optimization in the future
.flatMap(possibleEntity -> {

Map<String, Set<EntityDependencyNode>> bindingToWidgetNodesMap = new HashMap<>();
List<String> allBindings = new ArrayList<>();

widgetDynamicBindingsMap.forEach((widgetName, bindingsInWidget) -> {
EntityDependencyNode widgetDependencyNode =
new EntityDependencyNode(EntityReferenceType.WIDGET, widgetName, widgetName, null, null);

bindingsInWidget.forEach(binding -> {
bindingToWidgetNodesMap
.computeIfAbsent(binding, bindingKey -> new HashSet<>())
.add(widgetDependencyNode);
allBindings.add(binding);
});
});

Mono<Map<String, Set<EntityDependencyNode>>> bindingToPossibleEntityMapMono = getPossibleEntityReferencesMap(
executableNameToExecutableMapMono, allBindings, evalVersion, executableBindingsInDslRef);

return bindingToPossibleEntityMapMono
.flatMapMany(bindingToPossibleEntityMap -> Flux.fromIterable(bindingToPossibleEntityMap.entrySet()))
.flatMap(bindingEntry -> {
String binding = bindingEntry.getKey();
Set<EntityDependencyNode> possibleEntities = bindingEntry.getValue();

// Get all widget nodes associated with the binding
Set<EntityDependencyNode> widgetDependencyNodes =
bindingToWidgetNodesMap.getOrDefault(binding, Set.of());

// Process each possibleEntity for the current binding
return Flux.fromIterable(possibleEntities).flatMap(possibleEntity -> Flux.fromIterable(
widgetDependencyNodes) // Iterate all associated widgets
.flatMap(widgetDependencyNode -> {
if (getExecutableTypes().contains(possibleEntity.getEntityReferenceType())) {
edgesRef.add(new ExecutableDependencyEdge(possibleEntity, widgetDependencyNode));
// This executable is directly referenced in the DSL. This executable is an ideal
// candidate
// for on page load
// candidate for on page load
executablesUsedInDSLRef.add(possibleEntity.getValidEntityName());

return updateExecutableSelfReferencingPaths(possibleEntity)
.name(UPDATE_EXECUTABLE_SELF_REFERENCING_PATHS)
.tap(Micrometer.observation(observationRegistry))
Expand All @@ -668,7 +754,7 @@ private Mono<Set<ExecutableDependencyEdge>> addDirectlyReferencedExecutablesToGr
.thenReturn(possibleEntity);
}
return Mono.just(possibleEntity);
});
}));
})
.collectList()
.thenReturn(edgesRef);
Expand Down Expand Up @@ -1134,7 +1220,7 @@ private Mono<Set<ExecutableDependencyEdge>> addWidgetRelationshipToGraph(
// This part will ensure that we are discovering widget to widget relationships.
return Flux.fromIterable(widgetBindingMap.entrySet())
.flatMap(widgetBindingEntries -> getPossibleEntityParentsMap(
widgetBindingEntries.getValue(), entityTypes, evalVersion)
new ArrayList<>(widgetBindingEntries.getValue()), entityTypes, evalVersion)
.map(possibleParentsMap -> {
possibleParentsMap.entrySet().stream().forEach(entry -> {
if (entry.getValue() == null || entry.getValue().isEmpty()) {
Expand Down
Loading

0 comments on commit 155ea6a

Please sign in to comment.