ktorio / ktor

Framework for quickly creating connected applications in Kotlin with minimal effort
https://ktor.io
Apache License 2.0
13.09k stars 1.07k forks source link

Crash while chaning contentType for protobuf response #1624

Open Coneys opened 4 years ago

Coneys commented 4 years ago

Ktor Version and Engine Used (client or server and name) 1.3 Server

Describe the bug When trying to change contentType framework throws exception: io.ktor.http.UnsafeHeaderException: Header Content-Type is controlled by the engine and cannot be set explicitly

To Reproduce Steps to reproduce the behavior:

  1. I Wrote Protobuf converter for Kotlin serialization library and it works great when client asks for application/protobuf, but response doens't contain any content type
 class ProtobufConverter() : ContentConverter {
    override suspend fun convertForSend(
            context: PipelineContext<Any, ApplicationCall>,
            contentType: ContentType,
            value: Any
    ): Any? {
        context.context.response.header(HttpHeaders.ContentType,"application/protobuf")
        return ProtoBuf.dump(serializerForSending(value) as KSerializer<Any>, value)
    }

    override suspend fun convertForReceive(context: PipelineContext<ApplicationReceiveRequest, ApplicationCall>): Any? {
        val request = context.subject
        val channel = request.value as? ByteReadChannel ?: return null
        val serializer = serializerByTypeInfo(request.typeInfo)
        val content = channel.readRemaining().readBytes()
        return ProtoBuf.load(serializer, content)
    }

}
  1. When I am trying to change contentType with: context.context.response.header(HttpHeaders.ContentType,"application/protobuf") framework throws exception
  2. In docs i found that ApplicationResponse class should have function "contentType" but it is not there in Ktor 1.3

Expected behavior Any way to change contentType manually when application asks for application/protobuf

Coneys commented 4 years ago

There is workaround (which works):

context.call.response.headers.append(HttpHeaders.ContentType,"application/protobuf",false)

cy6erGn0m commented 4 years ago

Setting content type header for ApplicationResponse is not allowed. This header should be tied to a response object. For example, TextContent has it.

ContentConverter by design is not about headers but about content. So you shouldn't touch headers at all. A content converter is registered to ContentNegotiation feature.

https://ktor.io/servers/features/content-negotiation.html

Coneys commented 4 years ago

Okey, but then how ContentType header should be set? It cannot be tied to respond object, because respond object is simple Plain Old Kotlin Object with @Serializable annotation. After creating ContentConverter I just pass my object with call.respond(myObject) and its client decision to pick application/json or application/protobuf, my respond code is always the same

cy6erGn0m commented 4 years ago

~It is set by the content negotiation feature during negotiation procedure~ oops, my bad. It is actually set by converter but to a response object.

cy6erGn0m commented 4 years ago

Your response object is a byte array (produced by the dump function), not kotlin object. To be able to set extra parameters, you need an OutgoingContent instance. For example, ByteArrayContent provides a way to specify it.

Coneys commented 4 years ago

Correct me if I am wrong, you are saying, that I should not return simple byte array from dump function, but insted wrap it with ByteArrayContent? Then framework will change content type?

cy6erGn0m commented 4 years ago

Exactly. See how it is done in GsonSupport

Returning a byte array works as well but it will produce no content type.

Coneys commented 4 years ago

Thank you! I understand now. By the way, will there be official support for protobuf in Ktor? Writing converter is pretty straightforward so I wonder, if there are any other issues?

cy6erGn0m commented 4 years ago

Yeah, writing converters is a bit advanced feature. People usually use json, even XML is not that popular. Too few people actually need protobuf. This is the reason.

Coneys commented 4 years ago

Don't you think that lack of popularity comes from fact, that serializing and deserializing Protobuf was really hard without Kotlin? Now it is really straightforward, and we get way smaller output (in our company, some responses became 3x smaller)

cy6erGn0m commented 4 years ago

Most likely Protobuf support will be provided via kotlinx.serialization. We just need to design a converter config API for that purpose.