Fix: use http.rewrites to configure the evaluator in message passing API (#1141)

This is a bugfix, where the `rewrites` option is currently ignored.

Also: return any illegal argument errors from creating the evaluator as
an error in the CreateEvaluatorResponse
This commit is contained in:
Daniel Chao
2025-07-24 07:30:31 -07:00
committed by GitHub
parent ae046a804b
commit 1c4fbe7c1c
6 changed files with 117 additions and 48 deletions
@@ -183,6 +183,7 @@ class Server(private val transport: MessageTransport) : AutoCloseable {
} }
private fun createEvaluator(message: CreateEvaluatorRequest, evaluatorId: Long): BinaryEvaluator { private fun createEvaluator(message: CreateEvaluatorRequest, evaluatorId: Long): BinaryEvaluator {
try {
val modulePaths = message.modulePaths ?: emptyList() val modulePaths = message.modulePaths ?: emptyList()
val resolver = ModulePathResolver(modulePaths) val resolver = ModulePathResolver(modulePaths)
val allowedModules = message.allowedModules?.map { Pattern.compile(it) } ?: emptyList() val allowedModules = message.allowedModules?.map { Pattern.compile(it) } ?: emptyList()
@@ -200,6 +201,7 @@ class Server(private val transport: MessageTransport) : AutoCloseable {
proxy.noProxy?.let { System.setProperty("http.nonProxyHosts", it.joinToString("|")) } proxy.noProxy?.let { System.setProperty("http.nonProxyHosts", it.joinToString("|")) }
} }
message.http?.caCertificates?.let(::addCertificates) message.http?.caCertificates?.let(::addCertificates)
message.http?.rewrites?.let(::setRewrites)
buildLazily() buildLazily()
} }
val dependencies = val dependencies =
@@ -226,6 +228,9 @@ class Server(private val transport: MessageTransport) : AutoCloseable {
dependencies, dependencies,
message.outputFormat, message.outputFormat,
) )
} catch (e: IllegalArgumentException) {
throw ProtocolException(e.message ?: "Failed to create an evalutor. $e", e)
}
} }
private fun createResourceReaders( private fun createResourceReaders(
@@ -99,8 +99,8 @@ class ServerMessagePackDecoder(unpacker: MessageUnpacker) : BaseMessagePackDecod
getNullable(httpMap, "rewrites") getNullable(httpMap, "rewrites")
?.asMapValue() ?.asMapValue()
?.map() ?.map()
?.mapKeys { it.key.asStringValue().asString() } ?.mapKeys { URI(it.key.asStringValue().asString()) }
?.mapValues { it.value.asStringValue().asString() } ?.mapValues { URI(it.value.asStringValue().asString()) }
return Http(caCertificates, proxy, rewrites) return Http(caCertificates, proxy, rewrites)
} }
@@ -48,8 +48,8 @@ class ServerMessagePackEncoder(packer: MessagePacker) : BaseMessagePackEncoder(p
packString("rewrites") packString("rewrites")
packMapHeader(rewrites.size) packMapHeader(rewrites.size)
for ((key, value) in rewrites) { for ((key, value) in rewrites) {
packString(key) packString(key.toString())
packString(value) packString(value.toString())
} }
} }
} }
@@ -57,7 +57,7 @@ data class Http(
/** Proxy settings */ /** Proxy settings */
val proxy: Proxy?, val proxy: Proxy?,
/** HTTP rewrites */ /** HTTP rewrites */
val rewrites: Map<String, String>?, val rewrites: Map<URI, URI>?,
) { ) {
override fun equals(other: Any?): Boolean { override fun equals(other: Any?): Boolean {
if (this === other) return true if (this === other) return true
@@ -60,6 +60,26 @@ abstract class AbstractServerTest {
abstract val client: TestTransport abstract val client: TestTransport
private val blankCreateEvaluatorRequest =
CreateEvaluatorRequest(
requestId = 1,
http = null,
allowedModules = null,
allowedResources = null,
clientModuleReaders = null,
clientResourceReaders = null,
modulePaths = null,
env = null,
properties = null,
timeout = null,
rootDir = null,
cacheDir = null,
outputFormat = null,
project = null,
externalModuleReaders = null,
externalResourceReaders = null,
)
@Test @Test
fun `create and close evaluator`() { fun `create and close evaluator`() {
val evaluatorId = client.sendCreateEvaluatorRequest(123) val evaluatorId = client.sendCreateEvaluatorRequest(123)
@@ -931,6 +951,50 @@ abstract class AbstractServerTest {
) )
} }
@Test
fun `http rewrites`() {
val evaluatorId =
client.sendCreateEvaluatorRequest(
http =
Http(
caCertificates = null,
proxy = null,
rewrites = mapOf(URI("https://example.com/") to URI("https://example.example/")),
)
)
client.send(
EvaluateRequest(
1,
evaluatorId,
URI("repl:text"),
"res = import(\"https://example.com/foo.pkl\")",
"output.text",
)
)
val response = client.receive<EvaluateResponse>()
assertThat(response.error)
.contains(
"request was rewritten: https://example.com/foo.pkl -> https://example.example/foo.pkl"
)
}
@Test
fun `http rewrites -- invalid rule`() {
client.send(
blankCreateEvaluatorRequest.copy(
http =
Http(
caCertificates = null,
proxy = null,
rewrites = mapOf(URI("https://example.com") to URI("https://example.example/")),
)
)
)
val response = client.receive<CreateEvaluatorResponse>()
assertThat(response.error)
.contains("Rewrite rule must end with '/', but was 'https://example.com'")
}
private val ByteArray.debugYaml private val ByteArray.debugYaml
get() = MessagePackDebugRenderer(this).output.trimIndent() get() = MessagePackDebugRenderer(this).output.trimIndent()
@@ -96,7 +96,7 @@ class ServerMessagePackCodecTest {
Http( Http(
proxy = Proxy(URI("http://foo.com:1234"), listOf("bar", "baz")), proxy = Proxy(URI("http://foo.com:1234"), listOf("bar", "baz")),
caCertificates = byteArrayOf(1, 2, 3, 4), caCertificates = byteArrayOf(1, 2, 3, 4),
rewrites = mapOf("https://foo.com" to "https://bar.com"), rewrites = mapOf(URI("https://foo.com/") to URI("https://bar.com/")),
), ),
externalModuleReaders = mapOf("external" to externalReader, "external2" to externalReader), externalModuleReaders = mapOf("external" to externalReader, "external2" to externalReader),
externalResourceReaders = mapOf("external" to externalReader), externalResourceReaders = mapOf("external" to externalReader),