You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2025/05/01 12:06:51 UTC
[PR] Fix NPE in InternalAbstractHttpAsyncClient by adding null check for resolvedTarget [httpcomponents-client]
arturobernalg opened a new pull request, #634:
URL: https://github.com/apache/httpcomponents-client/pull/634
Resolves a `NullPointerException` in `InternalAbstractHttpAsyncClient` by adding a null check for resolvedTarget when RoutingSupport.determineHost returns null. Throws `IllegalStateException` for invalid requests, ensuring robust error handling. Addresses [HTTPCLIENT-2367.](https://issues.apache.org/jira/browse/HTTPCLIENT-2367)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fix NPE in InternalAbstractHttpAsyncClient by adding null check for resolvedTarget [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #634:
URL: https://github.com/apache/httpcomponents-client/pull/634#discussion_r2071369880
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalAbstractHttpAsyncClient.java:
##########
@@ -219,6 +220,7 @@ protected <T> Future<T> doExecute(
setupContext(clientContext);
final HttpHost resolvedTarget = target != null ? target : RoutingSupport.determineHost(request);
+ Args.notNull(resolvedTarget, "Target host for the request");
Review Comment:
@arturobernalg This is not right. The target may be null if the request URI cannot be parsed. It should be up to the specific protocol handler to decide if such a request can be executed. H2 will likely reject it but HTTP/1.1 may accept it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fix NPE in InternalAbstractHttpAsyncClient by adding null check for resolvedTarget [httpcomponents-client]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #634:
URL: https://github.com/apache/httpcomponents-client/pull/634#issuecomment-2847375418
đź‘Ť
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fix NPE in InternalAbstractHttpAsyncClient by adding null check for resolvedTarget [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg merged PR #634:
URL: https://github.com/apache/httpcomponents-client/pull/634
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fix NPE in InternalAbstractHttpAsyncClient by adding null check for resolvedTarget [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #634:
URL: https://github.com/apache/httpcomponents-client/pull/634#issuecomment-2847381326
> @arturobernalg I tweaked the test case a little to make it behave it more similarly to the default route planner.
thank you @ok2c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fix NPE in InternalAbstractHttpAsyncClient by adding null check for resolvedTarget [httpcomponents-client]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #634:
URL: https://github.com/apache/httpcomponents-client/pull/634#discussion_r2070198100
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/InternalHttpClient.java:
##########
@@ -161,11 +161,15 @@ protected CloseableHttpResponse doExecute(
setupContext(localcontext);
final HttpHost resolvedTarget = target != null ? target : RoutingSupport.determineHost(request);
- if (request.getScheme() == null) {
- request.setScheme(resolvedTarget.getSchemeName());
- }
- if (request.getAuthority() == null) {
- request.setAuthority(new URIAuthority(resolvedTarget));
+ if (resolvedTarget != null) {
Review Comment:
HI @arturobernalg đź‘‹
We could throw a more precise exception using `Objects.requireNonNull(...)`, which would reduce the nesting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fix NPE in InternalAbstractHttpAsyncClient by adding null check for resolvedTarget [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #634:
URL: https://github.com/apache/httpcomponents-client/pull/634#issuecomment-2847337227
@arturobernalg I tweaked the test case a little to make it behave it more similarly to the default route planner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fix NPE in InternalAbstractHttpAsyncClient by adding null check for resolvedTarget [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #634:
URL: https://github.com/apache/httpcomponents-client/pull/634#issuecomment-2847196895
> @ok2c should I wait for the original reporter’s feedback before merging?
@arturobernalg I do not think it is necessary. The fix is quite straight-forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fix NPE in InternalAbstractHttpAsyncClient by adding null check for resolvedTarget [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #634:
URL: https://github.com/apache/httpcomponents-client/pull/634#issuecomment-2847026570
@ok2c should I wait for the original reporter’s feedback before merging?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fix NPE in InternalAbstractHttpAsyncClient by adding null check for resolvedTarget [httpcomponents-client]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #634:
URL: https://github.com/apache/httpcomponents-client/pull/634#discussion_r2070198100
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/InternalHttpClient.java:
##########
@@ -161,11 +161,15 @@ protected CloseableHttpResponse doExecute(
setupContext(localcontext);
final HttpHost resolvedTarget = target != null ? target : RoutingSupport.determineHost(request);
- if (request.getScheme() == null) {
- request.setScheme(resolvedTarget.getSchemeName());
- }
- if (request.getAuthority() == null) {
- request.setAuthority(new URIAuthority(resolvedTarget));
+ if (resolvedTarget != null) {
Review Comment:
HI @arturobernalg đź‘‹
We could throw a more precise exception using `Objects.requireNonNull(...)`, which would reduce the nesting. See also `org.apache.hc.core5.util.Args.notNull(T, String)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org