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

feat: added commit changes #37922

Merged
merged 3 commits into from
Dec 4, 2024
Merged

feat: added commit changes #37922

merged 3 commits into from
Dec 4, 2024

Conversation

sondermanish
Copy link
Contributor

@sondermanish sondermanish commented Dec 3, 2024

Description

Fixes #37437

Automation

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

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12152404326
Commit: 6dc1f5a
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.

@sondermanish sondermanish requested a review from a team as a code owner December 3, 2024 16:14
Copy link
Contributor

coderabbitai bot commented Dec 3, 2024

Walkthrough

The changes in this pull request primarily involve enhancements to the Git-related functionality within the application. Key updates include the introduction of new methods for acquiring and releasing file locks, committing artifacts, and managing repository privacy. Additionally, several method signatures have been modified to improve clarity and consistency, particularly regarding artifact identification. The overall structure of various classes and interfaces has been adjusted to integrate new dependencies and improve error handling across Git operations.

Changes

File Change Summary
app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java Updated addFileLock method signature and logging; added acquireGitLock and releaseFileLock methods with JavaDoc.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java Added commitArtifact method to the interface.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java Updated constructor to include GitRedisUtils and ObservationRegistry.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java Added commitArtifact method; updated importArtifactFromGit and deleteArtifactCreatedFromGitImport methods.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java Updated constructor to include GitRedisUtils and ObservationRegistry.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java Added methods: isRepoPrivate, prepareChangesToBeCommitted, and commitArtifact.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/dtos/ArtifactJsonTransformationDTO.java Renamed artifactId to baseArtifactId.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java Added methods: prepareChangesToBeCommitted and commitArtifact; updated parameter names for consistency.
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java Added publishArtifactPostCommit method.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java Added publishArtifactPostCommit method to the interface.

Assessment against linked issues

Objective Addressed Explanation
Switch git commit and push method to super service (#37437)

Possibly related PRs

Suggested labels

Git Platform

Suggested reviewers

  • nidhi-nair
  • sharat87
  • abhvsn

"In the realm of code where Git does dwell,
New methods arise, and all is well.
Locks are acquired, artifacts commit,
With changes so clear, we surely won't quit.
So here's to the code, both tidy and bright,
In the world of Git, we soar to new height!" 🎉


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sondermanish sondermanish added the ok-to-test Required label for CI label Dec 3, 2024
@github-actions github-actions bot added Git Product Issues related to version control product Packages & Git Pod All issues belonging to Packages and Git potential-duplicate This label marks issues that are potential duplicates of already open issues Task A simple Todo Enhancement New feature or request labels Dec 3, 2024
@@ -54,4 +54,20 @@ public Mono<Boolean> releaseFileLock(String defaultApplicationId) {
.name(GitSpan.RELEASE_FILE_LOCK)
.tap(Micrometer.observation(observationRegistry));
}

public Mono<Boolean> acquireGitLock(String baseArtifactId, String commandName, boolean isLockRequired) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Gotta add artifact type here as well

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (8)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/dtos/ArtifactJsonTransformationDTO.java (1)

Line range hint 7-11: Address TODO comment and enhance class documentation

The TODO comment indicates a need for a better class name. Additionally, while documentation exists, it could be more specific about the transformation process and the DTO's role in git operations.

Would you like help brainstorming a more descriptive name for this DTO and enhancing its documentation? Some suggestions could include GitArtifactTransformationDTO or GitResourceMappingDTO.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)

628-630: Avoid duplicate default commit message assignment

The default commit message is being set multiple times across methods. Consider centralizing this logic to prevent duplication and ensure consistency.

Apply this diff to remove the redundant assignment:

-    if (commitMessage == null || commitMessage.isEmpty()) {
-        commitDTO.setMessage(DEFAULT_COMMIT_MESSAGE + GitDefaultCommitMessage.CONNECT_FLOW.getReason());
-    }

642-653: Simplify Git user retrieval logic

The logic for obtaining gitUserMono can be simplified to improve readability and reduce nesting.

Apply this diff to streamline the code:

Mono<GitUser> gitUserMono = Mono.justOrEmpty(commitDTO.getAuthor())
-        .flatMap(gitUser -> {
-            if (!StringUtils.hasText(gitUser.getEmail()) || !StringUtils.hasText(gitUser.getName())) {
-                return getGitUserForArtifactId(baseArtifact.getId());
-            }
-            return Mono.just(gitUser);
-        })
+        .filter(gitUser -> StringUtils.hasText(gitUser.getEmail()) && StringUtils.hasText(gitUser.getName()))
         .switchIfEmpty(getGitUserForArtifactId(baseArtifact.getId()));
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (1)

22-25: Add Javadoc for new interface methods

The methods commitArtifact and pushArtifact lack documentation. Please add Javadoc comments to describe their purpose and parameters for better clarity.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (2)

50-53: Method naming consistency

Consider renaming prepareChangesToBeCommitted to prepareChangesForCommit to align with standard naming conventions and improve readability.


53-53: Missing @Override annotation

The commitArtifact method appears to override a method from a superclass or interface. Please add the @Override annotation for better clarity.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java (2)

59-61: Simplify boolean conditions.

The boolean comparison can be simplified.

-if (!Boolean.TRUE.equals(isLockRequired)) {
+if (!isLockRequired) {

Also applies to: 67-69


58-64: Consider consistent method naming.

The method name acquireGitLock differs from the underlying addFileLock. Consider using consistent terminology across the codebase to avoid confusion.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 008a946 and 26bbb0b.

📒 Files selected for processing (10)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java (4 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (13 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java (4 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/dtos/ArtifactJsonTransformationDTO.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCECompatibleImpl.java (3 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (11 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceImpl.java (3 hunks)
🔇 Additional comments (9)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceImpl.java (2)

3-3: LGTM: Clean import addition

The GitExecutor import follows the established package structure and naming conventions.


Line range hint 46-82: LGTM: Well-structured constructor modification

The GitExecutor parameter is properly integrated into both the constructor and super call, maintaining consistent parameter ordering and following dependency injection best practices.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/dtos/ArtifactJsonTransformationDTO.java (2)

Line range hint 13-27: LGTM! Class structure follows DTO best practices

The class is well-structured with appropriate use of Lombok's @DaTa annotation and clear field definitions.


18-18: Verify consistent usage of baseArtifactId

The renaming from artifactId to baseArtifactId looks good, but let's ensure consistent usage across the codebase.

✅ Verification successful

Based on the search results, I can see that the renaming from artifactId to baseArtifactId has been consistently applied across the codebase. The field is used extensively in git-related operations and all references have been updated to use the new name. The change appears to be part of a broader refactoring to make the artifact ID naming more specific and clearer in its purpose.

Renaming is consistently implemented

The field renaming has been properly implemented across all relevant files, with no lingering references to the old name that would cause issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining references to old artifactId field
rg -g '*.java' -A 2 -B 2 'artifactId' app/server/appsmith-server/src/main/java/com/appsmith/server/git/

# Check usage of new baseArtifactId field
rg -g '*.java' -A 2 -B 2 'baseArtifactId' app/server/appsmith-server/src/main/java/com/appsmith/server/git/

Length of output: 105972

app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCECompatibleImpl.java (2)

3-3: LGTM: Import statement aligns with the new dependency.

The addition of GitExecutor import is consistent with the constructor changes.


46-46: LGTM: Constructor updated correctly with GitExecutor dependency.

The GitExecutor parameter is properly added to both the constructor signature and super call, maintaining the correct parameter order.

Let's verify the consistency of GitExecutor usage across related classes:

Also applies to: 70-70

✅ Verification successful

GitExecutor dependency is consistently used across Git service implementations

Based on the search results, GitExecutor is properly integrated across the Git service hierarchy:

  • Used as a constructor parameter in GitFSServiceCECompatibleImpl
  • Consistently injected in CommonGitServiceImpl and related classes
  • Properly autowired in test configurations
  • Imported from the correct package (com.appsmith.external.git.GitExecutor)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if GitExecutor is consistently used across related Git service implementations
ast-grep --pattern 'class $_ extends GitFSServiceCEImpl {
  $$$
  $_($_, GitExecutor $_, $$$) {
    $$$
  }
}'

Length of output: 112


Script:

#!/bin/bash
# Let's try a different approach to check GitExecutor usage
# First, find the base class implementation
ast-grep --pattern 'class GitFSServiceCEImpl {
  $$$
}'

# Also check for GitExecutor parameter in constructors
rg "GitExecutor" -A 5 -B 5 "GitFSServiceCECompatibleImpl.java"

# And check other files that might use GitExecutor
fd "Git.*Service.*\.java$" --exec rg "GitExecutor" {} -A 5 -B 5

Length of output: 6659

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java (1)

37-38: Ensure new constructor dependencies are utilized

The newly added constructor parameters gitRedisUtils and observationRegistry are passed to the superclass but it's unclear if they are being used appropriately. Please verify their usage in the superclass to ensure they are necessary.

Also applies to: 52-53

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java (1)

Line range hint 37-54: LGTM! Constructor updated with necessary dependencies.

The addition of GitRedisUtils and ObservationRegistry is consistent with implementing distributed locking and monitoring capabilities for Git operations.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java (1)

58-72: Verify thread safety of lock operations.

The lock acquisition and release operations need to be atomic to prevent race conditions in a distributed environment.

✅ Verification successful

Let me search for the Redis implementation to verify the atomicity of lock operations.


Let me check the actual Redis lock implementation to verify atomicity.


Lock operations are atomic and thread-safe through Redis distributed locking

The implementation uses Redis atomic operations with proper expiration (120s timeout) and error handling:

  • Lock acquisition uses atomic set with expiry
  • Lock release uses atomic delete operation
  • Concurrent access is prevented by checking key existence
  • Failed lock attempts throw GIT_FILE_IN_USE exception
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for synchronized blocks or other thread safety mechanisms
rg -A 3 "synchronized|Lock|Semaphore|AtomicReference" app/server/appsmith-server/src/main/java/com/appsmith/server/git/

Length of output: 66330


Script:

#!/bin/bash
# Search for Redis lock implementation
rg -A 10 "class.*RedisUtils" app/server/appsmith-server/src/main/java/com/appsmith/server/

Length of output: 2918


Script:

#!/bin/bash
# Search for Redis lock implementation methods
rg -A 10 "addFileLock|releaseFileLock" app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java

Length of output: 1643

Comment on lines 343 to 349
return gitExecutor
.commitArtifact(
repoSuffix,
commitDTO.getMessage(),
commitDTO.getAuthor().getName(),
commitDTO.getAuthor().getEmail(),
true,
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

Add null checks for commitDTO and its author

In the commitArtifact method, commitDTO and commitDTO.getAuthor() may be null, which could lead to a NullPointerException when accessing their properties. Please add null checks to ensure they are not null before proceeding.

Apply this diff to add null checks:

+if (commitDTO == null || commitDTO.getAuthor() == null) {
+    return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "Invalid commitDTO or author"));
+}
return gitExecutor
        .commitArtifact(
            repoSuffix,
            commitDTO.getMessage(),
            commitDTO.getAuthor().getName(),
            commitDTO.getAuthor().getEmail(),
            true,
            false)

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)

734-735: Address the TODO comment regarding condition splitting.

The comment suggests a need to separate the visibility status check and private repository conditions.

Would you like me to propose a refactored version of this condition?


742-752: Consider extracting the GitAuth copy logic into a separate method.

The comment block explains important encryption handling, which warrants its own method for better maintainability.

+private GitAuth createGitAuthCopy(GitAuth sourceAuth) {
+    GitAuth copiedGitAuth = new GitAuth();
+    copyNestedNonNullProperties(sourceAuth, copiedGitAuth);
+    return copiedGitAuth;
+}
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)

499-538: Consider adding retry mechanism for non-critical push failures

While the error recovery is well implemented, consider adding a retry mechanism for transient failures before performing a hard reset.

Example implementation:

private Mono<String> pushWithRetry(Path path, GitArtifactMetadata metadata, int maxRetries) {
    return gitExecutor
            .pushApplication(path, metadata.getRemoteUrl(), metadata.getGitAuth().getPublicKey(),
                    metadata.getGitAuth().getPrivateKey(), metadata.getBranchName())
            .retryWhen(Retry.backoff(maxRetries, Duration.ofSeconds(1))
                    .filter(e -> isTransientError(e)));
}
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (2)

51-52: Clarify the success/failure conditions

The method returns a boolean Mono but the conditions for success/failure aren't immediately clear. Consider adding Javadoc to specify:

  • What constitutes a successful preparation
  • Error conditions that lead to false

54-55: LGTM: Well-structured commit method

The method signature effectively captures both the updated artifact and commit result. This aligns well with the PR objective of switching to the super service.

Consider adding a transaction boundary around this operation if not already handled by the implementation, as this appears to be an atomic operation affecting both the artifact and git state.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 26bbb0b and f0c7fd1.

📒 Files selected for processing (6)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (13 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (3 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (10 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java
🔇 Additional comments (11)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (3)

4-4: LGTM: New dependencies and fields are properly integrated.

The additions of GitRedisUtils and ObservationRegistry enhance the service's capabilities for Git operations and monitoring.

Also applies to: 28-28, 41-41, 49-49, 61-64, 68-69, 94-96


607-611: LGTM: Well-structured method overloading.

The overloaded method provides a convenient way to commit with default file lock behavior.


872-913: LGTM: Well-structured helper methods with proper documentation.

The getBaseAndBranchedArtifacts methods are well-implemented with:

  • Clear documentation
  • Proper null checks
  • Good error handling
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java (1)

69-70: LGTM: Clean interface method addition

The new publishArtifactPostCommit method follows reactive patterns and maintains consistency with existing interface methods.

app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java (1)

321-324: LGTM: Clean implementation of interface method

Implementation correctly delegates to existing publishArtifact method with appropriate manual publish flag.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (4)

4-4: LGTM: GitExecutor dependency properly injected

The addition of GitExecutor as a dependency aligns well with the class's responsibilities for Git operations.

Also applies to: 83-83


311-335: LGTM: Well-structured error handling in prepareChangesToBeCommitted

The method properly handles different error scenarios and maintains clear separation of concerns.


380-497: LGTM: Comprehensive push implementation with proper error handling

The push implementation properly handles:

  • File locking mechanism
  • Git authentication
  • Error scenarios
  • Resource cleanup

337-378: ⚠️ Potential issue

Add null checks for commitDTO and its properties

The method should validate the commitDTO object and its properties before use to prevent potential NullPointerException.

Apply this diff:

    public Mono<Tuple2<? extends Artifact, String>> commitArtifact(
            Artifact branchedArtifact, CommitDTO commitDTO, ArtifactJsonTransformationDTO jsonTransformationDTO) {
+       if (commitDTO == null || commitDTO.getAuthor() == null || 
+           commitDTO.getMessage() == null || commitDTO.getAuthor().getName() == null || 
+           commitDTO.getAuthor().getEmail() == null) {
+           return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "Invalid commit details"));
+       }
        String workspaceId = jsonTransformationDTO.getWorkspaceId();

Likely invalid or redundant comment.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (2)

11-11: LGTM: Required import for Tuple2

The import is necessary for the new commitArtifact method's return type.


23-23: Verify the relationship between both isRepoPrivate implementations

Please ensure that both isRepoPrivate methods maintain consistent behavior. Consider documenting the difference between using GitConnectDTO vs GitArtifactMetadata.

✅ Verification successful

Both isRepoPrivate implementations are consistent and well-designed

The implementations in GitFSServiceCEImpl.java maintain consistent behavior:

  • Both methods (one taking GitConnectDTO, other taking GitArtifactMetadata) delegate to a private isRepoPrivate(String remoteUrl) method
  • This private method converts SSH URLs to browser-supported URLs and uses GitUtils.isRepoPrivate for the actual check
  • The difference in parameter types (GitConnectDTO vs GitArtifactMetadata) is just for convenience based on the calling context, but the core logic remains the same
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all implementations to verify consistency
ast-grep --pattern 'isRepoPrivate($_) {
  $$$
}'

Length of output: 50


Script:

#!/bin/bash
# Search for isRepoPrivate method declarations and implementations using ripgrep
rg "isRepoPrivate" -A 10

# Also search for interface and class definitions containing isRepoPrivate
ast-grep --pattern 'interface $_ {
  $$$
  isRepoPrivate($$$)
  $$$
}'

ast-grep --pattern 'class $_ {
  $$$
  isRepoPrivate($$$)
  $$$
}'

Length of output: 69171

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)

513-541: Consider using enum for push error types

The error recovery logic uses string comparisons which are error-prone and hard to maintain.

+private enum PushErrorType {
+    REJECTED_NONFASTFORWARD,
+    REJECTED_OTHERREASON,
+    PRE_RECEIVE_HOOK_DECLINED
+}

 private Mono<String> pushArtifactErrorRecovery(String pushResult, Artifact artifact) {
-    if (pushResult.contains("REJECTED_NONFASTFORWARD")) {
+    if (pushResult.contains(PushErrorType.REJECTED_NONFASTFORWARD.name())) {
         // ... error handling
-    } else if (pushResult.contains("REJECTED_OTHERREASON") || pushResult.contains("pre-receive hook declined")) {
+    } else if (pushResult.contains(PushErrorType.REJECTED_OTHERREASON.name()) || 
+               pushResult.contains(PushErrorType.PRE_RECEIVE_HOOK_DECLINED.name())) {

377-500: Consider adding retry mechanism for push operations

Git push operations can fail due to temporary network issues. Adding a retry mechanism with exponential backoff would improve reliability.

Consider using Spring Retry or a custom retry mechanism:

@Retryable(
    value = { TransportException.class },
    maxAttempts = 3,
    backoff = @Backoff(delay = 1000, multiplier = 2)
)
protected Mono<String> pushArtifact(Artifact branchedArtifact, boolean isFileLock) {
    // existing implementation
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f0c7fd1 and 6dc1f5a.

📒 Files selected for processing (2)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java
🔇 Additional comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (4)

4-4: LGTM: New imports are relevant to the changes

The added imports for GitConstants and error message constants align well with the new functionality.

Also applies to: 61-63


152-161: Good refactoring of repository privacy checks

The extraction of common logic into a private method improves code reusability and maintainability.


334-375: ⚠️ Potential issue

Enhance error handling in commitArtifact method

The method should handle potential NullPointerException when accessing commitDTO properties.

 @Override
 public Mono<Tuple2<? extends Artifact, String>> commitArtifact(
         Artifact branchedArtifact, CommitDTO commitDTO, ArtifactJsonTransformationDTO jsonTransformationDTO) {
+    if (commitDTO == null || commitDTO.getAuthor() == null) {
+        return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "Invalid commit details"));
+    }
     // ... rest of the method

Likely invalid or redundant comment.


308-332: 🛠️ Refactor suggestion

Add transaction boundary for file system operations

The prepareChangesToBeCommitted method performs file system operations that should be wrapped in a transaction to ensure atomicity.

 @Override
 public Mono<Boolean> prepareChangesToBeCommitted(
         ArtifactJsonTransformationDTO jsonTransformationDTO, ArtifactExchangeJson artifactExchangeJson) {
+    return Mono.from(transactionalOperator.transactional(
         commonGitFileUtils
                 .saveArtifactToLocalRepoWithAnalytics(repoSuffix, artifactExchangeJson, branchName)
                 .map(ignore -> Boolean.TRUE)
                 .onErrorResume(e -> {
                     // ... error handling ...
-                });
+                })));

Likely invalid or redundant comment.

@nidhi-nair nidhi-nair merged commit 1078a03 into release Dec 4, 2024
44 checks passed
@nidhi-nair nidhi-nair deleted the feat/git-commit branch December 4, 2024 05:03
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Dec 9, 2024
## Description

Fixes appsmithorg#37437 

## Automation

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

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/12152404326>
> Commit: 6dc1f5a
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12152404326&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Git`
> Spec:
> <hr>Wed, 04 Dec 2024 04:58:02 UTC
<!-- end of auto-generated comment: Cypress test results  -->


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


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## 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.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Git Product Issues related to version control product ok-to-test Required label for CI Packages & Git Pod All issues belonging to Packages and Git potential-duplicate This label marks issues that are potential duplicates of already open issues Task A simple Todo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Switch git commit and push method to super service
2 participants