From 1078a03b23da5606f53b80f09f5d3c8e3596f15d Mon Sep 17 00:00:00 2001 From: Manish Kumar <107841575+sondermanish@users.noreply.github.com> Date: Wed, 4 Dec 2024 10:33:25 +0530 Subject: [PATCH 1/2] feat: added commit changes (#37922) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Fixes #37437 ## Automation /ok-to-test tags="@tag.Git" ### :mag: Cypress test results > [!TIP] > 🟒 🟒 🟒 All cypress tests have passed! πŸŽ‰ πŸŽ‰ πŸŽ‰ > Workflow run: > Commit: 6dc1f5a35764f8dc602cb1b5a7a18c76be534e6e > Cypress dashboard. > Tags: `@tag.Git` > Spec: >
Wed, 04 Dec 2024 04:58:02 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced methods for acquiring and releasing Git locks, enhancing the locking mechanism. - Added `commitArtifact` method for committing artifacts in Git operations. - New method `publishArtifactPostCommit` for publishing artifacts after a commit. - **Improvements** - Enhanced error handling and parameter naming consistency across various Git-related services. - **Refactor** - Updated method signatures and added detailed documentation for clarity and maintainability. --- .../git/GitApplicationHelperCEImpl.java | 5 + .../appsmith/server/git/GitRedisUtils.java | 51 ++- .../git/central/CentralGitServiceCE.java | 4 + .../CentralGitServiceCECompatibleImpl.java | 10 +- .../git/central/CentralGitServiceCEImpl.java | 358 +++++++++++++++++- .../git/central/CentralGitServiceImpl.java | 10 +- .../git/central/GitHandlingServiceCE.java | 9 + .../dtos/ArtifactJsonTransformationDTO.java | 2 +- .../server/git/fs/GitFSServiceCEImpl.java | 265 ++++++++++++- .../services/ce/GitArtifactHelperCE.java | 2 + 10 files changed, 686 insertions(+), 30 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java index 9e5d38dfd71..e59d74b7ae6 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java @@ -317,4 +317,9 @@ public Application getNewArtifact(String workspaceId, String repoName) { newApplication.setGitApplicationMetadata(new GitArtifactMetadata()); return newApplication; } + + @Override + public Mono publishArtifactPostCommit(Artifact committedArtifact) { + return publishArtifact(committedArtifact, true); + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java index f61661f02e8..6c27759f3ea 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java @@ -23,15 +23,20 @@ public class GitRedisUtils { private final RedisUtils redisUtils; private final ObservationRegistry observationRegistry; - public Mono addFileLock(String defaultApplicationId, String commandName, Boolean isRetryAllowed) { + /** + * Adds a baseArtifact id as a key in redis, the presence of this key represents a symbolic lock, essentially meaning that no new operations + * should be performed till this key remains present. + * @param baseArtifactId : base id of the artifact for which the key is getting added. + * @param commandName : Name of the operation which is trying to acquire the lock, this value will be added against the key + * @param isRetryAllowed : Boolean for whether retries for adding the value is allowed + * @return a boolean publisher for the added file locks + */ + public Mono addFileLock(String baseArtifactId, String commandName, Boolean isRetryAllowed) { long numberOfRetries = Boolean.TRUE.equals(isRetryAllowed) ? MAX_RETRIES : 0L; - log.info( - "Git command {} is trying to acquire the lock for application id {}", - commandName, - defaultApplicationId); + log.info("Git command {} is trying to acquire the lock for application id {}", commandName, baseArtifactId); return redisUtils - .addFileLock(defaultApplicationId, commandName) + .addFileLock(baseArtifactId, commandName) .retryWhen(Retry.fixedDelay(numberOfRetries, RETRY_DELAY) .onRetryExhaustedThrow((retryBackoffSpec, retrySignal) -> { if (retrySignal.failure() instanceof AppsmithException) { @@ -54,4 +59,38 @@ public Mono releaseFileLock(String defaultApplicationId) { .name(GitSpan.RELEASE_FILE_LOCK) .tap(Micrometer.observation(observationRegistry)); } + + /** + * This is a wrapper method for acquiring git lock, since multiple ops are used in sequence + * for a complete composite operation not all ops require to acquire the lock hence a dummy flag is sent back for + * operations in that is getting executed in between + * @param baseArtifactId : id of the base artifact for which ops would be locked + * @param isLockRequired : is lock really required or is it a proxy function + * @return : Boolean for whether the lock is acquired + */ + // TODO @Manish add artifactType reference in incoming prs. + public Mono acquireGitLock(String baseArtifactId, String commandName, boolean isLockRequired) { + if (!Boolean.TRUE.equals(isLockRequired)) { + return Mono.just(Boolean.TRUE); + } + + return addFileLock(baseArtifactId, commandName); + } + + /** + * This is a wrapper method for releasing git lock, since multiple ops are used in sequence + * for a complete composite operation not all ops require to acquire the lock hence a dummy flag is sent back for + * operations in that is getting executed in between + * @param baseArtifactId : id of the base artifact for which ops would be locked + * @param isLockRequired : is lock really required or is it a proxy function + * @return : Boolean for whether the lock is released + */ + // TODO @Manish add artifactType reference in incoming prs + public Mono releaseFileLock(String baseArtifactId, boolean isLockRequired) { + if (!Boolean.TRUE.equals(isLockRequired)) { + return Mono.just(Boolean.TRUE); + } + + return releaseFileLock(baseArtifactId); + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java index acc212ab68a..38c19abeb8c 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java @@ -1,5 +1,6 @@ package com.appsmith.server.git.central; +import com.appsmith.git.dto.CommitDTO; import com.appsmith.server.constants.ArtifactType; import com.appsmith.server.domains.Artifact; import com.appsmith.server.dtos.ArtifactImportDTO; @@ -17,4 +18,7 @@ Mono connectArtifactToGit( String originHeader, ArtifactType artifactType, GitType gitType); + + Mono commitArtifact( + CommitDTO commitDTO, String branchedArtifactId, ArtifactType artifactType, GitType gitType); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java index 909f7ffee58..b996e584912 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java @@ -2,6 +2,7 @@ import com.appsmith.server.datasources.base.DatasourceService; import com.appsmith.server.exports.internal.ExportService; +import com.appsmith.server.git.GitRedisUtils; import com.appsmith.server.git.resolver.GitArtifactHelperResolver; import com.appsmith.server.git.resolver.GitHandlingServiceResolver; import com.appsmith.server.git.utils.GitAnalyticsUtils; @@ -12,6 +13,7 @@ import com.appsmith.server.services.UserDataService; import com.appsmith.server.services.WorkspaceService; import com.appsmith.server.solutions.DatasourcePermission; +import io.micrometer.observation.ObservationRegistry; import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; @@ -32,7 +34,9 @@ public CentralGitServiceCECompatibleImpl( WorkspaceService workspaceService, PluginService pluginService, ImportService importService, - ExportService exportService) { + ExportService exportService, + GitRedisUtils gitRedisUtils, + ObservationRegistry observationRegistry) { super( gitProfileUtils, gitAnalyticsUtils, @@ -45,6 +49,8 @@ public CentralGitServiceCECompatibleImpl( workspaceService, pluginService, importService, - exportService); + exportService, + gitRedisUtils, + observationRegistry); } } 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 b075fc7048f..097cf8e56a0 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 @@ -1,6 +1,7 @@ package com.appsmith.server.git.central; import com.appsmith.external.constants.AnalyticsEvents; +import com.appsmith.external.git.constants.GitConstants; import com.appsmith.external.models.Datasource; import com.appsmith.external.models.DatasourceStorage; import com.appsmith.git.dto.CommitDTO; @@ -24,6 +25,7 @@ import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.exports.internal.ExportService; +import com.appsmith.server.git.GitRedisUtils; import com.appsmith.server.git.dtos.ArtifactJsonTransformationDTO; import com.appsmith.server.git.resolver.GitArtifactHelperResolver; import com.appsmith.server.git.resolver.GitHandlingServiceResolver; @@ -36,6 +38,7 @@ import com.appsmith.server.services.UserDataService; import com.appsmith.server.services.WorkspaceService; import com.appsmith.server.solutions.DatasourcePermission; +import io.micrometer.observation.ObservationRegistry; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.eclipse.jgit.api.errors.InvalidRemoteException; @@ -43,6 +46,7 @@ import org.springframework.stereotype.Service; import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; +import reactor.core.observability.micrometer.Micrometer; import reactor.core.publisher.Mono; import reactor.util.function.Tuple2; @@ -54,10 +58,15 @@ import java.util.concurrent.TimeoutException; import static com.appsmith.external.git.constants.ce.GitConstantsCE.DEFAULT_COMMIT_MESSAGE; +import static com.appsmith.external.git.constants.ce.GitConstantsCE.GIT_CONFIG_ERROR; import static com.appsmith.external.git.constants.ce.GitConstantsCE.GIT_PROFILE_ERROR; +import static com.appsmith.external.git.constants.ce.GitSpanCE.OPS_COMMIT; +import static com.appsmith.external.helpers.AppsmithBeanUtils.copyNestedNonNullProperties; import static com.appsmith.server.constants.FieldName.DEFAULT; import static com.appsmith.server.constants.SerialiseArtifactObjective.VERSION_CONTROL; import static java.lang.Boolean.FALSE; +import static java.lang.Boolean.TRUE; +import static org.springframework.util.StringUtils.hasText; @Slf4j @Service @@ -82,6 +91,9 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE { private final ImportService importService; private final ExportService exportService; + private final GitRedisUtils gitRedisUtils; + private final ObservationRegistry observationRegistry; + protected Mono isRepositoryLimitReachedForWorkspace(String workspaceId, Boolean isRepositoryPrivate) { if (!isRepositoryPrivate) { return Mono.just(FALSE); @@ -195,7 +207,7 @@ public Mono importArtifactFromGit( ArtifactJsonTransformationDTO jsonMorphDTO = new ArtifactJsonTransformationDTO(); jsonMorphDTO.setWorkspaceId(workspaceId); - jsonMorphDTO.setArtifactId(artifact.getId()); + jsonMorphDTO.setBaseArtifactId(artifact.getId()); jsonMorphDTO.setArtifactType(artifactType); jsonMorphDTO.setRepoName(gitArtifactMetadata.getRepoName()); jsonMorphDTO.setRefType(RefType.BRANCH); @@ -274,7 +286,7 @@ private Mono deleteArtifactCreatedFromGitImport( return gitHandlingService .removeRepository(artifactJsonTransformationDTO) - .zipWith(gitArtifactHelper.deleteArtifact(artifactJsonTransformationDTO.getArtifactId())) + .zipWith(gitArtifactHelper.deleteArtifact(artifactJsonTransformationDTO.getBaseArtifactId())) .map(Tuple2::getT2); } @@ -443,7 +455,7 @@ public Mono connectArtifactToGit( ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); jsonTransformationDTO.setWorkspaceId(artifact.getWorkspaceId()); - jsonTransformationDTO.setArtifactId(artifact.getId()); + jsonTransformationDTO.setBaseArtifactId(artifact.getId()); jsonTransformationDTO.setRepoName(repoName); jsonTransformationDTO.setArtifactType(artifactType); @@ -468,7 +480,7 @@ public Mono connectArtifactToGit( ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); jsonTransformationDTO.setWorkspaceId(artifact.getWorkspaceId()); - jsonTransformationDTO.setArtifactId(artifact.getId()); + jsonTransformationDTO.setBaseArtifactId(artifact.getId()); jsonTransformationDTO.setRepoName(repoName); jsonTransformationDTO.setArtifactType(artifactType); @@ -524,7 +536,7 @@ public Mono connectArtifactToGit( .flatMap(artifact -> { ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); jsonTransformationDTO.setWorkspaceId(artifact.getWorkspaceId()); - jsonTransformationDTO.setArtifactId(artifact.getId()); + jsonTransformationDTO.setBaseArtifactId(artifact.getId()); jsonTransformationDTO.setArtifactType(artifactType); jsonTransformationDTO.setRepoName(repoName); @@ -556,7 +568,7 @@ public Mono connectArtifactToGit( commitDTO.setIsAmendCommit(FALSE); commitDTO.setMessage(commitMessage); - return this.commitArtifact(baseArtifactId, commitDTO, artifactType, gitType) + return this.commitArtifact(commitDTO, artifact.getId(), artifactType, gitType) .onErrorResume(error -> // If the push fails remove all the cloned files from local repo this.detachRemote(baseArtifactId, artifactType) @@ -592,13 +604,251 @@ public Mono connectArtifactToGit( sink -> connectedArtifactMono.subscribe(sink::success, sink::error, null, sink.currentContext())); } - /** - * TODO: commit artifact - * @return - */ - public Mono commitArtifact( - String baseArtifactId, CommitDTO commitDTO, ArtifactType artifactType, GitType gitType) { - return null; + @Override + public Mono commitArtifact( + CommitDTO commitDTO, String branchedArtifactId, ArtifactType artifactType, GitType gitType) { + return commitArtifact(commitDTO, branchedArtifactId, artifactType, gitType, TRUE); + } + + public Mono commitArtifact( + CommitDTO commitDTO, + String branchedArtifactId, + ArtifactType artifactType, + GitType gitType, + Boolean isFileLock) { + /* + 1. Check if application exists and user have sufficient permissions + 2. Check if branch name exists in git metadata + 3. Save application to the existing local repo + 4. Commit application : git add, git commit (Also check if git init required) + */ + + String commitMessage = commitDTO.getMessage(); + + if (commitMessage == null || commitMessage.isEmpty()) { + commitDTO.setMessage(DEFAULT_COMMIT_MESSAGE + GitDefaultCommitMessage.CONNECT_FLOW.getReason()); + } + + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); + AclPermission artifactEditPermission = gitArtifactHelper.getArtifactEditPermission(); + Mono> baseAndBranchedArtifactMono = getBaseAndBranchedArtifacts( + branchedArtifactId, artifactType, artifactEditPermission) + .cache(); + + return baseAndBranchedArtifactMono.flatMap(artifactTuples -> { + Artifact baseArtifact = artifactTuples.getT1(); + Artifact branchedArtifact = artifactTuples.getT2(); + + GitUser author = commitDTO.getAuthor(); + Mono gitUserMono = Mono.justOrEmpty(author) + .flatMap(gitUser -> { + if (author == null + || !StringUtils.hasText(author.getEmail()) + || !StringUtils.hasText(author.getName())) { + return getGitUserForArtifactId(baseArtifact.getId()); + } + + return Mono.just(gitUser); + }) + .switchIfEmpty(getGitUserForArtifactId(baseArtifact.getId())); + + return gitUserMono.flatMap(gitUser -> { + commitDTO.setAuthor(gitUser); + commitDTO.setCommitter(gitUser); + return commitArtifact(commitDTO, baseArtifact, branchedArtifact, gitType, isFileLock); + }); + }); + } + + private Mono commitArtifact( + CommitDTO commitDTO, + Artifact baseArtifact, + Artifact branchedArtifact, + GitType gitType, + boolean isFileLock) { + + String commitMessage = commitDTO.getMessage(); + + if (commitMessage == null || commitMessage.isEmpty()) { + commitDTO.setMessage(DEFAULT_COMMIT_MESSAGE + GitDefaultCommitMessage.CONNECT_FLOW.getReason()); + } + + GitUser author = commitDTO.getAuthor(); + if (author == null || !StringUtils.hasText(author.getEmail()) || !StringUtils.hasText(author.getName())) { + + String errorMessage = "Unable to find git author configuration for logged-in user. You can set " + + "up a git profile from the user profile section."; + + return gitAnalyticsUtils + .addAnalyticsForGitOperation( + AnalyticsEvents.GIT_COMMIT, + branchedArtifact, + AppsmithError.INVALID_GIT_CONFIGURATION.getErrorType(), + AppsmithError.INVALID_GIT_CONFIGURATION.getMessage(errorMessage), + branchedArtifact.getGitArtifactMetadata().getIsRepoPrivate()) + .then(Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, errorMessage))); + } + + boolean isSystemGenerated = commitDTO.getMessage().contains(DEFAULT_COMMIT_MESSAGE); + + GitArtifactHelper gitArtifactHelper = + gitArtifactHelperResolver.getArtifactHelper(baseArtifact.getArtifactType()); + GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType); + GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata(); + GitArtifactMetadata branchedGitMetadata = branchedArtifact.getGitArtifactMetadata(); + + if (isBaseGitMetadataInvalid(baseGitMetadata, gitType)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, GIT_CONFIG_ERROR)); + } + + if (branchedGitMetadata == null) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, GIT_CONFIG_ERROR)); + } + + final String branchName = branchedGitMetadata.getBranchName(); + if (!hasText(branchName)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.BRANCH_NAME)); + } + + Mono isBranchProtectedMono = gitPrivateRepoHelper.isBranchProtected(baseGitMetadata, branchName); + Mono commitMono = isBranchProtectedMono + .flatMap(isBranchProtected -> { + if (!TRUE.equals(isBranchProtected)) { + return gitRedisUtils.acquireGitLock( + baseGitMetadata.getDefaultArtifactId(), + GitConstants.GitCommandConstants.COMMIT, + isFileLock); + } + + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "commit", + "Cannot commit to protected branch " + branchName)); + }) + .flatMap(fileLocked -> { + // Check if the repo is public for current artifact and if the user have changed the access after + // the connection + + return gitHandlingService.isRepoPrivate(baseGitMetadata).flatMap(isPrivate -> { + // Check the repo limit if the visibility status is updated, or it is private + // TODO: split both of these conditions @Manish + if (isPrivate.equals(baseGitMetadata.getIsRepoPrivate() && !Boolean.TRUE.equals(isPrivate))) { + return Mono.just(baseArtifact); + } + + baseGitMetadata.setIsRepoPrivate(isPrivate); + baseArtifact.setGitArtifactMetadata(baseGitMetadata); + + /** + * A separate GitAuth object has been created in which the private key for + * authentication is held. It's done to avoid getting the encrypted value back + * for private key after mongo save. + * + * When an object having an encrypted attribute is saved, the response is still encrypted. + * The value in db would be corrupted if it's saved again, + * as it would encrypt and already encrypted field + * Private key is using encrypted annotation, which means that it's encrypted before + * being persisted in the db. When it's fetched from db, the listener decrypts it. + */ + GitAuth copiedGitAuth = new GitAuth(); + copyNestedNonNullProperties(baseGitMetadata.getGitAuth(), copiedGitAuth); + + return gitArtifactHelper + .saveArtifact(baseArtifact) + .map(artifact -> { + baseArtifact.getGitArtifactMetadata().setGitAuth(copiedGitAuth); + return artifact; + }) + .then(Mono.defer( + () -> gitArtifactHelper.isPrivateRepoLimitReached(baseArtifact, false))); + }); + }) + .flatMap(artifact -> { + String errorEntity = ""; + if (!StringUtils.hasText(branchedGitMetadata.getBranchName())) { + errorEntity = "branch name"; + } else if (!StringUtils.hasText(branchedGitMetadata.getDefaultArtifactId())) { + errorEntity = "default artifact"; + } else if (!StringUtils.hasText(branchedGitMetadata.getRepoName())) { + errorEntity = "repository name"; + } + + if (!errorEntity.isEmpty()) { + return Mono.error(new AppsmithException( + AppsmithError.INVALID_GIT_CONFIGURATION, "Unable to find " + errorEntity)); + } + + return exportService.exportByArtifactId( + branchedArtifact.getId(), VERSION_CONTROL, branchedArtifact.getArtifactType()); + }) + .flatMap(artifactExchangeJson -> { + ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); + jsonTransformationDTO.setRefType(RefType.BRANCH); + jsonTransformationDTO.setWorkspaceId(baseArtifact.getWorkspaceId()); + jsonTransformationDTO.setBaseArtifactId(baseArtifact.getId()); + jsonTransformationDTO.setRepoName( + branchedArtifact.getGitArtifactMetadata().getRepoName()); + jsonTransformationDTO.setArtifactType(artifactExchangeJson.getArtifactJsonType()); + jsonTransformationDTO.setRefName( + branchedArtifact.getGitArtifactMetadata().getBranchName()); + + return gitHandlingService + .prepareChangesToBeCommitted(jsonTransformationDTO, artifactExchangeJson) + .then(updateArtifactWithGitMetadataGivenPermission(branchedArtifact, branchedGitMetadata)); + }) + .flatMap(updatedBranchedArtifact -> { + GitArtifactMetadata gitArtifactMetadata = updatedBranchedArtifact.getGitArtifactMetadata(); + ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); + jsonTransformationDTO.setRefType(RefType.BRANCH); + jsonTransformationDTO.setWorkspaceId(updatedBranchedArtifact.getWorkspaceId()); + jsonTransformationDTO.setBaseArtifactId(gitArtifactMetadata.getDefaultArtifactId()); + jsonTransformationDTO.setRepoName(gitArtifactMetadata.getRepoName()); + jsonTransformationDTO.setArtifactType(branchedArtifact.getArtifactType()); + jsonTransformationDTO.setRefName(gitArtifactMetadata.getBranchName()); + + return gitHandlingService + .commitArtifact(updatedBranchedArtifact, commitDTO, jsonTransformationDTO) + .onErrorResume(error -> { + return gitAnalyticsUtils + .addAnalyticsForGitOperation( + AnalyticsEvents.GIT_COMMIT, + updatedBranchedArtifact, + error.getClass().getName(), + error.getMessage(), + gitArtifactMetadata.getIsRepoPrivate()) + .then(Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, "commit", error.getMessage()))); + }); + }) + .flatMap(tuple2 -> { + return Mono.zip( + Mono.just(tuple2.getT2()), gitArtifactHelper.publishArtifactPostCommit(tuple2.getT1())); + }) + .flatMap(tuple -> { + String status = tuple.getT1(); + Artifact artifactFromBranch = tuple.getT2(); + Mono releaseFileLockMono = gitRedisUtils.releaseFileLock( + artifactFromBranch.getGitArtifactMetadata().getDefaultArtifactId(), isFileLock); + + Mono updatedArtifactMono = + gitArtifactHelper.updateArtifactWithSchemaVersions(artifactFromBranch); + + return Mono.zip(updatedArtifactMono, releaseFileLockMono) + .then(gitAnalyticsUtils.addAnalyticsForGitOperation( + AnalyticsEvents.GIT_COMMIT, + artifactFromBranch, + "", + "", + artifactFromBranch.getGitArtifactMetadata().getIsRepoPrivate(), + isSystemGenerated)) + .thenReturn(status) + .name(OPS_COMMIT) + .tap(Micrometer.observation(observationRegistry)); + }); + + return Mono.create(sink -> { + commitMono.subscribe(sink::success, sink::error, null, sink.currentContext()); + }); } /** @@ -618,4 +868,86 @@ private boolean isBaseGitMetadataInvalid(GitArtifactMetadata gitArtifactMetadata .getGitHandlingService(gitType) .isGitAuthInvalid(gitArtifactMetadata.getGitAuth()); } + + /** + * Returns baseArtifact and branchedArtifact + * This operation is quite frequently used, hence providing the right set + * + * @param branchedArtifactId : id of the branchedArtifactId + * @param artifactPermission : permission required for getting artifact. + * @return : A tuple of Artifacts + */ + protected Mono> getBaseAndBranchedArtifacts( + String branchedArtifactId, ArtifactType artifactType, AclPermission artifactPermission) { + if (!hasText(branchedArtifactId)) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ID)); + } + + GitArtifactHelper artifactGitHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); + Mono branchedArtifactMono = artifactGitHelper + .getArtifactById(branchedArtifactId, artifactPermission) + .cache(); + + return branchedArtifactMono.flatMap(branchedArtifact -> { + GitArtifactMetadata branchedMetadata = branchedArtifact.getGitArtifactMetadata(); + if (branchedMetadata == null || !hasText(branchedMetadata.getDefaultArtifactId())) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, GIT_CONFIG_ERROR)); + } + + String baseArtifactId = branchedMetadata.getDefaultArtifactId(); + Mono baseArtifactMono = Mono.just(branchedArtifact); + + if (!baseArtifactId.equals(branchedArtifactId)) { + baseArtifactMono = artifactGitHelper.getArtifactById(baseArtifactId, artifactPermission); + } + + return baseArtifactMono.zipWith(branchedArtifactMono); + }); + } + + protected Mono> getBaseAndBranchedArtifacts( + String branchedArtifactId, ArtifactType artifactType) { + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); + AclPermission artifactPermission = gitArtifactHelper.getArtifactEditPermission(); + return getBaseAndBranchedArtifacts(branchedArtifactId, artifactType, artifactPermission); + } + + private Mono getGitUserForArtifactId(String baseArtifactId) { + Mono currentUserMono = userDataService + .getForCurrentUser() + .filter(userData -> !CollectionUtils.isEmpty(userData.getGitProfiles())) + .switchIfEmpty( + Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, GIT_PROFILE_ERROR))); + + return currentUserMono.map(userData -> { + GitProfile profile = userData.getGitProfileByKey(baseArtifactId); + if (profile == null + || Boolean.TRUE.equals(profile.getUseGlobalProfile()) + || !StringUtils.hasText(profile.getAuthorName())) { + profile = userData.getGitProfileByKey(DEFAULT); + } + + GitUser gitUser = new GitUser(); + gitUser.setName(profile.getAuthorName()); + gitUser.setEmail(profile.getAuthorEmail()); + return gitUser; + }); + } + + private Mono updateArtifactWithGitMetadataGivenPermission( + Artifact artifact, GitArtifactMetadata gitMetadata) { + + if (gitMetadata == null) { + return Mono.error( + new AppsmithException(AppsmithError.INVALID_PARAMETER, "Git metadata values cannot be null")); + } + + artifact.setGitArtifactMetadata(gitMetadata); + // For default application we expect a GitAuth to be a part of gitMetadata. We are using save method to leverage + // @Encrypted annotation used for private SSH keys + // applicationService.save sets the transient fields so no need to set it again from this method + return gitArtifactHelperResolver + .getArtifactHelper(artifact.getArtifactType()) + .saveArtifact(artifact); + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java index 49c110d6dca..7c642d832d0 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java @@ -2,6 +2,7 @@ import com.appsmith.server.datasources.base.DatasourceService; import com.appsmith.server.exports.internal.ExportService; +import com.appsmith.server.git.GitRedisUtils; import com.appsmith.server.git.resolver.GitArtifactHelperResolver; import com.appsmith.server.git.resolver.GitHandlingServiceResolver; import com.appsmith.server.git.utils.GitAnalyticsUtils; @@ -12,6 +13,7 @@ import com.appsmith.server.services.UserDataService; import com.appsmith.server.services.WorkspaceService; import com.appsmith.server.solutions.DatasourcePermission; +import io.micrometer.observation.ObservationRegistry; import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; @@ -31,7 +33,9 @@ public CentralGitServiceImpl( WorkspaceService workspaceService, PluginService pluginService, ImportService importService, - ExportService exportService) { + ExportService exportService, + GitRedisUtils gitRedisUtils, + ObservationRegistry observationRegistry) { super( gitProfileUtils, gitAnalyticsUtils, @@ -44,6 +48,8 @@ public CentralGitServiceImpl( workspaceService, pluginService, importService, - exportService); + exportService, + gitRedisUtils, + observationRegistry); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java index 66fc2dcbffb..747bddbd34b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java @@ -8,6 +8,7 @@ import com.appsmith.server.dtos.GitConnectDTO; import com.appsmith.server.git.dtos.ArtifactJsonTransformationDTO; import reactor.core.publisher.Mono; +import reactor.util.function.Tuple2; import java.util.Set; @@ -19,6 +20,8 @@ public interface GitHandlingServiceCE { Mono isRepoPrivate(GitConnectDTO gitConnectDTO); + Mono isRepoPrivate(GitArtifactMetadata gitArtifactMetadata); + // TODO: modify git auth class for native implementation Mono getGitAuthForUser(); @@ -44,4 +47,10 @@ Mono initialiseReadMe( String originHeader); Mono createFirstCommit(ArtifactJsonTransformationDTO jsonTransformationDTO, CommitDTO commitDTO); + + Mono prepareChangesToBeCommitted( + ArtifactJsonTransformationDTO jsonTransformationDTO, ArtifactExchangeJson artifactExchangeJson); + + Mono> commitArtifact( + Artifact branchedArtifact, CommitDTO commitDTO, ArtifactJsonTransformationDTO jsonTransformationDTO); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/dtos/ArtifactJsonTransformationDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/dtos/ArtifactJsonTransformationDTO.java index 899a545b28b..8000d539c75 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/dtos/ArtifactJsonTransformationDTO.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/dtos/ArtifactJsonTransformationDTO.java @@ -15,7 +15,7 @@ public class ArtifactJsonTransformationDTO { String workspaceId; - String artifactId; + String baseArtifactId; String repoName; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java index 563a34b15b3..5199170230b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java @@ -1,10 +1,13 @@ package com.appsmith.server.git.fs; import com.appsmith.external.constants.AnalyticsEvents; +import com.appsmith.external.git.constants.GitConstants; import com.appsmith.external.git.constants.GitSpan; import com.appsmith.external.git.handler.FSGitHandler; import com.appsmith.git.dto.CommitDTO; +import com.appsmith.server.acl.AclPermission; import com.appsmith.server.configurations.EmailConfig; +import com.appsmith.server.constants.ArtifactType; import com.appsmith.server.datasources.base.DatasourceService; import com.appsmith.server.domains.Artifact; import com.appsmith.server.domains.GitArtifactMetadata; @@ -38,13 +41,16 @@ import io.micrometer.observation.ObservationRegistry; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.eclipse.jgit.api.errors.EmptyCommitException; import org.eclipse.jgit.api.errors.InvalidRemoteException; import org.eclipse.jgit.api.errors.TransportException; +import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.springframework.stereotype.Service; import org.springframework.transaction.reactive.TransactionalOperator; import org.springframework.util.StringUtils; import reactor.core.observability.micrometer.Micrometer; import reactor.core.publisher.Mono; +import reactor.util.function.Tuple2; import java.io.IOException; import java.nio.file.Path; @@ -52,6 +58,9 @@ import java.util.Set; import java.util.concurrent.TimeoutException; +import static com.appsmith.external.git.constants.ce.GitConstantsCE.EMPTY_COMMIT_ERROR_MESSAGE; +import static com.appsmith.external.git.constants.ce.GitConstantsCE.GIT_CONFIG_ERROR; + @Slf4j @Service @RequiredArgsConstructor @@ -140,7 +149,16 @@ public String getRepoName(GitConnectDTO gitConnectDTO) { @Override public Mono isRepoPrivate(GitConnectDTO gitConnectDTO) { - return GitUtils.isRepoPrivate(GitUtils.convertSshUrlToBrowserSupportedUrl(gitConnectDTO.getRemoteUrl())); + return isRepoPrivate(gitConnectDTO.getRemoteUrl()); + } + + @Override + public Mono isRepoPrivate(GitArtifactMetadata gitArtifactMetadata) { + return isRepoPrivate(gitArtifactMetadata.getRemoteUrl()); + } + + private Mono isRepoPrivate(String remoteUrl) { + return GitUtils.isRepoPrivate(GitUtils.convertSshUrlToBrowserSupportedUrl(remoteUrl)); } @Override @@ -213,7 +231,7 @@ public Mono reconstructArtifactJsonFromGitReposi ArtifactJsonTransformationDTO artifactJsonTransformationDTO) { return commonGitFileUtils.reconstructArtifactExchangeJsonFromGitRepoWithAnalytics( artifactJsonTransformationDTO.getWorkspaceId(), - artifactJsonTransformationDTO.getArtifactId(), + artifactJsonTransformationDTO.getBaseArtifactId(), artifactJsonTransformationDTO.getRepoName(), artifactJsonTransformationDTO.getRefName(), artifactJsonTransformationDTO.getArtifactType()); @@ -225,7 +243,7 @@ public Mono removeRepository(ArtifactJsonTransformationDTO artifactJson gitArtifactHelperResolver.getArtifactHelper(artifactJsonTransformationDTO.getArtifactType()); Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( artifactJsonTransformationDTO.getWorkspaceId(), - artifactJsonTransformationDTO.getArtifactId(), + artifactJsonTransformationDTO.getBaseArtifactId(), artifactJsonTransformationDTO.getRepoName()); return commonGitFileUtils.deleteLocalRepo(repoSuffix); } @@ -236,7 +254,7 @@ public Mono validateEmptyRepository(ArtifactJsonTransformationDTO artif gitArtifactHelperResolver.getArtifactHelper(artifactJsonTransformationDTO.getArtifactType()); Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( artifactJsonTransformationDTO.getWorkspaceId(), - artifactJsonTransformationDTO.getArtifactId(), + artifactJsonTransformationDTO.getBaseArtifactId(), artifactJsonTransformationDTO.getRepoName()); try { @@ -257,7 +275,7 @@ public Mono initialiseReadMe( gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType()); Path readmePath = gitArtifactHelper.getRepoSuffixPath( jsonTransformationDTO.getWorkspaceId(), - jsonTransformationDTO.getArtifactId(), + jsonTransformationDTO.getBaseArtifactId(), jsonTransformationDTO.getRepoName()); try { return gitArtifactHelper @@ -275,7 +293,7 @@ public Mono createFirstCommit(ArtifactJsonTransformationDTO jsonTransfor gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType()); Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( jsonTransformationDTO.getWorkspaceId(), - jsonTransformationDTO.getArtifactId(), + jsonTransformationDTO.getBaseArtifactId(), jsonTransformationDTO.getRepoName()); return fsGitHandler.commitArtifact( @@ -286,4 +304,239 @@ public Mono createFirstCommit(ArtifactJsonTransformationDTO jsonTransfor true, commitDTO.getIsAmendCommit()); } + + @Override + public Mono prepareChangesToBeCommitted( + ArtifactJsonTransformationDTO jsonTransformationDTO, ArtifactExchangeJson artifactExchangeJson) { + String workspaceId = jsonTransformationDTO.getWorkspaceId(); + String baseArtifactId = jsonTransformationDTO.getBaseArtifactId(); + String repoName = jsonTransformationDTO.getRepoName(); + String branchName = jsonTransformationDTO.getRefName(); + + ArtifactType artifactType = jsonTransformationDTO.getArtifactType(); + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); + Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName); + + return commonGitFileUtils + .saveArtifactToLocalRepoWithAnalytics(repoSuffix, artifactExchangeJson, branchName) + .map(ignore -> Boolean.TRUE) + .onErrorResume(e -> { + log.error("Error in commit flow: ", e); + if (e instanceof RepositoryNotFoundException) { + return Mono.error(new AppsmithException(AppsmithError.REPOSITORY_NOT_FOUND, baseArtifactId)); + } else if (e instanceof AppsmithException) { + return Mono.error(e); + } + return Mono.error(new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage())); + }); + } + + @Override + public Mono> commitArtifact( + Artifact branchedArtifact, CommitDTO commitDTO, ArtifactJsonTransformationDTO jsonTransformationDTO) { + String workspaceId = jsonTransformationDTO.getWorkspaceId(); + String baseArtifactId = jsonTransformationDTO.getBaseArtifactId(); + String repoName = jsonTransformationDTO.getRepoName(); + + ArtifactType artifactType = jsonTransformationDTO.getArtifactType(); + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); + Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName); + + StringBuilder result = new StringBuilder(); + result.append("Commit Result : "); + + Mono gitCommitMono = fsGitHandler + .commitArtifact( + repoSuffix, + commitDTO.getMessage(), + commitDTO.getAuthor().getName(), + commitDTO.getAuthor().getEmail(), + true, + false) + .onErrorResume(error -> { + if (error instanceof EmptyCommitException) { + return Mono.just(EMPTY_COMMIT_ERROR_MESSAGE); + } + + return Mono.error(error); + }); + + return Mono.zip(gitCommitMono, gitArtifactHelper.getArtifactById(branchedArtifact.getId(), null)) + .flatMap(tuple -> { + String commitStatus = tuple.getT1(); + result.append(commitStatus); + + result.append(".\nPush Result : "); + return Mono.zip( + Mono.just(tuple.getT2()), + pushArtifact(tuple.getT2(), false) + .map(pushResult -> result.append(pushResult).toString())); + }); + } + + /** + * Used for pushing commits present in the given branched artifact. + * @param branchedArtifactId : id of the branched artifact. + * @param artifactType : type of the artifact + * @return : returns a string which has details of operations + */ + public Mono pushArtifact(String branchedArtifactId, ArtifactType artifactType) { + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); + AclPermission artifactEditPermission = gitArtifactHelper.getArtifactEditPermission(); + + return gitArtifactHelper + .getArtifactById(branchedArtifactId, artifactEditPermission) + .flatMap(branchedArtifact -> pushArtifact(branchedArtifact, true)); + } + + /** + * Push flow for dehydrated apps + * + * @param branchedArtifact application which needs to be pushed to remote repo + * @return Success message + */ + protected Mono pushArtifact(Artifact branchedArtifact, boolean isFileLock) { + GitArtifactHelper gitArtifactHelper = + gitArtifactHelperResolver.getArtifactHelper(branchedArtifact.getArtifactType()); + Mono gitArtifactMetadataMono = Mono.just(branchedArtifact.getGitArtifactMetadata()); + + if (!branchedArtifact + .getId() + .equals(branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId())) { + gitArtifactMetadataMono = gitArtifactHelper + .getArtifactById(branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), null) + .map(baseArtifact -> { + branchedArtifact + .getGitArtifactMetadata() + .setGitAuth( + baseArtifact.getGitArtifactMetadata().getGitAuth()); + return branchedArtifact.getGitArtifactMetadata(); + }); + } + + // Make sure that ssh Key is unEncrypted for the use. + Mono gitPushResult = gitArtifactMetadataMono + .flatMap(gitMetadata -> { + return gitRedisUtils + .acquireGitLock( + gitMetadata.getDefaultArtifactId(), + GitConstants.GitCommandConstants.PUSH, + isFileLock) + .thenReturn(branchedArtifact); + }) + .flatMap(artifact -> { + GitArtifactMetadata gitData = artifact.getGitArtifactMetadata(); + + if (gitData == null + || !StringUtils.hasText(gitData.getBranchName()) + || !StringUtils.hasText(gitData.getDefaultArtifactId()) + || !StringUtils.hasText(gitData.getGitAuth().getPrivateKey())) { + + return Mono.error( + new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, GIT_CONFIG_ERROR)); + } + + Path baseRepoSuffix = gitArtifactHelper.getRepoSuffixPath( + artifact.getWorkspaceId(), gitData.getDefaultArtifactId(), gitData.getRepoName()); + GitAuth gitAuth = gitData.getGitAuth(); + + return fsGitHandler + .checkoutToBranch( + baseRepoSuffix, + artifact.getGitArtifactMetadata().getBranchName()) + .then(Mono.defer(() -> fsGitHandler + .pushApplication( + baseRepoSuffix, + gitData.getRemoteUrl(), + gitAuth.getPublicKey(), + gitAuth.getPrivateKey(), + gitData.getBranchName()) + .zipWith(Mono.just(artifact)))) + .onErrorResume(error -> gitAnalyticsUtils + .addAnalyticsForGitOperation( + AnalyticsEvents.GIT_PUSH, + artifact, + error.getClass().getName(), + error.getMessage(), + artifact.getGitArtifactMetadata().getIsRepoPrivate()) + .flatMap(application1 -> { + if (error instanceof TransportException) { + return Mono.error( + new AppsmithException(AppsmithError.INVALID_GIT_SSH_CONFIGURATION)); + } + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, "push", error.getMessage())); + })); + }) + .flatMap(tuple -> { + String pushResult = tuple.getT1(); + Artifact artifact = tuple.getT2(); + return pushArtifactErrorRecovery(pushResult, artifact).zipWith(Mono.just(artifact)); + }) + // Add BE analytics + .flatMap(tuple2 -> { + String pushStatus = tuple2.getT1(); + Artifact artifact = tuple2.getT2(); + Mono fileLockReleasedMono = Mono.just(Boolean.TRUE).flatMap(flag -> { + if (!Boolean.TRUE.equals(isFileLock)) { + return Mono.just(flag); + } + return Mono.defer(() -> releaseFileLock( + artifact.getGitArtifactMetadata().getDefaultArtifactId())); + }); + + return pushArtifactErrorRecovery(pushStatus, artifact) + .then(fileLockReleasedMono) + .then(gitAnalyticsUtils.addAnalyticsForGitOperation( + AnalyticsEvents.GIT_PUSH, + artifact, + artifact.getGitArtifactMetadata().getIsRepoPrivate())) + .thenReturn(pushStatus); + }) + .name(GitSpan.OPS_PUSH) + .tap(Micrometer.observation(observationRegistry)); + + return Mono.create(sink -> gitPushResult.subscribe(sink::success, sink::error, null, sink.currentContext())); + } + + /** + * This method is used to recover from the errors that can occur during the push operation + * Mostly happens when the remote branch is protected or any specific rules in place on the branch. + * Since the users will be in a bad state where the changes are committed locally, but they are + * not able to push them changes or revert the changes either. + * 1. Push rejected due to branch protection rules on remote, reset hard prev commit + * + * @param pushResult status of git push operation + * @param artifact artifact data to be used for analytics + * @return status of the git push flow + */ + private Mono pushArtifactErrorRecovery(String pushResult, Artifact artifact) { + GitArtifactMetadata gitMetadata = artifact.getGitArtifactMetadata(); + GitArtifactHelper gitArtifactHelper = + gitArtifactHelperResolver.getArtifactHelper(artifact.getArtifactType()); + + if (pushResult.contains("REJECTED_NONFASTFORWARD")) { + return gitAnalyticsUtils + .addAnalyticsForGitOperation( + AnalyticsEvents.GIT_PUSH, + artifact, + AppsmithError.GIT_UPSTREAM_CHANGES.getErrorType(), + AppsmithError.GIT_UPSTREAM_CHANGES.getMessage(), + gitMetadata.getIsRepoPrivate()) + .flatMap(application1 -> Mono.error(new AppsmithException(AppsmithError.GIT_UPSTREAM_CHANGES))); + } else if (pushResult.contains("REJECTED_OTHERREASON") || pushResult.contains("pre-receive hook declined")) { + + Path path = gitArtifactHelper.getRepoSuffixPath( + artifact.getWorkspaceId(), gitMetadata.getDefaultArtifactId(), gitMetadata.getRepoName()); + + return fsGitHandler + .resetHard(path, gitMetadata.getBranchName()) + .then(Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "push", + "Unable to push changes as pre-receive hook declined. Please make sure that you don't have any rules enabled on the branch " + + gitMetadata.getBranchName()))); + } + return Mono.just(pushResult); + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java index 434f3bf4baa..346c0149b4f 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java @@ -66,4 +66,6 @@ public interface GitArtifactHelperCE { Boolean isContextInArtifactEmpty(ArtifactExchangeJson artifactExchangeJson); T getNewArtifact(String workspaceId, String repoName); + + Mono publishArtifactPostCommit(Artifact committedArtifact); } From 1f07be34fd6eecbe8babd18dfd1e8051265f5604 Mon Sep 17 00:00:00 2001 From: sneha122 Date: Wed, 4 Dec 2024 15:40:32 +0530 Subject: [PATCH 2/2] fix: appsmith ai app git import issue fixed (#37921) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description This PR fixes the issue when trying to import git connected app which has Appsmith AI plugin and query added. Steps to test the fixes: 1. Create an app and connect it to git 2. Create appsmith ai plugin and add a query 3. Commit all the changes 4. In a new workspace, import this app from git, import should go through successfully Fixes #37833 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Datasource" ### :mag: Cypress test results > [!TIP] > 🟒 🟒 🟒 All cypress tests have passed! πŸŽ‰ πŸŽ‰ πŸŽ‰ > Workflow run: > Commit: f4c90b129022c328b667bf2e34fe9e635d6d0ce7 > Cypress dashboard. > Tags: `@tag.Datasource` > Spec: >
Tue, 03 Dec 2024 17:11:00 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **Bug Fixes** - Enhanced null safety checks in the `getFileIds` method to prevent potential `NullPointerExceptions`. - **Tests** - Introduced unit tests for the `FileUtils` class to validate the behavior of the `getFileIds` method under various conditions. Co-authored-by: β€œsneha122” <β€œsneha@appsmith.com”> --- .../com/external/plugins/utils/FileUtils.java | 3 +- .../plugins/services/FileUtilTest.java | 44 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 app/server/appsmith-plugins/appsmithAiPlugin/src/test/java/com/external/plugins/services/FileUtilTest.java diff --git a/app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/utils/FileUtils.java b/app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/utils/FileUtils.java index 9f224b4deec..27acf3d3bc0 100644 --- a/app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/utils/FileUtils.java +++ b/app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/utils/FileUtils.java @@ -19,7 +19,8 @@ public static boolean hasFiles(DatasourceConfiguration datasourceConfiguration) } public static List getFileIds(DatasourceConfiguration datasourceConfiguration) { - if (datasourceConfiguration.getProperties() != null + if (datasourceConfiguration != null + && datasourceConfiguration.getProperties() != null && datasourceConfiguration.getProperties().size() > 0) { for (Property property : datasourceConfiguration.getProperties()) { if (property.getKey().equalsIgnoreCase(FILES) diff --git a/app/server/appsmith-plugins/appsmithAiPlugin/src/test/java/com/external/plugins/services/FileUtilTest.java b/app/server/appsmith-plugins/appsmithAiPlugin/src/test/java/com/external/plugins/services/FileUtilTest.java new file mode 100644 index 00000000000..c242428d61f --- /dev/null +++ b/app/server/appsmith-plugins/appsmithAiPlugin/src/test/java/com/external/plugins/services/FileUtilTest.java @@ -0,0 +1,44 @@ +package com.external.plugins.services; + +import com.appsmith.external.models.DatasourceConfiguration; +import com.appsmith.external.models.Property; +import com.external.plugins.utils.FileUtils; +import org.junit.jupiter.api.Test; +import org.testcontainers.junit.jupiter.Testcontainers; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +@Testcontainers +public class FileUtilTest { + @Test + public void getFileIds_withNullDatasourceConfig_returnsEmptyList() { + DatasourceConfiguration datasourceConfiguration = null; + List actualFileIds = FileUtils.getFileIds(datasourceConfiguration); + assertThat(actualFileIds).isEmpty(); + } + + @Test + public void getFileIds_withValidDatasourceConfig_returnsFileIdList() { + DatasourceConfiguration datasourceConfiguration = new DatasourceConfiguration(); + datasourceConfiguration.setUrl("https://example.com"); + + // create file object + Map fileMap = new HashMap(); + fileMap.put("id", "fileId"); + fileMap.put("name", "fileName"); + fileMap.put("size", 10); + fileMap.put("mimetype", "fileMimetype"); + + Property property = new Property(); + property.setKey("Files"); + property.setValue(List.of(fileMap)); + + datasourceConfiguration.setProperties(List.of(property)); + List actualFileIds = FileUtils.getFileIds(datasourceConfiguration); + assertThat(actualFileIds).contains("fileId"); + } +}