From 1e1d3a60317ce52c1279a83438fe5e9c4eaf216d Mon Sep 17 00:00:00 2001 From: Mitch Ware Date: Tue, 31 Oct 2023 11:32:58 -0400 Subject: [PATCH 01/10] Add failing test for GzipCompressionPool.decompress w/empty Buffer --- .../com/connectrpc/compression/GzipCompressionPoolTest.kt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/src/test/kotlin/com/connectrpc/compression/GzipCompressionPoolTest.kt b/library/src/test/kotlin/com/connectrpc/compression/GzipCompressionPoolTest.kt index 99badacc..45ea3a17 100644 --- a/library/src/test/kotlin/com/connectrpc/compression/GzipCompressionPoolTest.kt +++ b/library/src/test/kotlin/com/connectrpc/compression/GzipCompressionPoolTest.kt @@ -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("") + } } From e785f9c7d9790f59dd1fb90fbc8b13b667acc3bd Mon Sep 17 00:00:00 2001 From: Mitch Ware Date: Tue, 31 Oct 2023 11:43:54 -0400 Subject: [PATCH 02/10] reproduce EOFException error with new failing tests in ConnectInterceptorTest --- .../protocols/ConnectInterceptorTest.kt | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/library/src/test/kotlin/com/connectrpc/protocols/ConnectInterceptorTest.kt b/library/src/test/kotlin/com/connectrpc/protocols/ConnectInterceptorTest.kt index 6f1982fa..212a3cd7 100644 --- a/library/src/test/kotlin/com/connectrpc/protocols/ConnectInterceptorTest.kt +++ b/library/src/test/kotlin/com/connectrpc/protocols/ConnectInterceptorTest.kt @@ -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( @@ -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( From 6bca00f6b756daf595b01a0e0ec67f80716e8341 Mon Sep 17 00:00:00 2001 From: Mitch Ware Date: Tue, 31 Oct 2023 17:07:09 -0400 Subject: [PATCH 03/10] Add okhttp mockwebserver to version catalog --- gradle/libs.versions.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 4a20e1d6..de01588c 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -44,6 +44,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" } From 91e733fa6e1b10cc35c51d37db034d7a684e3326 Mon Sep 17 00:00:00 2001 From: Mitch Ware Date: Tue, 31 Oct 2023 17:14:30 -0400 Subject: [PATCH 04/10] Add coroutines test dependency to version catalog --- gradle/libs.versions.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index de01588c..a124ce22 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -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" } From acbf39cdded3cb1e7cd9fcca8d25c5505d781166 Mon Sep 17 00:00:00 2001 From: Mitch Ware Date: Tue, 31 Oct 2023 17:15:51 -0400 Subject: [PATCH 05/10] Add failing test w/MockWebServer to demonstrate issue 138 Gzip decompression fails when empty error responses are returned --- okhttp/build.gradle.kts | 6 ++ .../connectrpc/okhttp/MockWebServerTests.kt | 61 +++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt diff --git a/okhttp/build.gradle.kts b/okhttp/build.gradle.kts index 4b96f1fc..b5784d83 100644 --- a/okhttp/build.gradle.kts +++ b/okhttp/build.gradle.kts @@ -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(":conformance:google-java")) } mavenPublishing { diff --git a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt new file mode 100644 index 00000000..1c190324 --- /dev/null +++ b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt @@ -0,0 +1,61 @@ +package com.connectrpc.okhttp + +import com.connectrpc.ProtocolClientConfig +import com.connectrpc.RequestCompression +import com.connectrpc.compression.GzipCompressionPool +import com.connectrpc.conformance.v1.TestServiceClient +import com.connectrpc.conformance.v1.simpleRequest +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 okhttp3.mockwebserver.MockWebServer +import org.assertj.core.api.Assertions.assertThat +import org.junit.Test + +class MockWebServerTests { + + @Test + fun `compressed empty failure response is parsed correctly`() = runTest { + val mockWebServer = MockWebServer() + mockWebServer.start() + + mockWebServer.enqueue( + MockResponse().apply { + addHeader("accept-encoding", "gzip") + addHeader("content-encoding", "gzip") + setBody("{}") + setResponseCode(401) + }, + ) + + val host = mockWebServer.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 request = simpleRequest {} + TestServiceClient(protocolClient).unaryCall(request) + + mockWebServer.takeRequest().apply { + assertThat(path).isEqualTo("/connectrpc.conformance.v1.TestService/UnaryCall") + } + + mockWebServer.shutdown() + } +} \ No newline at end of file From 4969ccc24f59303a853539b89f758d461e79189f Mon Sep 17 00:00:00 2001 From: Mitch Ware Date: Tue, 31 Oct 2023 11:33:19 -0400 Subject: [PATCH 06/10] fix EOFException on gzip decompress w/empty buffer --- .../kotlin/com/connectrpc/compression/GzipCompressionPool.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/src/main/kotlin/com/connectrpc/compression/GzipCompressionPool.kt b/library/src/main/kotlin/com/connectrpc/compression/GzipCompressionPool.kt index affcb68e..74fee5af 100644 --- a/library/src/main/kotlin/com/connectrpc/compression/GzipCompressionPool.kt +++ b/library/src/main/kotlin/com/connectrpc/compression/GzipCompressionPool.kt @@ -38,6 +38,8 @@ object GzipCompressionPool : CompressionPool { override fun decompress(buffer: Buffer): Buffer { val result = Buffer() + if (buffer.size == 0L) return result + GzipSource(buffer).use { while (it.read(result, Int.MAX_VALUE.toLong()) != -1L) { // continue reading. From 09359deaccc62e4c72d70aff8669360a370c1b00 Mon Sep 17 00:00:00 2001 From: Mitch Ware Date: Tue, 31 Oct 2023 17:49:01 -0400 Subject: [PATCH 07/10] Add license header --- .../com/connectrpc/okhttp/MockWebServerTests.kt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt index 1c190324..d917e56e 100644 --- a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt +++ b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt @@ -1,3 +1,17 @@ +// 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.ProtocolClientConfig From 5c620eb2c088ab92bdad7936349bf87d8e466abd Mon Sep 17 00:00:00 2001 From: Mitch Ware Date: Fri, 3 Nov 2023 14:54:03 -0400 Subject: [PATCH 08/10] Update MockWebServer tests to depend on generated java protos --- okhttp/build.gradle.kts | 2 +- .../com/connectrpc/okhttp/MockWebServerTests.kt | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/okhttp/build.gradle.kts b/okhttp/build.gradle.kts index b5784d83..e39dcd76 100644 --- a/okhttp/build.gradle.kts +++ b/okhttp/build.gradle.kts @@ -21,7 +21,7 @@ dependencies { testImplementation(libs.okhttp.mockwebserver) testImplementation(libs.kotlin.coroutines.test) testImplementation(project(":extensions:google-java")) - testImplementation(project(":conformance:google-java")) + testImplementation(project(":examples:generated-google-java")) } mavenPublishing { diff --git a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt index d917e56e..4c895935 100644 --- a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt +++ b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt @@ -14,11 +14,12 @@ package com.connectrpc.okhttp +import com.connectrpc.Code import com.connectrpc.ProtocolClientConfig import com.connectrpc.RequestCompression import com.connectrpc.compression.GzipCompressionPool -import com.connectrpc.conformance.v1.TestServiceClient -import com.connectrpc.conformance.v1.simpleRequest +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 @@ -63,13 +64,14 @@ class MockWebServerTests { ), ) - val request = simpleRequest {} - TestServiceClient(protocolClient).unaryCall(request) + val response = ElizaServiceClient(protocolClient).say(sayRequest { sentence = "hello" }) mockWebServer.takeRequest().apply { - assertThat(path).isEqualTo("/connectrpc.conformance.v1.TestService/UnaryCall") + assertThat(path).isEqualTo("/connectrpc.eliza.v1.ElizaService/Say") } + assertThat(response.code).isEqualTo(Code.UNKNOWN) + mockWebServer.shutdown() } } \ No newline at end of file From 2c75ee6061f3c8cf4ba77e1d330cacdb20c2fc9f Mon Sep 17 00:00:00 2001 From: Mitch Ware Date: Fri, 3 Nov 2023 16:49:23 -0400 Subject: [PATCH 09/10] Add MockWebServer JUnit4 dependency & use server Rule in mock test --- .../connectrpc/okhttp/MockWebServerRule.kt | 38 +++++++++++++++++++ .../connectrpc/okhttp/MockWebServerTests.kt | 15 +++----- 2 files changed, 44 insertions(+), 9 deletions(-) create mode 100644 okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerRule.kt diff --git a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerRule.kt b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerRule.kt new file mode 100644 index 00000000..99556ca4 --- /dev/null +++ b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerRule.kt @@ -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() + } +} \ No newline at end of file diff --git a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt index 4c895935..660eee13 100644 --- a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt +++ b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt @@ -27,18 +27,17 @@ import kotlinx.coroutines.test.runTest import okhttp3.OkHttpClient import okhttp3.Protocol import okhttp3.mockwebserver.MockResponse -import okhttp3.mockwebserver.MockWebServer 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 { - val mockWebServer = MockWebServer() - mockWebServer.start() - - mockWebServer.enqueue( + mockWebServerRule.server.enqueue( MockResponse().apply { addHeader("accept-encoding", "gzip") addHeader("content-encoding", "gzip") @@ -47,7 +46,7 @@ class MockWebServerTests { }, ) - val host = mockWebServer.url("/") + val host = mockWebServerRule.server.url("/") val protocolClient = ProtocolClient( ConnectOkHttpClient( @@ -66,12 +65,10 @@ class MockWebServerTests { val response = ElizaServiceClient(protocolClient).say(sayRequest { sentence = "hello" }) - mockWebServer.takeRequest().apply { + mockWebServerRule.server.takeRequest().apply { assertThat(path).isEqualTo("/connectrpc.eliza.v1.ElizaService/Say") } assertThat(response.code).isEqualTo(Code.UNKNOWN) - - mockWebServer.shutdown() } } \ No newline at end of file From ff93b5147793c159fd6b2031b48c37662617df09 Mon Sep 17 00:00:00 2001 From: Mitch Ware Date: Fri, 3 Nov 2023 16:54:57 -0400 Subject: [PATCH 10/10] spotless apply --- .../test/kotlin/com/connectrpc/okhttp/MockWebServerRule.kt | 2 +- .../test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerRule.kt b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerRule.kt index 99556ca4..4efb8e71 100644 --- a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerRule.kt +++ b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerRule.kt @@ -35,4 +35,4 @@ class MockWebServerRule( super.finished(description) server.shutdown() } -} \ No newline at end of file +} diff --git a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt index 660eee13..5651df42 100644 --- a/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt +++ b/okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt @@ -52,7 +52,7 @@ class MockWebServerTests { ConnectOkHttpClient( OkHttpClient.Builder() .protocols(listOf(Protocol.HTTP_2, Protocol.HTTP_1_1)) - .build() + .build(), ), ProtocolClientConfig( host = host.toString(), @@ -71,4 +71,4 @@ class MockWebServerTests { assertThat(response.code).isEqualTo(Code.UNKNOWN) } -} \ No newline at end of file +}