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

Handle application/octet-stream in verification-client #276

Merged
merged 16 commits into from
Dec 3, 2019

Conversation

robert3005
Copy link
Contributor

Before this PR

We would try to deserialize application/octet-stream as json

After this PR

==COMMIT_MSG==
Don't try to deserialize applicaiton/octet-stream and handle it as a binary stream
==COMMIT_MSG==

Possible downsides?

N/A - this was missing functionality

@changelog-app
Copy link

changelog-app bot commented Dec 3, 2019

Generate changelog in changelog/@unreleased

Type

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

Description

Don't try to deserialize applicaiton/octet-stream and handle it as a binary stream

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from CRogers December 3, 2019 14:34
@robert3005
Copy link
Contributor Author

Tested against palantir/conjure-java-runtime and the binary client tests pass with this change

@@ -158,7 +161,7 @@ impl VerificationClientResource {
let content_type = response
Copy link
Contributor

Choose a reason for hiding this comment

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

a content type should always be set, we should fail here if its not. It should also make the rest of the code cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not that much but can unwrap the optional as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do not send content-type in 204 responses

Robert Kruszewski added 3 commits December 3, 2019 19:22
@ferozco
Copy link
Contributor

ferozco commented Dec 3, 2019

👍

@bulldozer-bot bulldozer-bot bot merged commit 26a6c43 into develop Dec 3, 2019
@bulldozer-bot bulldozer-bot bot deleted the rk/send-deserialized branch December 3, 2019 18:23
@svc-autorelease
Copy link
Collaborator

Released 0.18.2

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.

3 participants