chore: CE split for git constants and AGF #38464
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request involves a significant refactoring of constants and artifact handling in the Appsmith Git-related classes. The changes primarily focus on transitioning from Changes
Suggested labels
Suggested reviewers
Poem
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-git/src/main/java/com/appsmith/git/constants/ce/CommonConstantsCE.java (1)
1-38: Consider makingfileFormatVersiona primitive and final.If
fileFormatVersionis not intended to change at runtime, marking it aspublic static final intrather thanIntegerimproves clarity and prevents accidental reassignment.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/server/appsmith-git/src/main/java/com/appsmith/git/constants/CommonConstants.java(1 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/constants/ce/CommonConstantsCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ArtifactGitFileUtils.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(2 hunks)
🔇 Additional comments (11)
app/server/appsmith-git/src/main/java/com/appsmith/git/constants/CommonConstants.java (1)
3-5: Inheritance approach looks fine.Extending
CommonConstantsCEcentralizes all constants in one shared base class, which is consistent with the consolidation strategy.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
87-87: Refactor to ArtifactExchangeJson
This protected field is now typed withArtifactExchangeJson, aligning well with the broader refactoring effort. No immediate issues.
104-104: Constructor param type
The constructor now acceptsArtifactGitFileUtils<ArtifactExchangeJson>instead ofApplicationGitReference. Ensure all references and usage sites are updated.✅ Verification successful
Constructor parameter change is correctly implemented
The change from
ApplicationGitReferencetoArtifactGitFileUtils<ArtifactExchangeJson>in the constructor is properly handled:
- The constructor parameter is used correctly as a class field
- The implementation class
ApplicationGitFileUtilsCEImplimplementsArtifactGitFileUtilsCE<ArtifactExchangeJson>interface- All usages of the parameter are consistent with the new type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check any references of 'CommonGitFileUtilsCE' constructor calls to confirm correct parameter usage rg -A 3 "new\s+CommonGitFileUtilsCE\s*\("Length of output: 43
Script:
#!/bin/bash # Search for constructor calls using ast-grep for better accuracy ast-grep --pattern 'new CommonGitFileUtilsCE($$$)' # Also search for class definition and constructor to understand the parameters ast-grep --pattern 'class CommonGitFileUtilsCE { $$$ CommonGitFileUtilsCE($$$) { $$$ } $$$ }' # Search for any references to the class name rg -l "CommonGitFileUtilsCE"Length of output: 65962
Script:
#!/bin/bash # Search for files containing CommonGitFileUtilsCE rg -l "CommonGitFileUtilsCE" # Search for constructor implementations with context rg -A 5 "CommonGitFileUtilsCE\s*\([^)]*\)\s*\{" # Search for constructor calls with context rg -A 5 "new\s+CommonGitFileUtilsCE\s*\("Length of output: 426
Script:
#!/bin/bash # Get the content of the files to understand the implementation and usage cat app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java # Get the content of the implementation class cat app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java # Get the content of the non-CE class cat app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.javaLength of output: 81046
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ArtifactGitFileUtils.java (2)
3-3: New import for ArtifactExchangeJson
This import is part of the broader migration toArtifactExchangeJson. No concerns.
6-6: Refactor to use ArtifactExchangeJson
Switching the generic type toArtifactExchangeJsonaligns with the new artifact model.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsImpl.java (2)
5-5: Import for ArtifactExchangeJson
The import signals the transition to the new artifact structure.
15-15: Implements ArtifactGitFileUtils with the new type
ReplacingApplicationGitReferencewithArtifactExchangeJsonis consistent with the overall codebase changes.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java (2)
12-12: Updated generic constraint
Switched from<T extends ArtifactGitReference>to<T extends ArtifactExchangeJson>, reflecting the new artifact approach.
14-14: Method return type mismatch
The method returnsArtifactGitReferencealthoughTextendsArtifactExchangeJson. Consider whether the return type should matchArtifactExchangeJsonor remainArtifactGitReference.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java (2)
7-7: Import for ArtifactExchangeJson
This import is consistent with the refactoring to useArtifactExchangeJson.
24-24: Constructor param signature
Aligning the constructor withArtifactGitFileUtils<ArtifactExchangeJson>ensures consistency across the refactored code.
| @@ -85,7 +85,7 @@ | |||
| @Slf4j | |||
| @Component | |||
| @Import({FileUtilsImpl.class}) | |||
| public class ApplicationGitFileUtilsCEImpl implements ArtifactGitFileUtilsCE<ApplicationGitReference> { | |||
| public class ApplicationGitFileUtilsCEImpl implements ArtifactGitFileUtilsCE<ArtifactExchangeJson> { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure method return types match the new generic parameter.
This class now implements ArtifactGitFileUtilsCE<ArtifactExchangeJson>, yet it still declares createArtifactReferenceObject() returning ApplicationGitReference. Revisit and align the return types/methods to prevent confusion or type mismatches.
| @@ -12,7 +12,7 @@ | |||
|
|
|||
| @Component | |||
| public class ApplicationGitFileUtilsImpl extends ApplicationGitFileUtilsCEImpl | |||
| implements ArtifactGitFileUtils<ApplicationGitReference> { | |||
| implements ArtifactGitFileUtils<ArtifactExchangeJson> { | |||
There was a problem hiding this comment.
will have EE implications probably
| @@ -102,7 +101,7 @@ public class CommonGitFileUtilsCE { | |||
| protected final ObjectMapper objectMapper; | |||
|
|
|||
| public CommonGitFileUtilsCE( | |||
| ArtifactGitFileUtils<ApplicationGitReference> applicationGitFileUtils, | |||
| ArtifactGitFileUtils<ArtifactExchangeJson> applicationGitFileUtils, | |||
There was a problem hiding this comment.
Shouldn't this be applicationJson?
| @@ -12,7 +12,7 @@ | |||
|
|
|||
| @Component | |||
| public class ApplicationGitFileUtilsImpl extends ApplicationGitFileUtilsCEImpl | |||
| implements ArtifactGitFileUtils<ApplicationGitReference> { | |||
| implements ArtifactGitFileUtils<ArtifactExchangeJson> { | |||
There was a problem hiding this comment.
This should be ApplicationJson?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java (2)
16-18: Consider providing a more meaningful default implementation.The default implementation returning null could lead to NPEs. Consider throwing UnsupportedOperationException or providing a more meaningful default.
- default ArtifactGitReference createArtifactReferenceObject() { - return null; - } + default ArtifactGitReference createArtifactReferenceObject() { + throw new UnsupportedOperationException("Operation not supported by default"); + }
29-30: Consider documenting the empty default implementation.Empty default implementations should be documented to explain why no implementation is needed.
+ /** + * Default empty implementation for backward compatibility. + * Implementing classes can override if needed. + */ default void addArtifactReferenceFromExportedJson( ArtifactExchangeJson artifactExchangeJson, ArtifactGitReference artifactGitReference) {}app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (1)
124-124: Method visibility changed to protected.The
getArtifactBasedFileHelpermethod is nowprotectedinstead ofprivate, allowing subclasses to override or access this method. This change suggests preparation for extensibility.Consider documenting the intended use cases for subclasses that might need to override this method.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java (3)
6-7: LGTM: Exception imports added for error handling.The addition of AppsmithError and AppsmithException imports supports the new error handling in default methods.
24-27: LGTM: Appropriate error handling for unsupported operation.The default implementation correctly throws an AppsmithException with UNSUPPORTED_OPERATION error.
14-14: Verify impact of generic type constraint change.The interface's generic type constraint has been updated from
ArtifactGitReferencetoArtifactExchangeJson. This is a significant change that could affect implementing classes.✅ Verification successful
Let me search for implementations and usages of these types to verify the impact of this change. The first command will help identify classes implementing the interface, while the second will show where these types are used in the codebase.
The generic type constraint change has been properly implemented across the codebase
The search results confirm that:
- The interface
ArtifactGitFileUtilsCEhas been updated to useArtifactExchangeJsonas its generic type constraint- The only implementation
ApplicationGitFileUtilsCEImplcorrectly implements the interface withArtifactExchangeJson- All usages of the interface and implementation are consistent with this change
The change appears to be part of a larger refactoring to standardize on
ArtifactExchangeJsonas the base type for artifact exchange operations. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all implementations of this interface to verify they've been updated ast-grep --pattern 'class $_ implements $$$ArtifactGitFileUtilsCE<$_>$$$'Length of output: 73130
Script:
#!/bin/bash # Find all implementations of this interface rg -l "implements.*ArtifactGitFileUtilsCE" # Find all references to ArtifactGitReference and ArtifactExchangeJson rg "ArtifactGitReference|ArtifactExchangeJson"Length of output: 69428
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
104-104: Constructor parameter type updated to match field type.The constructor parameter type has been updated to maintain consistency with the field type change.
87-87: Verify the impact of type parameter change.The field type has changed from
ArtifactGitFileUtils<ApplicationGitReference>toArtifactGitFileUtils<ApplicationJson>. This is a significant change that could affect Git operations.✅ Verification successful
Let me verify the usage of
ApplicationGitReferencein the codebase to understand the impact of the type parameter change.
Type parameter change is safe to proceed
The change from
ArtifactGitFileUtils<ApplicationGitReference>toArtifactGitFileUtils<ApplicationJson>is consistent with the implementation. TheApplicationGitFileUtilsCEImplandApplicationGitFileUtilsImplalready useArtifactExchangeJsonas their type parameter, andApplicationJsonextendsArtifactExchangeJson. The change aligns the field type with its actual usage in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to ApplicationGitReference rg "ApplicationGitReference" -A 3 -B 3 # Check for any classes extending or implementing ArtifactGitFileUtils ast-grep --pattern 'class $_ implements ArtifactGitFileUtils<$_> { $$$ }'Length of output: 63085
Script:
#!/bin/bash # Check for any remaining references to ArtifactGitFileUtils rg "ArtifactGitFileUtils" -A 3 -B 3 # Check for any classes implementing ArtifactGitReference ast-grep --pattern 'class $_ implements ArtifactGitReference { $$$ }'Length of output: 20048
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (1)
Line range hint
132-140: Consider deprecatingcreateArtifactReferenceObject()Since the class now uses
ApplicationJsonas its type parameter, thecreateArtifactReferenceObject()method returningApplicationGitReferenceseems out of place. Consider:
- Deprecating this method
- Updating callers to use
createArtifactExchangeJsonObject()- Planning for removal in a future release
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (2)
Line range hint
142-236: Consider breaking down complex resource handling logicThe
setArtifactDependentResourcesmethod is handling multiple concerns:
- Application resource mapping
- Metadata handling
- Pages and widgets processing
Consider extracting these into separate private methods for better maintainability and testability.
Example structure:
public void setArtifactDependentResources(...) { ApplicationJson applicationJson = (ApplicationJson) artifactExchangeJson; Map<GitResourceIdentity, Object> resourceMap = gitResourceMap.getGitResourceMap(); - // application - Application application = applicationJson.getExportedApplication(); - // ... current implementation + setApplicationResource(applicationJson, resourceMap); + setMetadataResource(applicationJson, resourceMap); + setPagesAndWidgetsResources(applicationJson, resourceMap); } +private void setApplicationResource(ApplicationJson applicationJson, Map<GitResourceIdentity, Object> resourceMap) { + // Extract application handling logic +}
Line range hint
676-684: Enhance error messages for JSON parsing failuresThe error message for JSON parsing failures could be more descriptive to aid in debugging. Consider including more context such as the file path or specific parsing stage.
- page.getUnpublishedPage().getName(), + String.format("Failed to parse DSL for page '%s'. Error: %s", + page.getUnpublishedPage().getName(), + e.getMessage()),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsImpl.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsImpl.java
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (1)
88-88: Verify type parameter change impact across the codebaseThe change from
ApplicationGitReferencetoApplicationJsonas the type parameter is significant. While the implementation looks correct, we should verify all usages of this class to ensure they handle the new type parameter correctly.✅ Verification successful
Let me check for any usage of this class as a dependency to ensure the type parameter change is handled correctly.
Let me check where this class is being used in the application context and verify the bean configuration.
Let me check one final aspect - the service layer implementations that might be using this utility.
Type parameter change is correctly implemented
The change from
ApplicationGitReferencetoApplicationJsonis properly aligned with the interface hierarchy. The implementation correctly follows the contract where:
ArtifactGitFileUtilsCE<T extends ArtifactExchangeJson>is the base interfaceApplicationJsonsatisfies the boundArtifactExchangeJson- Both
ApplicationGitFileUtilsCEImpland its subclassApplicationGitFileUtilsImplconsistently useApplicationJson🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to ApplicationGitFileUtilsCEImpl to verify type parameter usage rg "ApplicationGitFileUtilsCEImpl" -A 2Length of output: 1425
Script:
#!/bin/bash # Search for bean definitions and dependency injections rg -l "ApplicationGitFileUtils|@Autowired.*ApplicationGitFileUtils" # Search for the interface definition to understand the contract ast-grep --pattern 'interface ArtifactGitFileUtilsCE<$_> { $$$ }'Length of output: 361
Script:
#!/bin/bash # Search for configuration classes that might be setting up these beans rg -l "@Configuration.*class" | xargs rg "ApplicationGitFileUtils" -A 3 # Check for any direct usage of this class in service layer rg "ApplicationGitFileUtils.*applicationGitFileUtils" -A 2Length of output: 2429
Script:
#!/bin/bash # Search for service classes that might be using this utility rg "@Service" -l | xargs rg "ApplicationGitFileUtils" -B 2 -A 2 # Check for the interface hierarchy rg "interface.*ArtifactGitFileUtils" -A 2Length of output: 752
Failed server tests
|
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
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
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12626330445
Commit: dd115a5
Cypress dashboard.
Tags: @tag.Git
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Mon, 06 Jan 2025 04:09:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Refactor
ArtifactExchangeJsoninstead ofApplicationGitReference.Chores