Do PoC on module action entering into action execution flow (#24645) #2
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
21 file(s) reviewed, 25 comment(s)
Edit PR Review Bot Settings | Greptile
| @JsonView(Views.Public.class) | ||
| private String publicActionId; | ||
|
|
||
| @Transient |
There was a problem hiding this comment.
style: Several fields (packageId through pluginId) lack private access modifier - should be explicitly marked as private for better encapsulation
| @JsonView(Views.Public.class) | ||
| Boolean executeOnLoad; | ||
|
|
||
| @JsonView(Views.Public.class) | ||
| Boolean clientSideExecution; |
There was a problem hiding this comment.
logic: Boolean fields executeOnLoad and clientSideExecution should be primitive boolean to avoid potential null pointer exceptions since no default values are set
| @JsonView(Views.Public.class) | ||
| public Datasource getDatasource() { | ||
| if (this.datasource == null) { | ||
| this.datasource = new Datasource(); | ||
| this.datasource.setIsAutoGenerated(true); | ||
| } | ||
| return datasource; | ||
| } |
There was a problem hiding this comment.
logic: getDatasource() creates a new Datasource object if null - this mutates state during a getter which violates principle of least surprise
| public void sanitiseToExportDBObject() { | ||
| this.setDefaultResources(null); | ||
| this.setCacheResponse(null); | ||
| if (this.getDatasource() != null) { | ||
| this.getDatasource().setCreatedAt(null); | ||
| this.getDatasource().setDatasourceStorages(null); | ||
| } | ||
| if (this.getUserPermissions() != null) { | ||
| this.getUserPermissions().clear(); | ||
| } | ||
| if (this.getPolicies() != null) { | ||
| this.getPolicies().clear(); | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: sanitiseToExportDBObject() method calls getDatasource() which may create a new object just to check if it's null - should access field directly
| import com.appsmith.server.constants.Url; | ||
| import com.appsmith.server.controllers.ce.ActionControllerCE; | ||
| import com.appsmith.server.controllers.ce.ModuleControllerCE; | ||
| import com.appsmith.server.repositories.CustomModuleRepository; | ||
| import com.appsmith.server.repositories.ModuleRepository; | ||
| import com.appsmith.server.services.LayoutActionService; | ||
| import com.appsmith.server.services.ModuleService; | ||
| import com.appsmith.server.services.NewActionService; | ||
| import com.appsmith.server.solutions.ActionExecutionSolution; | ||
| import com.appsmith.server.solutions.RefactoringSolution; |
There was a problem hiding this comment.
style: Remove unused imports: Url, ActionControllerCE, CustomModuleRepository, NewActionService, ActionExecutionSolution, RefactoringSolution
| import reactor.core.publisher.Mono; | ||
|
|
||
| public interface ModuleServiceCE extends CrudService<Module, String> { | ||
| Mono<ModuleDTO> createModule(ModuleDTO moduleDTO); |
| moduleDTO.setId(module.getId()); | ||
| moduleDTO.setPublicActionId(moduleAction.getId()); |
There was a problem hiding this comment.
logic: Using moduleAction.getId() instead of savedAction.getId() may cause inconsistency since moduleAction hasn't been saved yet
| return moduleRepository.save(module) | ||
| .flatMap(savedModule -> { | ||
| moduleAction.setModuleId(savedModule.getId()); | ||
| return Mono.just(moduleAction); | ||
| }).flatMap(mAction -> { | ||
| return newActionService.save(mAction) | ||
| .flatMap(savedAction-> { | ||
| module.setPublicActionId(savedAction.getId()); | ||
| moduleDTO.setId(module.getId()); | ||
| moduleDTO.setPublicActionId(moduleAction.getId()); | ||
| return moduleRepository.save(module).thenReturn(moduleDTO); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
style: Consider using Mono.zip() to combine the parallel save operations and reduce sequential database calls
| for(DefaultEdge e : graph.edgeSet()){ | ||
| System.out.println(graph.getEdgeSource(e) + " --> " + graph.getEdgeTarget(e)); | ||
| } |
There was a problem hiding this comment.
style: Debug logging should use proper logger instead of System.out.println to follow logging best practices and enable log level control
| int level = bfsIterator.getDepth(vertex); | ||
| System.out.println(String.format("%s : Level %s", vertex, level)); |
There was a problem hiding this comment.
style: Debug logging should use proper logger instead of System.out.println to follow logging best practices and enable log level control
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
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=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Greptile Summary
This PR introduces module functionality to support action execution flow in Appsmith, with significant architectural changes across multiple layers.
Moduledomain model and correspondingModuleDTOwith proper MongoDB mapping and JSON serializationModuleRepository,CustomModuleRepositoryand implementations)ActionDTOandNewActionto includemoduleIdfield but missing export/import handling