Curl Logging interceptor

[doyaaaaaken]

Implements https://github.com/rybalkinsd/kohttp/issues/139

I don't know if this PR is correct direction. So, feel free to decline this PR.

[DeviantBadge]

Good changes, you are on the right way, @doyaaaaaken

[codecov[bot]]

Codecov Report

Merging #141 into master will increase coverage by 0.31%. The diff coverage is 96.55%.

@@             Coverage Diff             @@
##             master    #141      +/-   ##
===========================================
+ Coverage     90.59%   90.9%   +0.31%     
- Complexity      120     130      +10     
===========================================
  Files            40      42       +2     
  Lines           372     396      +24     
  Branches         45      45              
===========================================
+ Hits            337     360      +23     
  Misses            9       9              
- Partials         26      27       +1

Impacted Files	Coverage Δ	Complexity Δ
...kohttp/interceptors/logging/HttpLoggingStrategy.kt	100% <100%> (ø)	3 <3> (?)
...balkinsd/kohttp/interceptors/LoggingInterceptor.kt	100% <100%> (ø)	2 <1> (-1)	:arrow_down:
...kohttp/interceptors/logging/CurlLoggingStrategy.kt	95% <95%> (ø)	8 <8> (?)

---

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 c6dfcba...c64641a. Read the comment docs.

[doyaaaaaken]

@rybalkinsd

1. Do we really want to mix Log and Curl interceptors?

Yes, I think so, because the responsibility of LoggingInterceptor is to intercept logging process and to customize the way of logging (log message, log output target, etc...). This responsibility contains to output curl command log.

2. If we do - outputCurlCommand: Boolean seems to be not the best choice. Imagine we will decide to print pure HTTP request (not curl, or log) in the future

Yes, I agree. How do you think about below LoggingInterceptor class signature. The idea is to add setOutputCurlCommand(boolean) method in LoggingInterceptor, instead of outputCurlCommand constructor parameter.

val client = defaultHttpClient.fork {
    interceptors {
       // old version code
       +LoggingInterceptor()
        +LoggingInterceptor { msg ->
            println(msg)
         }
       // new version code
       +LoggingInterceptor().setOutputCurlCommand(true)
       +LoggingInterceptor { msg ->
            println(msg)
         }.setOutputCurlCommand(true)
}

[rybalkinsd]

I would recommend to think about enum of output strategies: plain http, curl, m/b something else.

Our top priority is to make a DSL. That’s the main reason I would recommend not to create methods like .setOutput...() and choose function parameter with good default value.

[doyaaaaaken]

@rybalkinsd

I see! So how about below LoggingInterceptor class signature?

class LoggingInterceptor(
        private val formats: List<LoggingFormat> = listOf(LoggingFormat.PLAIN),
        private val log: (String) -> Unit = ::println
) : Interceptor { 
//...
}

enum LoggingFormat { PLAIN, CURL_COMMAND }

[rybalkinsd]

@doyaaaaaken

It looks much more user friendly! I would suggest to use single format. Also probably need to think about better name instead of ‘PLAIN’ - any ideas?

 class LoggingInterceptor(
         private val format: LoggingFormat = PLAIN,
         private val log: (String) -> Unit = ::println
 ) : Interceptor { }

enum LoggingFormat { PLAIN, CURL }

@DeviantBadge your opinion?

[doyaaaaaken]

I come up with the enum name candidates: DEFAULT, NATIVE, SUMMARY, ORIGINAL. But any doesn't fit, so please decide any you want.

[rybalkinsd]

@doyaaaaaken I take some time to think about. And come up with HTTP and CURL at the beginning. HTTP should be a default. Currently we don’t have a correct HTTP logging, we should implement it in a different issue, replacing the current default.

So let’s

1. introduce enum in this PR
2. Change interceptor signature
3. Put HTTP as default
4. Make TODO in default implementation
5. Make correct implementation for CURL
6. Create an issue to implement HTTP
7. Make sure we have uptodate tests

[rybalkinsd]

Great! That looks much better in terms of DevX. let me have a look at code in next two days.

[doyaaaaaken]

Thank you! Let me have time to look at this from next Sunday.

On Thu, Aug 1, 2019, 20:14 Sergei Rybalkin notifications@github.com wrote:

@rybalkinsd requested changes on this pull request.

@doyaaaaaken https://github.com/doyaaaaaken We're close to making it production-ready. So, let's review the thing I mentioned.

I also suggest you to sign in our Gitter channel to get some discussions if you'll need

In README.md https://github.com/rybalkinsd/kohttp/pull/141#discussion_r309638713:

 Usage:

 ```kotlin

 val client = defaultHttpClient.fork {

                 interceptors {

• +LoggingInterceptor()

• +LoggingInterceptor(false)

this change looks outdated now

In src/main/kotlin/io/github/rybalkinsd/kohttp/ext/RequestExt.kt https://github.com/rybalkinsd/kohttp/pull/141#discussion_r309639386:

@@ -0,0 +1,38 @@

+package io.github.rybalkinsd.kohttp.ext

+

+import okhttp3.Request

+import okio.Buffer

+import java.nio.charset.Charset

+

+/**

• • Build curl command of the request

• *

• • @author doyaaaaaken

• */

+internal fun Request.buildCurlCommand(): String {

• return StringBuilder().apply {

  1. I suggest to avoid a big-big builder scope here.
  2. we also have buildString to simplify

---

In src/main/kotlin/io/github/rybalkinsd/kohttp/ext/RequestExt.kt https://github.com/rybalkinsd/kohttp/pull/141#discussion_r309639742:

+import okhttp3.Request

+import okio.Buffer

+import java.nio.charset.Charset

+

+/**

• • Build curl command of the request

• *

• • @author doyaaaaaken

• */

+internal fun Request.buildCurlCommand(): String {

• return StringBuilder().apply {

• append("curl -X ${method()}")

• //headers

• val headers = headers()

• for (i in 0 until headers.size()) {

we have asSequence() method for headers

In src/main/kotlin/io/github/rybalkinsd/kohttp/ext/RequestExt.kt https://github.com/rybalkinsd/kohttp/pull/141#discussion_r309640552:

+

+/**

• • Build curl command of the request

• *

• • @author doyaaaaaken

• */

+internal fun Request.buildCurlCommand(): String {

• return StringBuilder().apply {

• append("curl -X ${method()}")

• //headers

• val headers = headers()

• for (i in 0 until headers.size()) {

• val name = headers.name(i)

• var value = headers.value(i)

• if (value[0] == '"' && value[value.length - 1] == '"') {

this segment of code looks rather complicated for me, probably it will be easier with the sequence, or will need further simplification

please check .last() and .first() methods

In src/main/kotlin/io/github/rybalkinsd/kohttp/ext/RequestExt.kt https://github.com/rybalkinsd/kohttp/pull/141#discussion_r309640768:

+/**

• • Build curl command of the request

• *

• • @author doyaaaaaken

• */

+internal fun Request.buildCurlCommand(): String {

• return StringBuilder().apply {

• append("curl -X ${method()}")

• //headers

• val headers = headers()

• for (i in 0 until headers.size()) {

• val name = headers.name(i)

• var value = headers.value(i)

• if (value[0] == '"' && value[value.length - 1] == '"') {

• value = "\\"" + value.substring(1, value.length - 1) + "\\""

probably, """ string""" triple quoted string will look better here

In src/main/kotlin/io/github/rybalkinsd/kohttp/ext/RequestExt.kt https://github.com/rybalkinsd/kohttp/pull/141#discussion_r309641252:

• return StringBuilder().apply {

• append("curl -X ${method()}")

• //headers

• val headers = headers()

• for (i in 0 until headers.size()) {

• val name = headers.name(i)

• var value = headers.value(i)

• if (value[0] == '"' && value[value.length - 1] == '"') {

• value = "\\"" + value.substring(1, value.length - 1) + "\\""

• }

• append(" -H \"$name: $value\"")

• }

• //body

• body()?.let {

please double-check LoggingInterceptor. it looks like a code duplication

In src/main/kotlin/io/github/rybalkinsd/kohttp/interceptors/LoggingInterceptor.kt https://github.com/rybalkinsd/kohttp/pull/141#discussion_r309641852:

         }

     }

 }

+

• private fun logAsHttp(request: Request) {

I suggest to refactor as a strategy pattern.

In src/main/kotlin/io/github/rybalkinsd/kohttp/interceptors/LoggingInterceptor.kt https://github.com/rybalkinsd/kohttp/pull/141#discussion_r309642078:

         }

     }

 }

+

• private fun logAsHttp(request: Request) {

• //TODO: output http request format log.

• // see https://github.com/rybalkinsd/kohttp/pull/141#issuecomment-516428314

• request.headers().asSequence().forEach { log("${it.name}: ${it.value}") }

• Buffer().use {

• request.body()?.writeTo(it)

• log(it.readByteString().utf8())

• }

• }

• private fun logAsCurl(request: Request) {

• val command = request.buildCurlCommand()

• log("╭--- cURL command -------------------------------")

It's much better to keep the identical output style for both http and curl

In src/main/kotlin/io/github/rybalkinsd/kohttp/interceptors/LoggingInterceptor.kt https://github.com/rybalkinsd/kohttp/pull/141#discussion_r309642482:

         }

     }

 }

+

• private fun logAsHttp(request: Request) {

• //TODO: output http request format log.

• // see https://github.com/rybalkinsd/kohttp/pull/141#issuecomment-516428314

• request.headers().asSequence().forEach { log("${it.name}: ${it.value}") }

• Buffer().use {

• request.body()?.writeTo(it)

• log(it.readByteString().utf8())

• }

• }

• private fun logAsCurl(request: Request) {

• val command = request.buildCurlCommand()

• log("╭--- cURL command -------------------------------")

here we don't really know the user's screen width, so ---- line m/b too long

In src/main/kotlin/io/github/rybalkinsd/kohttp/ext/RequestExt.kt https://github.com/rybalkinsd/kohttp/pull/141#discussion_r309643836:

@@ -0,0 +1,38 @@

+package io.github.rybalkinsd.kohttp.ext

+

+import okhttp3.Request

+import okio.Buffer

+import java.nio.charset.Charset

+

+/**

• • Build curl command of the request

• *

• • @author doyaaaaaken

• */

+internal fun Request.buildCurlCommand(): String {

After reading PR several times, I want to ask if we really need this ext function, or we simply can split it into several methods within strategy pattern, as I mentioned in another comment and put it close to the LogginInterceptor package?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rybalkinsd/kohttp/pull/141?email_source=notifications&email_token=ABJNJMMIB5GCZNDKWPO6DXTQCLAPXA5CNFSM4IFNUCB2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCAIOOLI#pullrequestreview-269543213, or mute the thread https://github.com/notifications/unsubscribe-auth/ABJNJMPVSKCZ2NKP2TZCYU3QCLAPXANCNFSM4IFNUCBQ .

[doyaaaaaken]

I agree with all of your review comments!

Not yet finish to fix, so please wait. I already finish to fix for the comments attached with 👍 emoji.

[rybalkinsd]

@doyaaaaaken Also added some more thoughts after your changes. As I see now, it should be possible to target 0.11.0 version with this PR

[doyaaaaaken]

@rybalkinsd Thank you for your detail review! 🙇 I improved code. Please review it when you have the time.