From 0260c095c3e073eb82740a520d005e3ea863b666 Mon Sep 17 00:00:00 2001 From: Manish Kumar <107841575+sondermanish@users.noreply.github.com> Date: Fri, 11 Apr 2025 21:06:10 +0530 Subject: [PATCH] fix: fix for discard and repository not found exceptions (#40228) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 #40213 #40216 ## Automation /ok-to-test tags="@tag.Git, @tag.ImportExport" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 97e5bcbf8152ba14629c1ad299430eb4a529931a > Cypress dashboard. > Tags: `@tag.Git, @tag.ImportExport` > Spec: >
Fri, 11 Apr 2025 15:31:32 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit - **Bug Fixes** - File processing now selectively reads only JSON formatted content, ensuring consistent resource identification. - Branch handling during repository operations has been streamlined to eliminate duplicates and improve error reporting. - Enhanced JSON parsing with robust error handling to prevent unexpected disruptions and improve overall stability. --- .../appsmith/git/files/FileUtilsCEImpl.java | 7 +- .../git/central/CentralGitServiceCEImpl.java | 77 +++++++++++-------- .../helpers/ce/CommonGitFileUtilsCE.java | 12 ++- 3 files changed, 59 insertions(+), 37 deletions(-) diff --git a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java index 8503257247..9e50cac97a 100644 --- a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java +++ b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java @@ -885,7 +885,12 @@ public class FileUtilsCEImpl implements FileInterface { protected Tuple2 getGitResourceIdentity(Path baseRepoPath, String filePath) { Path path = baseRepoPath.resolve(filePath); GitResourceIdentity identity; - Object contents = fileOperations.readFile(path); + Object contents = null; + + if (filePath.endsWith(JSON_EXTENSION)) { + contents = fileOperations.readFile(path); + } + if (!filePath.contains("/")) { identity = new GitResourceIdentity(GitResourceType.ROOT_CONFIG, filePath, filePath); } else if (filePath.matches(DATASOURCE_DIRECTORY + "/.*")) { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java index 6786886795..77b0d82d42 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java @@ -80,6 +80,7 @@ import java.io.IOException; import java.time.Instant; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -2537,9 +2538,15 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE { .fetchRemoteRepository(gitConnectDTO, gitAuth, baseArtifact, baseGitMetadata.getRepoName()) .flatMap(defaultBranch -> gitHandlingService.listReferences(jsonTransformationDTO, true)) .flatMap(refDTOs -> { - List branchesToCheckout = new ArrayList<>(); - List gitRefDTOs = new ArrayList<>(); + ArtifactType artifactType = baseArtifact.getArtifactType(); + String workspaceId = baseArtifact.getWorkspaceId(); + String baseArtifactId = baseGitMetadata.getDefaultArtifactId(); + String repoName = baseGitMetadata.getRepoName(); + ArtifactJsonTransformationDTO branchCheckoutDTO = + new ArtifactJsonTransformationDTO(workspaceId, baseArtifactId, repoName, artifactType); + + Set branchesToCheckout = new HashSet<>(); for (GitRefDTO gitRefDTO : refDTOs) { if (!gitRefDTO.getRefName().startsWith(ORIGIN)) { continue; @@ -2547,44 +2554,46 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE { // remove `origin/` prefix from the remote branch name String branchName = gitRefDTO.getRefName().replace(ORIGIN, REMOTE_NAME_REPLACEMENT); + branchesToCheckout.add(branchName); - // The baseArtifact is cloned already, hence no need to check out it again - if (!branchName.equals(baseGitMetadata.getBranchName())) { - branchesToCheckout.add(branchName); - } + // TODO: base artifact wouldn't be cloned if from remote the default branch is changed, + // hence we need to check it out again if that is present in local. + // For this use case we are assuming that whatever default branch name is present + // in base metadata is default branch } - ArtifactJsonTransformationDTO branchCheckoutDTO = new ArtifactJsonTransformationDTO(); - branchCheckoutDTO.setWorkspaceId(baseArtifact.getWorkspaceId()); - branchCheckoutDTO.setArtifactType(baseArtifact.getArtifactType()); - branchCheckoutDTO.setRepoName(baseGitMetadata.getRepoName()); - branchCheckoutDTO.setRefType(jsonTransformationDTO.getRefType()); + // checkout the branch locally + List gitRefDTOs = new ArrayList<>(); + return gitArtifactHelper + .getAllArtifactByBaseId(baseGitMetadata.getDefaultArtifactId(), artifactReadPermission) + .flatMap(artifact -> { + GitArtifactMetadata branchMetadata = artifact.getGitArtifactMetadata(); + if (branchMetadata == null + || RefType.tag.equals(branchMetadata.getRefType()) + || !StringUtils.hasText(branchMetadata.getRefName()) + || !branchesToCheckout.contains(branchMetadata.getRefName())) { + return Mono.just(""); + } - return Flux.fromIterable(branchesToCheckout) - .flatMap(branchName -> gitArtifactHelper - .getArtifactByBaseIdAndBranchName( - baseGitMetadata.getDefaultArtifactId(), - branchName, - artifactReadPermission) - // checkout the branch locally - .flatMap(artifact -> { - // Add the locally checked out branch to the branchList - GitRefDTO gitRefDTO = new GitRefDTO(); - gitRefDTO.setRefName(branchName); + // Add the locally checked out branch to the branchList + String branchName = branchMetadata.getRefName(); + GitRefDTO gitRefDTO = new GitRefDTO(); + gitRefDTO.setRefName(branchName); - // set the default branch flag if there's a match. - // This can happen when user has changed the default branch other - // than remote - gitRefDTO.setDefault(baseGitMetadata - .getDefaultBranchName() - .equals(branchName)); + // set the default branch flag if there's a match. + // This can happen when user has changed the default branch other + // than remote + gitRefDTO.setDefault(baseGitMetadata + .getDefaultBranchName() + .equals(branchName)); - gitRefDTOs.add(gitRefDTO); - branchCheckoutDTO.setRefName(branchName); - return gitHandlingService.checkoutRemoteReference(branchCheckoutDTO); - }) - // Return empty mono when the branched defaultArtifact is not in db - .onErrorResume(throwable -> Mono.empty())) + gitRefDTOs.add(gitRefDTO); + branchCheckoutDTO.setRefName(branchName); + branchCheckoutDTO.setRefType(RefType.branch); + return gitHandlingService.checkoutRemoteReference(branchCheckoutDTO); + }) + // Return empty mono when the branched defaultArtifact is not in db + .onErrorResume(throwable -> Mono.empty()) .then(Mono.just(gitRefDTOs)); }); }); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java index 33db75c8dc..aa78434d9f 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java @@ -39,6 +39,7 @@ import com.appsmith.server.newactions.base.NewActionService; import com.appsmith.server.services.AnalyticsService; import com.appsmith.server.services.FeatureFlagService; import com.appsmith.server.services.SessionUserService; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.MapperFeature; import com.fasterxml.jackson.databind.ObjectMapper; @@ -517,8 +518,15 @@ public class CommonGitFileUtilsCE { } if (PluginType.REMOTE.equals(newAction.getPluginType())) { - Map formData = objectMapper.convertValue(data, new TypeReference<>() {}); - actionConfiguration.setFormData(formData); + try { + Object file = objectMapper.readValue(data.toString(), Object.class); + Map formData = objectMapper.convertValue(file, new TypeReference<>() {}); + actionConfiguration.setFormData(formData); + + } catch (JsonProcessingException e) { + throw new RuntimeException(e); + } + } else if (data != null) { String body = String.valueOf(data); actionConfiguration.setBody(body);