Skip to content
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.

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

Do PoC on module action entering into action execution flow (#24645) #2

Open
wants to merge 1 commit into
base: release
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ public class ActionDTO implements Identifiable {
@JsonView(Views.Public.class)
String applicationId;

@Transient
@JsonView(Views.Public.class)
String moduleId;

@Transient
@JsonView(Views.Public.class)
String workspaceId;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
package com.appsmith.external.models;

import com.appsmith.external.exceptions.ErrorDTO;
import com.appsmith.external.helpers.Identifiable;
import com.appsmith.external.views.Views;
import com.fasterxml.jackson.annotation.JsonFormat;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonView;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.Setter;
import lombok.ToString;
import org.springframework.data.annotation.Transient;

import java.time.Instant;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

@Getter
@Setter
@NoArgsConstructor
@ToString
public class ModuleDTO implements Identifiable {

@Transient
@JsonView(Views.Public.class)
private String id;

@Transient
@JsonView(Views.Public.class)
private String publicActionId;

@Transient
Copy link

Choose a reason for hiding this comment

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

style: Several fields (packageId through pluginId) lack private access modifier - should be explicitly marked as private for better encapsulation

@JsonView(Views.Public.class)
String packageId;

@Transient
@JsonView(Views.Public.class)
String workspaceId;

@Transient
@JsonView(Views.Public.class)
PluginType pluginType;

// name of the plugin. used to log analytics events where pluginName is a required attribute
// It'll be null if not set
@Transient
@JsonView(Views.Public.class)
String pluginName;

@Transient
@JsonView(Views.Public.class)
String pluginId;

@JsonView(Views.Public.class)
String name;

// The FQN for an action will also include any collection it is a part of as collectionName.actionName
@JsonView(Views.Public.class)
String fullyQualifiedName;

@JsonView(Views.Public.class)
Datasource datasource;

@JsonView(Views.Public.class)
String pageId;

@JsonView(Views.Public.class)
ActionConfiguration actionConfiguration;

//this attribute carries error messages while processing the actionCollection
@JsonProperty(access = JsonProperty.Access.READ_ONLY)
@Transient
@JsonView(Views.Public.class)
List<ErrorDTO> errorReports;

@JsonView(Views.Public.class)
Boolean executeOnLoad;

@JsonView(Views.Public.class)
Boolean clientSideExecution;
Comment on lines +78 to +82
Copy link

Choose a reason for hiding this comment

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

logic: Boolean fields executeOnLoad and clientSideExecution should be primitive boolean to avoid potential null pointer exceptions since no default values are set


/*
* This is a list of fields specified by the client to signify which fields have dynamic bindings in them.
* TODO: The server can use this field to simplify our Mustache substitutions in the future
*/
@JsonView(Views.Public.class)
List<Property> dynamicBindingPathList;

@JsonProperty(access = JsonProperty.Access.READ_ONLY)
@JsonView(Views.Public.class)
Boolean isValid;

@JsonProperty(access = JsonProperty.Access.READ_ONLY)
@JsonView(Views.Public.class)
Set<String> invalids;

@Transient
@JsonProperty(access = JsonProperty.Access.READ_ONLY)
@JsonView(Views.Public.class)
Set<String> messages = new HashSet<>();


// This is a list of keys that the client whose values the client needs to send during action execution.
// These are the Mustache keys that the server will replace before invoking the API
@JsonProperty(access = JsonProperty.Access.READ_ONLY)
@JsonView(Views.Public.class)
Set<String> jsonPathKeys;

@JsonView(Views.Internal.class)
String cacheResponse;

@Transient
@JsonView(Views.Public.class)
String templateId; //If action is created via a template, store the id here.

@Transient
@JsonView(Views.Public.class)
String providerId; //If action is created via a template, store the template's provider id here.

@Transient
@JsonView(Views.Public.class)
ActionProvider provider;

@JsonView(Views.Internal.class)
Boolean userSetOnLoad = false;

@JsonView(Views.Public.class)
Boolean confirmBeforeExecute = false;

@Transient
@JsonView(Views.Public.class)
Documentation documentation;

@JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss'Z'", timezone = "UTC")
@JsonView(Views.Public.class)
Instant deletedAt = null;

@Deprecated
@JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss'Z'", timezone = "UTC")
@JsonView(Views.Public.class)
Instant archivedAt = null;

@Transient
@JsonView(Views.Internal.class)
protected Set<Policy> policies = new HashSet<>();

@Transient
@JsonView(Views.Public.class)
public Set<String> userPermissions = new HashSet<>();

// This field will be used to store the default/root actionId and applicationId for actions generated for git
// connected applications and will be used to connect actions across the branches
@JsonView(Views.Internal.class)
DefaultResources defaultResources;

@JsonView(Views.Internal.class)
protected Instant createdAt;

@JsonView(Views.Internal.class)
protected Instant updatedAt;

/**
* If the Datasource is null, create one and set the autoGenerated flag to true. This is required because spring-data
* cannot add the createdAt and updatedAt properties for null embedded objects. At this juncture, we couldn't find
* a way to disable the auditing for nested objects.
*
* @return
*/
@JsonView(Views.Public.class)
public Datasource getDatasource() {
if (this.datasource == null) {
this.datasource = new Datasource();
this.datasource.setIsAutoGenerated(true);
}
return datasource;
}
Comment on lines +171 to +178
Copy link

Choose a reason for hiding this comment

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

logic: getDatasource() creates a new Datasource object if null - this mutates state during a getter which violates principle of least surprise


@JsonView(Views.Public.class)
public String getValidName() {
if (this.fullyQualifiedName == null) {
return this.name;
} else {
return this.fullyQualifiedName;
}
}

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();
}
}
Comment on lines +189 to +202
Copy link

Choose a reason for hiding this comment

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

logic: sanitiseToExportDBObject() method calls getDatasource() which may create a new object just to check if it's null - should access field directly

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.appsmith.server.controllers;

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;
Comment on lines +3 to +12
Copy link

Choose a reason for hiding this comment

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

style: Remove unused imports: Url, ActionControllerCE, CustomModuleRepository, NewActionService, ActionExecutionSolution, RefactoringSolution

import lombok.extern.slf4j.Slf4j;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

@RestController
@RequestMapping("/api/v1/modules")
@Slf4j
public class ModuleController extends ModuleControllerCE {

public ModuleController(ModuleService moduleService,
ModuleRepository moduleRepository,
LayoutActionService layoutActionService) {
Comment on lines +22 to +24
Copy link

Choose a reason for hiding this comment

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

style: Fix inconsistent indentation in constructor parameters - align all parameters to same level

super(moduleService, moduleRepository, layoutActionService);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package com.appsmith.server.controllers.ce;

import com.appsmith.external.helpers.AppsmithEventContext;
import com.appsmith.external.helpers.AppsmithEventContextType;
import com.appsmith.external.models.ActionDTO;
import com.appsmith.external.models.ModuleDTO;
import com.appsmith.external.views.Views;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.dtos.ResponseDTO;
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.fasterxml.jackson.annotation.JsonView;
import jakarta.validation.Valid;
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.web.bind.annotation.*;
import org.springframework.web.server.ServerWebExchange;
import reactor.core.publisher.Mono;

@Slf4j
@RequestMapping("/api/v1/modules")
public class ModuleControllerCE {

private final ModuleService moduleService;
private final ModuleRepository moduleRepository;
Copy link

Choose a reason for hiding this comment

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

logic: moduleRepository is injected but never used in this class

private final LayoutActionService layoutActionService;

@Autowired
public ModuleControllerCE(ModuleService moduleService,
ModuleRepository moduleRepository,
LayoutActionService layoutActionService) {
this.moduleService = moduleService;
this.moduleRepository = moduleRepository;
this.layoutActionService = layoutActionService;
}

@JsonView(Views.Public.class)
@PostMapping
@ResponseStatus(HttpStatus.CREATED)
public Mono<ResponseDTO<ModuleDTO>> createModule(@Valid @RequestBody ModuleDTO resource,
@RequestHeader(name = FieldName.BRANCH_NAME, required = false) String branchName,
@RequestHeader(name = "Origin", required = false) String originHeader,
ServerWebExchange exchange) {
log.debug("Going to create resource {}", resource.getClass().getName());
return moduleService.createModule(resource)
.map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null));
Comment on lines +48 to +49
Copy link

Choose a reason for hiding this comment

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

style: missing error handling - should use onErrorResume to handle exceptions and return appropriate error responses

}

@JsonView(Views.Public.class)
@PostMapping("/action")
@ResponseStatus(HttpStatus.CREATED)
public Mono<ResponseDTO<ActionDTO>> createAction(@Valid @RequestBody ActionDTO resource,
@RequestHeader(name = FieldName.BRANCH_NAME, required = false) String branchName,
@RequestHeader(name = "Origin", required = false) String originHeader,
ServerWebExchange exchange) {
log.debug("Going to create resource {}", resource.getClass().getName());
return layoutActionService.createAction(resource, new AppsmithEventContext(AppsmithEventContextType.DEFAULT), false)
.map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null));
Comment on lines +60 to +61
Copy link

Choose a reason for hiding this comment

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

style: consider adding error handling for action creation failures

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package com.appsmith.server.domains;

import com.appsmith.external.models.ActionDTO;
Copy link

Choose a reason for hiding this comment

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

logic: ActionDTO is imported but never used in this class

import com.appsmith.external.models.BranchAwareDomain;
import com.appsmith.external.models.Documentation;
import com.appsmith.external.models.PluginType;
import com.appsmith.external.views.Views;
import com.fasterxml.jackson.annotation.JsonView;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.Setter;
import lombok.ToString;
import org.springframework.data.mongodb.core.mapping.Document;

@Getter
@Setter
@ToString
@NoArgsConstructor
@Document
public class Module extends BranchAwareDomain {
Copy link

Choose a reason for hiding this comment

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

style: Consider adding @EntityListeners(AuditingEntityListener.class) for automatic auditing of created/modified timestamps


// Fields in action that are not allowed to change between published and unpublished versions
@JsonView(Views.Public.class)
String packageId;

@JsonView(Views.Public.class)
String workspaceId;

@JsonView(Views.Public.class)
PluginType pluginType;

@JsonView(Views.Public.class)
String pluginId;

Comment on lines +24 to +34
Copy link

Choose a reason for hiding this comment

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

style: All String fields should be marked with @NotNull or @nullable to explicitly define nullability constraints

@JsonView(Views.Public.class)
String templateId; //If action is created via a template, store the id here.

@JsonView(Views.Public.class)
Documentation documentation; // Documentation for the template using which this action was created

// Action specific fields that are allowed to change between published and unpublished versions
@JsonView(Views.Public.class)
String publicActionId;
Comment on lines +42 to +43
Copy link

Choose a reason for hiding this comment

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

style: publicActionId field comment is missing to explain its purpose and usage



}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ public class NewAction extends BranchAwareDomain {
@JsonView(Views.Public.class)
String applicationId;

@JsonView(Views.Public.class)
String moduleId;
Comment on lines +26 to +27
Copy link

Choose a reason for hiding this comment

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

style: Add documentation comments explaining the purpose and usage of the moduleId field

//Organizations migrated to workspaces, kept the field as deprecated to support the old migration
@Deprecated
@JsonView(Views.Public.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.appsmith.server.domains.ActionCollection;
import com.appsmith.server.domains.Application;
import com.appsmith.server.domains.CustomJSLib;
import com.appsmith.server.domains.Module;
import com.appsmith.server.domains.NewAction;
import com.appsmith.server.domains.NewPage;
import com.appsmith.server.domains.Theme;
Expand Down Expand Up @@ -110,4 +111,6 @@ public class ApplicationJson {
@Deprecated
@JsonView(Views.Public.class)
Map<String, Set<String>> unpublishedLayoutmongoEscapedWidgets;

List<Module> modules;
Copy link

Choose a reason for hiding this comment

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

logic: Missing @JSONVIEW annotation - this field needs appropriate view annotations to be included in serialization, likely should be {Views.Public.class, Views.Export.class} like other core application components

Copy link

Choose a reason for hiding this comment

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

style: Missing documentation comment explaining the purpose and usage of the modules field

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.appsmith.server.repositories;

import com.appsmith.server.repositories.ce.CustomModuleRepositoryCE;

public interface CustomModuleRepository extends CustomModuleRepositoryCE {

}
Loading