ronantakizawa/codereview-bench huggingface dataset:
Repository: square/okhttp
File: okhttp/src/test/java/okhttp3/HttpUrlTest.kt (Kotlin)

Code before:
      .nonPrintableAscii(Encoding.FORBIDDEN)
      .nonAscii(Encoding.PUNYCODE)
      .override(
        Encoding.FORBIDDEN,
        '\t'.code,
        '\n'.code,
        '\u000c'.code,
        '\r'.code,
        ' '.code
      )
      .override(
        Encoding.FORBIDDEN,
        '#'.code,
        '%'.code,
        '/'.code,
        ':'.code,
        '?'.code,
        '@'.code,
        '['.code,
        '\\'.code,
        ']'.code
      )
      .override(
        Encoding.IDENTITY,
        '\"'.code,
        '<'.code,
        '>'.code,
        '^'.code,
        '`'.code,
        '{'.code,
        '|'.code,
        '}'.code
      )
      .test(UrlComponentEncodingTester.Component.HOST)
  }

  @Test
  fun hostIpv6() {
    // Square braces are absent from host()...
    assertThat(parse("http://[::1]/").host).isEqualTo("::1")

    // ... but they're included in toString().
    assertThat(parse("http://[::1]/").toString()).isEqualTo("http://[::1]/")

    // IPv6 colons don't interfere with port numbers or passwords.
    assertThat(parse("http://[::1]:8080/").port).isEqualTo(8080)
    assertThat(parse("http://user:password@[::1]/").password).isEqualTo("password")
    assertThat(parse("http://user:password@[::1]:8080/").host).isEqualTo("::1")

    // Permit the contents of IPv6 addresses to be percent-encoded...
    assertThat(parse("http://[%3A%3A%31]/").host).isEqualTo("::1")

Review comment (bug):
@swankjesse I'm not sure this is right, is there a better way to make assertions here? That it now fails for URL?

Code after:
    UrlComponentEncodingTester.newInstance()
      .nonPrintableAscii(Encoding.FORBIDDEN)
      .nonAscii(Encoding.PUNYCODE)
      .override(
        Encoding.FORBIDDEN,
        '\t'.code,
        '\n'.code,
        '\u000c'.code,
        '\r'.code,
        ' '.code
      )
      .override(
        Encoding.FORBIDDEN,
        '#'.code,
        '%'.code,
        '/'.code,
        ':'.code,
        '?'.code,
        '@'.code,
        '['.code,
        '\\'.code,
        ']'.code
      )
      .override(
        // java.net.URL got stricter
        if (PlatformVersion.majorVersion >= 21)  Encoding.SKIP else Encoding.IDENTITY,
        '\"'.code,
        '<'.code,
        '>'.code,
        '^'.code,
        '`'.code,
        '{'.code,
        '|'.code,
        '}'.code
      )
      .test(UrlComponentEncodingTester.Component.HOST)
  }

  @Test
  fun hostIpv6() {
    // Square braces are absent from host()...
    assertThat(parse("http://[::1]/").host).isEqualTo("::1")

    // ... but they're included in toString().
    assertThat(parse("http://[::1]/").toString()).isEqualTo("http://[::1]/")

    // IPv6 colons don't interfere with port numbers or passwords.
    assertThat(parse("http://[::1]:8080/").port).isEqualTo(8080)
    assertThat(parse("http://user:password@[::1]/").password).isEqualTo("password")
    assertThat(parse("http://user:password@[::1]:8080/").host).isEqualTo("::1")
Repository: square/okhttp
File: okhttp-android/src/main/kotlin/okhttp3/android/AndroidAsyncDns.kt (Kotlin)

Code before:
      network,
      hostname,
      dnsClass.type,
      DnsResolver.FLAG_EMPTY,
      executor,
      null,
      object : DnsResolver.Callback<List<InetAddress>> {
        override fun onAnswer(
          addresses: List<InetAddress>,
          rCode: Int,
        ) {
          callback.onResponse(hostname, addresses)
        }

        override fun onError(e: DnsResolver.DnsException) {
          callback.onFailure(
            hostname,
            UnknownHostException(e.message).apply {
              initCause(e)
            },
          )
        }
      },
    )
  }

  companion object {
    @RequiresApi(Build.VERSION_CODES.Q)
    val IPv4 = AndroidAsyncDns(dnsClass = AsyncDns.DnsClass.IPV4)

    @RequiresApi(Build.VERSION_CODES.Q)
    val IPv6 = AndroidAsyncDns(dnsClass = AsyncDns.DnsClass.IPV6)
  }
}


Review comment (bug):
Any idea what kinds of exceptions we’ll be potentially hiding here? We’re attributing an arbitrary failure like an IllegalStateException as an unknown host

Code after:
      resolver.query(
        network,
        hostname,
        dnsClass.type,
        DnsResolver.FLAG_EMPTY,
        executor,
        null,
        object : DnsResolver.Callback<List<InetAddress>> {
          override fun onAnswer(
            addresses: List<InetAddress>,
            rCode: Int,
          ) {
            callback.onResponse(hostname, addresses)
          }

          override fun onError(e: DnsResolver.DnsException) {
            callback.onFailure(
              hostname,
              UnknownHostException(e.message).apply {
                initCause(e)
              },
            )
          }
        },
      )
    } catch (e: Exception) {
      callback.onFailure(
        hostname,
        UnknownHostException(e.message).apply {
          initCause(e)
        },
      )
    }
  }

  companion object {
    @RequiresApi(Build.VERSION_CODES.Q)
    val IPv4 = AndroidAsyncDns(dnsClass = AsyncDns.DnsClass.IPV4)

    @RequiresApi(Build.VERSION_CODES.Q)
    val IPv6 = AndroidAsyncDns(dnsClass = AsyncDns.DnsClass.IPV6)
  }
}
Repository: square/okhttp
File: okhttp/src/test/java/okhttp3/CacheTest.kt (Kotlin)

Code before:
  }

  @Test
  fun requestMethodOptionsIsNotCached() {
    testRequestMethod("OPTIONS", false)
  }

  @Test
  fun requestMethodGetIsCached() {
    testRequestMethod("GET", true)
  }

  @Test
  fun requestMethodHeadIsNotCached() {
    // We could support this but choose not to for implementation simplicity
    testRequestMethod("HEAD", false)
  }

  @Test
  fun requestMethodPostIsNotCached() {
    // We could support this but choose not to for implementation simplicity
    testRequestMethod("POST", false)
  }

  @Test
  fun requestMethodPostIsNotCachedUnlessOverriden() {
    // Supported via cacheUrlOverride
    testRequestMethod("POST", true, withOverride = true)
  }

  @Test
  fun requestMethodPutIsNotCached() {
    testRequestMethod("PUT", false)
  }

  @Test
  fun requestMethodPutIsNotCachedEvenWithOverride() {
    testRequestMethod("PUT", false, withOverride = true)
  }

  @Test
  fun requestMethodDeleteIsNotCached() {
    testRequestMethod("DELETE", false)
  }

  @Test
  fun requestMethodTraceIsNotCached() {
    testRequestMethod("TRACE", false)
  }

  private fun testRequestMethod(

Review comment (performance):
```suggestion
  fun requestMethodPostIsNotCachedUnlessOverridden() {
```

Code after:
  }

  @Test
  fun requestMethodOptionsIsNotCached() {
    testRequestMethod("OPTIONS", false)
  }

  @Test
  fun requestMethodGetIsCached() {
    testRequestMethod("GET", true)
  }

  @Test
  fun requestMethodHeadIsNotCached() {
    // We could support this but choose not to for implementation simplicity
    testRequestMethod("HEAD", false)
  }

  @Test
  fun requestMethodPostIsNotCached() {
    // We could support this but choose not to for implementation simplicity
    testRequestMethod("POST", false)
  }

  @Test
  fun requestMethodPostIsNotCachedUnlessOverridden() {
    // Supported via cacheUrlOverride
    testRequestMethod("POST", true, withOverride = true)
  }

  @Test
  fun requestMethodPutIsNotCached() {
    testRequestMethod("PUT", false)
  }

  @Test
  fun requestMethodPutIsNotCachedEvenWithOverride() {
    testRequestMethod("PUT", false, withOverride = true)
  }

  @Test
  fun requestMethodDeleteIsNotCached() {
    testRequestMethod("DELETE", false)
  }

  @Test
  fun requestMethodTraceIsNotCached() {
    testRequestMethod("TRACE", false)
  }

  private fun testRequestMethod(
Repository: square/okhttp
File: android-test/src/androidTest/java/okhttp/android/test/StrictModeTest.kt (Kotlin)

Code before:
    assertThat(violations[0].message).isEqualTo("newSSLContext")
  }

  @Test
  fun testNewCall() {
    Platform.resetForTests()

    val client = OkHttpClient()

    applyStrictMode()

    // Safe on main
    client.newCall(Request("https://google.com/robots.txt".toHttpUrl()))

    assertThat(violations).isEmpty()
  }

  private fun applyStrictMode() {
    StrictMode.setThreadPolicy(
      ThreadPolicy.Builder()
        .detectCustomSlowCalls()
        .penaltyListener({ it.run() }) {
          violations.add(it)
        }
        .build(),
    )
  }
}


Review comment (suggestion):
Hmmm. When was this added? I would assume after our minSdk, so it should really have an `@SdkSuppress(minSdk=something)` on the class.

Code after:

    assertThat(violations).hasSize(1)
    assertThat(violations[0].message).isEqualTo("newSSLContext")
  }

  @Test
  fun testNewCall() {
    Platform.resetForTests()

    val client = OkHttpClient()

    applyStrictMode()

    // Safe on main
    client.newCall(Request("https://google.com/robots.txt".toHttpUrl()))

    assertThat(violations).isEmpty()
  }

  private fun applyStrictMode() {
    StrictMode.setThreadPolicy(
      ThreadPolicy.Builder()
        .detectCustomSlowCalls()
        .penaltyListener({ it.run() }) {
          violations.add(it)
        }
        .build(),
    )
  }
}
Repository: square/okhttp
File: samples/guide/src/main/java/okhttp3/recipes/UploadProgress.java (Java)

Code before:

	private static class ProgressRequestBody extends RequestBody {

		private final ProgressListener progressListener;
		private final RequestBody requestBody;

		public ProgressRequestBody(RequestBody requestBody, ProgressListener progressListener) {
			this.requestBody = requestBody;
			this.progressListener = progressListener;
		}

		@Override
		public MediaType contentType() {
			return requestBody.contentType();
		}

		@Override
		public long contentLength() throws IOException {
			return requestBody.contentLength();
		}

		@Override
		public void writeTo(BufferedSink sink) throws IOException {
			BufferedSink bufferSink = Okio.buffer(sink(sink));
			requestBody.writeTo(bufferSink);
			bufferSink.close();
		}

		public Sink sink(Sink sink) {
			return new ForwardingSink(sink) {
				private long totalBytesWritten = 0L;

				@Override
				public void write(Buffer source, long byteCount) throws IOException {
					super.write(source, byteCount);
					totalBytesWritten += byteCount;
					progressListener.update(totalBytesWritten, contentLength(), false);
				}

				@Override
				public void close() throws IOException {
					super.close();
					progressListener.update(totalBytesWritten, contentLength(),true);
				}
			};
		}
	}

	interface ProgressListener {
		void update(long bytesWritten, long contentLength, boolean done);
	}

Review comment (refactor):
Remove this, the implementation of `writeTo()` shouldn’t close `sink`

Code after:

	private static class ProgressRequestBody extends RequestBody {

		private final ProgressListener progressListener;
		private final RequestBody delegate;

		public ProgressRequestBody(RequestBody delegate, ProgressListener progressListener) {
			this.delegate = delegate;
			this.progressListener = progressListener;
		}

		@Override
		public MediaType contentType() {
			return delegate.contentType();
		}

		@Override
		public long contentLength() throws IOException {
			return delegate.contentLength();
		}

		@Override
		public void writeTo(BufferedSink sink) throws IOException {
			BufferedSink bufferedSink = Okio.buffer(sink(sink));
			delegate.writeTo(bufferedSink);
			bufferedSink.flush();
		}

		public Sink sink(Sink sink) {
			return new ForwardingSink(sink) {
				private long totalBytesWritten = 0L;
				private boolean completed = false;

				@Override
				public void write(Buffer source, long byteCount) throws IOException {
					super.write(source, byteCount);
					totalBytesWritten += byteCount;
					progressListener.update(totalBytesWritten, contentLength(), completed);
				}

				@Override
				public void close() throws IOException {
					super.close();
					if (!completed) {
						completed = true;
						progressListener.update(totalBytesWritten, contentLength(), completed);
					}
				}
			};
		}
	}
Repository: square/okhttp
File: mockwebserver-deprecated/build.gradle.kts (Kotlin)

Code before:
import com.vanniktech.maven.publish.JavadocJar
import com.vanniktech.maven.publish.KotlinJvm

plugins {
  kotlin("jvm")
  id("org.jetbrains.dokka")
  id("com.vanniktech.maven.publish.base")
  id("binary-compatibility-validator")
}

tasks.jar {
  manifest {
    attributes("Automatic-Module-Name" to "okhttp3.mockwebserver")
  }
}

dependencies {
  "friends"(projects.okhttp)
  api(projects.mockwebserver3)
  api(libs.junit)

  testImplementation(projects.okhttpTestingSupport)
  testImplementation(projects.okhttpTls)
  testImplementation(libs.kotlin.test.common)
  testImplementation(libs.kotlin.test.junit)
}

mavenPublishing {
  configure(KotlinJvm(javadocJar = JavadocJar.Empty()))
}


Review comment (suggestion):
Seems like this requires two configurations: `implementationFriend` and `apiFriend`. Otherwise you are changing the visibility of the transitive dependency, as evidence by all the unrelated changes below where you are having to add the dependency yourself when it should be automatically available.

Code after:
import com.vanniktech.maven.publish.JavadocJar
import com.vanniktech.maven.publish.KotlinJvm

plugins {
  kotlin("jvm")
  id("org.jetbrains.dokka")
  id("com.vanniktech.maven.publish.base")
  id("binary-compatibility-validator")
}

tasks.jar {
  manifest {
    attributes("Automatic-Module-Name" to "okhttp3.mockwebserver")
  }
}

dependencies {
  "friendsApi"(projects.okhttp)
  api(projects.mockwebserver3)
  api(libs.junit)

  testImplementation(projects.okhttpTestingSupport)
  testImplementation(projects.okhttpTls)
  testImplementation(libs.kotlin.test.common)
  testImplementation(libs.kotlin.test.junit)
}

mavenPublishing {
  configure(KotlinJvm(javadocJar = JavadocJar.Empty()))
}
Repository: square/okhttp
File: okhttp/src/jvmTest/kotlin/okhttp3/MultipartReaderTest.kt (Kotlin)

Code before:
    val buffer =
      Buffer().apply {
        multipartBody.writeTo(this)
      }

    val multipartReader = MultipartReader(buffer, "foo")
    while (true) {
      val part = multipartReader.nextPart()

      if (part == null) break

      assertThat(part.headers["header-name"]).isEqualTo("header-value")
      while (true) {
        val readBuff = Buffer()
        val read = part.body.read(readBuff, (1024).toLong())
        if (read == -1L) {
          break
        } else {
          assertThat(readBuff.readUtf8()).isEqualTo(
            "a".repeat(read.toInt()),
          )
        }
      }
    }
  }
}


Review comment (suggestion):
... well I guess one behavior change is we’re now asserting that the final byte count is as expected.

Code after:
              }
            }
          },
        ).build()
    val buffer = Buffer()
    multipartBody.writeTo(buffer)

    val multipartReader = MultipartReader(buffer, "foo")
    val onlyPart = multipartReader.nextPart()!!
    assertThat(onlyPart.headers).isEqualTo(
      headersOf(
        "header-name",
        "header-value",
        "Content-Type",
        "application/octet-stream",
      ),
    )
    val readBuff = Buffer()
    var byteCount = 0L
    while (true) {
      val readByteCount = onlyPart.body.read(readBuff, 1024L)
      if (readByteCount == -1L) break
      byteCount += readByteCount
      assertThat(readBuff.readUtf8()).isEqualTo("a".repeat(readByteCount.toInt()))
    }
    assertThat(byteCount).isEqualTo(1024L * 1024L * 100L)
    assertThat(multipartReader.nextPart()).isNull()
  }
}
Repository: square/okhttp
File: mockwebserver/src/main/kotlin/mockwebserver3/internal/duplex/MockSocketHandler.kt (Kotlin)

Code before:
      responseSent.countDown()
    }
  }

  fun exhaustResponse() =
    apply {
      actions += { stream -> stream.sink.close() }
    }

  fun sleep(
    duration: Long,
    unit: TimeUnit,
  ) = apply {
    actions += { Thread.sleep(unit.toMillis(duration)) }
  }

  override fun handle(socket: Socket) {
    val task = serviceSocketTask(socket)
    results.add(task)
    task.run()
  }

  /** Returns a task that processes both request and response from [socket]. */
  private fun serviceSocketTask(socket: Socket): FutureTask<Void> {
    return FutureTask<Void> {
      socket.source.use {
        socket.sink.use {
          while (true) {
            val action = actions.poll() ?: break
            action(BufferedSocket(socket))
          }
        }
      }
      return@FutureTask null
    }
  }

  /** Returns once all stream actions complete successfully. */
  fun awaitSuccess() {
    val futureTask =
      results.poll(5, TimeUnit.SECONDS)
        ?: throw AssertionError("no onRequest call received")
    futureTask.get(5, TimeUnit.SECONDS)
  }
}


Review comment (suggestion):
I was wondering if you can .use the socket. But explained here

https://square.github.io/okio/3.x/okio/okio/okio/-socket/index.html

Code after:
      responseSent.countDown()
    }
  }

  fun exhaustResponse() =
    apply {
      actions += { stream -> stream.sink.close() }
    }

  fun sleep(
    duration: Long,
    unit: TimeUnit,
  ) = apply {
    actions += { Thread.sleep(unit.toMillis(duration)) }
  }

  override fun handle(socket: Socket) {
    val task = serviceSocketTask(BufferedSocket(socket))
    results.add(task)
    task.run()
  }

  /** Returns a task that processes both request and response from [socket]. */
  private fun serviceSocketTask(socket: BufferedSocket): FutureTask<Void> {
    return FutureTask<Void> {
      socket.source.use {
        socket.sink.use {
          while (true) {
            val action = actions.poll() ?: break
            action(socket)
          }
        }
      }
      return@FutureTask null
    }
  }

  /** Returns once all stream actions complete successfully. */
  fun awaitSuccess() {
    val futureTask =
      results.poll(5, TimeUnit.SECONDS)
        ?: throw AssertionError("no onRequest call received")
    futureTask.get(5, TimeUnit.SECONDS)
  }
}
Repository: square/okhttp
File: okhttp/src/commonJvmAndroid/kotlin/okhttp3/Request.kt (Kotlin)

Code before:
        .append("\"")
        .append(name)
        .append(": ")
        .append(value)
        .append("\"")
    }

    body?.let { requestBody ->
      val buffer = okio.Buffer()
      requestBody.writeTo(buffer)

      val contentType = requestBody.contentType()?.toString() ?: ""

      if (contentType.startsWith("text/") ||
        contentType.contains("json") ||
        contentType.contains("xml") ||
        contentType.contains("form-urlencoded")
      ) {
        val bodyString = buffer.readUtf8()
        curl
          .append(" --data ")
          .append("\"")
          .append(bodyString.replace("\"", "\\\""))
          .append("\"")
      } else {
        curl.append(" --data-binary \"[binary body omitted]\"")
      }
    }

    curl.append(" \"").append(url).append("\"")
    return curl.toString()
  }
}


Review comment (suggestion):
I would think that we would want to hex-encode binary data rather than drop it. Plus we need to use an algorithm to detect binary data rather than base it on the content type.

Code after:
   *
   * @param binaryFileName default file name to use when dumping binary body data to a file (default: `"request_body.bin"`)
   * @param binaryMode default mode to use when writing binary body data (default: `"BinaryMode.STDIN"`)
   * @return a cURL command string representing this request.
   */
  @JvmOverloads
  fun toCurl(
    binaryMode: BinaryMode = BinaryMode.STDIN,
    binaryFileName: String? = "request_body.bin",
  ): String {
    val curl = StringBuilder("curl")

    // Add method if not GET
    if (method != "GET") {
      curl.append(" -X ").append(method)
    }

    // Append headers
    for ((name, value) in headers) {
      curl
        .append(" -H \"")
        .append(name)
        .append(": ")
        .append(value)
        .append("\"")
    }

    // Append body if present
    body?.let { requestBody ->
      val buffer = Buffer()
      requestBody.writeTo(buffer)

      // Clone so we can read multiple times without consuming
      val peekBuffer = buffer.clone()
      val isBinary = isBinaryData(peekBuffer)

      if (isBinary) {
        when (binaryMode) {
          BinaryMode.HEX -> {
            curl.append(" --data-binary \"")
            val hexBuffer = buffer.clone()
            while (!hexBuffer.exhausted()) {
              val b = hexBuffer.readByte().toInt() and 0xFF
              curl.append("%02x".format(b))
            }
            curl.append("\"")
          }
          BinaryMode.FILE -> {
            curl.append(" --data-binary @").append(binaryFileName)
          }
          BinaryMode.STDIN -> {
Repository: square/okhttp
File: okhttp/src/jvmTest/kotlin/okhttp3/RequestTest.kt (Kotlin)

Code before:
        "  \\\"roles\\\": [\\\"admin\\\", \\\"editor\\\"],\n" +
        "  \\\"active\\\": true\n" +
        "}\" \"https://api.example.com/users\"",
    )
  }

  @Test
  fun curlPostWithBinaryBody() {
    val mediaType = "application/octet-stream".toMediaType()
    val binaryData = byteArrayOf(0x00, 0x01, 0x02, 0x03, 0x04)

    val body = RequestBody.create(mediaType, binaryData)

    val request =
      Request
        .Builder()
        .url("https://api.example.com/upload")
        .post(body)
        .addHeader("Content-Type", "application/octet-stream")
        .build()

    val curl = request.curl()
    assertThat(
      curl,
    ).isEqualTo(
      "curl -X POST -H \"Content-Type: application/octet-stream\" --data-binary \"[binary body omitted]\" \"https://api.example.com/upload\"",
    )
  }

  private fun bodyToHex(body: RequestBody): String {
    val buffer = Buffer()
    body.writeTo(buffer)
    return buffer.readByteString().hex()
  }
}


Review comment (suggestion):
If the goal is that someone could run these, would it be worthwhile letting this be customised via optional param?

```
--data-binary @filename
--data-binary @-
```

Code after:
    assertThat(curl)
      .isEqualTo(
        "curl -X POST -H \"Content-Type: application/json\" -H \"Authorization: Bearer xyz789\" --data \"{\n" +
          "  \\\"user\\\": {\n" +
          "    \\\"id\\\": 123,\n" +
          "    \\\"name\\\": \\\"John Doe\\\"\n" +
          "  },\n" +
          "  \\\"roles\\\": [\\\"admin\\\", \\\"editor\\\"],\n" +
          "  \\\"active\\\": true\n" +
          "}\" \"https://api.example.com/users\"",
      )
  }

  @Test
  fun curlPostWithBinaryBody_DefaultSTDIN() {
    val mediaType = "application/octet-stream".toMediaType()
    val binaryData = byteArrayOf(0x00, 0x01, 0x02, 0x03)

    val body = binaryData.toRequestBody(mediaType)

    val request =
      Request
        .Builder()
        .url("https://api.example.com/upload")
        .post(body)
        .addHeader("Content-Type", "application/octet-stream")
        .build()

    val curl = request.toCurl() // default is BinaryMode.STDIN
    assertThat(curl)
      .isEqualTo(
        "curl -X POST -H \"Content-Type: application/octet-stream\" --data-binary @- \"https://api.example.com/upload\"",
      )
  }

  @Test
  fun curlPostWithBinaryBody_HexMode() {
    val mediaType = "application/octet-stream".toMediaType()
    val binaryData = byteArrayOf(0x00, 0x01, 0x02, 0x03)

    val body = binaryData.toRequestBody(mediaType)

    val request =
      Request
        .Builder()
        .url("https://api.example.com/upload")
        .post(body)
        .addHeader("Content-Type", "application/octet-stream")
        .build()

    val curl = request.toCurl(BinaryMode.HEX)