Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: added status changes #38170

Merged
merged 9 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/server/.run/ServerApplication.run.xml
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@
<option name="Make" enabled="true" />
</method>
</configuration>
</component>
</component>
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
/**
* DTO to convey the status local git repo
*/
// TODO: @Manish modify git status DTO accordingly
@Data
public class GitStatusCE_DTO {

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.appsmith.server.git.central;

import com.appsmith.external.dtos.GitStatusDTO;
import com.appsmith.git.dto.CommitDTO;
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.constants.ce.RefType;
Expand Down Expand Up @@ -33,4 +34,7 @@ Mono<String> fetchRemoteChanges(
RefType refType);

Mono<? extends Artifact> discardChanges(String branchedArtifactId, ArtifactType artifactType, GitType gitType);

Mono<GitStatusDTO> getStatus(
String branchedArtifactId, boolean compareRemote, ArtifactType artifactType, GitType gitType);
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.appsmith.server.git.central;

import com.appsmith.external.constants.AnalyticsEvents;
import com.appsmith.external.dtos.GitStatusDTO;
import com.appsmith.external.git.constants.GitConstants;
import com.appsmith.external.git.constants.GitSpan;
import com.appsmith.external.models.Datasource;
Expand Down Expand Up @@ -67,6 +68,7 @@
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.git.constants.ce.GitSpanCE.OPS_STATUS;
import static com.appsmith.external.helpers.AppsmithBeanUtils.copyNestedNonNullProperties;
import static com.appsmith.server.constants.FieldName.BRANCH_NAME;
import static com.appsmith.server.constants.FieldName.DEFAULT;
Expand Down Expand Up @@ -929,6 +931,137 @@ private boolean isBaseGitMetadataInvalid(GitArtifactMetadata gitArtifactMetadata
.isGitAuthInvalid(gitArtifactMetadata.getGitAuth());
}

private Mono<GitStatusDTO> getStatusAfterComparingWithRemote(
String baseArtifactId, boolean isFileLock, ArtifactType artifactType, GitType gitType) {
return getStatus(baseArtifactId, isFileLock, true, artifactType, gitType);
}

@Override
public Mono<GitStatusDTO> getStatus(
String branchedArtifactId, boolean compareRemote, ArtifactType artifactType, GitType gitType) {
return getStatus(branchedArtifactId, true, compareRemote, artifactType, gitType);
}

/**
* Get the status of the artifact for given branched id
*
* @param branchedArtifactId branched id of the artifact
* @param isFileLock if the locking is required, since the status API is used in the other flows of git
* Only for the direct hits from the client the locking will be added
* @param artifactType Type of artifact in context
* @param gitType Type of the service
* @return Map of json file names which are added, modified, conflicting, removed and the working tree if this is clean
*/
private Mono<GitStatusDTO> getStatus(
String branchedArtifactId,
boolean isFileLock,
boolean compareRemote,
ArtifactType artifactType,
GitType gitType) {

Mono<Tuple2<? extends Artifact, ? extends Artifact>> baseAndBranchedArtifacts =
getBaseAndBranchedArtifacts(branchedArtifactId, artifactType);

return baseAndBranchedArtifacts.flatMap(artifactTuple -> {
Artifact baseArtifact = artifactTuple.getT1();
Artifact branchedArtifact = artifactTuple.getT2();
return getStatus(baseArtifact, branchedArtifact, isFileLock, compareRemote, gitType);
});
}

protected Mono<GitStatusDTO> getStatus(
Artifact baseArtifact,
Artifact branchedArtifact,
boolean isFileLock,
boolean compareRemote,
GitType gitType) {

ArtifactType artifactType = baseArtifact.getArtifactType();
GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType);

GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata();
final String baseArtifactId = baseGitMetadata.getDefaultArtifactId();

GitArtifactMetadata branchedGitMetadata = branchedArtifact.getGitArtifactMetadata();
branchedGitMetadata.setGitAuth(baseGitMetadata.getGitAuth());

Comment on lines +987 to +988
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure branchedGitMetadata is not null before usage

There is a potential NullPointerException if branchedGitMetadata is null. Add a null check before setting gitAuth.

Suggested fix:

+        if (branchedGitMetadata == null) {
+            return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, GIT_CONFIG_ERROR));
+        }
         branchedGitMetadata.setGitAuth(baseGitMetadata.getGitAuth());

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

final String finalBranchName = branchedGitMetadata.getBranchName();

if (!StringUtils.hasText(finalBranchName)) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.BRANCH_NAME));
}

Mono<? extends ArtifactExchangeJson> exportedArtifactJsonMono =
exportService.exportByArtifactId(branchedArtifact.getId(), VERSION_CONTROL, artifactType);

Mono<GitStatusDTO> statusMono = exportedArtifactJsonMono
.flatMap(artifactExchangeJson -> {
return gitRedisUtils
.acquireGitLock(baseArtifactId, GitConstants.GitCommandConstants.STATUS, isFileLock)
.thenReturn(artifactExchangeJson);
})
.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(finalBranchName);

Mono<Boolean> prepareForStatus =
gitHandlingService.prepareChangesToBeCommitted(jsonTransformationDTO, artifactExchangeJson);
Mono<String> fetchRemoteMono;

if (compareRemote) {
fetchRemoteMono = Mono.defer(
() -> fetchRemoteChanges(baseArtifact, branchedArtifact, FALSE, gitType, RefType.BRANCH)
.onErrorResume(error -> Mono.error(new AppsmithException(
AppsmithError.GIT_GENERIC_ERROR, error.getMessage()))));
} else {
fetchRemoteMono = Mono.just("ignored");
}

return Mono.zip(prepareForStatus, fetchRemoteMono)
.then(gitHandlingService.getStatus(jsonTransformationDTO));
})
.doFinally(signalType -> gitRedisUtils.releaseFileLock(baseArtifactId, isFileLock))
.onErrorResume(throwable -> {
/*
in case of any error, the global exception handler will release the lock
hence we don't need to do that manually
*/
log.error(
"Error to get status for application: {}, branch: {}",
baseArtifactId,
finalBranchName,
throwable);
return Mono.error(new AppsmithException(AppsmithError.GIT_GENERIC_ERROR, throwable.getMessage()));
});

return Mono.zip(statusMono, sessionUserService.getCurrentUser())
.elapsed()
.flatMap(objects -> {
Long elapsedTime = objects.getT1();
GitStatusDTO gitStatusDTO = objects.getT2().getT1();
User currentUser = objects.getT2().getT2();
String flowName;
if (compareRemote) {
flowName = AnalyticsEvents.GIT_STATUS.getEventName();
} else {
flowName = AnalyticsEvents.GIT_STATUS_WITHOUT_FETCH.getEventName();
}

return gitAnalyticsUtils
.sendUnitExecutionTimeAnalyticsEvent(flowName, elapsedTime, currentUser, branchedArtifact)
.thenReturn(gitStatusDTO);
})
.name(OPS_STATUS)
.tap(Micrometer.observation(observationRegistry));
}

public Mono<String> fetchRemoteChanges(
Artifact baseArtifact, Artifact refArtifact, boolean isFileLock, GitType gitType, RefType refType) {

Expand Down Expand Up @@ -967,7 +1100,9 @@ public Mono<String> fetchRemoteChanges(
.then(Mono.defer(() ->
gitHandlingService.fetchRemoteChanges(jsonTransformationDTO, baseArtifactGitData.getGitAuth())))
.flatMap(fetchedRemoteStatusString -> {
return gitRedisUtils.releaseFileLock(baseArtifactId).thenReturn(fetchedRemoteStatusString);
return gitRedisUtils
.releaseFileLock(baseArtifactId, isFileLock)
.thenReturn(fetchedRemoteStatusString);
})
.onErrorResume(throwable -> {
/*
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.appsmith.server.git.central;

import com.appsmith.external.dtos.GitStatusDTO;
import com.appsmith.git.dto.CommitDTO;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.domains.GitArtifactMetadata;
Expand Down Expand Up @@ -51,6 +52,7 @@ Mono<Boolean> initialiseReadMe(

Mono<String> createFirstCommit(ArtifactJsonTransformationDTO jsonTransformationDTO, CommitDTO commitDTO);

// TODO: provide a proper name
Mono<Boolean> prepareChangesToBeCommitted(
ArtifactJsonTransformationDTO jsonTransformationDTO, ArtifactExchangeJson artifactExchangeJson);

Expand All @@ -61,4 +63,6 @@ Mono<Tuple2<? extends Artifact, String>> commitArtifact(

Mono<? extends ArtifactExchangeJson> recreateArtifactJsonFromLastCommit(
ArtifactJsonTransformationDTO jsonTransformationDTO);

Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransformationDTO);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.appsmith.external.constants.AnalyticsEvents;
import com.appsmith.external.dtos.GitBranchDTO;
import com.appsmith.external.dtos.GitStatusDTO;
import com.appsmith.external.git.constants.GitConstants;
import com.appsmith.external.git.constants.GitSpan;
import com.appsmith.external.git.handler.FSGitHandler;
Expand Down Expand Up @@ -617,4 +618,19 @@ public Mono<? extends ArtifactExchangeJson> recreateArtifactJsonFromLastCommit(
workspaceId, baseArtifactId, repoName, refName, artifactType);
});
}

@Override
public Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransformationDTO) {
String workspaceId = jsonTransformationDTO.getWorkspaceId();
String baseArtifactId = jsonTransformationDTO.getBaseArtifactId();
String repoName = jsonTransformationDTO.getRepoName();
String refName = jsonTransformationDTO.getRefName();

ArtifactType artifactType = jsonTransformationDTO.getArtifactType();
GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName);

Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
return fsGitHandler.getStatus(repoPath, refName);
}
Comment on lines +622 to +635
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for Git status operations

While the implementation is clean, it should include error handling for potential Git operation failures, similar to other methods in the class.

Consider wrapping the Git status operation with error handling:

 @Override
 public Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransformationDTO) {
     String workspaceId = jsonTransformationDTO.getWorkspaceId();
     String baseArtifactId = jsonTransformationDTO.getBaseArtifactId();
     String repoName = jsonTransformationDTO.getRepoName();
     String refName = jsonTransformationDTO.getRefName();

     ArtifactType artifactType = jsonTransformationDTO.getArtifactType();
     GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
     Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName);

     Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
-    return fsGitHandler.getStatus(repoPath, refName);
+    return fsGitHandler.getStatus(repoPath, refName)
+            .onErrorResume(error -> {
+                log.error("Error while fetching git status: {}", error.getMessage());
+                return Mono.error(new AppsmithException(
+                        AppsmithError.GIT_ACTION_FAILED,
+                        "status",
+                        error.getMessage()));
+            });
 }
📝 Committable suggestion

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

Suggested change
@Override
public Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransformationDTO) {
String workspaceId = jsonTransformationDTO.getWorkspaceId();
String baseArtifactId = jsonTransformationDTO.getBaseArtifactId();
String repoName = jsonTransformationDTO.getRepoName();
String refName = jsonTransformationDTO.getRefName();
ArtifactType artifactType = jsonTransformationDTO.getArtifactType();
GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName);
Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
return fsGitHandler.getStatus(repoPath, refName);
}
@Override
public Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransformationDTO) {
String workspaceId = jsonTransformationDTO.getWorkspaceId();
String baseArtifactId = jsonTransformationDTO.getBaseArtifactId();
String repoName = jsonTransformationDTO.getRepoName();
String refName = jsonTransformationDTO.getRefName();
ArtifactType artifactType = jsonTransformationDTO.getArtifactType();
GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName);
Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
return fsGitHandler.getStatus(repoPath, refName)
.onErrorResume(error -> {
log.error("Error while fetching git status: {}", error.getMessage());
return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"status",
error.getMessage()));
});
}

}
Loading