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 git resource map consumption #38470

Merged
merged 6 commits into from
Jan 5, 2025
Merged

Conversation

sondermanish
Copy link
Contributor

@sondermanish sondermanish commented Jan 3, 2025

Description

  • Added git resource map consumption

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" it=true

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12600209771
Commit: 8877a5b
Cypress dashboard.
Tags: @tag.Git
Spec:


Fri, 03 Jan 2025 16:35:41 UTC

Communication

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

  • Yes
  • No

Summary by CodeRabbit

Based on the comprehensive summary of changes, here are the release notes:

Release Notes

  • Git Integration Enhancements

    • Improved Git reference handling across multiple service methods
    • Enhanced support for artifact migration and JSON schema transformations
    • Refined parameter management for Git-related operations
  • Method Signature Updates

    • Standardized method signatures across Git-related services
    • Added support for more flexible reference type handling
    • Improved consistency in method parameter ordering
  • Backend Improvements

    • Updated JSON migration processes
    • Enhanced error handling in Git file system operations
    • Expanded support for different artifact types in Git workflows
  • Testing and Validation

    • Updated test cases to reflect new method signatures
    • Improved test coverage for Git-related functionality

These changes primarily focus on backend improvements to the Git integration system, providing more robust and flexible handling of artifacts and references.

Copy link
Contributor

coderabbitai bot commented Jan 3, 2025

Walkthrough

This pull request introduces enhancements to the Git-related functionality across multiple server-side Java classes. The primary changes focus on improving Git resource map construction, JSON migration processes, and standardizing method signatures related to artifact and Git operations. The modifications aim to provide more flexible and consistent handling of Git references and artifact transformations.

Changes

File Path Change Summary
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java Added constructGitResourceMapFromGitRepo method
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java Added new method signature for Git resource map construction
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java Updated method signatures and added JSON migration methods
Multiple Server Files Standardized method signatures for Git and artifact operations

Sequence Diagram

sequenceDiagram
    participant Client
    participant GitController
    participant GitService
    participant FileUtils
    
    Client->>GitController: Request Git Resource Map
    GitController->>GitService: Construct Resource Map
    GitService->>FileUtils: Fetch Git Resources
    FileUtils-->>GitService: Return Resource Map
    GitService-->>GitController: Return Constructed Map
    GitController-->>Client: Respond with Git Resource Map
Loading

Possibly related PRs

Suggested Labels

Enhancement, Git Product, ok-to-test, Task

Suggested Reviewers

  • nidhi-nair

Poem

🌿 Git's dance of code and change,
Branches weaving, lines rearrange,
Maps and methods, neatly spun,
Refactoring's elegant run!
🚀 Commit by commit, we ascend!


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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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.

@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Jan 3, 2025

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

1 similar comment
Copy link

github-actions bot commented Jan 3, 2025

Failed server tests

  • com.appsmith.server.git.ops.GitCommitTests#commitArtifact_whenLocalRepoNotAvailable_throwsRepoNotFoundException
  • com.appsmith.server.git.ops.GitCommitTests#commitArtifact_whenNoChangesInLocal_returnsWithEmptyCommitMessage
  • com.appsmith.server.git.ops.GitDiscardTests#discardChanges_whenCancelledMidway_discardsSuccessfully
  • com.appsmith.server.git.ops.GitDiscardTests#discardChanges_whenUpstreamChangesAvailable_discardsSuccessfully

Copy link

github-actions bot commented Jan 3, 2025

Failed server tests

  • com.appsmith.server.git.ops.GitConnectTests#connectArtifactToGit_whenUsingGlobalProfile_completesSuccessfully

@sondermanish sondermanish marked this pull request as ready for review January 3, 2025 15:11
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

🧹 Nitpick comments (17)
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java (1)

518-519: Enhance test coverage for multiple RefType values.
Only a generic matcher (any(RefType.class)) is used here. Consider introducing additional tests or parameterized testing to confirm behavior for each distinct RefType (e.g., BRANCH, TAG, COMMIT).

app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitCommitTests.java (1)

139-139: Use meaningful path strings in tests
A placeholder string "path" may limit test clarity and could be replaced with a more context-aware value for better maintainability.

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

212-213: Consider unifying JSON construction methods for consistency.
You're calling constructArtifactExchangeJsonFromGitRepositoryWithAnalytics here, while line 617 calls constructArtifactExchangeJsonFromGitRepository. If consistent instrumentation is desired for all reconstructions, consider streamlining to a single approach.


366-366: Use a more descriptive name than saveArtifactToLocalRepoNew.
The suffix "New" might become confusing over time. A more specific name can better convey purpose or differences from the old method.


375-375: Validate error leakage or adjust logging strategy.
Returning e.getMessage() may reveal internal details. Consider either adding contextual info or logging the error while returning a more general message to the client.

app/server/appsmith-server/src/test/java/com/appsmith/server/migrations/JsonSchemaMigrationTest.java (1)

58-58: Confirm the new parameter usage logic.
Similar to the previous comment, please validate that passing null for the additional parameters won't cause issues. If these parameters are truly optional, consider a more explicit default or optional pattern to make the intent clearer.

app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (1)

653-653: Avoid uninitialized refType
Explicitly initializing refType to null can obscure actual usage. Consider providing a helpful default.

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

476-485: Clean separation of migration logic
performJsonMigration neatly delegates JSON upgrades. Consider logging potential errors for better observability.

 } catch (SomeException e) {
+   log.error("Json migration failed for {}", artifactExchangeJson, e);
    return Mono.error(e);
 }
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)

163-175: Better error handling
Catching and returning a Mono.error() is good. Consider capturing more context before throwing.


560-593: Analytics wrapper
Using analytics around constructArtifactExchangeJsonFromGitRepository provides insight. Watch for performance overhead.

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

53-54: Consider adding robust documentation and error path handling.
The newly introduced constructGitResourceMapFromGitRepo method is a valuable addition. Please consider adding comprehensive Javadoc to clarify usage expectations and error conditions. This will help future maintainers understand the contract more easily.

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

478-478: Revisit error handling.

For checkoutRemoteReference, ensure that any error leading to an invalid baseArtifact or refDTO is properly caught and handled to avoid partial checkouts.


733-733: Validate the baseArtifactId parameter.

In deleteGitReference, ensure the parameter baseArtifactId has a valid format before proceeding. Consider adding unit tests to confirm correct handling of invalid IDs.


1477-1478: Enhance naming consistency.

getStatusAfterComparingWithRemote and getStatus share similar responsibilities. Clarify method names or add docstrings to highlight differences, if any.


1491-1491: Guard isFileLock usage carefully.

Take extra care with the isFileLock flag to avoid locking issues under heavy concurrency. Possibly add a fallback or lock acquisition timeout check.


1873-1873: Watch out for partial references.

When passing RefType refType, ensure consistency with all usage references. Mismatched ref types can lead to incomplete fetch or checkout operations.


1878-1878: Revise exception messages.

In fetchRemoteChanges, refine error messages to be more descriptive. This might ease future debugging, especially if multiple steps can fail during the fetch process.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1405aa6 and 8877a5b.

📒 Files selected for processing (24)
  • app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (1 hunks)
  • app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (3 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (2 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 (14 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (9 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (4 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (3 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java (3 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/CreateDBTablePageSolutionCEImpl.java (2 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java (1 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/AutoCommitServiceTest.java (4 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitCommitTests.java (4 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitConnectTests.java (10 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitDiscardTests.java (5 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitFileUtilsTest.java (1 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (2 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/migrations/JsonSchemaMigrationTest.java (2 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/transactions/ImportApplicationTransactionServiceTest.java (1 hunks)
  • app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderExtension.java (1 hunks)
🔇 Additional comments (77)
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java (2)

6-6: New import for RefType introduced.
Looks good. This import is essential for referencing the new parameter in the migration method.


439-439: Consider verifying the RefType parameter’s value.
Currently, the test uses any(RefType.class). It might be beneficial to use a more specific matcher or an argument captor to ensure the actual RefType is passed correctly. This helps validate logic under different reference types.

app/server/appsmith-server/src/test/java/com/appsmith/server/transactions/ImportApplicationTransactionServiceTest.java (1)

133-134: Consider verifying the new parameter usage in tests
Including the new parameter with null might hide logic or scenarios that rely on a non-null RefType. Ensure you either document or test the use of null to confirm no unintended side effects occur.

app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitCommitTests.java (3)

90-90: Ensure valid test data for local repo path
Returning an empty path might mask issues in actual scenarios. Consider confirming that this is intentional and needed for testing.


170-170: Validate the new parameter usage
The added "baseUrl" parameter should be verified downstream. Confirm it’s used or tested to ensure correctness.


228-228: Good coverage of repository not found scenario
This mock ensures the error path is tested thoroughly; great job.

app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (2)

364-365: Validate null usage for new method parameters
Consider verifying if null placeholders for the additional parameters can cause unintended behavior when migrating schemas.


2727-2728: Confirm partial copying and migration logic
Ensure that the partial copying of properties and subsequent migration both handle nested attributes safely.

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

617-617: Check alignment with analytics usage in line 212.
Here, constructArtifactExchangeJsonFromGitRepository is invoked without “withAnalytics.” Verify that you don’t need additional logging or instrumentation as in line 212.

app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java (1)

107-107: Consider verifying the correctness of passing all-null parameters.

Since the new method signature now accepts four parameters, and here all of them are null, please ensure the migration logic handles null values gracefully. This could prevent unforeseen NullPointerExceptions or other exceptions.

You can run this script to identify other usage sites of migrateArtifactExchangeJsonToLatestSchema and confirm whether null parameters are universally handled:

✅ Verification successful

Let me check the implementation of the method to understand how null parameters are handled.


Let me check the implementation of migrateApplicationJsonToLatestSchema to ensure null parameters are handled properly.


Let me try a different approach to find the method implementation.


Passing null parameters appears to be safe in this context

From the evidence gathered:

  1. The method is used in multiple test files with null parameters consistently
  2. The implementation in JsonSchemaMigration.java shows that it only uses artifactExchangeJson for type checking, while baseArtifactId, refName, and refType are optional parameters passed through
  3. The production usage in ApplicationGitFileUtilsCEImpl.java shows proper parameter passing when needed
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching usage of the method across the codebase to check parameter usage
rg -A 5 "migrateArtifactExchangeJsonToLatestSchema"

Length of output: 9930


Script:

#!/bin/bash
# Find the implementation of migrateArtifactExchangeJsonToLatestSchema
ast-grep --pattern 'public Mono<? extends ArtifactExchangeJson> migrateArtifactExchangeJsonToLatestSchema($$$) {
  $$$
}'

Length of output: 1650


Script:

#!/bin/bash
# Find the implementation of migrateApplicationJsonToLatestSchema
ast-grep --pattern 'private Mono<ApplicationJson> migrateApplicationJsonToLatestSchema($$$) {
  $$$
}'

Length of output: 104


Script:

#!/bin/bash
# Search for migrateApplicationJsonToLatestSchema implementation
rg -A 10 "migrateApplicationJsonToLatestSchema.*\{" 

Length of output: 53

app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderExtension.java (1)

115-115: 🛠️ Refactor suggestion

Consider providing non-null parameters or clarifying intent.
Currently, all additional parameters are passed as null, which may lead to potential NullPointerExceptions or missed migrations if the method relies on them. If this is intentional, consider documenting why nulls are acceptable; otherwise, pass meaningful references or overload the function to match usage scenarios.

To verify correctness and potential usage of these parameters elsewhere, run:

app/server/appsmith-server/src/test/java/com/appsmith/server/migrations/JsonSchemaMigrationTest.java (1)

42-42: Check passing null parameters.
You are passing null arguments for newly introduced parameters here. Confirm that your migration logic safely handles these null values, or consider introducing a default reference or fallback enum value rather than relying on null.

app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (4)

3-3: Import statement usage
It's great to see RefType being imported. Ensure it's leveraged effectively to avoid null references.


657-657: Nice retrieval of refType
Accessing getRefType() from the metadata is a good approach.


660-661: Validate fallback logic
Ensure refName and refType are handled gracefully when absent to prevent migration errors.


647-647: ⚠️ Potential issue

Null refType might cause unexpected migrations
Calling migrateApplicationJsonToLatestSchema with null refType could break certain Git flows.

- return jsonSchemaMigration.migrateApplicationJsonToLatestSchema(applicationJson, null, null, null);
+ return jsonSchemaMigration.migrateApplicationJsonToLatestSchema(applicationJson, null, null, RefType.branch /* or a valid fallback */);

Likely invalid or redundant comment.

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

33-33: Import statement
Including ArtifactJsonTransformationDTO is consistent with the new JSON migration flow.


472-472: Explicitly using RefType.branch
Passing RefType.branch helps clarify the reference context in JSON migrations.

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

33-33: New import for ArtifactJsonTransformationDTO
This aligns with the updated migration architecture.


595-620: Step-by-step reconstruction
constructArtifactExchangeJsonFromGitRepository is well-structured. Validate all fields in gitResourceMap to steer clear of partial data.


627-641: Metadata copy
copyMetadataToArtifactExchangeJson helps keep versioning consistent. Great job ensuring schema versions are stored.


643-649: Validation logic
isJsonTransformationDTOValid is straightforward. If adding more fields, keep them consistent here too.

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

33-34: New migration method
performJsonMigration in this interface standardizes JSON transformation across implementations.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (9)

28-28: Reordered parameters
ArtifactType artifactType now appears early in the signature. Assure all caller methods are updated.


39-39: Consistent branching
Listing branches for an artifact with reordered parameters is more intuitive.


44-44: File-lock param usage
Including isFileLock ensures clarity when fetching remote changes. Good improvement.


51-51: Refined method signature
getStatus clarifies whether remote comparison is desired.


57-57: checkoutReference
Rearranging the artifact type parameter is consistent with the broader signature structure.


63-63: createReference
Note that artifact type is placed earlier for uniformity across Git methods.


66-66: deleteGitReference
Make sure the new parameter order is tested thoroughly, including edge cases.


69-69: updateProtectedBranches
Housing artifactType earlier fosters consistent method signatures.


76-76: getAutoCommitProgress
The newly placed artifact type parameter clarifies which artifact's commit progress is retrieved.

app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java (6)

3-3: Import for RefType looks good.
The import aligns well with the new reference-based approach. No concerns here.


45-47: Javadoc improvements confirm clarity.
The new Javadoc annotations for refName and refType are clear and descriptive. Good work on clarifying parameter usage.


50-50: Updated method signature.
Including refName and RefType in the signature enhances flexibility. Please ensure that the new parameters are thoroughly tested with various reference types to confirm correctness.


54-54: Type safety on casting.
Casting artifactExchangeJson to ApplicationJson is valid if guaranteed at runtime. If there's any risk of encountering other artifact types, consider adding checks or exceptions for clarity.


61-61: Extended parameters for migration.
Accepting refName and RefType in migrateApplicationJsonToLatestSchema further generalizes the migration logic. Looks good.


71-71: Potential oversight: refType usage.
Although refType is introduced in the parents, it isn’t propagated here. Confirm whether omitting it from migrateServerSchema is intentional or an oversight.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (9)

71-71: New parameter ordering looks consistent.
connectArtifactToGit correctly includes ArtifactType before the DTOs. No issues spotted.


98-98: Consistent use of new parameter ordering.
createReference now properly takes ArtifactType before the gitRefDTO. All good.


107-107: Clearer readability with new argument sequence.
checkoutReference continues the pattern of placing ArtifactType up front, which helps maintain consistency across methods.


136-136: Method parameters are comprehensible.
getStatus includes compareRemote and GIT_TYPE in a logical order, matching the rest of the code changes.


147-147: RefType addition for remote fetching.
Enabling different references with the refType parameter in fetchRemoteChanges is a good enhancement for Git workflows.


157-157: Delete reference maintains standard approach.
deleteGitReference with the new ordering remains consistent with allied service calls.


176-176: Branch protection updates remain straightforward.
Positioning ArtifactType before the branch list keeps uniformity with the other methods.


202-202: Auto-commit progress parameter alignment.
getAutoCommitProgress references branchName after ArtifactType— consistent with the new standard.


222-222: Branch listing logic tidy.
listBranchForArtifact likewise exhibits matching ordering. Everything looks harmonized across methods.

app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitDiscardTests.java (5)

123-123: New save method.
Replacing older methods with saveArtifactToLocalRepoNew is fine but consider adding coverage to ensure any differences in behavior are validated.


154-154: Updated signature for connectArtifactToGit.
Placing ArtifactType and GitConnectDTO earlier in the call aligns with the rest of the codebase changes.


170-170: Passing null for new parameters.
Here, refName and refType are null. Confirm if the migration logic gracefully handles this scenario.


201-201: Additional usage of saveArtifactToLocalRepoNew.
Ensure that repeated calls with the same arguments are thoroughly tested for correct file path resolution.


246-246: Repeated test call.
Verify that the underlying logic remains consistent across multiple test invocations of saveArtifactToLocalRepoNew.

app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitFileUtilsTest.java (1)

87-88: Consider specifying a non-null RefType
Ensure this additional parameter is intentionally set to null. If the migration requires a known reference (e.g. branch-related context), consider providing a more explicit value like RefType.BRANCH.

app/server/appsmith-server/src/test/java/com/appsmith/server/git/ops/GitConnectTests.java (10)

87-87: Verify parameter reordering
The method call’s parameters now appear in a different order. Please confirm that all usages of connectArtifactToGit across the codebase match this updated signature.


111-111: Verify parameter reordering


154-154: Verify parameter reordering


174-174: Verify parameter reordering


254-254: Verify parameter reordering


292-292: Verify parameter reordering


328-328: Verify parameter reordering


378-378: Verify parameter reordering


424-424: Verify parameter reordering


473-473: Verify parameter reordering

app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java (4)

5-5: Thank you for adding the RefType import
This import is now used to support multi-parameter schema migrations.


259-259: Validate the new RefType usage
Ensure that passing any(RefType.class) or a specific RefType aligns with the intended test scenario.


548-548: Validate the new RefType usage


622-622: Validate the new RefType usage

app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/CreateDBTablePageSolutionCEImpl.java (2)

7-7: Import for RefType
This import clarifies the reference context used in schema migrations.


254-255: RefType usage in JSON migration
Good to see RefType.branch specified for clarity. This addresses the missing reference type in earlier calls and ensures consistent migration behavior.

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

367-367: Parameter ordering is consistent but verify usage references.

Looks like the parameter order has changed for checkoutReference. Make sure that all callers reflect this new order consistently throughout the codebase.


426-426: Double-check finalRefName usage.

Setting jsonTransformationDTO.setRefName(finalRefName) is fine, but confirm that upstream references align with this final name for all subsequent operations.


563-563: Check for null or missing fields.

When creating references, confirm that referencedArtifactId and refDTO are validated for null or missing required fields to prevent unexpected failures down the line.


1483-1484: Ensure remote comparison logic is correct.

When compareRemote is true, confirm that the code properly fetches and merges changes from remote. Missing or partial checks could lead to stale data in your output.


1871-1871: Validate refArtifactId carefully.

In fetchRemoteChanges, confirm that refArtifactId is not null or empty to avoid silent no-op or cryptic errors. Consider throwing an explicit exception if it's missing.


2395-2395: Consider boundary conditions.

When dealing with updateProtectedBranches, think about edge cases like an empty list or multiple default branches. This ensures robust behavior across all scenarios.

app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java (1)

295-296: Validate the additional parameter.

Adding another parameter to migrateArtifactExchangeJsonToLatestSchema is fine, but ensure that all solutions using the previous signature are properly updated to avoid runtime exceptions.

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

677-683: Confirm the need for an explicit checkout step.

constructGitResourceMapFromGitRepo fetches the Git resource map but may need to ensure the correct checkout of refName first. Verify that upstream code has performed the necessary checkout logic.

* @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,
ArtifactType artifactType,
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

Look out for potential concurrency pitfalls.

Multiple calls to getBaseAndBranchedArtifacts in a short time frame might need additional synchronization to prevent race conditions in code branches that depend on artifact states.

Copy link

github-actions bot commented Jan 3, 2025

Failed server tests

  • com.appsmith.server.services.ce.ActionServiceCE_Test#invalidCreateActionNullName

@sondermanish sondermanish self-assigned this Jan 3, 2025
@sondermanish sondermanish added the ok-to-test Required label for CI label Jan 3, 2025
@sondermanish sondermanish merged commit 649338d into release Jan 5, 2025
52 checks passed
@sondermanish sondermanish deleted the chore/resource-map branch January 5, 2025 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants