Migrate to Apache HttpClient / Core 5.x #281
Conversation
Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>
| @@ -83,7 +84,7 @@ public User( | |||
| * @param response | |||
| * @throws IOException | |||
| */ | |||
| public User(final Response response) throws IOException { | |||
| public User(final Response response) throws IOException, ParseException { | |||
There was a problem hiding this comment.
Can we avoid throwing this exception?
There was a problem hiding this comment.
thanks a lot @eirsep for review.
i think we cannot avoid it as it is thrown by the library method itself unless we want to transform it to IOException which may be misinterpreted. looks like core changes also throw the error https://github.com/opensearch-project/OpenSearch/blob/4833e080cc506237fee5aa90767d6f49d952bec3/client/rest-high-level/src/test/java/org/opensearch/client/OpenSearchRestHighLevelClientTestCase.java#L329
| @@ -437,6 +437,6 @@ class ClusterMetricsInputTests { | |||
| // THEN | |||
| assertEquals(path, clusterMetricsInput.path) | |||
| assertEquals(pathParams, clusterMetricsInput.pathParams) | |||
| assertEquals(testUrl, clusterMetricsInput.url) | |||
| assertEquals(testUrl.replace(",", "%2C"), clusterMetricsInput.url) | |||
There was a problem hiding this comment.
I suppose it's fine for a test, but you're URL-encoding/decoding here, so you should be calling url decode.
There was a problem hiding this comment.
@dblock fixed the logic so tests no longer need changes. kindly check my comment below.
| @@ -91,7 +91,7 @@ class ClusterMetricsInputTests { | |||
| // GIVEN | |||
| path = "/_cluster/health/" | |||
| pathParams = "index1,index2,index3,index4,index5" | |||
| url = "http://localhost:9200/_cluster/health/index1,index2,index3,index4,index5" | |||
| url = "http://localhost:9200/_cluster/health/index1%2Cindex2%2Cindex3%2Cindex4%2Cindex5" | |||
There was a problem hiding this comment.
This feels unexpected. Did the API use to take URL-decoded URL and pass it as is, but now requires the arguments to be url-encoded? Either way I think you want to be explicit and call a URL encode function instead of changing all , by %2C.
There was a problem hiding this comment.
@dblock fixed the logic so tests no longer need changes. https://github.com/opensearch-project/common-utils/blob/5a0b10dd4af42abd4099e846328deaeefc2166f5/src/main/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInput.kt#L208-#L218
Looks like it is a problem with httpcomponents 5.1.x library as it has inconsistent behavior across 2 of its constructors.
this encodes the path params
val uriBuilder = URIBuilder()
.setScheme(SUPPORTED_SCHEME)
.setHost(SUPPORTED_HOST)
.setPort(SUPPORTED_PORT)
.setPath(path + pathParams)
while this doesnt
val uriBuilder = URIBuilder("$SUPPORTED_SCHEME://$SUPPORTED_HOST:$SUPPORTED_PORT$path$pathParams")
i do see opensearch-core tests modified in similar way. https://github.com/opensearch-project/OpenSearch/blob/4833e080cc506237fee5aa90767d6f49d952bec3/client/rest/src/test/java/org/opensearch/client/RestClientSingleHostIntegTests.java#L426
As this class is used by alerting plugin i would like to keep the tests same as any change in behavior may break alerting cluster metrics scenario.
Kindly have a look.
Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>
Signed-off-by: Subhobrata Dey sbcd90@gmail.com
Description
this PR fixes the build failures in
common-utilsdue toupgrade of Apache HttpClient / Core to 5.xinopensearch-core.opensearch-project/OpenSearch@4833e08
Issues Resolved
#279
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.