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

Curl Logging interceptor #141

Merged
merged 27 commits into from
Aug 4, 2019

Conversation

doyaaaaaken
Copy link
Contributor

Implements #139

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

*/
internal fun Request.buildCurlCommand(): String {
var compressed = false
//TODO: test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I write test method sample in RequestExtTest.kt.
But not yet write test in many case.
(I write test only about simple GET request case)

After I confirm whether this PR's direction is correct, I'll write test code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are on a right way!
Several improvements and this changes will be accepted.

@@ -345,14 +345,15 @@ val forkedClient = defaultHttpClient.fork {
A Request Logging Interceptor.

Parameters:
1. `log: (String) -> Unit = ::println`: function as a parameter to consume the log message. It defaults to `println`. Logs Request body when present.
1. `outputCurlCommand: Boolean = false`: if it's true, output curl command on log message.
2. `log: (String) -> Unit = ::println`: function as a parameter to consume the log message. It defaults to `println`. Logs Request body when present.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is better to change parameters order:

  1. log: (String) -> Unit = ::println
  2. outputCurlCommand: Boolean = false
    First, because we do not break code of existing kohttp users (they will have compilation error with new library version)
    Second, because 'log' parameter represents main logic of LoggingInterceptor and 'outputCurlCommand' is additional parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because, log argument is function, this argument order cannot break code.
Below code is working correctly.

val client = defaultHttpClient.fork {
            interceptors {
                ///////old version code (outputCurlCommand argument does not exist)//////
                //no argument
                +LoggingInterceptor()
                //only log argument:
                +LoggingInterceptor { log ->
                    println(log)
                }
                ///////new version code (outputCurlCommand argument exists)//////
                //only outputCurlCommand
                +LoggingInterceptor(true)
                //outputCurlCommand and log arguments:
                +LoggingInterceptor(true) { log ->
                    println(log)
                }
            }
        }

I think log is function and need two or more line's code, so this order is better.
What do you think about it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are definitely right, I was wrong.

*/
internal fun Request.buildCurlCommand(): String {
var compressed = false
//TODO: test
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are on a right way!
Several improvements and this changes will be accepted.

class LoggingInterceptor(private val log: (String) -> Unit = ::println) : Interceptor {
class LoggingInterceptor(
private val outputCurlCommand: Boolean = false,
private val log: (String) -> Unit = ::println
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also one more time - change order of parameters

val command = request.buildCurlCommand()
log("╭--- cURL command -------------------------------")
log(command)
log("╰--- (copy and paste the above line to a terminal)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is better to put it before kotlin chain.proceed(request) because we logging here only request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! Fixed in 054f8f0

/**
* @author doyaaaaaken
*/
class RequestExtTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember that you will write more tests after our approval, but small comment - you should focus your attention on headers and also cookies

@DeviantBadge
Copy link
Collaborator

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

@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

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

Impacted file tree graph

@@             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) ⬇️
...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.

Copy link
Contributor Author

@doyaaaaaken doyaaaaaken left a comment

Choose a reason for hiding this comment

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

@doyaaaaaken Thank you your feedback!

@@ -345,14 +345,15 @@ val forkedClient = defaultHttpClient.fork {
A Request Logging Interceptor.

Parameters:
1. `log: (String) -> Unit = ::println`: function as a parameter to consume the log message. It defaults to `println`. Logs Request body when present.
1. `outputCurlCommand: Boolean = false`: if it's true, output curl command on log message.
2. `log: (String) -> Unit = ::println`: function as a parameter to consume the log message. It defaults to `println`. Logs Request body when present.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because, log argument is function, this argument order cannot break code.
Below code is working correctly.

val client = defaultHttpClient.fork {
            interceptors {
                ///////old version code (outputCurlCommand argument does not exist)//////
                //no argument
                +LoggingInterceptor()
                //only log argument:
                +LoggingInterceptor { log ->
                    println(log)
                }
                ///////new version code (outputCurlCommand argument exists)//////
                //only outputCurlCommand
                +LoggingInterceptor(true)
                //outputCurlCommand and log arguments:
                +LoggingInterceptor(true) { log ->
                    println(log)
                }
            }
        }

I think log is function and need two or more line's code, so this order is better.
What do you think about it?

val command = request.buildCurlCommand()
log("╭--- cURL command -------------------------------")
log(command)
log("╰--- (copy and paste the above line to a terminal)")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! Fixed in 054f8f0

@doyaaaaaken doyaaaaaken changed the title [WIP] Curl Logging interceptor Curl Logging interceptor Jul 23, 2019
Copy link
Owner

@rybalkinsd rybalkinsd left a comment

Choose a reason for hiding this comment

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

Hi all,

I had 10 mins to have a look at this PR. General direction is good! And I have several suggestions and questions to think about:

  1. Do we really want to mix Log and Curl interceptors?
  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

@doyaaaaaken
Copy link
Contributor Author

@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.

  1. 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
Copy link
Owner

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
Copy link
Contributor Author

@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
Copy link
Owner

@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
Copy link
Contributor Author

doyaaaaaken commented Jul 29, 2019

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

@rybalkinsd
Copy link
Owner

@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

Copy link
Contributor Author

@doyaaaaaken doyaaaaaken left a comment

Choose a reason for hiding this comment

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

@rybalkinsd
I improved my code!

  1. Create an issue to implement HTTP

This issue should be created after this PR will be merged. So, I don't create it yet.

@@ -55,4 +55,16 @@ class LoggingInterceptorTest {
assertEquals(200, response.code())
}

@Test
fun `curl command logging happens without exceptions `() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test's output is below.

GET 200 - 1118ms https://postman-echo.com/get
╭--- cURL command -------------------------------
curl -X GET "https://postman-echo.com/get"
╰--- (copy and paste the above line to a terminal)

And a test result in the case of LoggingFormat.HTTP is below.

POST 200 - 2076ms http://postman-echo.com/post
--80d27159-868d-4252-8b65-392dd2596483
Content-Disposition: form-data; name="file"; filename="cat.gif"
Content-Length: 1046038

	���"��#��+'!5+!95/=1"F;1H7%LB4LG@T��YL?ZH3]SE^YThUAh`Wm]HpbRqh`t��xaHxtp}nc}ys~jS~mZ�iK�|u�u`��7�{m�w^��~�z^��s��e�����{��g��F��r�����{�����������]������¸�Ʋ��þ�����|�ɡ���������������!��NETSCAPE2.0�����!���
��-"���Q�]t�ZB��C���x�q��w#�4�
qߋR9�
M!
�?��8���+[򒜂+���I�d"F�P��seiqSXa�b���=e�e(��c�/���z骎�j�*�6������e�O�v���a ������w����de�\��>YU:��A�4���z?�8� Z�ϸߓ�
�^�'+
���
-c�f"4��oG�ID�9[��\�9��� h� �07�%�HڥX堟�	M]�n�p��P:t�r^�-�BI$�F��{
......... skip the rest

@rybalkinsd
Copy link
Owner

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

Copy link
Owner

@rybalkinsd rybalkinsd left a comment

Choose a reason for hiding this comment

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

@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

README.md Outdated

Usage:

```kotlin
val client = defaultHttpClient.fork {
interceptors {
+LoggingInterceptor()
+LoggingInterceptor(false)
Copy link
Owner

Choose a reason for hiding this comment

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

this change looks outdated now

* @author doyaaaaaken
*/
internal fun Request.buildCurlCommand(): String {
return StringBuilder().apply {
Copy link
Owner

Choose a reason for hiding this comment

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

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


//headers
val headers = headers()
for (i in 0 until headers.size()) {
Copy link
Owner

Choose a reason for hiding this comment

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

we have asSequence() method for 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] == '"') {
Copy link
Owner

Choose a reason for hiding this comment

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

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

val name = headers.name(i)
var value = headers.value(i)
if (value[0] == '"' && value[value.length - 1] == '"') {
value = "\\\"" + value.substring(1, value.length - 1) + "\\\""
Copy link
Owner

Choose a reason for hiding this comment

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

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

}

//body
body()?.let {
Copy link
Owner

Choose a reason for hiding this comment

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

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

}
}
}

private fun logAsHttp(request: Request) {
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest to refactor as a strategy pattern.


private fun logAsCurl(request: Request) {
val command = request.buildCurlCommand()
log("╭--- cURL command -------------------------------")
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

*
* @author doyaaaaaken
*/
internal fun Request.buildCurlCommand(): String {
Copy link
Owner

Choose a reason for hiding this comment

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

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?

@doyaaaaaken
Copy link
Contributor Author

doyaaaaaken commented Aug 2, 2019 via email

@doyaaaaaken
Copy link
Contributor Author

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.

private fun buildCurlHeaderOption(headers: Headers): String {
return headers.asSequence().map { header ->
val value = header.value.let { v ->
if (v.startsWith('"') && v.endsWith('"')) {
Copy link
Owner

@rybalkinsd rybalkinsd Aug 2, 2019

Choose a reason for hiding this comment

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

I would suggest introducing private method String.isQuoted()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that's better.
And I suggest String.trimDoubleQuote() method is more simpler? ( ecae2fd )

Copy link
Owner

Choose a reason for hiding this comment

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

yes, agreed

}

private fun buildCurlBodyOption(body: RequestBody?): String {
return if (body == null) {
Copy link
Owner

Choose a reason for hiding this comment

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

Kotlin has a smart casting and also nullability checks.
It's possible to simplify code like this

Suggested change
return if (body == null) {
if (body == null) return ""

and it will be also possible to omit else statement

val buffer = Buffer().apply { body.writeTo(this) }
val utf8 = Charset.forName("UTF-8")
val charset = body.contentType()?.charset(utf8) ?: utf8
" --data $'" + buffer.readString(charset).replace("\n", "\\n") + "'"
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please share why we need to replace \n here?

Also looks like utf-8 should be introduced as a const for both HTTP and CURL at least

Copy link
Contributor Author

Choose a reason for hiding this comment

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

utf-8 should be introduced as a const for both HTTP and CURL at least

In case of HTTP, ByteString.utf8() method is used currently.

And in case of CURL, we can use Buffer.readUtf8() method in stead of assigning Charset class.
Which do you think better?

  1. Use Charset class. So, I fix logic of HTTP case.
  2. Use Buffer.readUtf8() method. So, I fix logic of CURL case.

Copy link
Contributor Author

@doyaaaaaken doyaaaaaken Aug 4, 2019

Choose a reason for hiding this comment

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

Could you please share why we need to replace \n here?

Because we can output curl command in one-liner.

For example, if we wouldn't replace \n, we would output below curl command.

curl -X POST --data $'{
   "aa": "bb",
   "aa1": "bb2"
}' "https://postman-echo.com/post"

But , with replacing \n, we could output command as one-liner.

curl -X POST --data $'{\n  "aa": "bb",\n  "aa1": "bb2"\n}' "https://postman-echo.com/post"

Copy link
Owner

@rybalkinsd rybalkinsd Aug 4, 2019

Choose a reason for hiding this comment

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

I see, make sense


@Test
fun `build curl command of request with header`() {
val request = Request.Builder()
Copy link
Owner

Choose a reason for hiding this comment

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

Probably, we need to introduce Request dsl builder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot find Request dsl builder in existing code.
So, do you mean that we should implement new dsl method like this?

request {
            url = "https://postman-echo.com/post"
            post = requestBody {
                contentType = "application/x-www-form-urlencoded"
                body = ""
            }
}

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, unfortunately, there is no one at the moment.
But I think it's better to introduce it as a separate issue later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree!

@rybalkinsd
Copy link
Owner

@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
Copy link
Contributor Author

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

Copy link
Owner

@rybalkinsd rybalkinsd left a comment

Choose a reason for hiding this comment

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

LGFM!

@rybalkinsd rybalkinsd merged commit c88850f into rybalkinsd:master Aug 4, 2019
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.

3 participants