Skip to content
This repository was 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.
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ktor 0.9.0
2 participants