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

Fixed serialization of empty body #1952

Merged
merged 4 commits into from Jun 18, 2020
Merged

Conversation

wkornewald
Copy link
Contributor

@wkornewald wkornewald commented Jun 17, 2020

Subsystem
Client: JsonFeature/JsonSerializer

Motivation
When using JsonFeature the empty body (e.g. a get() request without a body) gets serialized to {} instead of an empty string. This causes problems with some backends which expect the body to be empty.

For more context, let's assume we define an HttpClient with JsonFeature and a default Content-Type header like this:

val client = HttpClient(...) {
    defaultRequest {
        contentType(ContentType.Application.Json)
    }
    install(JsonFeature) {
        // ...
    }
}

This gives us some more convenience because we don't need to add the Content-Type header to each post()/put() call. However, now every request where we don't assign a body (e.g. a simple get("https://...")) gets turned into a request containing {} in the body and having a Content-Type: application/json header.

This causes some backends to return an error because they expect an empty body for get() requests. Often, the backend isn't under our control, so we have to comply with the usual expection: No body in get() should mean no body in the resulting HTTP request.

Solution
With this PR, JsonFeature simply passes through EmptyContent. I think this really is the most sensible behavior, mapping the intent 1:1. It also removes the Content-Type header because there really is no content.

If someone wants to send an empty {} then that's still possible by explicitly setting e.g. body = Unit (or any serializable object without attributes). At least, now it's explicit.

If someone wants to send an empty body, but with Content-Type set (for obscure backend requirements), that's also still possible by customizing the serializer and using a special object as a marker.

@wkornewald
Copy link
Contributor Author

@e5l Since I can't add reviewers, could you please have a look at this fix? :)

@wkornewald
Copy link
Contributor Author

wkornewald commented Jun 18, 2020

I've updated the solution to directly deal with this in JsonFeature. At first I thought it would be better to leave this decision to the serializer (because who knows how some serialization formats represent "no content"), but in the end that's an invitation for inconsistency across serializers and thus it's too error-prone (I almost forgot about KotlinxSerializer because it's not covered by JsonTest).

@e5l e5l self-requested a review June 18, 2020 08:23
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

Hi @wkornewald, thanks for the PR.

LGTM! Just needs the minor fix to get unified behavior.

@wkornewald
Copy link
Contributor Author

@e5l You mean, like in the new commit I just pushed?

@e5l e5l merged commit 0aea426 into ktorio:master Jun 18, 2020
@e5l
Copy link
Member

e5l commented Jun 18, 2020

LGTM, merged. Thanks for the fast response!

@wkornewald wkornewald deleted the fix/gson-serializer branch August 26, 2020 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants