logbook icon indicating copy to clipboard operation
logbook copied to clipboard

logbook-ktor-server removes Content-Type header from response

Open grassehh opened this issue 1 year ago • 5 comments

When using logbook-ktor-server with Ktor Content Negotation server plugin, the Content-Type header is not returned in the response.

Description

Content Negotation feature for Ktor server automatically detects the Content-Type based on the data returned. For instance, configuring it with Jackon as a deserializer allows Ktor server to add the Content-Type: application/json header in the response. However, also installing logbook-ktor-server result in the header being removed in the response.

Expected Behavior

The Content-Type: application/json header should be returned in the response

Actual Behavior

The Content-Type header is missing in the response

Steps to Reproduce

  1. Create a simple Ktor server app with these features installed:
install(ContentNegotiation) {
        jackson()
}
install(LogbookServer) {
        logbook = defaultLogbook
}
  1. Add a route that returns some JSON object
data class SomeData(val someProp: String)
route("/test") {
     get {
       call.respond(SomeData("someValue")) 
     }
}
  1. Run the server then call the retour
$curl http://localhost:8080/test

Context

I had some integration tests assertions with RestAssured that failed after adding logbook-ktor-server because the Content-Type header was no longer returned, thus RestAssured is no longer able to parse the response.

Your Environment

ktor 2.3.3 logbook 3.4.0

I haven't had time to make a sample yet sorry.

grassehh avatar Sep 12 '23 18:09 grassehh

TLDR: The issue is more general than using the ContentNegotiation plugin. It has to do with requests that don't have a body.

While running the server with a GET endpoint, and sending a GET request, I see this in the logs:

java.lang.IllegalStateException: No instance for key AttributeKey: Logbook.ResponseProcessingStage

The issue is in a map lookup here (where responseProcessingStageKey = AttributeKey("Logbook.ResponseProcessingStage")):

https://github.com/zalando/logbook/blob/60314c2181dd4d03fcd892db6e01f9e350970e27/logbook-ktor-server/src/main/kotlin/org/zalando/logbook/server/LogbookServer.kt#L58

The map was supposed to be filled by the proper value here:

https://github.com/zalando/logbook/blob/60314c2181dd4d03fcd892db6e01f9e350970e27/logbook-ktor-server/src/main/kotlin/org/zalando/logbook/server/LogbookServer.kt#L53

The above line is executed when intercepting the Receive Pipeline during the ApplicationReceivePipeline.Before phase:

https://github.com/zalando/logbook/blob/60314c2181dd4d03fcd892db6e01f9e350970e27/logbook-ktor-server/src/main/kotlin/org/zalando/logbook/server/LogbookServer.kt#L40

As per Ktor docs for ApplicationReceivePipeline:

A pipeline for processing incoming content. When executed, this pipeline starts with an instance of ByteReadChannel.

Using a debugger, I noted that this phase is only executed when the request has some content (body). The unit tests also set up the server with POST endpoints:

https://github.com/zalando/logbook/blob/60314c2181dd4d03fcd892db6e01f9e350970e27/logbook-ktor-server/src/test/kotlin/org/zalando/logbook/server/LogbookServerTest.kt#L62-L76


@grassehh Based on the above explanation, do you know how to fix the issue?

msdousti avatar Sep 19 '23 20:09 msdousti

PS: The receive pipeline can be manually triggered by something like this (here, I added call.receiveText() to your code):

data class SomeData(val someProp: String)
route("/test") {
     get {
       call.receiveText()
       call.respond(SomeData("someValue")) 
     }
}

msdousti avatar Sep 19 '23 21:09 msdousti

Would like to diagnose and fix this issue if its up for grabs

adarshjhaa100 avatar Sep 19 '23 23:09 adarshjhaa100

Would like to diagnose and fix this issue if its up for grabs

@adarshjhaa100 Thanks a lot, true open source spirit! Any help is appreciated. I did a very preliminary diagnosis in the two comments above yours. For the rest, I'll count on your expertise buddy 💯

msdousti avatar Sep 20 '23 06:09 msdousti

PS: The receive pipeline can be manually triggered by something like this (here, I added call.receiveText() to your code):

data class SomeData(val someProp: String)
route("/test") {
     get {
       call.receiveText()
       call.respond(SomeData("someValue")) 
     }
}

Thanks for the analysis. Indeed we also did notice this issue for GET routes on our project. The workaround as you mentionned is to manually trigger the pipeline by calling one of the call.receive() methods. However I did not mention that in this issue as we thought this is not related. Unfortunatly, I don't have enough Ktor expertise (and especially about its pipeline mecanism) in order to find the fix yet.

grassehh avatar Sep 20 '23 08:09 grassehh

Hello, @ChristianLohmann @msdousti, is there any update about the No instance for key AttributeKey: Logbook.ResponseProcessingStage issue ?

grassehh avatar Mar 13 '24 09:03 grassehh

Hello, I just noticed the same issue is true for logbook-ktor-client. As soon as I as a logback.xml file in my classpath, the Content-Type gets removed. This also means that using filters like JacksonJsonFieldBodyFilter do not work as they primarily check if there is an Content-Type: application/json header.

EDIT: @ChristianLohmann @sokomishalov shouldn't this line return it instead of ByteArrayContent(content) ? I have tried it and it seems to fix the Content-Type header being lost because Ktor actually carries the Content-Type in the OutgoingContent instance instead of actual context headers (Ktor removes the header, probably to avoid having it twice? I don't know).

I have also slightly modified the ClientRequest.getHeaders() and ClientRequest.getContentType() methods like that (it is in Kotlin sorry but I think you can get the idea):

 override fun getHeaders(): HttpHeaders = HttpHeaders.of(request.headers.build().toMap())
        .also {
            if (it.contains(ContentType).not() && request.body is OutgoingContent) return it.update(
                ContentType,
                (request.body as OutgoingContent).contentType.toString()
            )
        }

    override fun getContentType(): String? =
        request.contentType()?.toString()?.substringBefore(";") ?: (request.body as? OutgoingContent)?.contentType?.toString()

This way, features like the JacksonJsonFieldBodyFilter work again, and it also logs the Content-Type header correctly in the request. I don't know about the ClientResponse though.

grassehh avatar Apr 03 '24 18:04 grassehh

@grassehh: I opened a PR that hopefully addresses the issue with the logbook-ktor-server module; please take a look.

For the issue you reported with the logbook-ktor-client, I prefer to have a separate PR, to keep each PR easier to review. I suggest opening a separate issue for that.

msdousti avatar Apr 29 '24 01:04 msdousti

Thanks @msdousti. I have added some comments. As for the logbook-ktor-client I have created the issue with a possible fix: https://github.com/zalando/logbook/issues/1821

I have detected another issue when sending/receiving requests with large bodies, which leads to some kind of dead locks. I will create it later once I have the unit test.

grassehh avatar Apr 29 '24 08:04 grassehh