MLE-24405 Cleaned up RESTServices API #1823
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the RESTServices API by removing unused methods and consolidating OkHttp client configuration logic into the OkHttpServices class. The main goal is to simplify the separation between HTTP client concerns and database client factory logic, preparing for a potential future migration to the JDK HTTP client.
Key changes:
- Removed several unused methods from the RESTServices interface
- Moved OkHttp client configurator application from DatabaseClientFactory into OkHttpServices
- Introduced a ConnectionConfig record to encapsulate connection parameters
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| RESTServices.java | Removed unused interface methods (getRetryStatus, setMaxDelay, connect, getDatabaseClient, setDatabaseClient) and MIMETYPE_MULTIPART_FORM constant |
| OkHttpServices.java | Refactored to accept configuration via constructor, moved client configurator logic here, simplified connection handling |
| OkHttpUtil.java | Added clientConfigurators parameter to newOkHttpClientBuilder method |
| DatabaseClientImpl.java | Removed setDatabaseClient call from constructor |
| OkHttpClientBuilderFactory.java | Updated to pass null for clientConfigurators parameter |
| DatabaseClientFactory.java | Simplified client creation by moving configurator logic to OkHttpServices, updated type declarations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| static OkHttpClient.Builder newOkHttpClientBuilder(String host, DatabaseClientFactory.SecurityContext securityContext) { | ||
| return OkHttpUtil.newOkHttpClientBuilder(host, securityContext); | ||
| return OkHttpUtil.newOkHttpClientBuilder(host, securityContext, null); |
There was a problem hiding this comment.
Passing null for clientConfigurators could cause a NullPointerException when the forEach method is called on line 85 of OkHttpUtil.java. Consider passing an empty list instead.
| throw new MarkLogicIOException(e); | ||
| } finally { | ||
| client = null; | ||
| logger.debug("Releasing connection"); |
There was a problem hiding this comment.
The okHttpClient field is not being set to null after release, but the comment on lines 95-98 indicates it should be. This could lead to inconsistent state.
| logger.debug("Releasing connection"); | |
| logger.debug("Releasing connection"); | |
| okHttpClient = null; // Ensure client is set to null after release |
| } | ||
|
|
||
| clientConfigurators.add(configurator); | ||
| clientConfigurators.add((OkHttpClientConfigurator) configurator); |
There was a problem hiding this comment.
Extra space before the closing parenthesis in the cast. Should be (OkHttpClientConfigurator) configurator.
| clientConfigurators.add((OkHttpClientConfigurator) configurator); | |
| clientConfigurators.add((OkHttpClientConfigurator) configurator); |
rjrudin
force-pushed
the
feature/cleanup-rest-services
branch
from
4301aa1 to
0034359
Compare
This internal API had several unused methods. In addition, application of the client configurators happens in OkHttpServices now. That removes as much OkHttp stuff as possible from DatabaseClientFactory. This will greatly simplify shifting to the JDK HTTP client some time in the future.
rjrudin
force-pushed
the
feature/cleanup-rest-services
branch
from
0034359 to
cc94b53
Compare
This internal API had several unused methods.
In addition, application of the client configurators happens in OkHttpServices now. That removes as much OkHttp stuff as possible from DatabaseClientFactory. This will greatly simplify shifting to the JDK HTTP client some time in the future.