Skip to content
This repository has been archived by the owner on Jul 13, 2021. It is now read-only.

Upgrade ktor from 0.3.0 to 0.9.0 #29

Closed
wants to merge 2 commits into from
Closed

Upgrade ktor from 0.3.0 to 0.9.0 #29

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 6, 2017

resolves #27

@cy6erGn0m cy6erGn0m self-requested a review November 7, 2017 13:28
@@ -50,3 +51,15 @@ fun Application.main() {
}
}

class GsonConverter(private val gson: Gson = Gson()) : ContentConverter {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already done, isn't it ? It should be in ktor-gson

Copy link
Author

Choose a reason for hiding this comment

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

You're right, fixed this in d4b3be9


call.response.pipeline.intercept(ApplicationResponsePipeline.After) {
if(call.request.contentType().match(ContentType.Application.Json)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Request's content type for GET? I believe it makes no sense as GET requests has no body.
ETag should be always specified to get browser cache working

Copy link
Author

Choose a reason for hiding this comment

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

I tried to preserve the original behaviour, but I couldn't get the example to work without these changes. I could use some help on this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Most likely it doesn't work because of this change

-        override var headers: dynamic = json("Accept" to "application/json")
+        override var headers: dynamic = json("Content-Type" to contentType)

I'd say we actually need both Accept and Content-Type headers otherwise the server could reject it. Not sure what is the default Accept header value

@guenhter
Copy link

@dn1345 any progress on this? Really looking forward to see the upgrade.

@ghost ghost closed this Nov 10, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ktor 0.9.0
2 participants