rybalkinsd / kohttp

Kotlin DSL http client
https://kohttp.gitbook.io
Apache License 2.0
478 stars 42 forks source link

Add tests for HttpPostContext content-type #140

Closed doyaaaaaken closed 5 years ago

doyaaaaaken commented 5 years ago

Hello, I try to resolve #62 , but I can't do to the last. Because, this comment's ( https://github.com/rybalkinsd/kohttp/issues/62#issue-396198795 ) 1st way ( header { type } ) seems not to work.

So, at first I try to implement 2nd and 3rd way's test.

doyaaaaaken commented 5 years ago

header { type } seems not to work.

I try to run below test code, but failed.

@Test
    fun `application json content type when content type is set on 'header' method`() {
        val expected = "application/json; charset=utf-8"
        val context = HttpPostContext()
        context.header { "Content-Type" to "application/json; charset=utf-8" }
        context.body { string("content") }

        assertEquals(expected, context.makeBody().contentType().toString())
    }

Can I fix production code (below /src code) in other PR? (After this PR is merged)

rybalkinsd commented 5 years ago

Hi @doyaaaaaken ! Thanks for your contribution. I’ll be able to handle this PR in 10 days. @DeviantBadge would you have time to check earlier?

DeviantBadge commented 5 years ago

Hello, @doyaaaaaken ! Many thanks for your effort! I am ready to accept your pull request if you add information about last way to set Content-type (through headers directly), also we need to get approve from @rybalkinsd .

Question about setting Content-type with kotlin header{ "Content-type" to type } is little bit deeper. You know that we are using okhttp under the hood, so part of logic is transferred to okhttp library. If we dig deeper, we can find, that okhttp sets 'Content-type' header if body present and Content-type in it is not 'null' (Ref. 'BridgeInterceptor' line 54 in okhttp version '3.14.2')

So, if body was set and Content-type is not null, request will use Content-type from body and will ignore header If body was set and Content-type is null, request will use header if body was not set, request will also use header

You can also check it through postman-echo public api:

httpPost {
    url("https://postman-echo.com/post")
    header { "Content-Type" to "application/json; charset=utf-8" }
    body { form("content") }
}.use {
    // will print json with form Content-Type in it 
    print(it.body()?.string())
}

So, its very good that you tested HttpPostContext for setting contentType in body. But also it would be also great if you will add Integration tests with (for example) postman-echo, that will check request headers after all processing steps.

codecov[bot] commented 5 years ago

Codecov Report

Merging #140 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #140   +/-   ##
=========================================
  Coverage     90.59%   90.59%           
  Complexity      120      120           
=========================================
  Files            40       40           
  Lines           372      372           
  Branches         45       45           
=========================================
  Hits            337      337           
  Misses            9        9           
  Partials         26       26

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f217bd2...23e263b. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

Merging #140 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #140   +/-   ##
=========================================
  Coverage     90.59%   90.59%           
  Complexity      120      120           
=========================================
  Files            40       40           
  Lines           372      372           
  Branches         45       45           
=========================================
  Hits            337      337           
  Misses            9        9           
  Partials         26       26

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f217bd2...23e263b. Read the comment docs.

doyaaaaaken commented 5 years ago

@DeviantBadge Many thanks for your detail review! I improved my code by your review.

DeviantBadge commented 5 years ago

Great! Now we will merge your changes