Fix #96: update SDK HTTP clients to match REST API cleanups#97
Merged
orpiske merged 2 commits intowanaku-ai:mainfrom Mar 19, 2026
Merged
Fix #96: update SDK HTTP clients to match REST API cleanups#97orpiske merged 2 commits intowanaku-ai:mainfrom
orpiske merged 2 commits intowanaku-ai:mainfrom
Conversation
- DiscoveryServiceHttpClient: remove /register and /deregister action paths, change deregister from POST to DELETE, rename /ping to /heartbeats - ServicesHttpClient: remove action paths (/add, /list, /remove, etc.) from all API endpoints, use proper HTTP verbs (DELETE instead of PUT for removals, PUT for updates, GET for retrieval by name), update method signatures for updateTool, updateResource, updateForward (accept name parameter), and removeForward (accept name instead of ForwardReference) - executeDelete now uses DELETE instead of PUT - Add tests for both clients using embedded HTTP server with mock OIDC
Reviewer's GuideAligns the Discovery and Services SDK HTTP clients with the upstream REST API by making their endpoints RESTful (paths and verbs), fixing DELETE semantics, updating a few method signatures, and adding focused HTTP-level tests using embedded servers to validate all paths and methods. Sequence diagram for DiscoveryServiceHttpClient RESTful interactionssequenceDiagram
actor ClientApp
participant DiscoveryServiceHttpClient
participant DiscoveryAPI
ClientApp->>DiscoveryServiceHttpClient: register(serviceTarget)
DiscoveryServiceHttpClient->>DiscoveryAPI: POST serviceBasePath
DiscoveryAPI-->>DiscoveryServiceHttpClient: 201 Created
DiscoveryServiceHttpClient-->>ClientApp: HttpResponse
ClientApp->>DiscoveryServiceHttpClient: deregister(serviceTarget)
DiscoveryServiceHttpClient->>DiscoveryAPI: DELETE serviceBasePath
DiscoveryAPI-->>DiscoveryServiceHttpClient: 200 OK
DiscoveryServiceHttpClient-->>ClientApp: HttpResponse
ClientApp->>DiscoveryServiceHttpClient: ping(id)
DiscoveryServiceHttpClient->>DiscoveryAPI: GET serviceBasePath/heartbeats
DiscoveryAPI-->>DiscoveryServiceHttpClient: 200 OK
DiscoveryServiceHttpClient-->>ClientApp: HttpResponse
Sequence diagram for ServicesHttpClient RESTful tool lifecyclesequenceDiagram
actor SdkUser
participant ServicesHttpClient
participant ServicesAPI
SdkUser->>ServicesHttpClient: addTool(toolReference)
ServicesHttpClient->>ServicesAPI: POST /api/v1/tools
ServicesAPI-->>ServicesHttpClient: WanakuResponse ToolReference
ServicesHttpClient-->>SdkUser: WanakuResponse ToolReference
SdkUser->>ServicesHttpClient: getToolByName(name)
ServicesHttpClient->>ServicesAPI: GET /api/v1/tools/{name}
ServicesAPI-->>ServicesHttpClient: WanakuResponse ToolReference
ServicesHttpClient-->>SdkUser: WanakuResponse ToolReference
SdkUser->>ServicesHttpClient: updateTool(name, toolReference)
ServicesHttpClient->>ServicesAPI: PUT /api/v1/tools/{name}
ServicesAPI-->>ServicesHttpClient: 204 NoContent
ServicesHttpClient-->>SdkUser: void
SdkUser->>ServicesHttpClient: removeTool(toolName)
ServicesHttpClient->>ServicesAPI: DELETE /api/v1/tools/{toolName}
ServicesAPI-->>ServicesHttpClient: 204 NoContent
ServicesHttpClient-->>SdkUser: void
Updated class diagram for DiscoveryServiceHttpClient and ServicesHttpClientclassDiagram
class DiscoveryServiceHttpClient {
- String baseUrl
- String serviceBasePath
- HttpClient httpClient
- ServiceAuthenticator serviceAuthenticator
- Serializer serializer
+ HttpResponse register(ServiceTarget serviceTarget)
+ HttpResponse deregister(ServiceTarget serviceTarget)
+ HttpResponse ping(String id)
- HttpResponse executePost(String operationPath, ServiceTarget serviceTarget)
- HttpResponse executeDelete(String operationPath, ServiceTarget serviceTarget)
}
class ServicesHttpClient {
- String baseUrl
- HttpClient httpClient
- ServiceAuthenticator serviceAuthenticator
+ WanakuResponse addTool(ToolReference toolReference)
+ WanakuResponse addToolWithPayload(ToolPayload toolPayload)
+ WanakuResponse listTools()
+ WanakuResponse getToolByName(String name)
+ void updateTool(String name, ToolReference toolReference)
+ void removeTool(String toolName)
+ WanakuResponse exposeResource(ResourceReference resourceReference)
+ WanakuResponse exposeResourceWithPayload(ResourcePayload resourcePayload)
+ WanakuResponse listResources()
+ void updateResource(String name, ResourceReference resourceReference)
+ void removeResource(String resourceName)
+ void addForward(ForwardReference forwardReference)
+ WanakuResponse listForwards()
+ void updateForward(String name, ForwardReference forwardReference)
+ void removeForward(String name)
+ WanakuResponse listNamespaces()
+ WanakuResponse addDataStore(DataStore dataStore)
+ WanakuResponse listDataStores()
+ WanakuResponse getDataStoreById(String id)
+ WanakuResponse getDataStoresByName(String name)
+ void removeDataStore(String id)
+ void removeDataStoresByName(String name)
- void executeDelete(String path)
}
DiscoveryServiceHttpClient ..> ServiceTarget
ServicesHttpClient ..> ToolReference
ServicesHttpClient ..> ToolPayload
ServicesHttpClient ..> ResourceReference
ServicesHttpClient ..> ResourcePayload
ServicesHttpClient ..> ForwardReference
ServicesHttpClient ..> Namespace
ServicesHttpClient ..> DataStore
ServicesHttpClient ..> WanakuResponse
DiscoveryServiceHttpClient ..> WanakuException
ServicesHttpClient ..> WanakuException
File-Level Changes
Assessment against linked issues
Possibly linked issues
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 - I've found 1 issue, and left some high level feedback:
- The various
get*ByName,update*, andremove*methods inServicesHttpClientbuild URLs by simple string concatenation (e.g.,"/api/v1/tools/" + nameand query params like"?name=" + name); consider URL-encoding these dynamic segments/parameters to avoid issues with special characters in names and IDs. DiscoveryServiceHttpClient.executePostandexecuteDeleteare nearly identical aside from the HTTP method; consider extracting a shared private helper that takes the verb as a parameter to reduce duplication and keep behavior changes (headers, serialization, error handling) in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The various `get*ByName`, `update*`, and `remove*` methods in `ServicesHttpClient` build URLs by simple string concatenation (e.g., `"/api/v1/tools/" + name` and query params like `"?name=" + name`); consider URL-encoding these dynamic segments/parameters to avoid issues with special characters in names and IDs.
- `DiscoveryServiceHttpClient.executePost` and `executeDelete` are nearly identical aside from the HTTP method; consider extracting a shared private helper that takes the verb as a parameter to reduce duplication and keep behavior changes (headers, serialization, error handling) in one place.
## Individual Comments
### Comment 1
<location path="capabilities-discovery/src/test/java/ai/wanaku/capabilities/sdk/discovery/DiscoveryServiceHttpClientTest.java" line_range="60-62" />
<code_context>
+ });
+
+ // Discovery API endpoint
+ server.createContext("/api/v1/management/discovery", exchange -> {
+ String body = new String(exchange.getRequestBody().readAllBytes());
+ requests.add(new RequestRecord(
+ exchange.getRequestMethod(), exchange.getRequestURI().getPath(), body));
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting on the serialized request body for register/deregister
Since the server is already capturing the request body, consider extending the tests to assert on `requests.getFirst().body()` as well, verifying key `ServiceTarget` fields (e.g., `serviceName`, host, port). This would better validate the new `executeDelete` JSON payload and help catch regressions where the DELETE is sent without a body or with incorrect serialization.
</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
+60
to
+62
| server.createContext("/api/v1/management/discovery", exchange -> { | ||
| String body = new String(exchange.getRequestBody().readAllBytes()); | ||
| requests.add(new RequestRecord( |
There was a problem hiding this comment.
suggestion (testing): Consider asserting on the serialized request body for register/deregister
Since the server is already capturing the request body, consider extending the tests to assert on requests.getFirst().body() as well, verifying key ServiceTarget fields (e.g., serviceName, host, port). This would better validate the new executeDelete JSON payload and help catch regressions where the DELETE is sent without a body or with incorrect serialization.
- Merge duplicate executePost/executeDelete into unified executeRequest in DiscoveryServiceHttpClient - Add URL encoding for dynamic path segments and query params in ServicesHttpClient - Add request body assertions in discovery client tests
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
DiscoveryServiceHttpClientto match upstream REST API changes: remove/registerand/deregisteraction paths, change deregister from POST to DELETE, rename/pingto/heartbeatsServicesHttpClientto use RESTful paths and proper HTTP verbs across all API endpoints (tools, resources, forwards, namespaces, data stores)executeDeleteto use actual HTTP DELETE instead of PUTupdateTool,updateResource,updateForward(accept name + body) andremoveForward(accept name instead of ForwardReference)Test plan
DiscoveryServiceHttpClientTestwith embedded HTTP server verifying correct paths and HTTP methods for register, deregister, and pingServicesHttpClientTestwith embedded HTTP server verifying correct paths and HTTP methods for all tools, resources, forwards, namespaces, and data stores operationsmvn verifypassesReferences
Summary by Sourcery
Align service and discovery HTTP clients with updated REST API contracts and HTTP semantics.
Bug Fixes:
Enhancements:
Build:
Tests: