Skip to content
Merged
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 @@ -13,6 +13,8 @@ default String getBaseId() {

String getName();

void setName(String artifactName);

String getWorkspaceId();

Boolean getExportWithConfiguration();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2187,13 +2187,17 @@ public Mono<? extends Artifact> deleteBranch(String baseArtifactId, String branc
.onErrorResume(throwable -> {
log.error("Delete branch failed {}", throwable.getMessage());
if (throwable instanceof CannotDeleteCurrentBranchException) {
return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"delete branch",
"Cannot delete current checked out branch"));
return releaseFileLock(baseArtifactId)
.then(Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"delete branch",
"Cannot delete current checked out branch")));
}
return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED, "delete branch", throwable.getMessage()));
return releaseFileLock(baseArtifactId)
.then(Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"delete branch",
throwable.getMessage())));
})
.flatMap(isBranchDeleted ->
releaseFileLock(baseArtifactId).map(status -> isBranchDeleted))
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package com.appsmith.server.git.templates.contexts;

import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.dtos.ArtifactExchangeJson;
import com.appsmith.server.git.ArtifactBuilderExtension;
import org.junit.jupiter.api.extension.Extension;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.ParameterContext;
import org.junit.jupiter.api.extension.ParameterResolutionException;
import org.junit.jupiter.api.extension.ParameterResolver;
import org.junit.jupiter.api.extension.TestTemplateInvocationContext;

import java.util.List;

public class GitContext implements TestTemplateInvocationContext, ParameterResolver {

private final String fileName;

private final Class<? extends ArtifactExchangeJson> artifactExchangeJsonType;
private final ArtifactType artifactType;

public GitContext(
ExtensionContext extensionContext, String fileName, Class<? extends ArtifactExchangeJson> artifactExchangeJsonType, ArtifactType artifactType) {
this.artifactType = artifactType;
ExtensionContext.Store contextStore = extensionContext.getStore(ExtensionContext.Namespace.create(ArtifactBuilderExtension.class));
contextStore.put(ArtifactExchangeJson.class, artifactExchangeJsonType);
contextStore.put("filePath", fileName);
this.fileName = fileName;
this.artifactExchangeJsonType = artifactExchangeJsonType;
}
Comment on lines +22 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add parameter validation in constructor

Consider adding null checks and fileName validation to prevent issues during test execution.

 public GitContext(
     ExtensionContext extensionContext, String fileName, Class<? extends ArtifactExchangeJson> artifactExchangeJsonType, ArtifactType artifactType) {
+    if (extensionContext == null || fileName == null || artifactExchangeJsonType == null || artifactType == null) {
+        throw new IllegalArgumentException("All parameters must be non-null");
+    }
+    if (fileName.trim().isEmpty()) {
+        throw new IllegalArgumentException("fileName must not be empty");
+    }
     this.artifactType = artifactType;
     ExtensionContext.Store contextStore = extensionContext.getStore(ExtensionContext.Namespace.create(ArtifactBuilderExtension.class));
     contextStore.put(ArtifactExchangeJson.class, artifactExchangeJsonType);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public GitContext(
ExtensionContext extensionContext, String fileName, Class<? extends ArtifactExchangeJson> artifactExchangeJsonType, ArtifactType artifactType) {
this.artifactType = artifactType;
ExtensionContext.Store contextStore = extensionContext.getStore(ExtensionContext.Namespace.create(ArtifactBuilderExtension.class));
contextStore.put(ArtifactExchangeJson.class, artifactExchangeJsonType);
contextStore.put("filePath", fileName);
this.fileName = fileName;
this.artifactExchangeJsonType = artifactExchangeJsonType;
}
public GitContext(
ExtensionContext extensionContext, String fileName, Class<? extends ArtifactExchangeJson> artifactExchangeJsonType, ArtifactType artifactType) {
if (extensionContext == null || fileName == null || artifactExchangeJsonType == null || artifactType == null) {
throw new IllegalArgumentException("All parameters must be non-null");
}
if (fileName.trim().isEmpty()) {
throw new IllegalArgumentException("fileName must not be empty");
}
this.artifactType = artifactType;
ExtensionContext.Store contextStore = extensionContext.getStore(ExtensionContext.Namespace.create(ArtifactBuilderExtension.class));
contextStore.put(ArtifactExchangeJson.class, artifactExchangeJsonType);
contextStore.put("filePath", fileName);
this.fileName = fileName;
this.artifactExchangeJsonType = artifactExchangeJsonType;
}


@Override
public String getDisplayName(int invocationIndex) {
return fileName;
}

@Override
public List<Extension> getAdditionalExtensions() {
return List.of(this);
}

public String getFileName() {
return fileName;
}

public Class<? extends ArtifactExchangeJson> getArtifactExchangeJsonType() {
return artifactExchangeJsonType;
}

@Override
public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
throws ParameterResolutionException {
return true;
}

@Override
public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
throws ParameterResolutionException {
if (parameterContext.getParameter().getType().equals(ExtensionContext.class)) {
return extensionContext;
}

return this;
}
Comment on lines +50 to +64
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Restrict parameter resolution to supported types

The current implementation of supportsParameter is too permissive. It should only return true for types that can actually be resolved.

 @Override
 public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
     throws ParameterResolutionException {
-    return true;
+    Class<?> type = parameterContext.getParameter().getType();
+    return type.equals(ExtensionContext.class) || type.equals(GitContext.class);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Override
public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
throws ParameterResolutionException {
return true;
}
@Override
public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
throws ParameterResolutionException {
if (parameterContext.getParameter().getType().equals(ExtensionContext.class)) {
return extensionContext;
}
return this;
}
@Override
public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
throws ParameterResolutionException {
Class<?> type = parameterContext.getParameter().getType();
return type.equals(ExtensionContext.class) || type.equals(GitContext.class);
}
@Override
public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
throws ParameterResolutionException {
if (parameterContext.getParameter().getType().equals(ExtensionContext.class)) {
return extensionContext;
}
return this;
}


public ArtifactType getArtifactType() {
return artifactType;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.appsmith.server.git.templates.providers;

import com.appsmith.server.git.templates.providers.ce.GitBranchesTestTemplateProviderCE;
import org.springframework.stereotype.Component;

@Component
public class GitBranchesTestTemplateProvider extends GitBranchesTestTemplateProviderCE {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package com.appsmith.server.git.templates.providers.ce;

import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.dtos.ApplicationJson;
import com.appsmith.server.git.templates.contexts.GitContext;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.TestTemplateInvocationContext;
import org.junit.jupiter.api.extension.TestTemplateInvocationContextProvider;

import java.util.stream.Stream;

import static org.assertj.core.api.Assertions.assertThat;

public class GitBranchesTestTemplateProviderCE implements TestTemplateInvocationContextProvider {

@Override
public boolean supportsTestTemplate(ExtensionContext extensionContext) {
return true;
}

@Override
public Stream<TestTemplateInvocationContext> provideTestTemplateInvocationContexts(
ExtensionContext extensionContext) {
GitContext context = new GitContext(
extensionContext,
"com/appsmith/server/git/application.json",
ApplicationJson.class,
ArtifactType.APPLICATION);
return Stream.of(context);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package com.appsmith.server.git;

import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.dtos.ArtifactExchangeJson;
import org.junit.jupiter.api.extension.ExtensionContext;

public interface ArtifactBuilderContext extends ExtensionContext {

ArtifactType getArtifactType();

Class<? extends ArtifactExchangeJson> getArtifactJsonType();

String getArtifactJsonPath();

String getWorkspaceId();

void setWorkspaceId(String workspaceId);

String getArtifactId();

void setArtifactId(String artifactId);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
package com.appsmith.server.git;

import com.appsmith.server.applications.base.ApplicationService;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.domains.User;
import com.appsmith.server.domains.Workspace;
import com.appsmith.server.dtos.ArtifactExchangeJson;
import com.appsmith.server.imports.internal.ImportService;
import com.appsmith.server.migrations.JsonSchemaMigration;
import com.appsmith.server.services.ApplicationPageService;
import com.appsmith.server.services.UserService;
import com.appsmith.server.services.WorkspaceService;
import com.appsmith.server.solutions.ApplicationPermission;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.extension.AfterEachCallback;
import org.junit.jupiter.api.extension.BeforeEachCallback;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.core.io.ClassPathResource;
import org.springframework.stereotype.Component;
import reactor.core.publisher.Mono;

import java.io.IOException;
import java.nio.charset.Charset;
import java.util.UUID;

import static org.assertj.core.api.Assertions.assertThat;

/**
* This extension basically just creates a new workspace and initializes the artifact provided in context
* This artifact is provided in the form of a JSON file that is specified in the context itself
*/
@Component
public class ArtifactBuilderExtension implements AfterEachCallback, BeforeEachCallback {

@Autowired
UserService userService;

@Autowired
WorkspaceService workspaceService;

@Autowired
ImportService importService;

@Autowired
ObjectMapper objectMapper;

@Autowired
JsonSchemaMigration jsonSchemaMigration;

@Autowired
ApplicationService applicationService;

@Autowired
ApplicationPermission applicationPermission;

@Autowired
ApplicationPageService applicationPageService;

@Override
public void beforeEach(ExtensionContext extensionContext) throws Exception {
ExtensionContext.Store parentContextStore = extensionContext.getParent().get().getStore(ExtensionContext.Namespace.create(ArtifactBuilderExtension.class));
Class<? extends ArtifactExchangeJson> aClass = parentContextStore.get(ArtifactExchangeJson.class, Class.class);
String filePath = parentContextStore.get("filePath", String.class);
ExtensionContext.Store contextStore = extensionContext.getStore(ExtensionContext.Namespace.create(ArtifactBuilderExtension.class));

ArtifactExchangeJson artifactExchangeJson = createArtifactJson(filePath, aClass).block();
assertThat(artifactExchangeJson).isNotNull();

artifactExchangeJson.getArtifact().setName(aClass.getSimpleName() + "_" + UUID.randomUUID());

User apiUser = userService.findByEmail("api_user").block();
Workspace toCreate = new Workspace();
toCreate.setName("Workspace_" + UUID.randomUUID());
Workspace workspace =
workspaceService.create(toCreate, apiUser, Boolean.FALSE).block();
assertThat(workspace).isNotNull();

Artifact artifact = importService.importNewArtifactInWorkspaceFromJson(workspace.getId(), artifactExchangeJson).block();
assertThat(artifact).isNotNull();

contextStore.put(FieldName.WORKSPACE_ID, (workspace.getId()));
contextStore.put(FieldName.ARTIFACT_ID, (artifact.getId()));
}


@Override
public void afterEach(ExtensionContext extensionContext) {

ExtensionContext.Store contextStore = extensionContext.getStore(ExtensionContext.Namespace.create(ArtifactBuilderExtension.class));
String workspaceId = contextStore.get(FieldName.WORKSPACE_ID, String.class);

// Because right now we only have checks for apps
// Move this to artifact based model when we fix that
applicationService
.findByWorkspaceId(workspaceId, applicationPermission.getDeletePermission())
.flatMap(remainingApplication -> applicationPageService.deleteApplication(remainingApplication.getId()))
.collectList()
.block();
workspaceService.archiveById(workspaceId).block();

}

private Mono<? extends ArtifactExchangeJson> createArtifactJson(String filePath, Class<? extends ArtifactExchangeJson> exchangeJsonType) throws IOException {

ClassPathResource classPathResource = new ClassPathResource(filePath);

String artifactJson = classPathResource.getContentAsString(Charset.defaultCharset());

ArtifactExchangeJson artifactExchangeJson =
objectMapper.copy().disable(MapperFeature.USE_ANNOTATIONS).readValue(artifactJson, exchangeJsonType);

return jsonSchemaMigration.migrateArtifactExchangeJsonToLatestSchema(artifactExchangeJson, null, null);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package com.appsmith.server.git;

import com.appsmith.external.constants.PluginConstants;
import com.appsmith.external.models.ActionConfiguration;
import com.appsmith.external.models.ActionDTO;
import com.appsmith.external.models.Datasource;
import com.appsmith.external.models.PluginType;
import com.appsmith.server.domains.Application;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.domains.Plugin;
import com.appsmith.server.plugins.base.PluginService;
import com.appsmith.server.services.LayoutActionService;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpMethod;
import org.springframework.stereotype.Component;
import reactor.core.publisher.Mono;

import java.util.UUID;

@Component
public class GitArtifactTestUtils<T extends Artifact> {

@Autowired
LayoutActionService layoutActionService;
@Autowired
PluginService pluginService;

Mono<Void> createADiff(Artifact artifact) {

Application application = (Application) artifact;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add type safety check before casting

The unchecked cast could lead to runtime errors if a non-Application artifact is passed.

-        Application application = (Application) artifact;
+        if (!(artifact instanceof Application)) {
+            return Mono.error(new IllegalArgumentException("Artifact must be an Application"));
+        }
+        Application application = (Application) artifact;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Application application = (Application) artifact;
if (!(artifact instanceof Application)) {
return Mono.error(new IllegalArgumentException("Artifact must be an Application"));
}
Application application = (Application) artifact;


String pageId = application.getPages().get(0).getId();
Plugin plugin = pluginService.findByPackageName("restapi-plugin").block();
Comment on lines +32 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add null checks and avoid blocking calls

The code assumes the application has pages and uses a blocking call to find the plugin.

-        String pageId = application.getPages().get(0).getId();
-        Plugin plugin = pluginService.findByPackageName("restapi-plugin").block();
+        if (application.getPages() == null || application.getPages().isEmpty()) {
+            return Mono.error(new IllegalStateException("Application must have at least one page"));
+        }
+        String pageId = application.getPages().get(0).getId();
+        return pluginService.findByPackageName("restapi-plugin")
+            .switchIfEmpty(Mono.error(new IllegalStateException("REST API plugin not found")))
+            .flatMap(plugin -> {

Committable suggestion skipped: line range outside the PR's diff.


Datasource datasource = new Datasource();
datasource.setName(PluginConstants.DEFAULT_REST_DATASOURCE);
datasource.setWorkspaceId(application.getWorkspaceId());
datasource.setPluginId(plugin.getId());

ActionDTO action = new ActionDTO();
action.setPluginType(PluginType.API);
action.setName("aGetAction_" + UUID.randomUUID());
action.setDatasource(datasource);
action.setActionConfiguration(new ActionConfiguration());
action.getActionConfiguration().setHttpMethod(HttpMethod.GET);
action.setPageId(pageId);

return layoutActionService
.createSingleAction(action, Boolean.FALSE)
.then();
}
}
Loading