chore: add code-split for fixing package version change bug with in-memory Git #41442
Conversation
WalkthroughRefactors Git route handling into a CE base class (GitRouteAspectCE) with an FSM-driven orchestration; GitRouteAspect now extends that CE class. Also improves exception context in GitRouteArtifactCE.getArtifact by including artifactType and artifactId. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GitRouteAspect as GitRouteAspect
participant Context
participant FSM
participant Artifact as GitRouteArtifact
participant Redis
participant Bash as BashService
participant GitOps as Git (local FS / git)
Client->>GitRouteAspect: call method annotated `@GitRoute`
GitRouteAspect->>Context: create & populate Context
GitRouteAspect->>FSM: start at ROUTE_FILTER
rect `#E6F5E6`
FSM->>Artifact: resolve artifact & metadata
FSM->>FSM: validate metadata & parent resolution
end
rect `#E6F0FA`
FSM->>FSM: resolve GitProfile and GitAuth
FSM->>FSM: process SSH private key
end
rect `#FFF5E6`
FSM->>Redis: acquire repo lock / set lock with retry
FSM->>Redis: download/upload blobs if needed
FSM->>Bash: run clone/checkout scripts
Bash->>GitOps: perform git clone/checkout (SSH)
FSM->>Redis: release lock
end
alt success
FSM->>GitRouteAspect: proceed to business method
GitRouteAspect->>Client: return result
else error
FSM->>GitRouteAspect: propagate AppsmithException
GitRouteAspect->>Client: throw error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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: 3
🧹 Nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java (3)
78-78: BashService should be injected, not instantiated directly.Directly instantiating
BashServicebypasses Spring's dependency injection, making this harder to test and configure. Consider injecting it via constructor like other dependencies.- protected final BashService bashService = new BashService(); + protected final BashService bashService;Then add it to the constructor parameters and update the subclass accordingly.
871-874: Avoid repeated BouncyCastleProvider instantiation.Creating a new
BouncyCastleProviderinstance on each call is expensive. Consider using a static constant or registering the provider once at class initialization.+ private static final BouncyCastleProvider BC_PROVIDER = new BouncyCastleProvider(); + protected static KeyFactory getKeyFactory(String publicKey) throws Exception { String algo = publicKey.startsWith("ssh-rsa") ? "RSA" : "ECDSA"; - return KeyFactory.getInstance(algo, new BouncyCastleProvider()); + return KeyFactory.getInstance(algo, BC_PROVIDER); }
625-631: Assumes annotated methods returnMono.The unchecked cast to
Mono<Object>will throwClassCastExceptionat runtime if a@GitRoutemethod returns a different type. Consider documenting this contract in the@GitRouteannotation's Javadoc.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifactCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
📚 Learning: 2024-09-20T07:55:30.235Z
Learnt from: sondermanish
Repo: appsmithorg/appsmith PR: 36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-09-20T07:55:30.235Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifactCE.java
📚 Learning: 2025-01-15T20:46:05.401Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 38681
File: app/client/src/git/requests/helpers/urlArtifactType.ts:3-12
Timestamp: 2025-01-15T20:46:05.401Z
Learning: In URL helper functions like `urlArtifactType`, error handling in switch statements is not required as TypeScript's compile-time checks are sufficient.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifactCE.java
🧬 Code graph analysis (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java (1)
GitUtils(18-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/gitRoute/GitRouteArtifactCE.java (1)
28-29: Improved error context is a welcome change.Including
artifactTypeandartifactIdin the exception provides better diagnostics for debugging missing artifact scenarios in the FSM flow.app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java (1)
16-25: Clean CE/EE split implementation.The thin subclass correctly delegates to the base class while preserving AOP annotations. This follows the established pattern for code splitting in the codebase.
| return run(ctx, State.ROUTE_FILTER).flatMap(unused -> { | ||
| return this.result(ctx); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the result() method implementation
ast-grep --pattern $'protected Mono<?> result(Context $_) {
$$$
}'Repository: appsmithorg/appsmith
Length of output: 1820
🏁 Script executed:
#!/bin/bash
# Search for State.RESULT and how it's handled in the FSM
rg -A 5 "State\.RESULT" app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.javaRepository: appsmithorg/appsmith
Length of output: 2001
🏁 Script executed:
#!/bin/bash
# Check the State enum to understand the FSM transitions
rg -B 2 -A 2 "enum State" app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.javaRepository: appsmithorg/appsmith
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Look at the full run() method to understand the FSM flow
sed -n '253,307p' app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.javaRepository: appsmithorg/appsmith
Length of output: 2583
Remove redundant result() call in the flatMap.
The FSM's State.RESULT already transitions to invoke this.result(ctx) (line 693), which then transitions to State.DONE. After run() completes and returns at line 240, the explicit result(ctx) call in the flatMap is redundant and executes the result evaluation twice. Remove the explicit call since the FSM handles the final result state:
return run(ctx, State.ROUTE_FILTER);🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java
around lines 240 to 242, remove the explicit flatMap that calls this.result(ctx)
after run(ctx, State.ROUTE_FILTER) because the FSM will transition to
State.RESULT and invoke result(ctx) itself; replace the multi-statement return
with a single return of the run(...) call so result() is not executed twice.
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java
Show resolved
Hide resolved
| protected static String handlePemFormat(String privateKey, String publicKey) throws Exception { | ||
| byte[] content = | ||
| new PemReader(new StringReader(privateKey)).readPemObject().getContent(); | ||
| OpenSSHPrivateKeySpec privateKeySpec = new OpenSSHPrivateKeySpec(content); | ||
| KeyFactory keyFactory = getKeyFactory(publicKey); | ||
| PrivateKey generatedPrivateKey = keyFactory.generatePrivate(privateKeySpec); | ||
| return Base64.getEncoder().encodeToString(generatedPrivateKey.getEncoded()); | ||
| } |
There was a problem hiding this comment.
Resource leak: PemReader is not closed.
PemReader implements Closeable and should be closed after use to avoid resource leaks.
protected static String handlePemFormat(String privateKey, String publicKey) throws Exception {
- byte[] content =
- new PemReader(new StringReader(privateKey)).readPemObject().getContent();
+ byte[] content;
+ try (PemReader pemReader = new PemReader(new StringReader(privateKey))) {
+ content = pemReader.readPemObject().getContent();
+ }
OpenSSHPrivateKeySpec privateKeySpec = new OpenSSHPrivateKeySpec(content);
KeyFactory keyFactory = getKeyFactory(publicKey);
PrivateKey generatedPrivateKey = keyFactory.generatePrivate(privateKeySpec);
return Base64.getEncoder().encodeToString(generatedPrivateKey.getEncoded());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected static String handlePemFormat(String privateKey, String publicKey) throws Exception { | |
| byte[] content = | |
| new PemReader(new StringReader(privateKey)).readPemObject().getContent(); | |
| OpenSSHPrivateKeySpec privateKeySpec = new OpenSSHPrivateKeySpec(content); | |
| KeyFactory keyFactory = getKeyFactory(publicKey); | |
| PrivateKey generatedPrivateKey = keyFactory.generatePrivate(privateKeySpec); | |
| return Base64.getEncoder().encodeToString(generatedPrivateKey.getEncoded()); | |
| } | |
| protected static String handlePemFormat(String privateKey, String publicKey) throws Exception { | |
| byte[] content; | |
| try (PemReader pemReader = new PemReader(new StringReader(privateKey))) { | |
| content = pemReader.readPemObject().getContent(); | |
| } | |
| OpenSSHPrivateKeySpec privateKeySpec = new OpenSSHPrivateKeySpec(content); | |
| KeyFactory keyFactory = getKeyFactory(publicKey); | |
| PrivateKey generatedPrivateKey = keyFactory.generatePrivate(privateKeySpec); | |
| return Base64.getEncoder().encodeToString(generatedPrivateKey.getEncoded()); | |
| } |
🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java
around lines 840 to 847, the PemReader created for reading the privateKey is not
closed which can cause a resource leak; wrap the PemReader (and optionally the
StringReader) in a try-with-resources block so they are automatically closed
after use, keep the same logic to read the PEM content, construct the
OpenSSHPrivateKeySpec, generate the PrivateKey with the existing KeyFactory, and
return the Base64-encoded key as before.
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java (2)
240-242: Redundantresult()call after FSM completes.The FSM already transitions through
State.RESULTwhich invokesthis::resultand stores the outcome. Afterrun()returns, callingresult(ctx)again is duplicative.
840-847: Resource leak:PemReaderis not closed.
PemReaderimplementsCloseableand should be wrapped in try-with-resources.protected static String handlePemFormat(String privateKey, String publicKey) throws Exception { - byte[] content = - new PemReader(new StringReader(privateKey)).readPemObject().getContent(); + byte[] content; + try (PemReader pemReader = new PemReader(new StringReader(privateKey))) { + content = pemReader.readPemObject().getContent(); + } OpenSSHPrivateKeySpec privateKeySpec = new OpenSSHPrivateKeySpec(content); KeyFactory keyFactory = getKeyFactory(publicKey); PrivateKey generatedPrivateKey = keyFactory.generatePrivate(privateKeySpec); return Base64.getEncoder().encodeToString(generatedPrivateKey.getEncoded()); }
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java (2)
78-78: Consider injectingBashServicefor testability.Direct instantiation prevents mocking in unit tests. If you need to test FSM logic in isolation without invoking actual bash scripts, consider constructor injection.
571-586: Consider simplifying filter predicate.The filter logic can be more concise.
- .filter(artifact -> { - if (artifact.getGitArtifactMetadata() == null - || RefType.tag.equals( - artifact.getGitArtifactMetadata().getRefType())) { - return Boolean.FALSE; - } - return Boolean.TRUE; - }) + .filter(artifact -> artifact.getGitArtifactMetadata() != null + && !RefType.tag.equals(artifact.getGitArtifactMetadata().getRefType()))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/GitRouteAspectTest.java(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
📚 Learning: 2024-12-10T10:52:38.873Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts` and similar Git sagas, error handling for `baseArtifactId` is managed outside the scope, so validation checks for `baseArtifactId` within the saga functions are unnecessary.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java
📚 Learning: 2024-10-04T05:51:15.138Z
Learnt from: abhvsn
Repo: appsmithorg/appsmith PR: 36596
File: app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java:475-475
Timestamp: 2024-10-04T05:51:15.138Z
Learning: In `app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java`, the variable `newsletterSignedUpUserEmail` is always expected to be non-null because the user's email is always present in the user object, so an additional null check is unnecessary.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java
🧬 Code graph analysis (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java (1)
GitUtils(18-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: perform-test / build-docker-image / build-docker
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/GitRouteAspectTest.java (1)
6-6: LGTM!Test correctly updated to target
GitRouteAspectCEfor reflection since the FSM implementation now resides in the CE base class. The approach of instantiatingGitRouteAspectwhile reflecting onGitRouteAspectCEmethods is valid given the inheritance hierarchy.Also applies to: 70-70, 94-95
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java (1)
172-207: Well-structured FSM configuration.The state machine is clearly defined with explicit success/failure transitions. The architecture cleanly separates state configuration from execution logic.
| protected Mono<?> execute(Context ctx) { | ||
| try { | ||
| return (Mono<Object>) ctx.getJoinPoint().proceed(); | ||
| } catch (Throwable e) { | ||
| return Mono.error(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all @GitRoute annotated methods
rg '@GitRoute' -A 3 --type javaRepository: appsmithorg/appsmith
Length of output: 13488
🏁 Script executed:
# Find the @GitRoute annotation definition
fd -e java -x grep -l '@interface GitRoute' {}Repository: appsmithorg/appsmith
Length of output: 152
🏁 Script executed:
# Get more details about the annotation
find . -name '*.java' -exec grep -l '@interface GitRoute' {} \;Repository: appsmithorg/appsmith
Length of output: 152
🏁 Script executed:
cat -n app/server/appsmith-server/src/main/java/com/appsmith/server/annotations/GitRoute.javaRepository: appsmithorg/appsmith
Length of output: 890
🏁 Script executed:
# Get a few @GitRoute annotated methods with their full signatures
rg '@GitRoute' -A 5 app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java | head -80Repository: appsmithorg/appsmith
Length of output: 3703
🏁 Script executed:
# Get specific method return types from GitApplicationControllerCE
grep -A 2 '@GitRoute' app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java | grep -E '(Mono|public)' | head -20Repository: appsmithorg/appsmith
Length of output: 46
Document that @GitRoute annotated methods must return Mono<?>.
The unchecked cast to Mono<Object> in the execute() method assumes all intercepted methods return reactive types. While all current @GitRoute usages follow this pattern, the annotation definition does not enforce this constraint. A method returning a non-Mono type would cause ClassCastException at runtime. Add documentation to the @GitRoute annotation to clarify this requirement.
🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java
around lines 625 to 631, the execute() method performs an unchecked cast to
Mono<Object> which will throw a ClassCastException if an intercepted method
annotated with @GitRoute returns a non-Mono type; update the @GitRoute
annotation definition Javadoc to explicitly state that any method annotated with
@GitRoute must return a reactive Mono<?> and include an example signature and a
note that non-Mono return types will cause runtime errors, so callers should
change their method signatures to return Mono<?> to comply.
| protected Mono<?> result(Context ctx) { | ||
| Set<String> errorStates = Set.of( | ||
| AppsmithErrorCode.GIT_ROUTE_REDIS_DOWNLOAD_FAILED.getCode(), | ||
| AppsmithErrorCode.GIT_ROUTE_FS_CLEAN_UP_REQUIRED.getCode(), | ||
| AppsmithErrorCode.GIT_ROUTE_FS_OPS_NOT_REQUIRED.getCode(), | ||
| AppsmithErrorCode.GIT_ROUTE_METADATA_NOT_FOUND.getCode()); | ||
| if (ctx.getError() == null || errorStates.contains(ctx.getError().getAppErrorCode())) { | ||
| return Mono.just(ctx.getExecute()); | ||
| } | ||
|
|
||
| return Mono.error(ctx.getError()); | ||
| } |
There was a problem hiding this comment.
Potential NPE when ctx.getExecute() is null.
Mono.just() throws NullPointerException if passed null. If the FSM bypasses the EXECUTE state (e.g., via UNROUTED_EXECUTION → RESULT path when metadata filter passes but execute returns null), this will fail.
protected Mono<?> result(Context ctx) {
Set<String> errorStates = Set.of(
AppsmithErrorCode.GIT_ROUTE_REDIS_DOWNLOAD_FAILED.getCode(),
AppsmithErrorCode.GIT_ROUTE_FS_CLEAN_UP_REQUIRED.getCode(),
AppsmithErrorCode.GIT_ROUTE_FS_OPS_NOT_REQUIRED.getCode(),
AppsmithErrorCode.GIT_ROUTE_METADATA_NOT_FOUND.getCode());
if (ctx.getError() == null || errorStates.contains(ctx.getError().getAppErrorCode())) {
- return Mono.just(ctx.getExecute());
+ return Mono.justOrEmpty(ctx.getExecute());
}
return Mono.error(ctx.getError());
}🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java
around lines 693 to 704, the code calls Mono.just(ctx.getExecute()) which will
throw a NullPointerException if ctx.getExecute() is null; replace Mono.just(...)
with Mono.justOrEmpty(ctx.getExecute()) (or explicitly return Mono.empty() when
ctx.getExecute() is null) so the method safely returns an empty Mono instead of
throwing NPE while keeping the existing errorStates check and behavior.
| protected static KeyFactory getKeyFactory(String publicKey) throws Exception { | ||
| String algo = publicKey.startsWith("ssh-rsa") ? "RSA" : "ECDSA"; | ||
| return KeyFactory.getInstance(algo, new BouncyCastleProvider()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "ed25519|Ed25519" app/server/appsmith-server/src/main/java/ -C 3Repository: appsmithorg/appsmith
Length of output: 2200
🏁 Script executed:
# Also check for key type validation or error handling related to SSH keys
rg -n "ssh-ed25519|ssh-rsa|ssh-dss|KeyFactory\.getInstance" app/server/appsmith-server/src/main/java/ -C 2Repository: appsmithorg/appsmith
Length of output: 889
🏁 Script executed:
# Check if there's any documentation or comments about supported key types
rg -n "RSA|ECDSA" app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java -B 5 -A 5Repository: appsmithorg/appsmith
Length of output: 765
🏁 Script executed:
# Check the broader context of this method to understand its usage and impact
cat -n app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java | sed -n '860,885p'Repository: appsmithorg/appsmith
Length of output: 1387
Add ed25519 support to getKeyFactory or document unsupported key types.
The method currently supports only RSA and ECDSA, as documented on line 868. However, ed25519 keys (ssh-ed25519) would incorrectly attempt to use the ECDSA KeyFactory and fail. While the codebase already supports ed25519 in SignatureVerifier.java (via Ed25519Signer), this method needs to either handle ed25519 keys or explicitly reject them with a clear error message.
🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java
around lines 871 to 874, the getKeyFactory method only branches RSA vs ECDSA and
will mis-handle ssh-ed25519 keys; update the method to explicitly handle
"ssh-ed25519" by returning a KeyFactory for Ed25519 (e.g.
KeyFactory.getInstance("Ed25519", new BouncyCastleProvider())) or, if your
runtime/provider does not support Ed25519, throw a clear
UnsupportedOperationException mentioning ssh-ed25519 is unsupported and
referencing SignatureVerifier/Ed25519Signer for context; ensure the new branch
is first checked before the ECDSA fallback so ed25519 isn’t treated as ECDSA.
Failed server tests
|
…emory Git (#41442) ## Description EE Counterpart: appsmithorg/appsmith-ee#8468 Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/19896141793> > Commit: 478fefe > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=19896141793&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Wed, 03 Dec 2025 14:41:38 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [x] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error reporting for missing Git-related resources by including contextual details. * **Refactor** * Reorganized Git routing internals into a new layered implementation to simplify flow and improve reliability. * **Tests** * Updated tests to align with the restructured Git routing implementation. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit cc1ea71)
2 participants
Description
EE Counterpart: https://github.com/appsmithorg/appsmith-ee/pull/8468
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/19896141793
Commit: 478fefe
Cypress dashboard.
Tags:
@tag.GitSpec:
Wed, 03 Dec 2025 14:41:38 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.