-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #825 - NullPointerException in HttpClient connector #826
Conversation
87ffede
to
78cebce
Compare
Hey @olivierlemasle, Thanks for fixing! |
I've tested this request with Jersey2 (no issue) and with okhttp (I encountered an issue but it seems to be unrelated and I've not investigated yet). |
Thanks for sharing the info. |
Looks good to you @vinodborole ? |
@auhlig added a comment |
Agreed. null-check makes sense @vinodborole. Could you update the implementation @olivierlemasle? Thanks in advance. |
Do you mean remove the null check, @auhlig ? |
I think Vinod meant moving that logic to a separate function. Right, @vinodborole ? |
78cebce
to
0a09e59
Compare
@auhlig @vinodborole I've updated the commit. |
@@ -123,8 +125,13 @@ public String header(String name) { | |||
@Override | |||
public <T> T readEntity(Class<T> typeToReadAs) { | |||
HttpEntity entity = response.getEntity(); | |||
if (entity == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use checkNotNull(entity) here, this will make it more compact and readable, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because checkNotNull(entity)
would throw a NullPointerException
if entity == null
, which is exactly the issue #825. The goal of this pull request is to accept without error the case where entity == null
, e.g. for HEAD requests.
For example, in Jersey2 connector, readEntity
will return null
for a HEAD request.
LGTM, @auhlig your take? |
Thanks for explaining @olivierlemasle . LGTM too, Vinod. |
Fixes issue #825