chore: Moved git auth creation and updation to generified artifactservice #38312
Conversation
WalkthroughThe pull request introduces significant modifications to the application's artifact and service management architecture. Key changes include the introduction of a new Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (10)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceImpl.java (1)
11-14: Constructor clarity
The constructor is straightforward and properly calls the parent class constructor with the correct parameters. In future, consider adding a brief Javadoc comment if additional context is needed.app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermissionCE.java (1)
7-7: Great addition for artifact edit operations.Adding a dedicated method for edit permission enhances clarity and allows for more granular control. Consider documenting the intended usage, especially if new implementers need guidance.
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java (3)
39-40: Question the one-size-fits-all return of applicationService
Currently, getArtifactBasedService always returns applicationService, regardless of the ArtifactType. If more artifact types are introduced, consider either introducing a switch case or a mapping structure to handle future expansions.
57-71: Consider unifying root artifact logic
Lines 57–60 handle a root artifact that already has git data, and lines 65–71 handle a root artifact that has no git data. These branches are logically similar. Combining them or creating a helper function could make the code more concise.
107-112: Confirm acceptable fallback on analytics failure
The code logs a warning and proceeds if analyticsService.sendObjectEvent fails. Ensure this silent fallback is appropriate and doesn’t obscure significant errors that might require remediation.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java (2)
170-172: Clearer Documentation
The doc comment is informative. Consider adding explicit mention of the rationale for filtering out non-default branches.
959-962: Doc Clarification
The doc lines introduce mode-based retrieval. Consider making it easier to see how “branchName” and “defaultApplicationId” interplay in different environments.app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/artifactbased/ArtifactBasedServiceCE.java (1)
12-12: save Method
Accepts a generic Artifact, which is convenient. Again, confirm type safety for specialized artifacts.app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCE.java (1)
9-24: ArtifactServiceCE Overview
• The factory method getArtifactBasedService can improve extensibility.
• createOrUpdateSshKeyPair centralizes SSH key logic. Verify usage in all relevant places.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java (1)
3552-3553: Generating SSH key pair in chained flow.
When chaining multiple .flatMap calls, ensure that any errors are caught. Consider adding negative tests for coverage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactService.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/artifactbased/ArtifactBasedService.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/artifactbased/ArtifactBasedServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermission.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermissionCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationControllerCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ImportArtifactPermissionProvider.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ArtifactPermission.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationPermissionCE.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java(5 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ArtifactPermission.java
✅ Files skipped from review due to trivial changes (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactService.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermission.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ApplicationPermissionCE.java
🔇 Additional comments (24)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceImpl.java (3)
1-1: Package structure alignment
The package declaration follows a logical structure. No issues here.
3-7: Imports and annotations
The imports and the @service annotation are well-organized.
9-9: Class naming and hierarchy
This class name is clear, extends the correct parent, and implements the needed interface.
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermissionCE.java (1)
1-1: Package move aligns with the new artifact-based architecture.
Renaming the package from “solutions.ce” to “artifacts.permissions” is consistent with placing all artifact permission interfaces under a dedicated package. Ensure that references in the codebase have been updated accordingly.
✅ Verification successful
Let me verify the package move and its references by searching through the codebase.
Let me check the actual content and references of the ArtifactPermissionCE file to verify the package move.
Let me check if this is a new file or moved from the solutions package.
Package move is part of a new artifact-based architecture implementation
Based on the evidence:
- This is a new interface in the artifacts.permissions package, not moved from solutions
- It serves as a base interface for permission-related functionality in the new artifact-based architecture
- Other permission interfaces still exist in solutions.ce package, suggesting a gradual transition
- The interface is properly referenced by ArtifactPermission as its parent interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for references to the old package name "solutions.ce"
rg -l "solutions\.ce"
# Search for references to the new package name "artifacts.permissions"
rg -l "artifacts\.permissions"
# Find all Java files that might be importing or using ArtifactPermissionCE
rg -l "ArtifactPermissionCE"
# Check git history for the file to understand the package move
git log --oneline -- app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermissionCE.java
Length of output: 11323
Script:
#!/bin/bash
# Check the content of ArtifactPermissionCE
cat app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermissionCE.java
# Find references to ArtifactPermissionCE with context
rg -A 2 "ArtifactPermissionCE" --type java
Length of output: 1053
Script:
#!/bin/bash
# Check git history to see if this was moved from solutions package
git log --follow --name-status app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/permissions/ArtifactPermissionCE.java
# Search for any similar interface in solutions package
rg -l "interface.*Permission.*CE.*\{" --type java app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/
Length of output: 1892
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ImportArtifactPermissionProvider.java (2)
4-4: Refactored import to new package is aligned with artifact-centric architecture.
This change cleanly transitions the reference to “ArtifactPermission” into the dedicated “artifacts.permissions” package, reflecting the broader reorganization of permission handling for artifacts.
Line range hint 28-28: Add coverage for the new “requiredPermissionOnTargetArtifact” field in unit tests.
This extra permission field in the builder is crucial for artifact-related operations. Ensure all relevant test cases cover scenarios requiring this permission to avoid regressions.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ImportArtifactPermissionProviderCE.java (1)
6-6: Confirm artifact permission import consistency.
This line changes the import location of ArtifactPermission to a new package, aligning with the refactoring towards a dedicated artifacts namespace. Please verify that all references and usages of ArtifactPermission in related classes and packages are updated accordingly.
Run this script to ensure no leftover references to the old import exist:
✅ Verification successful
The previous search didn't find old imports, but let's verify all current imports and usages to ensure consistency. This will help us confirm if the new import path is being used correctly across the codebase.
Import change verified and consistent across codebase
All imports of ArtifactPermission consistently use the new package path com.appsmith.server.artifacts.permissions.ArtifactPermission. No references to the old import path remain in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for residual references to the old ArtifactPermission import
rg "import com\.appsmith\.server\.solutions\.ArtifactPermission"
Length of output: 66
Script:
#!/bin/bash
# Search for any imports of ArtifactPermission to verify consistency
rg "import.*ArtifactPermission"
# Search for usage of ArtifactPermission class to ensure all references are properly imported
ast-grep --pattern 'ArtifactPermission'
Length of output: 5855
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java (4)
6-6: New Import for Artifact Service
No immediate issues. Nice to see the artifact-based approach being modularized.
234-238: New Method getPermissionService
Method returns the correct artifact permission object. Good alignment with the new artifact-based structure.
79-79: Implements New Interface
Looks consistent with the new artifact-based interface. Remember to confirm that all method contracts are correctly implemented.
✅ Verification successful
Let me verify the implementation of the interface methods by searching through the codebase.
Let me try a different approach to find the interface and implementation details.
Let me check the implementation of the required methods from the interface.
Interface Implementation Verified Successfully
The ApplicationServiceCEImpl correctly implements both required methods from ArtifactBasedServiceCE interface: findById(String id, AclPermission aclPermission) and save(Artifact artifact) with proper type parameters and signatures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's find the ArtifactBasedServiceCE interface definition
ast-grep --pattern 'interface ArtifactBasedServiceCE<$_> {
$$$
}'
# Then, let's check the implementation in ApplicationServiceCEImpl
ast-grep --pattern 'class ApplicationServiceCEImpl $$$implements ApplicationServiceCE, ArtifactBasedServiceCE<Application> {
$$$
}'
Length of output: 205
Script:
#!/bin/bash
# Search for files containing ArtifactBasedServiceCE
fd -e java | xargs rg "ArtifactBasedServiceCE" -B 2 -A 5
# Search for method implementations in ApplicationServiceCEImpl
fd "ApplicationServiceCEImpl.java" | xargs rg "public.*implements.*ArtifactBasedServiceCE" -A 10
Length of output: 4385
Script:
#!/bin/bash
# Search for the required method implementations in ApplicationServiceCEImpl
fd "ApplicationServiceCEImpl.java" | xargs rg "public.*Mono<Application>.*findById|public.*Mono<Application>.*save" -B 1 -A 5
Length of output: 2104
218-219: Type Safety Check
Casting from Artifact to Application might cause runtime errors if non-Application artifacts are passed. Consider validating the type or using generics with safer constraints.
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/artifactbased/ArtifactBasedService.java (1)
1-5: New Interface
This interface is succinct and clearly extends the CE version with minimal overhead. Good layering.
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/artifactbased/ArtifactBasedServiceCE.java (2)
14-14: getPermissionService Method
This is a clean way to retrieve permissions for the artifact.
10-10: findById Method
Ensure that all permissions checks are correctly enforced.
✅ Verification successful
Let me search for the implementation and usage of this findById method to verify the permissions checks.
Let me search for the implementation details of the permission checks.
Let me search for the query builder implementation to understand how permissions are enforced.
Let me try a different search pattern to find the permission enforcement implementation.
Based on the code analysis, I can now provide a definitive response about the permission checks.
Permission Checks are Properly Enforced
The findById method correctly enforces permissions through the repository layer. The implementation in BaseAppsmithRepositoryCEImpl uses a query builder that validates the provided AclPermission against the permission groups in the policy map, ensuring proper access control.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the implementation of findById method in ArtifactBasedServiceCE
ast-grep --pattern 'public Mono<$_> findById(String $_, AclPermission $_) {
$$$
}'
# Search for all usages of findById method to check permission enforcement
rg "findById\(" -A 3
Length of output: 74093
Script:
#!/bin/bash
# Search for the base repository implementation to see how permissions are enforced
ast-grep --pattern 'class BaseAppsmithRepositoryCEImpl {
$$$
findById($_, $_, $_) {
$$$
}
$$$
}'
# Search for permission checks in query builder
rg "permission\(" -A 3 -B 3
Length of output: 118013
Script:
#!/bin/bash
# Search for the query builder implementation
ast-grep --pattern 'class $_ implements QueryBuilder {
$$$
permission($_, $_) {
$$$
}
$$$
}'
# Search for permission checks in repository implementation
ast-grep --pattern 'class BaseAppsmithRepositoryCEImpl {
$$$
queryBuilder() {
$$$
}
$$$
}'
Length of output: 211
Script:
#!/bin/bash
# Search for repository implementation with permission checks
rg "findById.*permission" -A 5 -B 5
# Search for query builder permission implementation
rg "permission\(.*\)" -A 5 -B 5 --type java
Length of output: 68920
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceImpl.java (2)
3-3: Artifact Service Import
Importing artifact-based service is consistent with broader refactoring.
25-26: Implements ArtifactBasedService
The extension harmonizes with the new structure, providing full artifact-based features for ApplicationServiceImpl.
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCE.java (2)
6-6: Import for Artifact domain appears consistent.
No issues with importing this domain class. Please ensure any additional fields or references within the Artifact object are documented for all callers.
32-32: Ensure usage alignment with the new parameter type.
Changing the parameter from Application to Artifact impacts callers. Verify that all consumers of this save method correctly build or convert their Application objects into an Artifact object.
✅ Verification successful
Let me search for usages of this save method to verify the parameter type change impact.
Let me try a different search pattern to find usages of this save method and its implementations.
Parameter type change is properly aligned with inheritance hierarchy
The search results show that the save(Artifact application) method is part of a proper inheritance chain:
ArtifactBasedServiceCEdefines the base contract withsave(Artifact artifact)ApplicationServiceCEinherits and maintains the same signatureApplicationServiceCEImplimplements it with proper type casting from Artifact to Application
No issues found with the parameter type change as it follows proper OOP principles and maintains type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for save method calls with Application/Artifact parameter
ast-grep --pattern 'save($_)'
# Search for direct references to ApplicationServiceCE
rg "ApplicationServiceCE" -A 2
# Search for implementations of save method
ast-grep --pattern 'save(Artifact $_) {
$$$
}'
Length of output: 65972
Script:
#!/bin/bash
# Search for ApplicationServiceCE class and its save method
ast-grep --pattern 'class ApplicationServiceCE {
$$$
save($_) {
$$$
}
$$$
}'
# Search for calls to save method with Application/Artifact parameter
rg "save\((Application|Artifact)" -A 2
Length of output: 1548
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/ApplicationControllerCE.java (2)
67-67: Field injection appears fine.
The ArtifactService dependency is clearly declared. Ensure consistent usage and confirm no cyclical dependencies exist.
254-255: Using ArtifactService for SSH key pair creation.
This change centralizes SSH key operations. Confirm that error scenarios (e.g., invalid branchedApplicationId) are handled robustly within the artifact service.
✅ Verification successful
Let me search for the error handling implementation in the artifact service.
Let me try a different search pattern to find the implementation.
Let me check the implementation and error handling in these identified files.
Error handling for SSH key pair creation is robust and well-implemented.
The implementation in ArtifactServiceCEImpl handles various error scenarios appropriately:
- Invalid branchedArtifactId throws NO_RESOURCE_FOUND exception
- Missing root artifact throws INVALID_GIT_CONFIGURATION exception
- Permission checks are enforced through artifactPermission.getEditPermission()
- Global exception handler properly formats and returns these errors to the client
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for ArtifactService implementation and error handling
ast-grep --pattern 'class $_ implements ArtifactService {
$$$
createOrUpdateSshKeyPair($$$) {
$$$
}
$$$
}'
# Also search for the interface definition
ast-grep --pattern 'interface ArtifactService {
$$$
createOrUpdateSshKeyPair($$$);
$$$
}'
# Look for any exception handling related to SSH keys
rg -A 5 "createOrUpdateSshKeyPair|SshKey" --type java
Length of output: 120
Script:
#!/bin/bash
# Search for ArtifactService class files
fd -e java "ArtifactService"
# Search for SSH key pair related code
rg -l "createOrUpdateSshKeyPair" --type java
# Look for exception handling patterns in artifact-related files
rg "throw\s+\w+Exception|@ExceptionHandler" --type java -l | xargs rg "Artifact|SSH|Key" -l
Length of output: 1077
Script:
#!/bin/bash
# Check the implementation in ArtifactServiceCEImpl
cat app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java
# Check the interface definition
cat app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCE.java
# Check exception handling
cat app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java
Length of output: 23992
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationServiceCETest.java (4)
17-18: New imports for ArtifactService and ArtifactType.
These imports align with the refactored SSH key creation flow. Ensure that these references don't break older tests relying on removed GitAuth calls.
181-183: Injecting ArtifactService in test class.
Confirm usage of this bean across test scenarios related to artifact-based functionalities. Avoid mixing older calls to applicationService for SSH key creation.
3563-3564: Creating SSH key pair for child application.
Keys remain null in child apps, which is the intended behavior. The test coverage looks appropriate. Keep verifying that business logic is consistent with the new artifact-based approach.
3522-3523: Creating or updating SSH key pair for newly saved application.
The test logic is sound. Validate that any test failures around key creation fail gracefully rather than silently passing.
✅ Verification successful
Let me gather information about the SSH key pair creation and error handling in the test.
Let me check how the SSH key pair creation is handled in the test and its error cases.
Let me try a different approach to find the test implementation and error handling.
Let me check the implementation of the SSH key pair creation method to understand the error handling.
Let me try one more search to find the implementation and error handling.
SSH key pair creation test has proper error handling and validation
The implementation in ArtifactServiceCEImpl already includes comprehensive error handling:
- Throws
AppsmithExceptionif artifact is not found - Validates root artifact existence
- Handles invalid git configuration cases
- Includes error handling for analytics events
The test in ApplicationServiceCETest verifies the success path with proper assertions on the generated keys and timestamps. The error cases are handled at the service layer with specific exceptions that would fail the test if thrown.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for SSH key pair related test cases and error handling
rg -l "createOrUpdateSshKeyPair" --type java
# Look for test assertions and error handling around SSH key creation
ast-grep --pattern 'artifactService.createOrUpdateSshKeyPair($$$)'
# Find related error handling test cases
rg "createOrUpdateSshKeyPair.*error|exception" --type java -A 3
Length of output: 70305
Script:
#!/bin/bash
# Look for the test method containing the SSH key pair creation
rg -A 10 "createOrUpdateSshKeyPair.*test" --type java
# Find the test assertions around SSH key pair creation
rg -B 5 -A 10 "artifactService.*createOrUpdateSshKeyPair.*assert" --type java
Length of output: 135
Script:
#!/bin/bash
# Find the test class and surrounding context
rg -B 10 -A 10 "createOrUpdateSshKeyPair.*savedApplication" --type java
# Look for error handling in the artifact service implementation
rg -B 5 -A 10 "class.*ArtifactServiceCEImpl.*implements.*ArtifactServiceCE" --type java
rg -B 5 -A 10 "createOrUpdateSshKeyPair.*throw" --type java
Length of output: 9043
Script:
#!/bin/bash
# Find the implementation of createOrUpdateSshKeyPair
rg -B 5 -A 15 "createOrUpdateSshKeyPair.*\{" --type java
# Look for error handling in the artifact service
rg -B 2 -A 5 "ArtifactServiceCE.*createOrUpdateSshKeyPair" --type java
Length of output: 131
Script:
#!/bin/bash
# Search for the method implementation in ArtifactServiceCEImpl
cat app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java
# Look for test assertions and error cases
rg -B 2 -A 5 "StepVerifier.*createOrUpdateSshKeyPair" --type java
Length of output: 6514
| // Fetch root application and then make updates. We are storing the git metadata only in root | ||
| // application | ||
| if (!StringUtils.hasLength(gitData.getDefaultArtifactId())) { | ||
| return Mono.error(new AppsmithException( | ||
| AppsmithError.INVALID_GIT_CONFIGURATION, | ||
| "Unable to find root " + artifactTypeName + ", please connect your " + artifactTypeName | ||
| + " to remote repo to resolve this issue.")); | ||
| } | ||
| gitAuth.setRegeneratedKey(true); | ||
|
|
||
| return artifactBasedService | ||
| .findById(gitData.getDefaultArtifactId(), artifactPermission.getEditPermission()) | ||
| .flatMap(baseApplication -> { | ||
| GitArtifactMetadata gitArtifactMetadata = baseApplication.getGitArtifactMetadata(); | ||
| gitArtifactMetadata.setDefaultApplicationId(baseApplication.getId()); | ||
| gitArtifactMetadata.setGitAuth(gitAuth); | ||
| baseApplication.setGitArtifactMetadata(gitArtifactMetadata); | ||
| return artifactBasedService.save(baseApplication); | ||
| }); | ||
| }) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the code analysis, I can now provide a final response:
Add optimistic locking to prevent concurrent SSH key updates
Add @Version field to GitAuth entity and use Spring's optimistic locking to handle concurrent modifications. This will ensure that if two processes try to update the SSH key simultaneously, one will fail with an OptimisticLockingFailureException rather than silently overwriting each other's changes.
Example locations that need attention:
GitAuth.java- Add version fieldArtifactServiceCEImpl.createOrUpdateSshKeyPair()- Handle the optimistic locking exception and retry if needed
🔗 Analysis chain
Flag potential concurrency scenario
If multiple clients simultaneously call this function for the same artifact, two processes might generate different SSH keys in rapid succession. Consider adding steps (e.g., optimistic locking or more robust concurrency control) to avoid inconsistent or lost updates.
Let me search for relevant code to verify the concurrency concerns.
Let me also check for any transaction or version control mechanisms in the artifact service:
This will help us understand if there are any existing concurrency controls and the full context of the SSH key generation flow.
Let me try a different search pattern to find the relevant code:
Let me check the underlying service implementation to understand if there are any concurrency controls:
Let me try one more search to find the Application entity and service implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the class and method definition to understand the context
ast-grep --pattern 'class $_ {
$$$
generateSSHKeyPair($$$) {
$$$
}
$$$
}'
# Search for any existing concurrency control mechanisms
rg -i "lock|synchronized|atomic|concurrent" app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/
Length of output: 205
Script:
# Search for transaction annotations or version fields
rg -i "@transaction|@version|optimisticlock" app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/
# Check the entity/model class for version fields
ast-grep --pattern 'class $_ {
$$$
@Version
private $_ version;
$$$
}'
Length of output: 199
Script:
#!/bin/bash
# Search for the artifact service implementation
rg -l "class.*ArtifactService.*Impl" app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/ | xargs cat
# Search for the entity class that might contain version field
fd "Artifact.*\.java" app/server/appsmith-server/src/main/java/com/appsmith/server/ -x cat {}
# Look for any save operations in the service
rg -A 5 "save\(" app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/
Length of output: 65961
Script:
# Search for the ArtifactBasedService implementation
fd "ArtifactBasedService.*Impl\.java" app/server/appsmith-server/src/main/java/ -x cat {}
# Search for any MongoDB optimistic locking annotations
rg -i "@version|@optimisticlock" app/server/appsmith-server/src/main/java/com/appsmith/server/domains/
Length of output: 198
Script:
# Search for Application entity class
rg -l "class Application" app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ | xargs cat
# Search for ApplicationService implementation
fd "ApplicationService.*Impl\.java" app/server/appsmith-server/src/main/java/ -x cat {}
# Look for any transaction or locking mechanisms in the service layer
rg -i "@transactional|@lock|synchronized" app/server/appsmith-server/src/main/java/com/appsmith/server/services/
Length of output: 77806
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ApplicationController.java (1)
Line range hint
26-38: Constructor injection
Including ArtifactService at the beginning of the parameter list clarifies its importance. Ensure any relevant usage is explained or documented so future maintainers understand why it’s now part of the constructor.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ApplicationController.java(3 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ApplicationController.java (1)
4-4: Import looks good
These changes properly import the new ArtifactService.
Failed server tests
|
| return artifactBasedService | ||
| .findById(branchedArtifactId, artifactPermission.getEditPermission()) | ||
| .switchIfEmpty(Mono.error( | ||
| new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, artifactTypeName, branchedArtifactId))) |
There was a problem hiding this comment.
Nit: ACL_NO_RESOURCE_FOUND is a preferred one
Edit: It's fine to leave as is, since it's not aligned with the intent of the PR
| @@ -248,7 +251,8 @@ public Mono<ResponseDTO<ArtifactImportDTO>> importApplicationFromFile( | |||
| @PostMapping("/ssh-keypair/{branchedApplicationId}") | |||
| public Mono<ResponseDTO<GitAuth>> generateSSHKeyPair( | |||
| @PathVariable String branchedApplicationId, @RequestParam(required = false) String keyType) { | |||
| return service.createOrUpdateSshKeyPair(branchedApplicationId, keyType) | |||
| return artifactService | |||
There was a problem hiding this comment.
QQ: What's the plan for the controller endpoints? Are we considering introducing dedicated endpoints for each artifact type when handling Git-related operations?
There was a problem hiding this comment.
Yes, the url path will be differentiated by artifact. This is to not introduce too many changes to the code base in one go. We're already in flux with feature based packages and the producer consumer patterns for features. We'll keep this in mind and think about moving to a single controller if it makes sense in the long run.
This is especially because as of today, feature parity need not exist between artifacts and the controller layer is our line of control.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCEImplTest.java (1)
290-290: Repeated stubbing.
You’re using doReturn() for the same method again. If multiple tests require the same stubbing, consider extracting a shared utility or a dedicated setup method to ensure consistency across tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/ApplicationControllerTest.java(0 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/autocommit/helpers/GitAutoCommitHelperImplTest.java(8 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCEImplTest.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationSnapshotServiceUnitTest.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/ApplicationControllerTest.java
🔇 Additional comments (28)
app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCEImplTest.java (2)
164-164: Stubbing approach is consistent.
Using doReturn() when() is appropriate here to avoid side effects. Confirm that your test remains encapsulated without relying on the real applicationService for unmocked methods.
69-69: Confirm that partial mocking is indeed needed.
Switching from @MockBean to @SpyBean for the ApplicationService will now preserve real method behavior, only overriding stubs where specified. Verify you truly require partial mocking for correct test coverage.
✅ Verification successful
Let me gather information about the test class and its usage of @SpyBean to verify if partial mocking is necessary.
@SpyBean usage is justified for ApplicationService
The partial mocking is necessary here as the test class uses applicationService in conjunction with findById method where real behavior is needed, while still allowing for potential stubbing of other methods. This is evident from the test setup where applicationService.findById is used to return an application with specific evaluation version for test scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the test file content to understand the usage of @SpyBean
cat "app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCEImplTest.java"
# Search for any mock/stub method calls on ApplicationService in the test file
rg "applicationService\." "app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCEImplTest.java" -B 2 -A 2
Length of output: 14872
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java (6)
81-81: Switching to @SpyBean looks good.
Using a spy instead of a mock is suitable for partial mocking. Ensure that you selectively mock critical methods to avoid unexpected interactions with real logic.
204-206: Stubbing with doReturn is appropriate.
This pattern is a good fit for partial mocking, ensuring synchronous returns from a reactive source. Continue to confirm actual method behaviors align with test expectations.
208-210: Consistent approach for partial mocking.
Again, stubbing the reactive call is properly done. Just double-check that the real methods aren’t unintentionally triggered.
228-230: Repeated stub usage is acceptable.
Ensuring consistent stubbing fosters reliable tests. Keep verifying that real methods only run where needed.
485-487: Maintaining uniformity in doReturn stubs.
This is in line with the partial mocking approach. Validate the underlying logic for your test scenario.
489-491: Reiterating the same stubbing strategy on findByBranchNameAndBaseApplicationId.
Continue to ensure that these calls are essential and that actual method calls are well tested.
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImplTest.java (15)
47-47: Switched from @MockBean to @SpyBean
This update allows partial mocking of real methods, ensuring more realistic test coverage. Good improvement!
84-86: Using doReturn for stubbing is appropriate
The switch to "doReturn(...).when()" matches the use of a spy object rather than a pure mock. This is the correct approach for partial mocks.
89-91: Consistent usage of doReturn for partial mocks
Repeated usage of doReturn for stubbing aligns well with the @SpyBean approach. This helps maintain clarity in test behavior.
109-111: Partial mock stubbing technique
Again, good job utilizing doReturn when stubbing methods on the spied service.
112-114: Uniform approach across multiple stubs
Maintaining consistency in the stubbing approach helps in readability and reduces confusion. This is well-aligned with @SpyBean usage.
131-133: Use of doReturn ensures correct invocation
This helps prevent calling the real method unintentionally, especially if the method might produce side effects. Nicely done!
136-138: Clarity in partial mocking logic
Continuing the same method stubbing pattern fosters test maintainability and better clarity.
164-166: Appropriate doReturn usage for real application service instance
The approach ensures minimal reliance on real dependencies, making tests more robust.
167-169: Consistent stubbing in test method
Repeated usage of the same doReturn pattern across methods is commendable, ensuring consistent testing practices.
263-265: Protecting real method calls
Using doReturn helps prevent unintended real method invocations that might affect test results. Looks good.
266-268: Ensuring consistent stubbing across application calls
Maintaining the uniform stubbing approach reduces confusion across tests.
299-301: SpyBean stubbing remains correct
Sustaining the doReturn approach allows a predictable, test-friendly environment.
303-305: Consistent mocking in server migration test
The uniform pattern assures consistency across multiple migration scenarios.
344-346: Reused partial mock approach for findById
Ensuring consistent test designs across the suite is a best practice. Good job maintaining it.
348-350: Stubbing matches the earlier lines
Continuing the same partial mock pattern cements a reliable testing methodology.
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java (1)
110-111: Verify test coverage with @SpyBean
The change from @MockBean to @SpyBean for ApplicationService is appropriate for allowing real method calls. However, ensure that the test suite adequately covers the ApplicationService methods that will now be executed.
Run the following script to check test coverage for ApplicationService methods used in these tests:
✅ Verification successful
Change from @MockBean to @SpyBean is appropriate
The change from @MockBean to @SpyBean for ApplicationService is correct and doesn't introduce any risks. The test class doesn't directly verify ApplicationService behavior, and the service is only injected as a dependency in ActionExecutionSolutionCEImpl. The real methods from ApplicationService are only used for analytics tracking, which is non-critical for the core test scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for ApplicationService methods
# Look for ApplicationService method calls in the test file
rg "applicationService\." app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java -A 2
# Look for test methods that might be affected by this change
ast-grep --pattern 'void test$_() {
$$$
applicationService.$_($$$)
$$$
}'
Length of output: 227
Script:
#!/bin/bash
# Let's try a broader search to understand the usage context
# First, check if applicationService is used in any test methods
rg "applicationService" app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java -B 2 -A 2
# Also check the implementation class to understand expected interactions
rg "ApplicationService" app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java -B 2 -A 2
# Look for any verify/when calls on applicationService in the test file
rg "(verify|when)\(applicationService" app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java
Length of output: 1527
Script:
#!/bin/bash
# Let's check the test class implementation to understand how ApplicationService is used
cat app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java
# Also check the implementation class to see what methods of ApplicationService are actually used
cat app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java
Length of output: 81358
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationSnapshotServiceUnitTest.java (4)
86-88: Mocking the findBranchedApplicationId method
Good use of doReturn for partial mocks to replace only targeted behavior.
27-27: Adopting partial mocking strategy
Adding the SpyBean import indicates partial mocking. Ensure that this approach is necessary and that services aren't unintentionally invoked.
✅ Verification successful
Let me verify the usage of @SpyBean in the test class and related files to understand the mocking strategy.
After analyzing the test file and the usage of @SpyBean across the codebase, I can provide a final response:
Partial mocking strategy is appropriate here
The use of @SpyBean for ApplicationService is justified as the test needs to selectively mock findBranchedApplicationId while allowing other methods to execute normally for snapshot creation and restoration tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the test file content to understand the spy usage
cat "app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationSnapshotServiceUnitTest.java"
# Find other usages of SpyBean in test files
rg "@SpyBean" --type java -A 2
Length of output: 52080
45-45: Using a spy bean for ApplicationService
This allows real method calls except stubs for specific methods. Verify there's no undesired behaviour from calling real code.
✅ Verification successful
Let me gather information about the usage of spy beans in the test class to verify if there are any potential issues.
Let me check the test class implementation to understand how the spy bean is being used.
Let me check the ApplicationService implementation to understand what real methods might be called.
Let me check the CE compatible service implementation.
Let me check the base CE implementation to understand the actual methods.
Based on the analysis of the test class and the ApplicationService implementation, I can now provide the final response.
@SpyBean usage is appropriate here
The spy bean for ApplicationService is used correctly in the test class. The test methods only mock specific methods (findBranchedApplicationId and findById) while allowing other real methods to be called. The mocked methods are well-defined and the test assertions verify the expected behavior without any undesired side effects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's look at the test class implementation to understand the context
rg -A 10 -B 10 "@SpyBean" "app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationSnapshotServiceUnitTest.java"
# Let's also check if there are any mocked methods or verifications on the spy
rg "when\(.*ApplicationService" "app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationSnapshotServiceUnitTest.java"
rg "verify\(.*ApplicationService" "app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationSnapshotServiceUnitTest.java"
Length of output: 961
Script:
#!/bin/bash
# Let's look at the full test class to understand the usage of applicationService spy
cat "app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ApplicationSnapshotServiceUnitTest.java"
# Also check the ApplicationService class to understand what methods might be called
fd -e java -E "test" ApplicationService.java
Length of output: 9111
Script:
#!/bin/bash
# Check the ApplicationService implementation
cat "app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationService.java"
Length of output: 317
Script:
#!/bin/bash
# Find and check the CE compatible service implementation
fd -e java ApplicationServiceCECompatible
cat $(fd -e java ApplicationServiceCECompatible | head -n 1)
Length of output: 2797
Script:
#!/bin/bash
# Check the base CE implementation
fd -e java ApplicationServiceCEImpl
cat $(fd -e java ApplicationServiceCEImpl | head -n 1)
Length of output: 50872
123-125: Mocking the findById call
Check if additional validations are needed to ensure the real method’s side effects don’t carry over in tests.
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"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12479630221
Commit: bc85f78
Cypress dashboard.
Tags:
@tag.GitSpec:
Tue, 24 Dec 2024 10:09:28 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
ArtifactService,ArtifactBasedService, andArtifactPermission.Bug Fixes
Documentation
Chores