Skip to content

base changes to populate system info to executeActionDTO #37091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nsarupr merged 4 commits into from
Oct 31, 2024
Merged

base changes to populate system info to executeActionDTO #37091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4955c00
base changes to populate system info to executeActionDTO
nsarupr Oct 25, 2024
79849be
update the constructor for ActionExecutionSolutionImpl
nsarupr Oct 25, 2024
6f02386
Merge branch 'release' into chore/refactor/ce-ee-populate-system-info
nsarupr Oct 31, 2024
2f5ab4b
user spybean instead of mockbean
nsarupr Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...smith-server/src/main/java/com/appsmith/server/helpers/ActionExecutionSolutionHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package com.appsmith.server.helpers;

import com.appsmith.server.helpers.ce.ActionExecutionSolutionHelperCE;

public interface ActionExecutionSolutionHelper extends ActionExecutionSolutionHelperCE {}
8 changes: 8 additions & 0 deletions ...h-server/src/main/java/com/appsmith/server/helpers/ActionExecutionSolutionHelperImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package com.appsmith.server.helpers;

import com.appsmith.server.helpers.ce.ActionExecutionSolutionHelperCEImpl;
import org.springframework.stereotype.Component;

@Component
public class ActionExecutionSolutionHelperImpl extends ActionExecutionSolutionHelperCEImpl
implements ActionExecutionSolutionHelper {}
8 changes: 8 additions & 0 deletions ...-server/src/main/java/com/appsmith/server/helpers/ce/ActionExecutionSolutionHelperCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package com.appsmith.server.helpers.ce;

import com.appsmith.external.dtos.ExecuteActionDTO;
import reactor.core.publisher.Mono;

public interface ActionExecutionSolutionHelperCE {
Mono<ExecuteActionDTO> populateExecuteActionDTOWithSystemInfo(ExecuteActionDTO executeActionDTO);
}
13 changes: 13 additions & 0 deletions ...ver/src/main/java/com/appsmith/server/helpers/ce/ActionExecutionSolutionHelperCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.appsmith.server.helpers.ce;

import com.appsmith.external.dtos.ExecuteActionDTO;
import org.springframework.stereotype.Component;
import reactor.core.publisher.Mono;

@Component
public class ActionExecutionSolutionHelperCEImpl implements ActionExecutionSolutionHelperCE {
@Override
public Mono<ExecuteActionDTO> populateExecuteActionDTOWithSystemInfo(ExecuteActionDTO executeActionDTO) {
return Mono.just(executeActionDTO);
}
Comment on lines +10 to +12
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add implementation to populate system information.

The current implementation returns the DTO without populating any system information, which doesn't fulfill the PR objective of populating system info into executeActionDTO.

Consider adding system information like:

 @Override
 public Mono<ExecuteActionDTO> populateExecuteActionDTOWithSystemInfo(ExecuteActionDTO executeActionDTO) {
-    return Mono.just(executeActionDTO);
+    return Mono.just(executeActionDTO)
+        .map(dto -> {
+            // Add relevant system information
+            // Example: OS, Java version, etc.
+            return dto;
+        })
+        .switchIfEmpty(Mono.error(new IllegalArgumentException("ExecuteActionDTO cannot be null")));
 }

Committable suggestion was skipped due to low confidence.

}
7 changes: 5 additions & 2 deletions ...smith-server/src/main/java/com/appsmith/server/solutions/ActionExecutionSolutionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.appsmith.server.configurations.CommonConfig;
import com.appsmith.server.datasources.base.DatasourceService;
import com.appsmith.server.datasourcestorages.base.DatasourceStorageService;
import com.appsmith.server.helpers.ActionExecutionSolutionHelper;
import com.appsmith.server.helpers.PluginExecutorHelper;
import com.appsmith.server.newactions.base.NewActionService;
import com.appsmith.server.newpages.base.NewPageService;
Expand Down Expand Up @@ -40,7 +41,8 @@ public ActionExecutionSolutionImpl(
EnvironmentPermission environmentPermission,
ConfigService configService,
TenantService tenantService,
CommonConfig commonConfig) {
CommonConfig commonConfig,
ActionExecutionSolutionHelper actionExecutionSolutionHelper) {
super(
newActionService,
actionPermission,
Expand All @@ -60,6 +62,7 @@ public ActionExecutionSolutionImpl(
environmentPermission,
configService,
tenantService,
commonConfig);
commonConfig,
actionExecutionSolutionHelper);
}
}
38 changes: 28 additions & 10 deletions ...-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.appsmith.server.dtos.ExecuteActionMetaDTO;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
import com.appsmith.server.helpers.ActionExecutionSolutionHelper;
import com.appsmith.server.helpers.DatasourceAnalyticsUtils;
import com.appsmith.server.helpers.DateUtils;
import com.appsmith.server.helpers.PluginExecutorHelper;
Expand Down Expand Up @@ -119,6 +120,7 @@ public class ActionExecutionSolutionCEImpl implements ActionExecutionSolutionCE
private final EnvironmentPermission environmentPermission;
private final ConfigService configService;
private final TenantService tenantService;
private final ActionExecutionSolutionHelper actionExecutionSolutionHelper;
private final CommonConfig commonConfig;

static final String PARAM_KEY_REGEX = "^k\\d+$";
Expand Down Expand Up @@ -147,7 +149,8 @@ public ActionExecutionSolutionCEImpl(
EnvironmentPermission environmentPermission,
ConfigService configService,
TenantService tenantService,
CommonConfig commonConfig) {
CommonConfig commonConfig,
ActionExecutionSolutionHelper actionExecutionSolutionHelper) {
this.newActionService = newActionService;
this.actionPermission = actionPermission;
this.observationRegistry = observationRegistry;
Expand All @@ -167,6 +170,7 @@ public ActionExecutionSolutionCEImpl(
this.configService = configService;
this.tenantService = tenantService;
this.commonConfig = commonConfig;
this.actionExecutionSolutionHelper = actionExecutionSolutionHelper;

this.patternList.add(Pattern.compile(PARAM_KEY_REGEX));
this.patternList.add(Pattern.compile(BLOB_KEY_REGEX));
Expand Down Expand Up @@ -245,15 +249,29 @@ private Mono<ExecuteActionDTO> populateExecuteActionDTO(ExecuteActionDTO execute
Mono<String> instanceIdMono = configService.getInstanceId();
Mono<String> defaultTenantIdMono = tenantService.getDefaultTenantId();

return Mono.zip(instanceIdMono, defaultTenantIdMono).map(tuple -> {
String instanceId = tuple.getT1();
String tenantId = tuple.getT2();
executeActionDTO.setActionId(newAction.getId());
executeActionDTO.setWorkspaceId(newAction.getWorkspaceId());
executeActionDTO.setInstanceId(instanceId);
executeActionDTO.setTenantId(tenantId);
return executeActionDTO;
});
Mono<ExecuteActionDTO> systemInfoPopulatedExecuteActionDTOMono =
actionExecutionSolutionHelper.populateExecuteActionDTOWithSystemInfo(executeActionDTO);

return systemInfoPopulatedExecuteActionDTOMono.flatMap(
populatedExecuteActionDTO -> Mono.zip(instanceIdMono, defaultTenantIdMono)
.map(tuple -> {
String instanceId = tuple.getT1();
String tenantId = tuple.getT2();
populatedExecuteActionDTO.setActionId(newAction.getId());
populatedExecuteActionDTO.setWorkspaceId(newAction.getWorkspaceId());
populatedExecuteActionDTO.setInstanceId(instanceId);
populatedExecuteActionDTO.setTenantId(tenantId);
return populatedExecuteActionDTO;
}));
}

/**
* Populates the requestParams with logged in userId.
* If the user is not logged in, set the parameter as anonymousUserId
*
*/
protected Mono<ExecuteActionDTO> populateExecuteActionDTOWithUserId(ExecuteActionDTO executeActionDTO) {
return Mono.just(executeActionDTO);
Comment on lines +268 to +274
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Implementation doesn't match method's documented behavior.

The method is documented to populate the userId but currently returns the DTO unmodified. This could lead to unexpected behavior.

Consider implementing the documented behavior:

protected Mono<ExecuteActionDTO> populateExecuteActionDTOWithUserId(ExecuteActionDTO executeActionDTO) {
-    return Mono.just(executeActionDTO);
+    return sessionUserService.getCurrentUser()
+            .map(user -> {
+                executeActionDTO.setUserId(user.getId());
+                return executeActionDTO;
+            })
+            .defaultIfEmpty(executeActionDTO);
}

Committable suggestion was skipped due to low confidence.

}

/**
Expand Down
7 changes: 6 additions & 1 deletion ...ver/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import com.appsmith.server.domains.NewAction;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
import com.appsmith.server.helpers.ActionExecutionSolutionHelper;
import com.appsmith.server.helpers.PluginExecutorHelper;
import com.appsmith.server.newactions.base.NewActionService;
import com.appsmith.server.newpages.base.NewPageService;
Expand Down Expand Up @@ -132,6 +133,9 @@ class ActionExecutionSolutionCEImplTest {
@SpyBean
CommonConfig commonConfig;

@SpyBean
ActionExecutionSolutionHelper actionExecutionSolutionHelper;

@Autowired
EnvironmentPermission environmentPermission;

Expand Down Expand Up @@ -162,7 +166,8 @@ public void beforeEach() {
environmentPermission,
configService,
tenantService,
commonConfig);
commonConfig,
actionExecutionSolutionHelper);

ObservationRegistry.ObservationConfig mockObservationConfig =
Mockito.mock(ObservationRegistry.ObservationConfig.class);
Expand Down