Merged
Conversation
Reviewer's GuideRefactors several client and utility classes for cleaner, more idiomatic Java usage (implicit Jackson TypeReference generics, lambdas, and direct returns), tightens exception types in the discovery HTTP client, simplifies configuration/data-file helpers, and modernizes Maven metadata URLs and coordinates. Sequence diagram for updated error handling in updateState HTTP callsequenceDiagram
actor ServiceClient
participant DiscoveryServiceHttpClient
participant HttpClient
ServiceClient->>DiscoveryServiceHttpClient: updateState(id, serviceState)
DiscoveryServiceHttpClient->>DiscoveryServiceHttpClient: serialize serviceState to json
alt JsonProcessingException
DiscoveryServiceHttpClient-->>ServiceClient: throw InvalidResponseDataException
else InterruptedException or IOException
DiscoveryServiceHttpClient-->>ServiceClient: throw WanakuException
else Success
DiscoveryServiceHttpClient->>HttpClient: send(request)
HttpClient-->>DiscoveryServiceHttpClient: HttpResponse
DiscoveryServiceHttpClient-->>ServiceClient: HttpResponse
end
Updated class diagram for DiscoveryServiceHttpClient exception behaviorclassDiagram
class DiscoveryServiceHttpClient {
-HttpClient httpClient
+HttpResponseString updateState(String id, ServiceState serviceState)
}
class WanakuException {
}
class InvalidResponseDataException {
}
DiscoveryServiceHttpClient ..> WanakuException : throws
DiscoveryServiceHttpClient ..> InvalidResponseDataException : throws
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The repeated
new TypeReference<>() {}usages inServicesHttpClientcould be extracted into reusable constants or helper methods to avoid creating many anonymous subclasses and to keep the call sites cleaner. - In
DiscoveryServiceHttpClient.updateState, consider whether using two different exception types (InvalidResponseDataExceptionfor JSON issues vsWanakuExceptionfor IO/interrupt) is intentional, and if not, aligning them to a single domain-specific exception would simplify error handling for callers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated `new TypeReference<>() {}` usages in `ServicesHttpClient` could be extracted into reusable constants or helper methods to avoid creating many anonymous subclasses and to keep the call sites cleaner.
- In `DiscoveryServiceHttpClient.updateState`, consider whether using two different exception types (`InvalidResponseDataException` for JSON issues vs `WanakuException` for IO/interrupt) is intentional, and if not, aligning them to a single domain-specific exception would simplify error handling for callers.
## Individual Comments
### Comment 1
<location> `capabilities-discovery/src/main/java/ai/wanaku/capabilities/sdk/discovery/DiscoveryServiceHttpClient.java:162-164` </location>
<code_context>
return httpClient.send(request, HttpResponse.BodyHandlers.ofString());
} catch (JsonProcessingException e) {
- throw new RuntimeException(e);
+ throw new InvalidResponseDataException(e);
} catch (InterruptedException | IOException e) {
- throw new RuntimeException(e);
+ throw new WanakuException(e);
}
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** InterruptedException is wrapped without restoring the thread’s interrupted status, which may cause subtle issues for callers that rely on interruption.
Catching `InterruptedException` clears the thread’s interrupted flag. Since this code is being modified anyway, please either re-set the flag with `Thread.currentThread().interrupt()` before throwing the wrapper, or (if possible) propagate `InterruptedException` directly so callers that rely on interruption semantics can still detect it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+162
to
+164
| throw new InvalidResponseDataException(e); | ||
| } catch (InterruptedException | IOException e) { | ||
| throw new RuntimeException(e); | ||
| throw new WanakuException(e); |
There was a problem hiding this comment.
suggestion (bug_risk): InterruptedException is wrapped without restoring the thread’s interrupted status, which may cause subtle issues for callers that rely on interruption.
Catching InterruptedException clears the thread’s interrupted flag. Since this code is being modified anyway, please either re-set the flag with Thread.currentThread().interrupt() before throwing the wrapper, or (if possible) propagate InterruptedException directly so callers that rely on interruption semantics can still detect it.
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.
Summary by Sourcery
Apply code and configuration cleanups across HTTP clients, data utilities, tests, and Maven metadata.
Bug Fixes:
Enhancements:
Build:
Chores: