[ISSUE #3197] NacosRestTemplate enhance #3198
Conversation
Maijh97
changed the title
|
please help to review this pr, thanks @KomachiSion |
|
We plan to release 1.3.1. We will stop merge PR before 1.3.1 released except code style PR and block issue PR. I will review PR, if approved, I will merge it after 1.3.1 release. |
| * @param requestBase requestBase | ||
| * @param httpClientConfig http config | ||
| */ | ||
| private static void getConfig(HttpRequestBase requestBase, HttpClientConfig httpClientConfig) { |
There was a problem hiding this comment.
I think the method name is not good.
|
|
||
| private final HttpClientRequest httpClientRequest; | ||
|
|
||
| private final Iterator<HttpClientRequestInterceptor> iterator; |
There was a problem hiding this comment.
iterator is a abstract name, use interceptors maybe better.
| private final Query query; | ||
|
|
||
| private Object body; | ||
|
|
||
| public RequestHttpEntity(Header header, Query query) { | ||
| handleHeader(header); | ||
| this.query = query; | ||
| this.httpClientConfig = null; |
There was a problem hiding this comment.
change this constructer as this(null, header, query, null)?
| public RequestHttpEntity(HttpClientConfig httpClientConfig, Header header, Query query) { | ||
| handleHeader(header); | ||
| this.httpClientConfig = httpClientConfig; | ||
| this.query = query; |
There was a problem hiding this comment.
change this constructer as this(httpClientConfig, header, query, null)?
| } | ||
|
|
||
| public RequestHttpEntity(Header header, Query query, Object body) { | ||
| handleHeader(header); | ||
| this.query = query; | ||
| this.body = body; | ||
| this.httpClientConfig = null; |
There was a problem hiding this comment.
change this constructer as this(null, header, query, body)?
| throws Exception { | ||
| if (iterator.hasNext()) { | ||
| HttpClientRequestInterceptor nextInterceptor = iterator.next(); | ||
| return nextInterceptor.intercept(uri, httpMethod, requestHttpEntity, this); |
There was a problem hiding this comment.
the Iterate method is too obscure, I suggest change this method.
|
I have adjusted the code.please help to review this pr again, thanks @KomachiSion |
KomachiSion
added
pending
# Conflicts: # common/src/main/java/com/alibaba/nacos/common/http/client/NacosRestTemplate.java
| private HttpClientRequest requestClient() { | ||
| if (CollectionUtils.isNotEmpty(interceptors)) { | ||
| if (LOGGER.isDebugEnabled()) { | ||
| LOGGER.debug("Execute via interceptor"); |
There was a problem hiding this comment.
Should we print the interceptor name or class name?
# Conflicts: # common/src/main/java/com/alibaba/nacos/common/http/client/NacosRestTemplate.java
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
NacosRestTemplate enhance to meet the actual http client use
#3197
Brief changelog
Add some parameter configuration of http client to the request method
Add an interceptor for nacos resttemplate, can add some logic through the interceptor
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=trueto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.