chore: additional test cases and method refactor to follow old standa…

…rds (#33734)

## Description
- Added fallback implementation for autocommit eligibility helper to
avoid accessing FS for git connected apps when feature flags are
switched off
- Added test cases to verify the same
- modified test cases names to follow standards
- refactored method to follow standard

## Automation

/ok-to-test tags="@tag.Git"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!WARNING]
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/9244442582>
> Commit: bb8e141
> Cypress dashboard url: <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9244442582&attempt=1"
target="_blank">Click here!</a>
> It seems like **no tests ran** 😔. We are not able to recognize it,
please check workflow <a
href="https://github.com/appsmithorg/appsmith/actions/runs/9244442582"
target="_blank">here.</a>

<!-- end of auto-generated comment: Cypress test results  -->



## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No

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<Boolean> 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<Boolean> addFileLockWithoutRetry(String defaultApplicationId) {
+        return redisUtils.addFileLock(defaultApplicationId);
+    }
+
+    public Mono<Boolean> releaseFileLock(String defaultApplicationId) {
+        return redisUtils.releaseFileLock(defaultApplicationId);
+    }
+}

app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java

@@ -312,18 +312,18 @@ public Mono<Map<String, Integer>> 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<Integer> getMetadataServerSchemaMigrationVersion(
             String workspaceId,
             String defaultArtifactId,
-            String branchName,
             String repoName,
+            String branchName,
             ArtifactType artifactType) {
 
         if (!hasText(workspaceId)) {
@@ -342,7 +342,7 @@ public Mono<Integer> 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);

app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibiltyHelper.java → 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<Boolean> isServerAutoCommitRequired(String workspaceId, GitArtifactMetadata gitMetadata);
 

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<Boolean> isServerAutoCommitRequired(String workspaceId, GitArtifactMetadata gitMetadata) {
+        return Mono.just(FALSE);
+    }
+
+    @Override
+    public Mono<Boolean> isClientMigrationRequired(PageDTO pageDTO) {
+        return Mono.just(FALSE);
+    }
+
+    @Override
+    public Mono<AutoCommitTriggerDTO> isAutoCommitRequired(
+            String workspaceId, GitArtifactMetadata gitArtifactMetadata, PageDTO pageDTO) {
+        return Mono.just(new AutoCommitTriggerDTO(FALSE, FALSE, FALSE));
+    }
+}

app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperImpl.java

@@ -1,57 +1,75 @@
 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;
 import com.appsmith.server.migrations.JsonSchemaVersions;
 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;
 
 import static java.lang.Boolean.FALSE;
 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<Boolean> isServerAutoCommitRequired(String workspaceId, GitArtifactMetadata gitMetadata) {
 
         String defaultApplicationId = gitMetadata.getDefaultArtifactId();
         String branchName = gitMetadata.getBranchName();
         String repoName = gitMetadata.getRepoName();
 
-        return commonGitFileUtils
+        Mono<Boolean> 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<Boolean> isClientMigrationRequired(PageDTO pageDTO) {
         return dslMigrationUtils
                 .getLatestDslVersion()
@@ -63,6 +81,7 @@ public Mono<Boolean> 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<Boolean> isClientMigrationRequired(PageDTO pageDTO) {
     @Override
     public Mono<AutoCommitTriggerDTO> isAutoCommitRequired(
             String workspaceId, GitArtifactMetadata gitArtifactMetadata, PageDTO pageDTO) {
-        String defaultApplicationId = gitArtifactMetadata.getDefaultArtifactId();
-        String branchName = gitArtifactMetadata.getBranchName();
 
         Mono<Boolean> isClientAutocommitRequiredMono =
                 isClientMigrationRequired(pageDTO).defaultIfEmpty(FALSE);
 
-        Mono<Boolean> 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<Boolean> isServerAutocommitRequiredMono = isServerAutoCommitRequired(workspaceId, gitArtifactMetadata);
 
         return isServerAutocommitRequiredMono
                 .zipWith(isClientAutocommitRequiredMono)

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;

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<NewAction> actionClonePageService,
             ClonePageService<ActionCollection> actionCollectionClonePageService) {
         super(
@@ -96,7 +96,7 @@ public ApplicationPageServiceImpl(
                 datasourcePermission,
                 dslMigrationUtils,
                 gitAutoCommitHelper,
-                autoCommitEligibiltyHelper,
+                autoCommitEligibilityHelper,
                 actionClonePageService,
                 actionCollectionClonePageService);
     }

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<NewAction> actionClonePageService;
     private final ClonePageService<ActionCollection> actionCollectionClonePageService;
 
@@ -358,7 +358,7 @@ private Mono<Boolean> migrateSchemasForGitConnectedApps(Application application,
         Mono<PageDTO> 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())) {

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);