Open
Conversation
Member
Author
|
This is interdependent with craftercms/studio#3906 |
WalkthroughRefactors git auth types from a String-constants interface to a typed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
git/src/main/java/org/craftercms/commons/git/utils/TypeBasedAuthConfiguratorBuilder.java (1)
49-66:⚠️ Potential issue | 🔴 CriticalUse unqualified enum constants in
switchcases (current code will not compile).The enum switch statement uses qualified labels like
case AuthenticationType.NONE:which is invalid Java syntax. All four case statements must use unqualified constants:case NONE:,case BASIC:,case TOKEN:, andcase PRIVATE_KEY:.✅ Suggested fix
switch (authType) { - case AuthenticationType.NONE: + case NONE: logger.debug("No authentication will be used"); return new NoopAuthConfigurator(); - case AuthenticationType.BASIC: + case BASIC: if (isEmpty(username) && isEmpty(password)) { throw new IllegalStateException("basic auth requires a username or password"); } logger.debug("Username/password authentication will be used"); return new BasicUsernamePasswordAuthConfigurator(username, password); - case AuthenticationType.TOKEN: + case TOKEN: if (isEmpty(username)) { throw new IllegalStateException("token auth requires a username"); } logger.debug("Token authentication will be used"); return new BasicUsernamePasswordAuthConfigurator(username, StringUtils.EMPTY); - case AuthenticationType.PRIVATE_KEY: + case PRIVATE_KEY: logger.debug("SSH private key authentication will be used"); return new SshPrivateKeyAuthConfigurator(sshConfig, privateKeyPath, privateKeyPassphrase); default: throw new IllegalStateException("Unsupported auth type " + authType); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@git/src/main/java/org/craftercms/commons/git/utils/TypeBasedAuthConfiguratorBuilder.java` around lines 49 - 66, The switch in TypeBasedAuthConfiguratorBuilder uses fully-qualified enum labels which don't compile; update the switch cases in the method handling authType to use unqualified enum constants (replace AuthenticationType.NONE/BASIC/TOKEN/PRIVATE_KEY with NONE/BASIC/TOKEN/PRIVATE_KEY) so the cases match Java syntax, leaving surrounding logic (checks for username/password, returned configurator classes NoopAuthConfigurator, BasicUsernamePasswordAuthConfigurator, and related logger messages) unchanged.
🧹 Nitpick comments (1)
git/src/main/java/org/craftercms/commons/git/utils/TypeBasedAuthConfiguratorBuilder.java (1)
42-44: Fail fast on nullauthTypein constructor.A null
authTypewill fail later duringswitch; validating in constructor gives a clearer error path.💡 Suggested hardening
+import java.util.Objects; + public TypeBasedAuthConfiguratorBuilder(File sshConfig, AuthenticationType authType) { super(sshConfig); - this.authType = authType; + this.authType = Objects.requireNonNull(authType, "authType must not be null"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@git/src/main/java/org/craftercms/commons/git/utils/TypeBasedAuthConfiguratorBuilder.java` around lines 42 - 44, Validate the constructor argument authType in TypeBasedAuthConfiguratorBuilder(File sshConfig, AuthenticationType authType) and fail fast: if authType is null throw an IllegalArgumentException (or NullPointerException) with a clear message (e.g., "authType must not be null") before assigning this.authType, so downstream switch logic in this class won't encounter a null and will produce a clearer error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@git/src/main/java/org/craftercms/commons/git/utils/TypeBasedAuthConfiguratorBuilder.java`:
- Around line 49-66: The switch in TypeBasedAuthConfiguratorBuilder uses
fully-qualified enum labels which don't compile; update the switch cases in the
method handling authType to use unqualified enum constants (replace
AuthenticationType.NONE/BASIC/TOKEN/PRIVATE_KEY with
NONE/BASIC/TOKEN/PRIVATE_KEY) so the cases match Java syntax, leaving
surrounding logic (checks for username/password, returned configurator classes
NoopAuthConfigurator, BasicUsernamePasswordAuthConfigurator, and related logger
messages) unchanged.
---
Nitpick comments:
In
`@git/src/main/java/org/craftercms/commons/git/utils/TypeBasedAuthConfiguratorBuilder.java`:
- Around line 42-44: Validate the constructor argument authType in
TypeBasedAuthConfiguratorBuilder(File sshConfig, AuthenticationType authType)
and fail fast: if authType is null throw an IllegalArgumentException (or
NullPointerException) with a clear message (e.g., "authType must not be null")
before assigning this.authType, so downstream switch logic in this class won't
encounter a null and will produce a clearer error path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 379739bb-f051-4658-acb7-9c0a558671bf
📒 Files selected for processing (4)
git/src/main/java/org/craftercms/commons/git/utils/AuthConfiguratorFactory.javagit/src/main/java/org/craftercms/commons/git/utils/AuthenticationType.javagit/src/main/java/org/craftercms/commons/git/utils/TypeBasedAuthConfiguratorBuilder.javagit/src/test/java/org/craftercms/commons/git/utils/TypeBasedAuthConfiguratorBuilderTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
utilities/src/main/java/org/craftercms/commons/jackson/CaseInsensitiveEnumDeserializer.java (2)
32-32: Add generic type parameter toJsonDeserializer.Using the raw type
JsonDeserializerloses type safety and generates compiler warnings. Specify the generic parameter to match thedeserializereturn type.♻️ Proposed fix
-public class CaseInsensitiveEnumDeserializer extends JsonDeserializer implements ContextualDeserializer { +public class CaseInsensitiveEnumDeserializer extends JsonDeserializer<Enum<?>> implements ContextualDeserializer {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/src/main/java/org/craftercms/commons/jackson/CaseInsensitiveEnumDeserializer.java` at line 32, The class CaseInsensitiveEnumDeserializer currently extends the raw JsonDeserializer which loses type safety; update its declaration to use a matching generic parameter (e.g., JsonDeserializer<T> or JsonDeserializer<Enum<?>>) so the generic matches the deserialize method's return type, adjust the class signature and any type parameters accordingly, and ensure the deserialize(...) and createContextual(...) method signatures use the same generic type to eliminate raw-type warnings and restore compile-time type checking.
54-65: Add@SuppressWarningsfor unchecked cast and consider handling JSON null.The unchecked cast on line 57 is unavoidable due to type erasure. Adding
@SuppressWarnings("unchecked")documents this intentional cast and suppresses compiler warnings.Also, if
fieldTypeis null (before thecreateContextualfix), this will throw NPE. After the fix, consider whether JSONnullshould returnnullor throw.♻️ Proposed fix
`@Override` + `@SuppressWarnings`("unchecked") public Enum<?> deserialize(JsonParser p, DeserializationContext ctxt) throws IOException, JacksonException { String value = p.getText(); + if (value == null) { + return null; + } Class<Enum<?>> enumClass = (Class<Enum<?>>) fieldType.getRawClass();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/src/main/java/org/craftercms/commons/jackson/CaseInsensitiveEnumDeserializer.java` around lines 54 - 65, The deserialize method performs an unavoidable unchecked cast of fieldType.getRawClass() to Class<Enum<?>> and needs an explicit `@SuppressWarnings`("unchecked") to document/silence it; add that annotation to the deserialize method (or immediately above the cast) and keep the cast as-is. Also guard against JSON null by checking the parser token (e.g., JsonToken.VALUE_NULL) at the start of deserialize and return null (or handle per desired behavior) instead of proceeding, and ensure you still reference fieldType (set via createContextual) when doing the enumClass lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@utilities/src/main/java/org/craftercms/commons/jackson/CaseInsensitiveEnumDeserializer.java`:
- Around line 42-52: createContextual currently returns "this" when property is
null leaving CaseInsensitiveEnumDeserializer.fieldType unset and causing an NPE
in deserialize(); change createContextual to use ctxt.getContextualType() as a
fallback: if property != null use property.getType(), else try JavaType ctxType
= ctxt.getContextualType(); if ctxType != null return new
CaseInsensitiveEnumDeserializer(ctxType); otherwise fall back to returning this
— this ensures fieldType is initialized for root-level enum deserialization and
prevents the NPE in deserialize().
---
Nitpick comments:
In
`@utilities/src/main/java/org/craftercms/commons/jackson/CaseInsensitiveEnumDeserializer.java`:
- Line 32: The class CaseInsensitiveEnumDeserializer currently extends the raw
JsonDeserializer which loses type safety; update its declaration to use a
matching generic parameter (e.g., JsonDeserializer<T> or
JsonDeserializer<Enum<?>>) so the generic matches the deserialize method's
return type, adjust the class signature and any type parameters accordingly, and
ensure the deserialize(...) and createContextual(...) method signatures use the
same generic type to eliminate raw-type warnings and restore compile-time type
checking.
- Around line 54-65: The deserialize method performs an unavoidable unchecked
cast of fieldType.getRawClass() to Class<Enum<?>> and needs an explicit
`@SuppressWarnings`("unchecked") to document/silence it; add that annotation to
the deserialize method (or immediately above the cast) and keep the cast as-is.
Also guard against JSON null by checking the parser token (e.g.,
JsonToken.VALUE_NULL) at the start of deserialize and return null (or handle per
desired behavior) instead of proceeding, and ensure you still reference
fieldType (set via createContextual) when doing the enumClass lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42b2b5af-dbf5-424f-828b-b70afbce5286
📒 Files selected for processing (1)
utilities/src/main/java/org/craftercms/commons/jackson/CaseInsensitiveEnumDeserializer.java
utilities/src/main/java/org/craftercms/commons/jackson/CaseInsensitiveEnumDeserializer.java
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make AuthenticationType an enum
craftercms/craftercms#6061
Summary by CodeRabbit
Refactor
New Features
Tests