Decouple request sending and response deserialization for login (RequestAgent + ResponseDeserializer)#3
Conversation
|
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR refactors request-response abstractions: removes Changes
Sequence Diagram(s)sequenceDiagram
participant LoginService as LoginServiceImpl
participant Agent as RequestAgent
participant Proxy as ResponseProxy
participant Deser as ResponseDeserializer
participant Domain as Response
LoginService->>Agent: send(Request<Void>)
Agent-->>Proxy: returns ResponseProxy
Proxy->>Domain: getResponse()
Domain-->>Proxy: Response(body/exception)
LoginService->>Proxy: proxy.getResponse().body()
Note right of LoginService: if body() throws -> propagate
LoginService->>Deser: deserialize(body, Credentials.class)
Deser-->>LoginService: Credentials or throws
alt deserialization throws
LoginService-->>LoginService: wrap in DeserializationException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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)✅ Unit Test PR creation complete.
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbda7931ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } catch (IncorrectUsernameOrPasswordException e) { | ||
| throw e; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
The generic catch (Exception e) in getCredentials also catches the DeserializationException created just above, so a deserializer failure is surfaced as RuntimeException("Login failed", ...) instead of DeserializationException. This breaks the new contract that deserialization errors are reported distinctly, and callers/tests that handle DeserializationException will never see it when responseDeserializer.deserialize(...) fails.
Useful? React with 👍 / 👎.
![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)
src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java (1)
62-82:⚠️ Potential issue | 🔴 CriticalCritical:
DeserializationExceptionis caught and re-wrapped by the outer catch block.The
DeserializationExceptionthrown at line 10 (inside the inner catch block) remains within the outer try block's scope. When it propagates, it encounters the outer catch clauses in order: firstcatch (IncorrectUsernameOrPasswordException)which doesn't match, thencatch (Exception)at line 14 which matches and re-wraps it asRuntimeException("Login failed", e).This breaks the intended exception contract where deserialization failures should surface as
DeserializationException. The testshouldMapUncheckedDeserializerException()expectsDeserializationException.classto be thrown but will receiveRuntimeExceptioninstead.Additionally, checked exceptions from
ResponseDeserializer.deserialize()(the interface declaresthrows Exception) bypass the innercatch (RuntimeException)entirely and are also wrapped asRuntimeException("Login failed")rather thanDeserializationException.Fix: Add
DeserializationExceptionto the outer catch-and-rethrow clause:- } catch (IncorrectUsernameOrPasswordException e) { + } catch (IncorrectUsernameOrPasswordException | DeserializationException e) { throw e; } catch (Exception e) {For complete handling of checked exceptions, also update the inner catch:
- } catch (RuntimeException e) { + } catch (Exception e) { throw new DeserializationException("Failed to deserialize login response", e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java` around lines 62 - 82, The getCredentials method wraps DeserializationException thrown from responseDeserializer.deserialize into a RuntimeException via the outer catch; change the inner catch to catch Exception (not just RuntimeException) and rethrow as DeserializationException when deserialize(...) fails, and add an outer catch clause for DeserializationException (placed before the generic catch) that simply rethrows it so it won't be wrapped by the catch (Exception) block; reference methods/classes: getCredentials, responseDeserializer.deserialize, DeserializationException, requestAgent, requestFactory.create, ResponseProxy, Credentials.
🧹 Nitpick comments (3)
src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java (1)
28-35: Consider documenting the deprecation reason or removal timeline.The
@Deprecated(forRemoval = true)annotation signals intent to remove this constructor, but there's no@deprecatedJavadoc explaining why or what to use instead. This helps consumers migrate before removal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java` around lines 28 - 35, Add a Javadoc block to the deprecated constructor in LoginServiceImpl explaining why it's deprecated, when it will be removed (or the planned timeline), and what to use instead (e.g., an alternative constructor or factory/method). Update the constructor's JavaDoc to include an `@deprecated` tag with a short migration note referencing the preferred mechanism (for example, the non-deprecated constructor or a builder/factory), so callers know how to migrate before removal.src/main/java/pl/vtt/wpi/core/domain/model/Response.java (1)
7-10: Consider enforcing the either-body-or-exception invariant in the constructor.The
Responseclass conceptually represents a result that is either a successful body or an error. Currently, the constructor permits invalid states: bothbodyandexceptionbeing non-null, or both being null. While the context notes that implementations "must ensure" proper usage, adding a constructor check would enforce this contract and prevent subtle bugs.♻️ Optional: Add constructor validation
public Response(String body, Exception exception) { + if (body != null && exception != null) { + throw new IllegalArgumentException("Response cannot have both body and exception"); + } this.body = body; this.exception = exception; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/domain/model/Response.java` around lines 7 - 10, The Response constructor currently allows both body and exception to be null or both non-null; update the Response(String body, Exception exception) constructor to validate that exactly one of the two is non-null (i.e., body == null XOR exception == null) and throw an IllegalArgumentException with a clear message if the invariant is violated, then assign the fields as before; reference: Response constructor and the body and exception fields.src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java (1)
125-133: Test relies on implementation detail that logout() doesn't use dependencies.Passing all
nulldependencies works becauselogout()currently only callsAuthorizationHolder.clear(). Iflogout()ever needs to make a server call or use other dependencies, this test would fail with NPE rather than a meaningful assertion failure.♻️ Optional: Use a valid instance for logout test
`@Test` `@DisplayName`("Should clear authorization on logout") void shouldClearAuthorizationOnLogout() { - LoginServiceImpl instance = new LoginServiceImpl(null, null, null, null); + LoginServiceImpl instance = instanceWithSuccessResponse(new Credentials(USERNAME, TOKEN)); AuthorizationHolder.authorize("Basic", "seeded-token"); assertNotNull(AuthorizationHolder.get()); instance.logout(); assertNull(AuthorizationHolder.get()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java` around lines 125 - 133, The test should not pass nulls to LoginServiceImpl because it relies on the implementation detail that logout() ignores dependencies; instead construct LoginServiceImpl with valid/mocked dependencies so a future change that uses them won't cause NPEs—replace the null arguments in the test with Mockito.mock(...) (or lightweight real instances) for the constructor parameters used by LoginServiceImpl, keep using AuthorizationHolder.authorize("Basic","seeded-token") and then call instance.logout() and assertNull(AuthorizationHolder.get()) to verify clearing behavior; target symbols: LoginServiceImpl constructor, logout(), and AuthorizationHolder.
🤖 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
`@src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java`:
- Around line 62-82: The getCredentials method wraps DeserializationException
thrown from responseDeserializer.deserialize into a RuntimeException via the
outer catch; change the inner catch to catch Exception (not just
RuntimeException) and rethrow as DeserializationException when deserialize(...)
fails, and add an outer catch clause for DeserializationException (placed before
the generic catch) that simply rethrows it so it won't be wrapped by the catch
(Exception) block; reference methods/classes: getCredentials,
responseDeserializer.deserialize, DeserializationException, requestAgent,
requestFactory.create, ResponseProxy, Credentials.
---
Nitpick comments:
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java`:
- Around line 28-35: Add a Javadoc block to the deprecated constructor in
LoginServiceImpl explaining why it's deprecated, when it will be removed (or the
planned timeline), and what to use instead (e.g., an alternative constructor or
factory/method). Update the constructor's JavaDoc to include an `@deprecated` tag
with a short migration note referencing the preferred mechanism (for example,
the non-deprecated constructor or a builder/factory), so callers know how to
migrate before removal.
In `@src/main/java/pl/vtt/wpi/core/domain/model/Response.java`:
- Around line 7-10: The Response constructor currently allows both body and
exception to be null or both non-null; update the Response(String body,
Exception exception) constructor to validate that exactly one of the two is
non-null (i.e., body == null XOR exception == null) and throw an
IllegalArgumentException with a clear message if the invariant is violated, then
assign the fields as before; reference: Response constructor and the body and
exception fields.
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java`:
- Around line 125-133: The test should not pass nulls to LoginServiceImpl
because it relies on the implementation detail that logout() ignores
dependencies; instead construct LoginServiceImpl with valid/mocked dependencies
so a future change that uses them won't cause NPEs—replace the null arguments in
the test with Mockito.mock(...) (or lightweight real instances) for the
constructor parameters used by LoginServiceImpl, keep using
AuthorizationHolder.authorize("Basic","seeded-token") and then call
instance.logout() and assertNull(AuthorizationHolder.get()) to verify clearing
behavior; target symbols: LoginServiceImpl constructor, logout(), and
AuthorizationHolder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f10a14d2-8b28-4d22-898c-c6fff540587b
📒 Files selected for processing (9)
README.mdsrc/main/java/pl/vtt/wpi/core/application/exception/DeserializationException.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/util/RequestAgent.javasrc/main/java/pl/vtt/wpi/core/application/util/RequestHandler.javasrc/main/java/pl/vtt/wpi/core/application/util/ResponseDeserializer.javasrc/main/java/pl/vtt/wpi/core/application/util/ResponseProxy.javasrc/main/java/pl/vtt/wpi/core/domain/model/Response.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java
💤 Files with no reviewable changes (1)
- src/main/java/pl/vtt/wpi/core/application/util/RequestHandler.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java`:
- Around line 68-72: The catch only handles RuntimeException so checked
exceptions from responseDeserializer.deserialize(...) escape your intended
DeserializationException; update the try/catch in LoginServiceImpl around
responseDeserializer.deserialize(responseBodyString, Credentials.class) to catch
Exception (or Throwable as appropriate), and if the caught exception is already
a DeserializationException rethrow it as-is, otherwise wrap it in a new
DeserializationException("Failed to deserialize login response", e) before
throwing; ensure no other generic RuntimeException wrapping at line 76 masks the
original cause.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: db479b7f-ad73-4cbe-822c-29b0caee1e23
📒 Files selected for processing (1)
src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java
| try { | ||
| responseBody = responseDeserializer.deserialize(responseBodyString, Credentials.class); | ||
| } catch (RuntimeException e) { | ||
| throw new DeserializationException("Failed to deserialize login response", e); | ||
| } |
There was a problem hiding this comment.
Checked exceptions from deserialize() escape DeserializationException wrapping.
The ResponseDeserializer.deserialize() interface declares throws Exception (checked), but this catch block only handles RuntimeException. Checked exceptions (e.g., IOException from JSON parsing) will bypass this block and be wrapped as generic RuntimeException("Login failed", e) at line 76 instead of DeserializationException.
To properly surface all deserialization failures as DeserializationException and also preserve any DeserializationException already thrown by the deserializer (avoiding double-wrapping):
🐛 Proposed fix
try {
responseBody = responseDeserializer.deserialize(responseBodyString, Credentials.class);
- } catch (RuntimeException e) {
+ } catch (DeserializationException e) {
+ throw e;
+ } catch (Exception e) {
throw new DeserializationException("Failed to deserialize login response", e);
}📝 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.
| try { | |
| responseBody = responseDeserializer.deserialize(responseBodyString, Credentials.class); | |
| } catch (RuntimeException e) { | |
| throw new DeserializationException("Failed to deserialize login response", e); | |
| } | |
| try { | |
| responseBody = responseDeserializer.deserialize(responseBodyString, Credentials.class); | |
| } catch (DeserializationException e) { | |
| throw e; | |
| } catch (Exception e) { | |
| throw new DeserializationException("Failed to deserialize login response", e); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java`
around lines 68 - 72, The catch only handles RuntimeException so checked
exceptions from responseDeserializer.deserialize(...) escape your intended
DeserializationException; update the try/catch in LoginServiceImpl around
responseDeserializer.deserialize(responseBodyString, Credentials.class) to catch
Exception (or Throwable as appropriate), and if the caught exception is already
a DeserializationException rethrow it as-is, otherwise wrap it in a new
DeserializationException("Failed to deserialize login response", e) before
throwing; ensure no other generic RuntimeException wrapping at line 76 masks the
original cause.
koder95
changed the base branch from
main
to
codex/split-requesthandler-into-three-interfaces
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
14 UNAVAILABLE: read ECONNRESET |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #4 |
1 participant
Motivation
AuthorizationHolderis cleared on error.Description
RequestHandlerwith aRequestAgentthat returns aResponseProxy, and addedResponseDeserializerandResponseProxyinterfaces to separate sending and parsing responsibilities.Responsemodel and aDeserializationExceptionto represent and propagate deserialization failures explicitly.LoginServiceImplto useRequestAgent+ResponseDeserializer, to extract response body viaResponseProxy, and to map unchecked deserialization errors toDeserializationExceptionwhile clearing authorization on failure.RequestHandlerand updated theREADME.mdto reflect the newutilinterfaces; tests were updated to exercise the new contracts and error paths.Testing
LoginServiceImplTestwith cases including successful login (shouldLoginSuccessfully), agent-thrown exception propagation (shouldPropagateAgentException), null-deserializer result handling (shouldThrowWhenDeserializerReturnsNull), mapping unchecked deserializer errors toDeserializationException(shouldMapUncheckedDeserializerException), parameterized invalid username/password tests, and logout behavior test (shouldClearAuthorizationOnLogout).mvn test, and all tests passed.Codex Task
Summary by CodeRabbit
New Features
Architecture
Documentation
Tests