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

Support empty buffers on Gzip decompression #139

Merged
merged 10 commits into from
Nov 8, 2023
2 changes: 2 additions & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ dokka-plugin = { module = "org.jetbrains.dokka:dokka-gradle-plugin", version.ref
junit = { module = "junit:junit", version.ref = "junit" }
kotlin-coroutines-android = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-android", version.ref = "coroutines" }
kotlin-coroutines-core = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-core", version.ref = "coroutines" }
kotlin-coroutines-test = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-test", version.ref = "coroutines" }
kotlin-jsr223 = { module = "org.jetbrains.kotlin:kotlin-scripting-jsr223", version.ref = "kotlin" }
kotlin-plugin = { module = "org.jetbrains.kotlin:kotlin-gradle-plugin", version.ref = "kotlin" }
kotlin-reflect = { module = "org.jetbrains.kotlin:kotlin-reflect", version.ref = "kotlin" }
Expand All @@ -44,6 +45,7 @@ moshiKotlin = { module = "com.squareup.moshi:moshi-kotlin", version.ref = "moshi
moshiKotlinCodegen = { module = "com.squareup.moshi:moshi-kotlin-codegen", version.ref = "moshi" }
okhttp-core = { module = "com.squareup.okhttp3:okhttp", version.ref = "okhttp" }
okhttp-tls = { module = "com.squareup.okhttp3:okhttp-tls", version.ref = "okhttp" }
okhttp-mockwebserver = { module = "com.squareup.okhttp3:mockwebserver", version.ref = "okhttp" }
okio-core = { module = "com.squareup.okio:okio", version.ref = "okio" }
protobuf-java = { module = "com.google.protobuf:protobuf-java", version.ref = "protobuf" }
protobuf-java-util = { module = "com.google.protobuf:protobuf-java-util", version.ref = "protobuf" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ object GzipCompressionPool : CompressionPool {

override fun decompress(buffer: Buffer): Buffer {
val result = Buffer()
if (buffer.size == 0L) return result
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense and is a good change (and we've amended the spec to account for it):

Servers must not attempt to decompress zero-length HTTP request content.

I think in this specific error case though, there was another issue. In

val (code, exception) = if (response.code != Code.OK) {
val error = parseConnectUnaryException(code = response.code, response.headers, response.message.buffer)
error.code to error
} else {
response.code to null
}
val message = compressionPool?.decompress(response.message.buffer) ?: response.message.buffer
, we're consuming the message body (if valid json) in parseConnectUnaryException, and then on line 104 we're again trying to consume the same message body a second time. If the code is not OK, we shouldn't be attempting to consume a response message and instead should just set message to an empty buffer.


GzipSource(buffer).use {
while (it.read(result, Int.MAX_VALUE.toLong()) != -1L) {
// continue reading.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,11 @@ class GzipCompressionPoolTest {
val resultString = compressionPool.decompress(result).readUtf8()
assertThat(resultString).isEqualTo("some_string")
}

@Test
fun emptyBufferGzipDecompression() {
val compressionPool = GzipCompressionPool
val resultString = compressionPool.decompress(Buffer()).readUtf8()
assertThat(resultString).isEqualTo("")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,34 @@ class ConnectInterceptorTest {
assertThat(decompressed.readUtf8()).isEqualTo("message")
}

@Test
fun compressedEmptyRequestMessage() {
val config = ProtocolClientConfig(
host = "https://connectrpc.com",
serializationStrategy = serializationStrategy,
requestCompression = RequestCompression(1, GzipCompressionPool),
compressionPools = listOf(GzipCompressionPool),
)
val connectInterceptor = ConnectInterceptor(config)
val unaryFunction = connectInterceptor.unaryFunction()

val request = unaryFunction.requestFunction(
HTTPRequest(
url = URL(config.host),
contentType = "content_type",
headers = emptyMap(),
message = "".commonAsUtf8ToByteArray(),
methodSpec = MethodSpec(
path = "",
requestClass = Any::class,
responseClass = Any::class,
),
),
)
val decompressed = GzipCompressionPool.decompress(Buffer().write(request.message!!))
assertThat(decompressed.readUtf8()).isEqualTo("")
}

@Test
fun uncompressedResponseMessage() {
val config = ProtocolClientConfig(
Expand Down Expand Up @@ -214,6 +242,28 @@ class ConnectInterceptorTest {
assertThat(response.message.readUtf8()).isEqualTo("message")
}

@Test
fun compressedEmptyResponseMessage() {
val config = ProtocolClientConfig(
host = "https://connectrpc.com",
serializationStrategy = serializationStrategy,
compressionPools = listOf(GzipCompressionPool),
)
val connectInterceptor = ConnectInterceptor(config)
val unaryFunction = connectInterceptor.unaryFunction()

val response = unaryFunction.responseFunction(
HTTPResponse(
code = Code.OK,
headers = mapOf(CONTENT_ENCODING to listOf(GzipCompressionPool.name())),
message = Buffer(),
trailers = emptyMap(),
tracingInfo = null,
),
)
assertThat(response.message.readUtf8()).isEqualTo("")
}

@Test
fun responseError() {
val config = ProtocolClientConfig(
Expand Down
6 changes: 6 additions & 0 deletions okhttp/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ dependencies {
implementation(libs.kotlin.coroutines.core)

api(project(":library"))

testImplementation(libs.assertj)
testImplementation(libs.okhttp.mockwebserver)
testImplementation(libs.kotlin.coroutines.test)
testImplementation(project(":extensions:google-java"))
testImplementation(project(":examples:generated-google-java"))
}

mavenPublishing {
Expand Down
38 changes: 38 additions & 0 deletions okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerRule.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2022-2023 The Connect Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.connectrpc.okhttp

import okhttp3.mockwebserver.MockWebServer
import org.junit.rules.TestWatcher
import org.junit.runner.Description

class MockWebServerRule(
private val port: Int = 0,
) : TestWatcher() {

lateinit var server: MockWebServer
private set

override fun starting(description: Description) {
super.starting(description)
server = MockWebServer()
server.start(port)
}

override fun finished(description: Description) {
super.finished(description)
server.shutdown()
}
}
74 changes: 74 additions & 0 deletions okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright 2022-2023 The Connect Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.connectrpc.okhttp

import com.connectrpc.Code
import com.connectrpc.ProtocolClientConfig
import com.connectrpc.RequestCompression
import com.connectrpc.compression.GzipCompressionPool
import com.connectrpc.eliza.v1.ElizaServiceClient
import com.connectrpc.eliza.v1.sayRequest
import com.connectrpc.extensions.GoogleJavaProtobufStrategy
import com.connectrpc.impl.ProtocolClient
import com.connectrpc.protocols.NetworkProtocol
import kotlinx.coroutines.test.runTest
import okhttp3.OkHttpClient
import okhttp3.Protocol
import okhttp3.mockwebserver.MockResponse
import org.assertj.core.api.Assertions.assertThat
import org.junit.Rule
import org.junit.Test

class MockWebServerTests {

@get:Rule val mockWebServerRule = MockWebServerRule()

@Test
fun `compressed empty failure response is parsed correctly`() = runTest {
mockWebServerRule.server.enqueue(
MockResponse().apply {
addHeader("accept-encoding", "gzip")
addHeader("content-encoding", "gzip")
setBody("{}")
setResponseCode(401)
},
)

val host = mockWebServerRule.server.url("/")

val protocolClient = ProtocolClient(
ConnectOkHttpClient(
OkHttpClient.Builder()
.protocols(listOf(Protocol.HTTP_2, Protocol.HTTP_1_1))
.build(),
),
ProtocolClientConfig(
host = host.toString(),
serializationStrategy = GoogleJavaProtobufStrategy(),
networkProtocol = NetworkProtocol.CONNECT,
requestCompression = RequestCompression(0, GzipCompressionPool),
compressionPools = listOf(GzipCompressionPool),
),
)

val response = ElizaServiceClient(protocolClient).say(sayRequest { sentence = "hello" })

mockWebServerRule.server.takeRequest().apply {
assertThat(path).isEqualTo("/connectrpc.eliza.v1.ElizaService/Say")
}

assertThat(response.code).isEqualTo(Code.UNKNOWN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrote a small connect-go example and verified that it gets the same result here. Thanks for adding these tests - they'll make it much easier to test different edge conditions outside of the conformance tests in the future.

}
}