chore: Adding logging mechanism to find out context of redis git locks

[sondermanish]

Description

• Introduction of context to redis file locks for git operations. Now the lock would be aware of what command has placed the it.

Fixes #Issue Number
or
Fixes Issue URL

Warning

If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.

Automation

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

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9318823149
Commit: 278716d
Cypress dashboard url: Click here!

Communication

Should the DevRel and Marketing teams inform users about this change?

• Yes
• No

[coderabbitai]

Walkthrough

Walkthrough

The changes primarily involve the introduction of new constants related to Git commands and the refactoring of Git-related file locking mechanisms to use a new utility class, GitRedisUtils, instead of the previous RedisUtils. This refactoring includes updating method signatures, constructors, and error handling to accommodate the new constants and utility class. Additionally, error messages and tests have been updated to reflect these changes.

Changes

Files	Change Summary
appsmith-interfaces/.../GitConstants.java	Added GitCommandConstants class within GitConstants.
appsmith-external/.../GitConstantsCE.java	Added GitCommandConstantsCE class with Git command constants.
appsmith-server/.../AppsmithError.java	Modified GIT_FILE_IN_USE error message to include placeholders for command name and status.
appsmith-server/.../AutoCommitEventHandlerCEImpl.java	Replaced redisUtils with gitRedisUtils for file locking; updated imports and methods.
appsmith-server/.../AutoCommitEventHandlerImpl.java	Added gitRedisUtils parameter to constructor.
appsmith-server/.../GitRedisUtils.java	Added commandName parameter to addFileLock methods; updated logging, error handling, and metrics.
appsmith-server/.../AutoCommitEligibilityHelperImpl.java	Updated gitRedisUtils.addFileLock method call to include GitCommandConstants.METADATA.
appsmith-server/.../RedisUtils.java	Updated addFileLock and startAutoCommit methods to accept gitCommand parameter.
appsmith-server/.../CommonGitServiceImpl.java	Replaced RedisUtils with GitRedisUtils for Git operations.
appsmith-server/.../GitServiceImpl.java	Replaced RedisUtils with GitRedisUtils; updated constructor parameter.
appsmith-server/.../CommonGitServiceCEImpl.java	Replaced RedisUtils with GitRedisUtils; updated method signatures and calls.
appsmith-server/.../GitServiceCEImpl.java	Replaced RedisUtils with GitRedisUtils; updated method calls to include GitCommandConstants.
appsmith-server/.../CommonGitServiceCECompatibleImpl.java	Replaced RedisUtils with GitRedisUtils; updated constructor parameters.
appsmith-server/.../GitServiceCECompatibleImpl.java	Replaced RedisUtils with GitRedisUtils; updated constructor parameters.
appsmith-server/.../AutoCommitEventHandlerImplTest.java	Added GitRedisUtils import and @SpyBean; updated initialization of AutoCommitEventHandlerImpl.
appsmith-server/.../AutoCommitEligibilityHelperTest.java	Added GitConstants import; replaced RedisUtils with GitRedisUtils; updated method calls.

Sequence Diagram(s) (Beta)

sequenceDiagram
    participant Client
    participant GitService
    participant GitRedisUtils
    participant GitCommandConstants

    Client->>GitService: Initiate Git Command
    GitService->>GitRedisUtils: addFileLock(commandName)
    GitRedisUtils->>GitCommandConstants: Retrieve Command Constants
    GitRedisUtils->>GitService: Return Lock Status
    GitService->>Client: Execute Git Command
    Client->>GitService: Command Execution Response
    GitService->>GitRedisUtils: Release File Lock
    GitRedisUtils->>GitService: Lock Released
    GitService->>Client: Final Response

Loading

This diagram illustrates the updated flow of handling a Git command with the new GitRedisUtils class and GitCommandConstants.

---

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between eb0c13d and 278716d.

Files selected for processing (16)

• app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/GitConstants.java (1 hunks)
• app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java (1 hunks)
• app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java (1 hunks)
• app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerCEImpl.java (7 hunks)
• app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerImpl.java (2 hunks)
• 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/autocommit/helpers/AutoCommitEligibilityHelperImpl.java (2 hunks)
• app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java (3 hunks)
• app/server/appsmith-server/src/main/java/com/appsmith/server/services/CommonGitServiceImpl.java (3 hunks)
• app/server/appsmith-server/src/main/java/com/appsmith/server/services/GitServiceImpl.java (3 hunks)
• app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java (23 hunks)
• app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCEImpl.java (19 hunks)
• app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/CommonGitServiceCECompatibleImpl.java (3 hunks)
• app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/GitServiceCECompatibleImpl.java (3 hunks)
• app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java (3 hunks)
• app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperTest.java (4 hunks)

Additional comments not posted (22)

app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/GitConstants.java (1)

9-9: The addition of GitCommandConstants is a good practice for centralizing Git command-related constants, enhancing maintainability and consistency.

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

24-24: Integration of ObservationRegistry for metrics is a modern practice that enhances observability. Ensure it is properly integrated and tested.

Also applies to: 47-47

Verification successful

---

The ObservationRegistry is properly integrated and utilized within the GitRedisUtils class, specifically in the .tap(Micrometer.observation(observationRegistry)) calls. This confirms that the metrics and observability enhancements are correctly implemented.

• GitRedisUtils.java
  • private final ObservationRegistry observationRegistry;
  • .tap(Micrometer.observation(observationRegistry));

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the integration of `ObservationRegistry` in `GitRedisUtils`.

# Test: Search for the usage of `ObservationRegistry`. Expect: Proper initialization and usage.
rg --type java $'ObservationRegistry'

Length of output: 7743

---

---

Script:

#!/bin/bash
# Description: Verify the usage of `observationRegistry` in `GitRedisUtils` methods.

# Test: Search for the usage of `observationRegistry` within `GitRedisUtils`.
rg --type java 'observationRegistry' app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java

Length of output: 313

app/server/appsmith-server/src/main/java/com/appsmith/server/services/CommonGitServiceImpl.java (1)

30-30: Replacement of RedisUtils with GitRedisUtils in CommonGitServiceImpl aligns with the PR's objectives to enhance Git operation handling.

Also applies to: 47-47

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/CommonGitServiceCECompatibleImpl.java (1)

32-32: Consistent use of GitRedisUtils across service implementations enhances maintainability and functionality of Git operations.

Also applies to: 49-49

app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java (1)

35-50: Introduction of GitCommandConstantsCE centralizes Git command constants, reducing hard-coded strings and improving maintainability.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/GitServiceImpl.java (1)

Line range hint 57-85: Constructor updated to include new dependencies for enhanced Git operations and observability. Ensure that all new dependencies are properly managed and utilized within the service.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/GitServiceCECompatibleImpl.java (1)

Line range hint 59-87: Constructor updated to include new dependencies for enhanced Git operations and observability in the CE-compatible service implementation. Ensure that all new dependencies are properly managed and utilized within the service.

app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java (1)

29-40: Updated addFileLock method to include gitCommand parameter for more informative error handling. This enhancement aligns with the PR's objectives to improve error messaging.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java (1)

59-59: Method isServerAutoCommitRequired updated to use gitRedisUtils.addFileLock with GitCommandConstants.METADATA, enhancing the context-awareness of file locks. This change aligns with the PR's objectives and improves the traceability of operations.

app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperTest.java (1)

Line range hint 59-108: Updated unit tests to use GitRedisUtils and appropriately mock its methods. These changes ensure that the tests remain valid and functional after the modifications to the main class.

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

Line range hint 122-165: Methods autoCommitDSLMigration and autoCommitServerMigration updated to use gitRedisUtils.addFileLock with GitCommandConstants.AUTO_COMMIT, enhancing the context-awareness of file locks. This change aligns with the PR's objectives and improves the traceability of operations.

Also applies to: 288-288

app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java (2)

77-79: Introduced GitRedisUtils as a @SpyBean to replace RedisUtils for Git-related operations.

This change aligns with the PR's objective to enhance Git command handling and file locking mechanisms. Ensure that GitRedisUtils is adequately tested to handle the functionalities previously managed by RedisUtils.

---

120-120: The constructor of AutoCommitEventHandlerImpl now includes both gitRedisUtils and redisUtils.

Please confirm if retaining RedisUtils alongside GitRedisUtils is necessary. If RedisUtils is no longer needed for Git operations, consider removing it to simplify the dependencies.

app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java (1)

858-858: Enhanced error message for GIT_FILE_IN_USE provides more context, aiding in debugging and user communication.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java (4)

113-113: Ensure proper dependency injection for GitRedisUtils.

This change correctly replaces RedisUtils with GitRedisUtils to enhance Git-related operations with more specific utilities. Good use of dependency injection to manage dependencies.

---

1692-1693: Review exception handling in checkoutRemoteBranch.

Examine the exception handling in checkoutRemoteBranch to ensure that all potential error paths are correctly managed.

Verification successful

---

Review exception handling in checkoutRemoteBranch.

The checkoutRemoteBranch method in CommonGitServiceCEImpl.java does include exception handling. Specifically, it uses onErrorResume to handle errors and wraps them in a custom AppsmithException with a relevant error message.

• [Lines 1710-1712]: The method uses onErrorResume to catch errors during the checkoutRemoteBranch operation and wraps them in an AppsmithException.

This indicates that potential error paths are being managed within the method.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Check for exception handling in checkoutRemoteBranch method.
rg "checkoutRemoteBranch" --context 10 src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java

Length of output: 226

---

---

Script:

#!/bin/bash
# Locate the CommonGitServiceCEImpl.java file
fd CommonGitServiceCEImpl.java

# Once the file is located, search for the checkoutRemoteBranch method within it
# Assuming the file is located in src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java
rg "checkoutRemoteBranch" --context 10 $(fd CommonGitServiceCEImpl.java)

Length of output: 8269

---

298-298: Verify correct handling of file locks in getStatus.

Ensure that the file lock is correctly managed in the getStatus method, especially in error scenarios where the lock might not be released.

Verification successful

The search results indicate several TODO comments in the CommonGitServiceCEImpl.java file, but none of them are directly related to the handling of file locks in the getStatus method. To ensure thorough verification, we should inspect the getStatus method implementation for proper file lock handling, especially in error scenarios.

---

The ast-grep search did not return any results for the getStatus method in the specified file. This could mean that the method signature or its location might be different than expected. To proceed, we should search for the getStatus method more broadly within the file to ensure we capture its implementation.

---

The search results show multiple getStatus method implementations and usages within the CommonGitServiceCEImpl.java file. The primary implementation of interest is:

private Mono<GitStatusDTO> getStatus(
        String defaultArtifactId,
        String branchName,
        boolean isFileLock,
        boolean compareRemote,
        ArtifactType artifactType) {
    // Method implementation
}

This method includes logic for handling file locks, as indicated by the isFileLock parameter. However, to ensure that file locks are correctly managed, especially in error scenarios, we need to inspect the method's full implementation.

Let's extract the full implementation of this method to verify the handling of file locks.

---

The full implementation of the getStatus method has been retrieved. Here are the key points regarding file lock handling:

1. File Lock Acquisition:

   • The method checks if isFileLock is true and then attempts to acquire a file lock using addFileLock.
   • The elapsed time for acquiring the lock is logged.

2. File Lock Release:

   • If isFileLock is true, the lock is released after the status is successfully retrieved.
   • In case of an error, the global exception handler is responsible for releasing the lock, as indicated by the comment in the onErrorResume block.

The method appears to handle file locks correctly, both in normal execution and error scenarios. The lock is acquired before performing the status check and is released either after successful completion or by the global exception handler in case of errors.

• Locations in the codebase:
  • File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java
  • Lines: 428-595

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Check for any reported issues or TODOs related to file lock handling in getStatus method.
rg "TODO|FIXME" --context 5 src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java

Length of output: 217

---

---

Script:

#!/bin/bash
# Locate the correct path for CommonGitServiceCEImpl.java
fd CommonGitServiceCEImpl.java

# Once the file is located, check for any reported issues or TODOs related to file lock handling in getStatus method.
fd CommonGitServiceCEImpl.java -x rg "TODO|FIXME" --context 5 {}

Length of output: 4394

---

---

Script:

#!/bin/bash
# Search for the implementation of the getStatus method in CommonGitServiceCEImpl.java
ast-grep --lang java --pattern 'public Mono<GitStatusDTO> getStatus($_, $_, $_) { $$$ }' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java

Length of output: 191

---

---

Script:

#!/bin/bash
# Search for any method named getStatus in CommonGitServiceCEImpl.java
rg 'getStatus' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java -A 20 -B 5

Length of output: 14574

---

---

Script:

#!/bin/bash
# Extract the full implementation of the getStatus method
ast-grep --lang java --pattern 'private Mono<GitStatusDTO> getStatus(String defaultArtifactId, String branchName, boolean isFileLock, boolean compareRemote, ArtifactType artifactType) { $$$ }' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java

Length of output: 27018

---

1478-1479: Confirm correct behavior of pushArtifact under different network conditions.

Test the pushArtifact method to ensure it handles different network conditions robustly, especially the handling of SSH key errors and push rejections.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCEImpl.java (4)

11-11: The addition of GitRedisUtils and GitCommandConstants imports aligns with the refactoring described, where RedisUtils is replaced with GitRedisUtils for enhanced Git operation handling.

Also applies to: 47-47

---

159-159: The update to the constructor to use GitRedisUtils is consistent with the refactoring strategy to enhance Git-related operations.

---

3298-3299: The enhancement to the addFileLock method to include a commandName parameter allows for more granular control and logging of file locks, aligning with the changes aimed at improving Git operations handling.

---

2848-2849: The addition of the updateProtectedBranches method enhances the Git service by providing functionality to manage protected branches, including appropriate error handling and transaction management.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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>.
  • 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 generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @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 as 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.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

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.

[coderabbitai]

Actionable comments posted: 5