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 new file mode 100644 index 000000000000..7bd9ce122426 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java @@ -0,0 +1,36 @@ +package com.appsmith.server.git; + +import com.appsmith.server.exceptions.AppsmithError; +import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.helpers.RedisUtils; +import lombok.RequiredArgsConstructor; +import org.springframework.stereotype.Component; +import reactor.core.publisher.Mono; +import reactor.util.retry.Retry; + +import static com.appsmith.server.helpers.GitUtils.MAX_RETRIES; +import static com.appsmith.server.helpers.GitUtils.RETRY_DELAY; + +@Component +@RequiredArgsConstructor +public class GitRedisUtils { + + private final RedisUtils redisUtils; + + public Mono addFileLock(String defaultApplicationId) { + return redisUtils + .addFileLock(defaultApplicationId) + .retryWhen(Retry.fixedDelay(MAX_RETRIES, RETRY_DELAY) + .onRetryExhaustedThrow((retryBackoffSpec, retrySignal) -> { + throw new AppsmithException(AppsmithError.GIT_FILE_IN_USE); + })); + } + + public Mono addFileLockWithoutRetry(String defaultApplicationId) { + return redisUtils.addFileLock(defaultApplicationId); + } + + public Mono releaseFileLock(String defaultApplicationId) { + return redisUtils.releaseFileLock(defaultApplicationId); + } +} 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 55de15718a8e..d415590b0419 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 @@ -312,18 +312,18 @@ public Mono> reconstructMetadataFromRepo( /** * Provides the server schema version in the application json for the given branch * - * @param workspaceId : workspaceId of the artifact + * @param workspaceId : workspaceId of the artifact * @param defaultArtifactId : default branch id of the artifact - * @param branchName : current branch name of the artifact - * @param repoName : repository name - * @param artifactType : artifact type of this operation + * @param repoName : repository name + * @param branchName : current branch name of the artifact + * @param artifactType : artifact type of this operation * @return the server schema migration version number */ public Mono getMetadataServerSchemaMigrationVersion( String workspaceId, String defaultArtifactId, - String branchName, String repoName, + String branchName, ArtifactType artifactType) { if (!hasText(workspaceId)) { @@ -342,7 +342,7 @@ public Mono getMetadataServerSchemaMigrationVersion( return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.REPO_NAME)); } - return reconstructMetadataFromRepo(workspaceId, defaultArtifactId, branchName, repoName, artifactType) + return reconstructMetadataFromRepo(workspaceId, defaultArtifactId, repoName, branchName, artifactType) .map(metadataMap -> { return metadataMap.getOrDefault( CommonConstants.SERVER_SCHEMA_VERSION, JsonSchemaVersions.serverVersion); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibiltyHelper.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelper.java similarity index 91% rename from app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibiltyHelper.java rename to app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelper.java index 4588a314e9aa..4e538fe1b279 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibiltyHelper.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelper.java @@ -4,7 +4,7 @@ import com.appsmith.server.dtos.PageDTO; import reactor.core.publisher.Mono; -public interface AutoCommitEligibiltyHelper { +public interface AutoCommitEligibilityHelper { Mono isServerAutoCommitRequired(String workspaceId, GitArtifactMetadata gitMetadata); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperFallbackImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperFallbackImpl.java new file mode 100644 index 000000000000..9f408609d921 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperFallbackImpl.java @@ -0,0 +1,30 @@ +package com.appsmith.server.helpers.ce.autocommit; + +import com.appsmith.server.domains.GitArtifactMetadata; +import com.appsmith.server.dtos.PageDTO; +import lombok.extern.slf4j.Slf4j; +import org.springframework.stereotype.Component; +import reactor.core.publisher.Mono; + +import static java.lang.Boolean.FALSE; + +@Slf4j +@Component +public class AutoCommitEligibilityHelperFallbackImpl implements AutoCommitEligibilityHelper { + + @Override + public Mono isServerAutoCommitRequired(String workspaceId, GitArtifactMetadata gitMetadata) { + return Mono.just(FALSE); + } + + @Override + public Mono isClientMigrationRequired(PageDTO pageDTO) { + return Mono.just(FALSE); + } + + @Override + public Mono isAutoCommitRequired( + String workspaceId, GitArtifactMetadata gitArtifactMetadata, PageDTO pageDTO) { + return Mono.just(new AutoCommitTriggerDTO(FALSE, FALSE, FALSE)); + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperImpl.java index f086ef997dfe..f75d25ef5ba2 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperImpl.java @@ -1,9 +1,12 @@ package com.appsmith.server.helpers.ce.autocommit; +import com.appsmith.external.annotations.FeatureFlagged; +import com.appsmith.external.enums.FeatureFlagEnum; import com.appsmith.server.constants.ArtifactType; import com.appsmith.server.domains.GitArtifactMetadata; import com.appsmith.server.domains.Layout; import com.appsmith.server.dtos.PageDTO; +import com.appsmith.server.git.GitRedisUtils; import com.appsmith.server.helpers.CommonGitFileUtils; import com.appsmith.server.helpers.DSLMigrationUtils; import com.appsmith.server.helpers.GitUtils; @@ -11,6 +14,7 @@ import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import net.minidev.json.JSONObject; +import org.springframework.context.annotation.Primary; import org.springframework.stereotype.Component; import reactor.core.publisher.Mono; @@ -18,40 +22,54 @@ import static java.lang.Boolean.TRUE; @Slf4j +@Primary @Component @RequiredArgsConstructor -public class AutoCommitEligibilityHelperImpl implements AutoCommitEligibiltyHelper { +public class AutoCommitEligibilityHelperImpl extends AutoCommitEligibilityHelperFallbackImpl + implements AutoCommitEligibilityHelper { private final CommonGitFileUtils commonGitFileUtils; private final DSLMigrationUtils dslMigrationUtils; + private final GitRedisUtils gitRedisUtils; @Override + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_server_autocommit_feature_enabled) public Mono isServerAutoCommitRequired(String workspaceId, GitArtifactMetadata gitMetadata) { String defaultApplicationId = gitMetadata.getDefaultArtifactId(); String branchName = gitMetadata.getBranchName(); String repoName = gitMetadata.getRepoName(); - return commonGitFileUtils + Mono isServerMigrationRequiredMonoCached = commonGitFileUtils .getMetadataServerSchemaMigrationVersion( - workspaceId, defaultApplicationId, branchName, repoName, ArtifactType.APPLICATION) + workspaceId, defaultApplicationId, repoName, branchName, ArtifactType.APPLICATION) .map(serverSchemaVersion -> { - if (JsonSchemaVersions.serverVersion > serverSchemaVersion) { - return TRUE; - } - - return FALSE; + log.info( + "server schema for application id : {} and branch name : {} is : {}", + defaultApplicationId, + branchName, + serverSchemaVersion); + return JsonSchemaVersions.serverVersion > serverSchemaVersion ? TRUE : FALSE; }) + .defaultIfEmpty(FALSE) + .cache(); + + return Mono.defer(() -> gitRedisUtils.addFileLockWithoutRetry(defaultApplicationId)) + .then(Mono.defer(() -> isServerMigrationRequiredMonoCached)) + .then(Mono.defer(() -> gitRedisUtils.releaseFileLock(defaultApplicationId))) + .then(Mono.defer(() -> isServerMigrationRequiredMonoCached)) .onErrorResume(error -> { log.debug( - "error while retrieving the metadata for defaultApplicationId : {}, branchName : {}", + "error while retrieving the metadata for defaultApplicationId : {}, branchName : {} error : {}", defaultApplicationId, - branchName); - return Mono.just(FALSE); + branchName, + error.getMessage()); + return gitRedisUtils.releaseFileLock(defaultApplicationId).then(Mono.just(FALSE)); }); } @Override + @FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled) public Mono isClientMigrationRequired(PageDTO pageDTO) { return dslMigrationUtils .getLatestDslVersion() @@ -63,6 +81,7 @@ public Mono isClientMigrationRequired(PageDTO pageDTO) { JSONObject layoutDsl = layout.getDsl(); return GitUtils.isMigrationRequired(layoutDsl, latestDslVersion); }) + .defaultIfEmpty(FALSE) .onErrorResume(error -> { log.debug("Error fetching latest DSL version"); return Mono.just(Boolean.FALSE); @@ -72,21 +91,11 @@ public Mono isClientMigrationRequired(PageDTO pageDTO) { @Override public Mono isAutoCommitRequired( String workspaceId, GitArtifactMetadata gitArtifactMetadata, PageDTO pageDTO) { - String defaultApplicationId = gitArtifactMetadata.getDefaultArtifactId(); - String branchName = gitArtifactMetadata.getBranchName(); Mono isClientAutocommitRequiredMono = isClientMigrationRequired(pageDTO).defaultIfEmpty(FALSE); - Mono isServerAutocommitRequiredMono = isServerAutoCommitRequired(workspaceId, gitArtifactMetadata) - .defaultIfEmpty(FALSE) - .onErrorResume(error -> { - log.debug( - "Error in checking server migration for application id : {} branch name : {}", - defaultApplicationId, - branchName); - return Mono.just(Boolean.FALSE); - }); + Mono isServerAutocommitRequiredMono = isServerAutoCommitRequired(workspaceId, gitArtifactMetadata); return isServerAutocommitRequiredMono .zipWith(isClientAutocommitRequiredMono) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitTriggerDTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitTriggerDTO.java index a29b20152779..76bdc6fdf3a5 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitTriggerDTO.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitTriggerDTO.java @@ -1,8 +1,12 @@ package com.appsmith.server.helpers.ce.autocommit; +import lombok.AllArgsConstructor; import lombok.Data; +import lombok.RequiredArgsConstructor; @Data +@AllArgsConstructor +@RequiredArgsConstructor public class AutoCommitTriggerDTO { private Boolean isAutoCommitRequired; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java index 6472fb6c943a..4454c7bfd001 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ApplicationPageServiceImpl.java @@ -9,7 +9,7 @@ import com.appsmith.server.helpers.DSLMigrationUtils; import com.appsmith.server.helpers.GitFileUtils; import com.appsmith.server.helpers.ResponseUtils; -import com.appsmith.server.helpers.ce.autocommit.AutoCommitEligibiltyHelper; +import com.appsmith.server.helpers.ce.autocommit.AutoCommitEligibilityHelper; import com.appsmith.server.helpers.ce.autocommit.GitAutoCommitHelper; import com.appsmith.server.layouts.UpdateLayoutService; import com.appsmith.server.newactions.base.NewActionService; @@ -64,7 +64,7 @@ public ApplicationPageServiceImpl( DatasourcePermission datasourcePermission, DSLMigrationUtils dslMigrationUtils, GitAutoCommitHelper gitAutoCommitHelper, - AutoCommitEligibiltyHelper autoCommitEligibiltyHelper, + AutoCommitEligibilityHelper autoCommitEligibilityHelper, ClonePageService actionClonePageService, ClonePageService actionCollectionClonePageService) { super( @@ -96,7 +96,7 @@ public ApplicationPageServiceImpl( datasourcePermission, dslMigrationUtils, gitAutoCommitHelper, - autoCommitEligibiltyHelper, + autoCommitEligibilityHelper, actionClonePageService, actionCollectionClonePageService); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java index ab2b974b5252..00b804a90765 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java @@ -40,7 +40,7 @@ import com.appsmith.server.helpers.GitUtils; import com.appsmith.server.helpers.ResponseUtils; import com.appsmith.server.helpers.UserPermissionUtils; -import com.appsmith.server.helpers.ce.autocommit.AutoCommitEligibiltyHelper; +import com.appsmith.server.helpers.ce.autocommit.AutoCommitEligibilityHelper; import com.appsmith.server.helpers.ce.autocommit.GitAutoCommitHelper; import com.appsmith.server.layouts.UpdateLayoutService; import com.appsmith.server.migrations.ApplicationVersion; @@ -128,7 +128,7 @@ public class ApplicationPageServiceCEImpl implements ApplicationPageServiceCE { private final DatasourcePermission datasourcePermission; private final DSLMigrationUtils dslMigrationUtils; private final GitAutoCommitHelper gitAutoCommitHelper; - private final AutoCommitEligibiltyHelper autoCommitEligibiltyHelper; + private final AutoCommitEligibilityHelper autoCommitEligibilityHelper; private final ClonePageService actionClonePageService; private final ClonePageService actionCollectionClonePageService; @@ -358,7 +358,7 @@ private Mono migrateSchemasForGitConnectedApps(Application application, Mono pageDTOMono = getPage(newPages.get(0), false); return pageDTOMono.flatMap(pageDTO -> { - return autoCommitEligibiltyHelper + return autoCommitEligibilityHelper .isAutoCommitRequired(workspaceId, gitMetadata, pageDTO) .flatMap(autoCommitTriggerDTO -> { if (Boolean.TRUE.equals(autoCommitTriggerDTO.getIsAutoCommitRequired())) { diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java index 767117f00493..316a041f1422 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/ApplicationPageServiceAutoCommitTest.java @@ -18,7 +18,8 @@ import com.appsmith.server.dtos.PageDTO; import com.appsmith.server.helpers.DSLMigrationUtils; import com.appsmith.server.helpers.GitPrivateRepoHelper; -import com.appsmith.server.helpers.ce.autocommit.AutoCommitEligibiltyHelper; +import com.appsmith.server.helpers.ce.autocommit.AutoCommitEligibilityHelper; +import com.appsmith.server.helpers.ce.autocommit.AutoCommitTriggerDTO; import com.appsmith.server.migrations.JsonSchemaMigration; import com.appsmith.server.migrations.JsonSchemaVersions; import com.appsmith.server.newpages.base.NewPageService; @@ -96,7 +97,7 @@ public class ApplicationPageServiceAutoCommitTest { GitPrivateRepoHelper gitPrivateRepoHelper; @SpyBean - AutoCommitEligibiltyHelper autoCommitEligibiltyHelper; + AutoCommitEligibilityHelper autoCommitEligibilityHelper; @MockBean BranchTrackingStatus branchTrackingStatus; @@ -228,16 +229,15 @@ public void afterTest() { } @Test - public void testAutoCommit_WhenOnlyServerIsEligibleForMigration_CommitSuccess() + public void testAutoCommit_whenOnlyServerIsEligibleForMigration_commitSuccess() throws URISyntaxException, IOException, GitAPIException { ApplicationJson applicationJson = gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); - // server migration as true - doReturn(Mono.just(TRUE)).when(autoCommitEligibiltyHelper).isServerAutoCommitRequired(any(), any()); - // client migration as false - doReturn(Mono.just(FALSE)).when(autoCommitEligibiltyHelper).isClientMigrationRequired(any()); + doReturn(Mono.just(new AutoCommitTriggerDTO(TRUE, FALSE, TRUE))) + .when(autoCommitEligibilityHelper) + .isAutoCommitRequired(anyString(), any(GitArtifactMetadata.class), any(PageDTO.class)); ApplicationJson applicationJson1 = new ApplicationJson(); AppsmithBeanUtils.copyNewFieldValuesIntoOldObject(applicationJson, applicationJson1); @@ -282,7 +282,7 @@ public void testAutoCommit_WhenOnlyServerIsEligibleForMigration_CommitSuccess() } @Test - public void testAutoCommit_WhenOnlyClientIsEligibleForMigration_CommitSuccess() + public void testAutoCommit_whenOnlyClientIsEligibleForMigration_commitSuccess() throws GitAPIException, IOException, URISyntaxException { ApplicationJson applicationJson = gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); @@ -297,8 +297,10 @@ public void testAutoCommit_WhenOnlyClientIsEligibleForMigration_CommitSuccess() .getAsNumber("version") .intValue(); - // server migration as false - doReturn(Mono.just(FALSE)).when(autoCommitEligibiltyHelper).isServerAutoCommitRequired(any(), any()); + doReturn(Mono.just(new AutoCommitTriggerDTO(TRUE, TRUE, FALSE))) + .when(autoCommitEligibilityHelper) + .isAutoCommitRequired(anyString(), any(GitArtifactMetadata.class), any(PageDTO.class)); + Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(pageDSLNumber + 1)); JSONObject dslAfterMigration = new JSONObject(); @@ -342,16 +344,15 @@ public void testAutoCommit_WhenOnlyClientIsEligibleForMigration_CommitSuccess() } @Test - public void testAutoCommit_WhenAutoCommitNotEligible_ReturnsFalse() + public void testAutoCommit_whenAutoCommitNotEligible_returnsFalse() throws URISyntaxException, IOException, GitAPIException { ApplicationJson applicationJson = gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource(APP_JSON_NAME)); - // server migration as false - doReturn(Mono.just(FALSE)).when(autoCommitEligibiltyHelper).isServerAutoCommitRequired(any(), any()); - // client migration as false - doReturn(Mono.just(FALSE)).when(autoCommitEligibiltyHelper).isClientMigrationRequired(any()); + doReturn(Mono.just(new AutoCommitTriggerDTO(FALSE, FALSE, FALSE))) + .when(autoCommitEligibilityHelper) + .isAutoCommitRequired(anyString(), any(GitArtifactMetadata.class), any(PageDTO.class)); gitFileSystemTestHelper.setupGitRepository( WORKSPACE_ID, DEFAULT_APP_ID, BRANCH_NAME, REPO_NAME, applicationJson); diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperTest.java index a9c564a09d62..8947c28d4221 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperTest.java @@ -1,17 +1,28 @@ package com.appsmith.server.helpers.ce.autocommit; +import com.appsmith.external.dtos.ModifiedResources; +import com.appsmith.external.enums.FeatureFlagEnum; import com.appsmith.server.constants.ArtifactType; +import com.appsmith.server.domains.Application; import com.appsmith.server.domains.GitArtifactMetadata; import com.appsmith.server.domains.Layout; +import com.appsmith.server.domains.Theme; +import com.appsmith.server.dtos.ApplicationJson; import com.appsmith.server.dtos.PageDTO; import com.appsmith.server.helpers.CommonGitFileUtils; import com.appsmith.server.helpers.DSLMigrationUtils; +import com.appsmith.server.helpers.RedisUtils; import com.appsmith.server.migrations.JsonSchemaVersions; +import com.appsmith.server.services.FeatureFlagService; +import com.appsmith.server.testhelpers.git.GitFileSystemTestHelper; import lombok.extern.slf4j.Slf4j; import net.minidev.json.JSONObject; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mockito; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.MockBean; import org.springframework.boot.test.mock.mockito.SpyBean; @@ -19,12 +30,11 @@ import reactor.core.publisher.Mono; import reactor.test.StepVerifier; +import java.io.IOException; +import java.util.HashSet; import java.util.List; -import java.util.UUID; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; @Slf4j @SpringBootTest @@ -32,7 +42,7 @@ public class AutoCommitEligibilityHelperTest { @SpyBean - AutoCommitEligibiltyHelper autoCommitEligibiltyHelper; + AutoCommitEligibilityHelper autoCommitEligibilityHelper; @SpyBean CommonGitFileUtils commonGitFileUtils; @@ -40,23 +50,74 @@ public class AutoCommitEligibilityHelperTest { @MockBean DSLMigrationUtils dslMigrationUtils; - @Test - public void isAutoCommitRequired_WhenClientAndServerAreEligible_VerifyDTOReturnsTrue() { + @MockBean + FeatureFlagService featureFlagService; + + @MockBean + RedisUtils redisUtils; + + @Autowired + GitFileSystemTestHelper gitFileSystemTestHelper; + + private static final int RANDOM_DSL_VERSION_NUMBER = 123; + private static final String REPO_NAME = "test-repo"; + private static final String BRANCH_NAME = "develop"; + private static final String WORKSPACE_ID = "test-workspace"; + private static final String DEFAULT_APPLICATION_ID = "default-app"; + private GitArtifactMetadata createGitMetadata() { GitArtifactMetadata gitArtifactMetadata = new GitArtifactMetadata(); + gitArtifactMetadata.setDefaultApplicationId(DEFAULT_APPLICATION_ID); + gitArtifactMetadata.setRepoName(REPO_NAME); + gitArtifactMetadata.setBranchName(BRANCH_NAME); + return gitArtifactMetadata; + } + + private PageDTO createPageDTO(Integer dslVersionNumber) { + JSONObject jsonObject = new JSONObject(); + jsonObject.put("key", "value"); + jsonObject.put("version", dslVersionNumber); + + Layout layout1 = new Layout(); + layout1.setId("testLayoutId"); + layout1.setDsl(jsonObject); + PageDTO pageDTO = new PageDTO(); - String workspaceId = UUID.randomUUID().toString(); + pageDTO.setId("testPageId"); + pageDTO.setApplicationId("DEFAULT_APP_ID"); + pageDTO.setLayouts(List.of(layout1)); - Mockito.doReturn(Mono.just(Boolean.TRUE)) - .when(autoCommitEligibiltyHelper) - .isServerAutoCommitRequired(anyString(), any(GitArtifactMetadata.class)); + return pageDTO; + } + + @BeforeEach + public void beforeEach() { + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) + .thenReturn(Mono.just(Boolean.TRUE)); - Mockito.doReturn(Mono.just(Boolean.TRUE)) - .when(autoCommitEligibiltyHelper) - .isClientMigrationRequired(any(PageDTO.class)); + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_server_autocommit_feature_enabled)) + .thenReturn(Mono.just(Boolean.TRUE)); + + Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(RANDOM_DSL_VERSION_NUMBER)); + + Mockito.when(redisUtils.addFileLock(DEFAULT_APPLICATION_ID)).thenReturn(Mono.just(Boolean.TRUE)); + Mockito.when(redisUtils.releaseFileLock(DEFAULT_APPLICATION_ID)).thenReturn(Mono.just(Boolean.TRUE)); + } + + @Test + public void isAutoCommitRequired_whenClientAndServerAreEligible_verifyDTOReturnsTrue() { + + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); + PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER - 1); + + // this leads to server migration requirement as true + Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion - 1)) + .when(commonGitFileUtils) + .getMetadataServerSchemaMigrationVersion( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); Mono autoCommitTriggerDTOMono = - autoCommitEligibiltyHelper.isAutoCommitRequired(workspaceId, gitArtifactMetadata, pageDTO); + autoCommitEligibilityHelper.isAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata, pageDTO); StepVerifier.create(autoCommitTriggerDTOMono) .assertNext(autoCommitTriggerDTO -> { @@ -70,22 +131,19 @@ public void isAutoCommitRequired_WhenClientAndServerAreEligible_VerifyDTOReturns } @Test - public void isAutoCommitRequired_WhenClientAndServerAreNotEligible_VerifyDTOReturnFalse() { + public void isAutoCommitRequired_whenClientAndServerAreNotEligible_verifyDTOReturnFalse() { - GitArtifactMetadata gitArtifactMetadata = new GitArtifactMetadata(); - PageDTO pageDTO = new PageDTO(); - String workspaceId = UUID.randomUUID().toString(); - - Mockito.doReturn(Mono.just(Boolean.FALSE)) - .when(autoCommitEligibiltyHelper) - .isServerAutoCommitRequired(anyString(), any(GitArtifactMetadata.class)); + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); + PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER); - Mockito.doReturn(Mono.just(Boolean.FALSE)) - .when(autoCommitEligibiltyHelper) - .isClientMigrationRequired(any(PageDTO.class)); + // this leads to server migration requirement as false + Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion)) + .when(commonGitFileUtils) + .getMetadataServerSchemaMigrationVersion( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); Mono autoCommitTriggerDTOMono = - autoCommitEligibiltyHelper.isAutoCommitRequired(workspaceId, gitArtifactMetadata, pageDTO); + autoCommitEligibilityHelper.isAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata, pageDTO); StepVerifier.create(autoCommitTriggerDTOMono) .assertNext(autoCommitTriggerDTO -> { @@ -99,22 +157,19 @@ public void isAutoCommitRequired_WhenClientAndServerAreNotEligible_VerifyDTORetu } @Test - public void isAutoCommitRequired_WhenOnlyClientIsEligible_VerifyDTOReturnTrue() { - - GitArtifactMetadata gitArtifactMetadata = new GitArtifactMetadata(); - PageDTO pageDTO = new PageDTO(); - String workspaceId = UUID.randomUUID().toString(); + public void isAutoCommitRequired_whenOnlyClientIsEligible_verifyDTOReturnTrue() { - Mockito.doReturn(Mono.just(Boolean.FALSE)) - .when(autoCommitEligibiltyHelper) - .isServerAutoCommitRequired(anyString(), any(GitArtifactMetadata.class)); + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); + PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER - 1); - Mockito.doReturn(Mono.just(Boolean.TRUE)) - .when(autoCommitEligibiltyHelper) - .isClientMigrationRequired(any(PageDTO.class)); + // this leads to server migration requirement as false + Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion)) + .when(commonGitFileUtils) + .getMetadataServerSchemaMigrationVersion( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); Mono autoCommitTriggerDTOMono = - autoCommitEligibiltyHelper.isAutoCommitRequired(workspaceId, gitArtifactMetadata, pageDTO); + autoCommitEligibilityHelper.isAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata, pageDTO); StepVerifier.create(autoCommitTriggerDTOMono) .assertNext(autoCommitTriggerDTO -> { @@ -128,22 +183,19 @@ public void isAutoCommitRequired_WhenOnlyClientIsEligible_VerifyDTOReturnTrue() } @Test - public void isAutoCommitRequired_WhenOnlyServerIsEligible_VerifyDTOReturnTrue() { + public void isAutoCommitRequired_whenOnlyServerIsEligible_verifyDTOReturnTrue() { - GitArtifactMetadata gitArtifactMetadata = new GitArtifactMetadata(); - PageDTO pageDTO = new PageDTO(); - String workspaceId = UUID.randomUUID().toString(); - - Mockito.doReturn(Mono.just(Boolean.TRUE)) - .when(autoCommitEligibiltyHelper) - .isServerAutoCommitRequired(anyString(), any(GitArtifactMetadata.class)); + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); + PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER); - Mockito.doReturn(Mono.just(Boolean.FALSE)) - .when(autoCommitEligibiltyHelper) - .isClientMigrationRequired(any(PageDTO.class)); + // this leads to server migration requirement as true + Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion - 1)) + .when(commonGitFileUtils) + .getMetadataServerSchemaMigrationVersion( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); Mono autoCommitTriggerDTOMono = - autoCommitEligibiltyHelper.isAutoCommitRequired(workspaceId, gitArtifactMetadata, pageDTO); + autoCommitEligibilityHelper.isAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata, pageDTO); StepVerifier.create(autoCommitTriggerDTOMono) .assertNext(autoCommitTriggerDTO -> { @@ -157,101 +209,162 @@ public void isAutoCommitRequired_WhenOnlyServerIsEligible_VerifyDTOReturnTrue() } @Test - public void isServerMigrationRequired_WhenJsonSchemaIsNotAhead_ReturnsFalse() { - - String repoName = "test-repo"; - String branchName = "develop"; - String workspaceId = "test-workspace"; - String defaultApplicationId = "default-app"; - - GitArtifactMetadata gitArtifactMetadata = new GitArtifactMetadata(); - gitArtifactMetadata.setDefaultApplicationId(defaultApplicationId); - gitArtifactMetadata.setRepoName(repoName); - gitArtifactMetadata.setBranchName(branchName); + public void isServerMigrationRequired_whenJsonSchemaIsNotAhead_returnsFalse() { + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); - Mockito.when(commonGitFileUtils.getMetadataServerSchemaMigrationVersion( - workspaceId, defaultApplicationId, branchName, repoName, ArtifactType.APPLICATION)) - .thenReturn(Mono.just(JsonSchemaVersions.serverVersion)); + // this leads to server migration requirement as false + Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion)) + .when(commonGitFileUtils) + .getMetadataServerSchemaMigrationVersion( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); - Mono autoCommitTriggerDTOMono = - autoCommitEligibiltyHelper.isServerAutoCommitRequired(workspaceId, gitArtifactMetadata); + Mono isServerMigrationRequiredMono = + autoCommitEligibilityHelper.isServerAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata); - StepVerifier.create(autoCommitTriggerDTOMono) + StepVerifier.create(isServerMigrationRequiredMono) .assertNext(isServerMigrationRequired -> assertThat(isServerMigrationRequired).isFalse()) .verifyComplete(); } @Test - public void isServerMigrationRequired_WhenJsonSchemaIsAhead_ReturnsTrue() { - - String repoName = "test-repo"; - String branchName = "develop"; - String workspaceId = "test-workspace"; - String defaultApplicationId = "default-app"; + public void isServerMigrationRequired_whenJsonSchemaIsAhead_returnsTrue() { + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); - GitArtifactMetadata gitArtifactMetadata = new GitArtifactMetadata(); - gitArtifactMetadata.setDefaultApplicationId(defaultApplicationId); - gitArtifactMetadata.setRepoName(repoName); - gitArtifactMetadata.setBranchName(branchName); + // this leads to server migration requirement as true + Mockito.doReturn(Mono.just(JsonSchemaVersions.serverVersion - 1)) + .when(commonGitFileUtils) + .getMetadataServerSchemaMigrationVersion( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, REPO_NAME, BRANCH_NAME, ArtifactType.APPLICATION); - Mockito.when(commonGitFileUtils.getMetadataServerSchemaMigrationVersion( - workspaceId, defaultApplicationId, branchName, repoName, ArtifactType.APPLICATION)) - .thenReturn(Mono.just(JsonSchemaVersions.serverVersion - 1)); + Mono isServerMigrationRequiredMono = + autoCommitEligibilityHelper.isServerAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata); - Mono autoCommitTriggerDTOMono = - autoCommitEligibiltyHelper.isServerAutoCommitRequired(workspaceId, gitArtifactMetadata); - - StepVerifier.create(autoCommitTriggerDTOMono) + StepVerifier.create(isServerMigrationRequiredMono) .assertNext(isServerMigrationRequired -> assertThat(isServerMigrationRequired).isTrue()) .verifyComplete(); } - private PageDTO createPageDTO(Integer dsl_version_number) { - JSONObject jsonObject = new JSONObject(); - jsonObject.put("key", "value"); - jsonObject.put("version", dsl_version_number); + @Test + public void isServerMigrationRequired_whenFeatureIsFlagFalse_returnsFalse() { + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_server_autocommit_feature_enabled)) + .thenReturn(Mono.just(Boolean.FALSE)); - Layout layout1 = new Layout(); - layout1.setId("testLayoutId"); - layout1.setDsl(jsonObject); + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); - PageDTO pageDTO = new PageDTO(); - pageDTO.setId("testPageId"); - pageDTO.setApplicationId("DEFAULT_APP_ID"); - pageDTO.setLayouts(List.of(layout1)); + Mono isServerMigrationRequiredMono = + autoCommitEligibilityHelper.isServerAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata); - return pageDTO; + StepVerifier.create(isServerMigrationRequiredMono) + .assertNext(isServerMigrationRequired -> + assertThat(isServerMigrationRequired).isFalse()) + .verifyComplete(); } @Test - public void isClientMigrationRequired_WhenLatestDslIsNotAhead_ReturnsFalse() { - Integer dslNumber = 88; - PageDTO pageDTO = createPageDTO(dslNumber); + public void isClientMigrationRequired_whenLatestDslIsNotAhead_returnsFalse() { + PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER); + Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(RANDOM_DSL_VERSION_NUMBER)); - Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(dslNumber)); + Mono isClientMigrationRequiredMono = autoCommitEligibilityHelper.isClientMigrationRequired(pageDTO); - Mono autoCommitTriggerDTOMono = autoCommitEligibiltyHelper.isClientMigrationRequired(pageDTO); + StepVerifier.create(isClientMigrationRequiredMono) + .assertNext(isClientMigrationRequired -> + assertThat(isClientMigrationRequired).isFalse()) + .verifyComplete(); + } - StepVerifier.create(autoCommitTriggerDTOMono) + @Test + public void isClientMigrationRequired_whenLatestDslIsAhead_returnsTrue() { + PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER - 1); + Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(RANDOM_DSL_VERSION_NUMBER)); + + Mono isClientMigrationRequiredMono = autoCommitEligibilityHelper.isClientMigrationRequired(pageDTO); + + StepVerifier.create(isClientMigrationRequiredMono) + .assertNext(isClientMigrationRequired -> + assertThat(isClientMigrationRequired).isTrue()) + .verifyComplete(); + } + + @Test + public void isClientMigrationRequired_whenFeatureFlagIsFalse_returnsFalse() { + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) + .thenReturn(Mono.just(Boolean.FALSE)); + + PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER); + Mono isClientMigrationRequiredMono = autoCommitEligibilityHelper.isClientMigrationRequired(pageDTO); + + StepVerifier.create(isClientMigrationRequiredMono) .assertNext(isClientMigrationRequired -> assertThat(isClientMigrationRequired).isFalse()) .verifyComplete(); } @Test - public void isClientMigrationRequired_WhenLatestDslIsAhead_ReturnsTrue() { - Integer dslNumber = 88; - PageDTO pageDTO = createPageDTO(dslNumber - 1); + public void isAutoCommitRequired_whenFeatureIsFlagFalse_returnsAllFalse() { + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_autocommit_feature_enabled)) + .thenReturn(Mono.just(Boolean.FALSE)); - Mockito.when(dslMigrationUtils.getLatestDslVersion()).thenReturn(Mono.just(dslNumber)); + Mockito.when(featureFlagService.check(FeatureFlagEnum.release_git_server_autocommit_feature_enabled)) + .thenReturn(Mono.just(Boolean.FALSE)); - Mono autoCommitTriggerDTOMono = autoCommitEligibiltyHelper.isClientMigrationRequired(pageDTO); + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); + PageDTO pageDTO = createPageDTO(RANDOM_DSL_VERSION_NUMBER); + + Mono autoCommitTriggerDTOMono = + autoCommitEligibilityHelper.isAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata, pageDTO); StepVerifier.create(autoCommitTriggerDTOMono) - .assertNext(isClientMigrationRequired -> - assertThat(isClientMigrationRequired).isTrue()) + .assertNext(autoCommitTriggerDTO -> { + assertThat(autoCommitTriggerDTO.getIsAutoCommitRequired()).isFalse(); + assertThat(autoCommitTriggerDTO.getIsServerAutoCommitRequired()) + .isFalse(); + assertThat(autoCommitTriggerDTO.getIsClientAutoCommitRequired()) + .isFalse(); + }) .verifyComplete(); } + + @Test + public void isServerMigrationRequired_fileSystemOperation_returnsTrue() throws GitAPIException, IOException { + ApplicationJson applicationJson = new ApplicationJson(); + + Application application = new Application(); + application.setPolicies(new HashSet<>()); + + applicationJson.setExportedApplication(application); + applicationJson.setDatasourceList(List.of()); + applicationJson.setActionCollectionList(List.of()); + applicationJson.setActionList(List.of()); + applicationJson.setCustomJSLibList(List.of()); + applicationJson.setPageList(List.of()); + applicationJson.setPublishedTheme(new Theme()); + applicationJson.setEditModeTheme(new Theme()); + applicationJson.setClientSchemaVersion(JsonSchemaVersions.clientVersion); + applicationJson.setServerSchemaVersion(JsonSchemaVersions.serverVersion - 1); + + ModifiedResources modifiedResources = new ModifiedResources(); + modifiedResources.setAllModified(true); + applicationJson.setModifiedResources(new ModifiedResources()); + + GitArtifactMetadata gitArtifactMetadata = createGitMetadata(); + + try { + gitFileSystemTestHelper.setupGitRepository( + WORKSPACE_ID, DEFAULT_APPLICATION_ID, BRANCH_NAME, REPO_NAME, applicationJson); + + Mono isServerMigrationRequiredMono = + autoCommitEligibilityHelper.isServerAutoCommitRequired(WORKSPACE_ID, gitArtifactMetadata); + + StepVerifier.create(isServerMigrationRequiredMono) + .assertNext(isServerMigrationRequired -> + assertThat(isServerMigrationRequired).isTrue()) + .verifyComplete(); + + } finally { + gitFileSystemTestHelper.deleteWorkspaceDirectory(WORKSPACE_ID); + } + } } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/testhelpers/git/GitFileSystemTestHelper.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/testhelpers/git/GitFileSystemTestHelper.java index f04ee1cbbb8d..f55c768fc0cc 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/testhelpers/git/GitFileSystemTestHelper.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/testhelpers/git/GitFileSystemTestHelper.java @@ -46,6 +46,10 @@ public void setupGitRepository( Path gitCompletePath = gitExecutor.createRepoPath(suffix); String metadataFileName = CommonConstants.METADATA + CommonConstants.JSON_EXTENSION; + // Delete the repository if it already exists, + // this is to avoid left over repositories from older tests. + deleteWorkspaceDirectory(workspaceId); + // create a new repository log.debug("Setting up Git repository at path: {}", gitCompletePath); gitExecutor.createNewRepository(gitCompletePath);