Skip to content

Return null (empty target object) if status code is 204#792

Merged
kdavisk6 merged 1 commit into
OpenFeign:masterfrom
selimok:master
Sep 24, 2018
Merged

Return null (empty target object) if status code is 204#792
kdavisk6 merged 1 commit into
OpenFeign:masterfrom
selimok:master

Conversation

@selimok

@selimok selimok commented Sep 19, 2018

Copy link
Copy Markdown
Contributor

As mention in #791 JAXBDecoder should check also for status code 204 before trying to decode response body.

@kdavisk6 kdavisk6 added the bug Unexpected or incorrect behavior label Sep 19, 2018
@Override
public Object decode(Response response, Type type) throws IOException {
if (response.status() == 404)
if (response.status() == 404 || response.status() == 204)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds fair.

Could you just add a test for 204 as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we do this globally or just in this decoder? @velo?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or better yet, we should inspect the body before sending it to the Decoder here:

Object decode(Response response) throws Throwable {
try {
return decoder.decode(response, metadata.returnType());
} catch (FeignException e) {
throw e;
} catch (RuntimeException e) {
throw new DecodeException(e.getMessage(), e);
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I prefer a global way to handle it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we do this globally or just in this decoder?

@kdavisk6 we could, but that would break current API for all decoders...

If you think of a OptionalDecoder the result for a 204 or 404 is Optional.empty()

For a JAXB decoder null
For a List decoder may be Collections.emptyList() (which I currently use for a binary format decoder.

So, although I would like to see this in a single location, I don't think we can.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fair enough. Let's go with this for now and keep an eye out for other issues.

@kdavisk6 kdavisk6 added this to the 10.1.0 milestone Sep 24, 2018
@kdavisk6 kdavisk6 merged commit 3c7af57 into OpenFeign:master Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Unexpected or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants