Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/OAuth2CallbackRequestHandler.java (1)
94-98: Consider including the exception in the log statements for debugging.The log messages provide good contextual information, but the exception
eis not passed to the logger, which means the stacktrace and detailed cause chain are lost. This can make production debugging difficult.Also, checking only
e.getCause()may miss cases whereJsonProcessingExceptionis nested deeper in the cause chain.♻️ Suggested improvement
} catch (Exception e) { if (e.getCause() instanceof JsonProcessingException) { - log.warn("OAuth2 flow failed to reconstruct original exchange from data found in '{}'", originalExchangeStore.getClass().getSimpleName()); + log.warn("OAuth2 flow failed to reconstruct original exchange from data found in '{}': {}", originalExchangeStore.getClass().getSimpleName(), e.getMessage()); } else { - log.warn("OAuth2 flow could not find original exchange data in '{}'. This is likely caused by either an illegal state (e.g. the user pressing 'back' inside the OAuth2 flow) or an operational issue with the exchange store.", originalExchangeStore.getClass().getSimpleName()); + log.warn("OAuth2 flow could not find original exchange data in '{}'. This is likely caused by either an illegal state (e.g. the user pressing 'back' inside the OAuth2 flow) or an operational issue with the exchange store: {}", originalExchangeStore.getClass().getSimpleName(), e.getMessage()); }Alternatively, if full stacktraces are desired at debug level (based on learnings from this codebase), you could add
log.debug("Details:", e);after the warn log.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/OAuth2CallbackRequestHandler.java` around lines 94 - 98, The log.warn calls in OAuth2CallbackRequestHandler currently omit the exception and only check e.getCause() for JsonProcessingException; update the warning logs to include the exception e as the last parameter so the stacktrace is logged (e.g., pass e to log.warn), and improve the detection by inspecting the full cause chain (walk e.getCause() recursively or use org.apache.commons.lang3.exception.ExceptionUtils.getRootCause/contains) to detect nested JsonProcessingException when deciding which message to log; reference the originalExchangeStore and the existing log.warn sites to locate where to add the exception and cause-chain check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/OAuth2CallbackRequestHandler.java`:
- Around line 94-98: The log.warn calls in OAuth2CallbackRequestHandler
currently omit the exception and only check e.getCause() for
JsonProcessingException; update the warning logs to include the exception e as
the last parameter so the stacktrace is logged (e.g., pass e to log.warn), and
improve the detection by inspecting the full cause chain (walk e.getCause()
recursively or use
org.apache.commons.lang3.exception.ExceptionUtils.getRootCause/contains) to
detect nested JsonProcessingException when deciding which message to log;
reference the originalExchangeStore and the existing log.warn sites to locate
where to add the exception and cause-chain check.
rrayst
previously approved these changes
Feb 17, 2026
added 5 commits
February 17, 2026 16:05
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (1)
342-347: Handle potential null frome.getMessage()and consider consistency withcodeTokenRequest.Two observations:
Null safety:
e.getMessage()can returnnullfor some exceptions, resulting in "Error contacting... null" in the description.Inconsistency:
codeTokenRequest(lines 361-367) logs a warning before throwing and uses the plain description. This method now includes the exception message in the description but appears to have removed logging (per summary). Consider aligning error handling between both methods.♻️ Suggested fix for null safety
} catch (Exception e) { + log.warn("Error contacting OAuth2 Authorization Server during refresh request: {}", e.getMessage()); throw new OAuth2Exception( MEMBRANE_OAUTH2_SERVER_COMMUNICATION_ERROR, - "%s %s".formatted(MEMBRANE_OAUTH2_SERVER_COMMUNICATION_ERROR_DESCRIPTION, e.getMessage()), + "%s %s".formatted(MEMBRANE_OAUTH2_SERVER_COMMUNICATION_ERROR_DESCRIPTION, + e.getMessage() != null ? e.getMessage() : e.getClass().getSimpleName()), internalServerError().body(MEMBRANE_OAUTH2_SERVER_COMMUNICATION_ERROR_DESCRIPTION).build()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java` around lines 342 - 347, The catch block in AuthorizationService currently uses e.getMessage() directly which may be null and also omits the warning log present in codeTokenRequest; update this catch to (1) compute a safe message such as String msg = e.getMessage() == null ? "" : " - " + e.getMessage() (or similar fallback) and use that when building the formatted description so "null" never appears, and (2) add the same warning log call used in codeTokenRequest (e.g., processLogger.warn(...) or the existing logger) before throwing the OAuth2Exception to keep error handling consistent between the two methods.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/OAuth2CallbackRequestHandler.java (1)
94-98: Consider including the exception in the log statements for diagnostics.Both warning log statements omit the exception
e, which means the stack trace and original error details are lost. Including the exception would aid in debugging reconstruction failures.📋 Proposed fix to include exception in logs
} catch (Exception e) { if (e.getCause() instanceof JsonProcessingException) { - log.warn("OAuth2 flow failed to reconstruct original exchange from data found in '{}'", originalExchangeStore.getClass().getSimpleName()); + log.warn("OAuth2 flow failed to reconstruct original exchange from data found in '{}'", originalExchangeStore.getClass().getSimpleName(), e); } else { - log.warn("OAuth2 flow could not find original exchange data in '{}'. This is likely caused by either an illegal state (e.g. the user pressing 'back' inside the OAuth2 flow) or an operational issue with the exchange store.", originalExchangeStore.getClass().getSimpleName()); + log.warn("OAuth2 flow could not find original exchange data in '{}'. This is likely caused by either an illegal state (e.g. the user pressing 'back' inside the OAuth2 flow) or an operational issue with the exchange store.", originalExchangeStore.getClass().getSimpleName(), e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/OAuth2CallbackRequestHandler.java` around lines 94 - 98, The warning logs in OAuth2CallbackRequestHandler currently omit the exception details (variable e) when reconstructing the original exchange from originalExchangeStore; update both log.warn calls to include the exception as the last argument so the stack trace and root cause are logged (i.e., pass e into the log.warn invocations that reference originalExchangeStore.getClass().getSimpleName()) to aid diagnostics.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/token/AccessTokenRefresher.java (1)
59-65: Consider preserving the exception as a cause for debugging.The new conditional logging provides better context for OAuth2 errors by including the error description. However, neither branch logs the exception itself, which means the stack trace is lost. This could make debugging difficult when investigating token refresh failures.
♻️ Proposed fix to preserve the exception as a cause
} catch (Exception e) { if (e instanceof OAuth2Exception oae) { - log.warn("Failed to refresh access token, clearing session and restarting OAuth2 flow. [{}]", oae.getErrorDescription()); + log.warn("Failed to refresh access token, clearing session and restarting OAuth2 flow. [{}]", oae.getErrorDescription(), e); } else { - log.warn("Failed to refresh access token, clearing session and restarting OAuth2 flow."); + log.warn("Failed to refresh access token, clearing session and restarting OAuth2 flow.", e); } session.clearAuthentication(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/token/AccessTokenRefresher.java` around lines 59 - 65, The log statements in AccessTokenRefresher's catch block currently omit the exception object, losing the stack trace; update both log.warn calls to pass the caught Exception e as a throwable cause (e.g., log.warn("... [{}]", oae.getErrorDescription(), e) and log.warn("...", e)) so the exception and stack trace are preserved while retaining the existing messages, then keep the existing session.clearAuthentication() behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/session/SessionManagementException.java`:
- Around line 3-6: The constructor in SessionManagementException currently calls
super(getMessageFromCause(cause)) which loses the original exception chain;
update the constructor in class SessionManagementException to pass the original
cause into the RuntimeException (e.g., call super(getMessageFromCause(cause),
cause)) so getCause() and full stack traces are preserved; keep using
getMessageFromCause(cause) for the message so existing formatting remains.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java`:
- Around line 342-347: The catch block in AuthorizationService currently uses
e.getMessage() directly which may be null and also omits the warning log present
in codeTokenRequest; update this catch to (1) compute a safe message such as
String msg = e.getMessage() == null ? "" : " - " + e.getMessage() (or similar
fallback) and use that when building the formatted description so "null" never
appears, and (2) add the same warning log call used in codeTokenRequest (e.g.,
processLogger.warn(...) or the existing logger) before throwing the
OAuth2Exception to keep error handling consistent between the two methods.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/OAuth2CallbackRequestHandler.java`:
- Around line 94-98: The warning logs in OAuth2CallbackRequestHandler currently
omit the exception details (variable e) when reconstructing the original
exchange from originalExchangeStore; update both log.warn calls to include the
exception as the last argument so the stack trace and root cause are logged
(i.e., pass e into the log.warn invocations that reference
originalExchangeStore.getClass().getSimpleName()) to aid diagnostics.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/token/AccessTokenRefresher.java`:
- Around line 59-65: The log statements in AccessTokenRefresher's catch block
currently omit the exception object, losing the stack trace; update both
log.warn calls to pass the caught Exception e as a throwable cause (e.g.,
log.warn("... [{}]", oae.getErrorDescription(), e) and log.warn("...", e)) so
the exception and stack trace are preserved while retaining the existing
messages, then keep the existing session.clearAuthentication() behavior.
.../src/main/java/com/predic8/membrane/core/interceptor/session/SessionManagementException.java
Show resolved
Hide resolved
predic8
approved these changes
Feb 23, 2026
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
3 participants
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.
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements