feat(rt): add support for HTTP proxies #665
Conversation
| * # Disabling proxy selection explicitly | ||
| * | ||
| * ``` | ||
| * proxySelector = ProxySelector { _ -> ProxyConfig.Direct } |
There was a problem hiding this comment.
We should really just expose this for the caller.
fun interface ProxySelector {
fun select(url: Url): ProxyConfig
companion object {
val Direct = ProxySelector { ProxyConfig.Direct }
}
}
val hConfig = HttpClientEngineConfig {
proxySelector = ProxySelector.Direct
}
| private val emptyProxyList = mutableListOf<Proxy>() | ||
|
|
||
| internal class OkHttpProxySelector( | ||
| private val sdkSelector: SdkProxySelector | ||
| ) : ProxySelector() { | ||
| override fun select(uri: URI?): MutableList<Proxy> { | ||
| if (uri == null) return emptyProxyList | ||
| val url = uri.toUrl() |
There was a problem hiding this comment.
Comment: Returning a static/global MutableList to a caller that could modify it seems unsafe. Do we know for sure how this list is going to be used?
There was a problem hiding this comment.
It looks like compiler is happy with List or MutableList and intellij suggested MutableList when scaffolding the implementation and I didn't check the assumption. I will update it to use an immutable list
| } | ||
| override fun connectFailed(uri: URI?, sa: SocketAddress?, ioe: IOException?) { |
There was a problem hiding this comment.
This seems like something that should be enforced by ktlint/format if we are going to go that route...
| internal interface TestEngineFactory { | ||
| /** | ||
| * Unique name for the engine | ||
| */ | ||
| val name: String | ||
|
|
||
| /** | ||
| * Create a new [HttpClientEngine] instance configured by [block] | ||
| */ | ||
| fun create(block: HttpClientEngineConfig.Builder.() -> Unit): HttpClientEngine | ||
| } | ||
|
|
||
| internal fun TestEngineFactory(name: String, configure: (HttpClientEngineConfig.Builder.() -> Unit) -> HttpClientEngine): TestEngineFactory = | ||
| object : TestEngineFactory { | ||
| override val name: String = name | ||
| override fun create(block: HttpClientEngineConfig.Builder.() -> Unit): HttpClientEngine = configure(block) | ||
| } |
There was a problem hiding this comment.
Question: Why not just a data class?
data class TestEngineFactory(
val name: String,
val configure: (HttpClientEngineConfig.Builder.() -> Unit) -> HttpClientEngine,
) {
fun create(block: HttpClientEngineConfig.Builder.() -> Unit): HttpClientEngine = configure(block)
}Because the last constructor arg is a function type you can still instantiate it with a lambda:
TestEngineFactory("DefaultHttpEngine") { DefaultHttpEngine(it) }Or even more simply with a constructor reference:
TestEngineFactory("DefaultHttpEngine", ::DefaultHttpEngine)| } | ||
| internal fun KtorOkHttpEngine(block: HttpClientEngineConfig.Builder.() -> Unit): HttpClientEngine { |
| proxySelector = ProxySelector { _ -> | ||
| ProxyConfig.Http("http://127.0.0.1:$proxyPort") | ||
| } |
| test { _, client -> | ||
| val req = HttpRequest { | ||
| url(Url.parse("http://aws.amazon.com")) | ||
| url.path = "/" | ||
| header("Host", "aws.amazon.com") | ||
| } | ||
|
|
||
| val call = client.call(req) | ||
| try { | ||
| // will be a 301 for http -> https if proxy isn't setup or not configured properly | ||
| assertEquals(HttpStatusCode.OK, call.response.status, "${client.engine}") | ||
| val body = call.response.body.readAll()!!.decodeToString() | ||
| assertEquals("hello proxy", body) | ||
| } finally { | ||
| call.complete() | ||
| } | ||
| } |
There was a problem hiding this comment.
Question: I see a lot of duplication between this test and the one above it. Can they be refactored to use more common code?
| @@ -0,0 +1,45 @@ | |||
| """ | |||
There was a problem hiding this comment.
Comment: This file probably needs a copyright header just like our Kotlin files do.
|
Kudos, SonarCloud Quality Gate passed!
|
Issue #
closes #494
Description of changes
Adds support for common HTTP proxy setup including JVM system properties and environment variables
Does not include support for:
CONNECTrequest).BasicBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.