Skip to content
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

Properly handle errors on void methods #575

Merged
merged 8 commits into from
Mar 27, 2020
Merged

Conversation

ferozco
Copy link
Contributor

@ferozco ferozco commented Mar 27, 2020

Closes #574

Before this PR

We would silently drop errors on endpoints that did not expect a response

After this PR

==COMMIT_MSG==
Properly handle errors on void methods
==COMMIT_MSG==

Possible downsides?

N/A

@changelog-app
Copy link

changelog-app bot commented Mar 27, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Properly handle errors on void methods

Check the box to generate changelog(s)

  • Generate changelog entry

@ferozco ferozco requested a review from iamdanfox March 27, 2020 16:32
return null;
} finally {
// We should not fail if a server that previously returned nothing starts returning a response
response.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could use try with resources here if you like

@Test
public void testEmptyResponse_failure() {
TestResponse response = new TestResponse();
response.code = 400;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you merge in develop, I've just changed this to be a setter method, so new TestResponse().code(400)

@ferozco
Copy link
Contributor Author

ferozco commented Mar 27, 2020

Pending palantir/conjure-verification#326

Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated. I think there's an issue with the receiveBinaryAliasExample confirmation, not necessarily a client issue, so I've added the two cases to ignored-test-cases.yml.

@bulldozer-bot bulldozer-bot bot merged commit 9ca323b into develop Mar 27, 2020
@bulldozer-bot bulldozer-bot bot deleted the fo/empty-body-error branch March 27, 2020 23:45
@svc-autorelease
Copy link
Collaborator

Released 1.2.3

@ferozco
Copy link
Contributor Author

ferozco commented Mar 27, 2020

Thanks @carterkozak!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EmptyBodyDeserializer does not handle error responses
4 participants