-
Notifications
You must be signed in to change notification settings - Fork 13
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
client tolerates unexpected body #388
Conversation
Generate changelog in
|
} catch (IOException e) { | ||
throw new SafeRuntimeException("Failed to read from response body", e); | ||
} | ||
input.close(); |
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.
Let's leave a comment describing that void
->type
is allowed. Is there a quick test we can add?
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.
👍 I'll add a comment, I already added tests :)
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.
Oof yep this would be a super annoying constraint from back-compat perspective!
Released 0.9.1 |
@@ -141,6 +141,18 @@ public void testBlocking_stringToString_throwsWhenResponseBodyIsEmpty() { | |||
.hasMessageContaining("Failed to deserialize response stream. Syntax error?"); | |||
} | |||
|
|||
@Test | |||
public void testBlocking_voidToVoid_doesNotThrowsWhenResponseBodyIsNonEmpty() { |
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.
nit: doesNotThrowWhen
} catch (IOException e) { | ||
throw new SafeRuntimeException("Failed to read from response body", e); | ||
} | ||
// We should not fail if a server that previously return nothing starts returning a response |
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.
nit: grammar is a little funky here
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.
was there a bug here previously w.r.t. the missing input.close()
? if so, can you add a test?
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.
No there was not a bug, it was previously closed by the try-with-resource. It was simply more concise to call close directly
Before this PR
If a client received a response but was not expecting one it would throw
After this PR
==COMMIT_MSG==
Clients are tolerant of unexpected response body
==COMMIT_MSG==
Possible downsides?
N/A