Untangle external reader code (#776)

- move the following classes into package externalreader:
  - ExternalModuleResolver
  - ExternalResourceResolver
  - MessageTransportModuleResolver (renamed to ExternalModuleResolverImpl, made package-private)
  - MessageTransportResourceResolver (renamed to ExternalResourceResolverImpl, made package-private)
- replace interface ExternalModuleResolver.Spec with record ExternalModuleReaderSpec
- replace interface ExternalResourceResolver.Spec with record ExternalResourceReaderSpec
- translate between messaging.ResourceReaderSpec and ExternalResourceReaderSpec (eliminates dependency from messaging on higher layer)
- translate between messaging.ResourceResolverSpec and ExternalResourceResolverSpec (eliminates dependency from messaging on higher layer)
- add ServerMessages.ExternalReader and translate between this message component and the PklEvaluatorSettings.ExternalReader API
- add ServerMessages.Proxy and translate between this message component and the PklEvaluatorSettings.Proxy API
- change type of CreateEvaluatorRequest.allowedModules/allowedResources from List<Pattern>? to List<String>?
  - removes a lot of code
  - should not need to create a Pattern object to send a message
- deprecate method evaluatorSettings.PklEvaluatorSettings.Proxy.create()
  - only seems useful internally, inlined

Co-authored-by: Dan Chao <dan.chao@apple.com>

pkl-server/src/main/kotlin/org/pkl/server/ClientModuleKeyFactory.kt

@@ -17,9 +17,9 @@ package org.pkl.server
 
 import java.net.URI
 import java.util.Optional
+import org.pkl.core.externalreader.ExternalModuleResolver
+import org.pkl.core.externalreader.ModuleReaderSpec
 import org.pkl.core.messaging.*
-import org.pkl.core.messaging.MessageTransportModuleResolver
-import org.pkl.core.messaging.Messages.*
 import org.pkl.core.module.*
 
 internal class ClientModuleKeyFactory(
@@ -29,8 +29,7 @@ internal class ClientModuleKeyFactory(
 ) : ModuleKeyFactory {
   private val schemes = readerSpecs.map { it.scheme }
 
-  private val resolver: MessageTransportModuleResolver =
-    MessageTransportModuleResolver(transport, evaluatorId)
+  private val resolver: ExternalModuleResolver = ExternalModuleResolver.of(transport, evaluatorId)
 
   override fun create(uri: URI): Optional<ModuleKey> =
     when (uri.scheme) {

pkl-server/src/main/kotlin/org/pkl/server/Server.kt

@@ -21,13 +21,16 @@ import java.net.URI
 import java.util.concurrent.ConcurrentHashMap
 import java.util.concurrent.ExecutorService
 import java.util.concurrent.Executors
+import java.util.regex.Pattern
 import kotlin.random.Random
 import org.pkl.core.*
-import org.pkl.core.evaluatorSettings.PklEvaluatorSettings.ExternalReader
+import org.pkl.core.evaluatorSettings.PklEvaluatorSettings
 import org.pkl.core.externalreader.ExternalReaderProcess
+import org.pkl.core.externalreader.ExternalResourceResolver
+import org.pkl.core.externalreader.ModuleReaderSpec
+import org.pkl.core.externalreader.ResourceReaderSpec
 import org.pkl.core.http.HttpClient
 import org.pkl.core.messaging.MessageTransport
-import org.pkl.core.messaging.MessageTransportResourceResolver
 import org.pkl.core.messaging.MessageTransports
 import org.pkl.core.messaging.ProtocolException
 import org.pkl.core.module.ModuleKeyFactories
@@ -182,8 +185,8 @@ class Server(private val transport: MessageTransport) : AutoCloseable {
   private fun createEvaluator(message: CreateEvaluatorRequest, evaluatorId: Long): BinaryEvaluator {
     val modulePaths = message.modulePaths ?: emptyList()
     val resolver = ModulePathResolver(modulePaths)
-    val allowedModules = message.allowedModules ?: emptyList()
-    val allowedResources = message.allowedResources ?: emptyList()
+    val allowedModules = message.allowedModules?.map { Pattern.compile(it) } ?: emptyList()
+    val allowedResources = message.allowedResources?.map { Pattern.compile(it) } ?: emptyList()
     val rootDir = message.rootDir
     val env = message.env ?: emptyMap()
     val properties = message.properties ?: emptyMap()
@@ -247,8 +250,12 @@ class Server(private val transport: MessageTransport) : AutoCloseable {
     for (readerSpec in message.clientResourceReaders ?: emptyList()) {
       add(
         ResourceReaders.externalResolver(
-          readerSpec,
-          MessageTransportResourceResolver(transport, evaluatorId),
+          ResourceReaderSpec(
+            readerSpec.scheme,
+            readerSpec.hasHierarchicalUris,
+            readerSpec.isGlobbable,
+          ),
+          ExternalResourceResolver.of(transport, evaluatorId),
         )
       )
     }
@@ -261,7 +268,11 @@ class Server(private val transport: MessageTransport) : AutoCloseable {
   ): List<ModuleKeyFactory> = buildList {
     // add client-side module key factory first to ensure it wins over builtin ones
     if (message.clientModuleReaders?.isNotEmpty() == true) {
-      add(ClientModuleKeyFactory(message.clientModuleReaders, transport, evaluatorId))
+      val readerSpecs =
+        message.clientModuleReaders.map {
+          ModuleReaderSpec(it.scheme, it.hasHierarchicalUris, it.isLocal, it.isGlobbable)
+        }
+      add(ClientModuleKeyFactory(readerSpecs, transport, evaluatorId))
     }
     for ((scheme, spec) in message.externalModuleReaders ?: emptyMap()) {
       add(
@@ -285,5 +296,7 @@ class Server(private val transport: MessageTransport) : AutoCloseable {
   private fun getExternalProcess(evaluatorId: Long, spec: ExternalReader): ExternalReaderProcess =
     externalReaderProcesses
       .computeIfAbsent(evaluatorId) { ConcurrentHashMap() }
-      .computeIfAbsent(spec) { ExternalReaderProcess.of(it) }
+      .computeIfAbsent(spec) {
+        ExternalReaderProcess.of(PklEvaluatorSettings.ExternalReader(it.executable, it.arguments))
+      }
 }

pkl-server/src/main/kotlin/org/pkl/server/ServerMessagePackDecoder.kt

@@ -19,12 +19,9 @@ import java.io.InputStream
 import java.net.URI
 import java.nio.file.Path
 import java.time.Duration
-import java.util.regex.Pattern
 import org.msgpack.core.MessagePack
 import org.msgpack.core.MessageUnpacker
 import org.msgpack.value.Value
-import org.pkl.core.evaluatorSettings.PklEvaluatorSettings
-import org.pkl.core.evaluatorSettings.PklEvaluatorSettings.ExternalReader
 import org.pkl.core.messaging.BaseMessagePackDecoder
 import org.pkl.core.messaging.Message
 import org.pkl.core.packages.Checksums
@@ -38,8 +35,8 @@ class ServerMessagePackDecoder(unpacker: MessageUnpacker) : BaseMessagePackDecod
       Message.Type.CREATE_EVALUATOR_REQUEST ->
         CreateEvaluatorRequest(
           get(map, "requestId").asIntegerValue().asLong(),
-          unpackStringListOrNull(map, "allowedModules", Pattern::compile),
-          unpackStringListOrNull(map, "allowedResources", Pattern::compile),
+          unpackStringListOrNull(map, "allowedModules"),
+          unpackStringListOrNull(map, "allowedResources"),
           unpackListOrNull(map, "clientModuleReaders") { unpackModuleReaderSpec(it)!! },
           unpackListOrNull(map, "clientResourceReaders") { unpackResourceReaderSpec(it)!! },
           unpackStringListOrNull(map, "modulePaths", Path::of),
@@ -101,11 +98,11 @@ class ServerMessagePackDecoder(unpacker: MessageUnpacker) : BaseMessagePackDecod
     return Http(caCertificates, proxy)
   }
 
-  private fun Map<Value, Value>.unpackProxy(): PklEvaluatorSettings.Proxy? {
+  private fun Map<Value, Value>.unpackProxy(): Proxy? {
     val proxyMap = getNullable(this, "proxy")?.asMapValue()?.map() ?: return null
     val address = unpackString(proxyMap, "address")
     val noProxy = unpackStringListOrNull(proxyMap, "noProxy")
-    return PklEvaluatorSettings.Proxy.create(address, noProxy)
+    return Proxy(URI(address), noProxy)
   }
 
   private fun Map<Value, Value>.unpackDependencies(name: String): Map<String, Dependency> {

pkl-server/src/main/kotlin/org/pkl/server/ServerMessagePackEncoder.kt

@@ -20,7 +20,6 @@ import java.nio.file.Path
 import kotlin.io.path.pathString
 import org.msgpack.core.MessagePack
 import org.msgpack.core.MessagePacker
-import org.pkl.core.evaluatorSettings.PklEvaluatorSettings.ExternalReader
 import org.pkl.core.messaging.BaseMessagePackEncoder
 import org.pkl.core.messaging.Message
 import org.pkl.core.packages.Checksums
@@ -105,8 +104,8 @@ class ServerMessagePackEncoder(packer: MessagePacker) : BaseMessagePackEncoder(p
           msg.externalResourceReaders,
         )
         packKeyValue("requestId", msg.requestId())
-        packKeyValue("allowedModules", msg.allowedModules?.map { it.toString() })
-        packKeyValue("allowedResources", msg.allowedResources?.map { it.toString() })
+        packKeyValue("allowedModules", msg.allowedModules)
+        packKeyValue("allowedResources", msg.allowedResources)
         if (msg.clientModuleReaders != null) {
           packer.packString("clientModuleReaders")
           packer.packArrayHeader(msg.clientModuleReaders.size)

pkl-server/src/main/kotlin/org/pkl/server/ServerMessages.kt

@@ -19,23 +19,16 @@ import java.net.URI
 import java.nio.file.Path
 import java.time.Duration
 import java.util.*
-import java.util.regex.Pattern
-import org.pkl.core.evaluatorSettings.PklEvaluatorSettings.ExternalReader
-import org.pkl.core.evaluatorSettings.PklEvaluatorSettings.Proxy
 import org.pkl.core.messaging.Message
-import org.pkl.core.messaging.Messages.*
+import org.pkl.core.messaging.Messages
 import org.pkl.core.packages.Checksums
 
-private fun <T> T?.equalsNullable(other: Any?): Boolean {
-  return Objects.equals(this, other)
-}
-
 data class CreateEvaluatorRequest(
   private val requestId: Long,
-  val allowedModules: List<Pattern>?,
-  val allowedResources: List<Pattern>?,
-  val clientModuleReaders: List<ModuleReaderSpec>?,
-  val clientResourceReaders: List<ResourceReaderSpec>?,
+  val allowedModules: List<String>?,
+  val allowedResources: List<String>?,
+  val clientModuleReaders: List<Messages.ModuleReaderSpec>?,
+  val clientResourceReaders: List<Messages.ResourceReaderSpec>?,
   val modulePaths: List<Path>?,
   val env: Map<String, String>?,
   val properties: Map<String, String>?,
@@ -52,58 +45,12 @@ data class CreateEvaluatorRequest(
   override fun type(): Message.Type = Message.Type.CREATE_EVALUATOR_REQUEST
 
   override fun requestId(): Long = requestId
-
-  // need to implement this manually because [Pattern.equals] returns false for two patterns
-  // that have the same underlying pattern string.
-  override fun equals(other: Any?): Boolean {
-    if (other == null) return false
-    if (other !is CreateEvaluatorRequest) return false
-    return requestId == other.requestId &&
-      Objects.equals(
-        allowedModules?.map { it.pattern() },
-        other.allowedModules?.map { it.pattern() },
-      ) &&
-      Objects.equals(
-        allowedResources?.map { it.pattern() },
-        other.allowedResources?.map { it.pattern() },
-      ) &&
-      clientModuleReaders.equalsNullable(other.clientModuleReaders) &&
-      clientResourceReaders.equalsNullable(other.clientResourceReaders) &&
-      modulePaths.equalsNullable(other.modulePaths) &&
-      env.equalsNullable(other.env) &&
-      properties.equalsNullable(other.properties) &&
-      timeout.equalsNullable(other.timeout) &&
-      rootDir.equalsNullable(other.rootDir) &&
-      cacheDir.equalsNullable(other.cacheDir) &&
-      outputFormat.equalsNullable(other.outputFormat) &&
-      project.equalsNullable(other.project) &&
-      http.equalsNullable(other.http) &&
-      externalModuleReaders.equalsNullable(other.externalModuleReaders) &&
-      externalResourceReaders.equalsNullable(other.externalResourceReaders)
-  }
-
-  @Suppress("DuplicatedCode") // false duplicate within method
-  override fun hashCode(): Int {
-    var result = requestId.hashCode()
-    result = 31 * result + allowedModules?.map { it.pattern() }.hashCode()
-    result = 31 * result + allowedResources?.map { it.pattern() }.hashCode()
-    result = 31 * result + clientModuleReaders.hashCode()
-    result = 31 * result + clientResourceReaders.hashCode()
-    result = 31 * result + modulePaths.hashCode()
-    result = 31 * result + env.hashCode()
-    result = 31 * result + properties.hashCode()
-    result = 31 * result + timeout.hashCode()
-    result = 31 * result + rootDir.hashCode()
-    result = 31 * result + cacheDir.hashCode()
-    result = 31 * result + outputFormat.hashCode()
-    result = 31 * result + project.hashCode()
-    result = 31 * result + http.hashCode()
-    result = 31 * result + externalModuleReaders.hashCode()
-    result = 31 * result + externalResourceReaders.hashCode()
-    return result
-  }
 }
 
+data class ExternalReader(val executable: String, val arguments: List<String>?)
+
+data class Proxy(val address: URI?, val noProxy: List<String>?)
+
 data class Http(
   /** PEM-format CA certificates as raw bytes. */
   val caCertificates: ByteArray?,

pkl-server/src/test/kotlin/org/pkl/server/AbstractServerTest.kt

@@ -19,7 +19,6 @@ import java.net.URI
 import java.nio.file.Path
 import java.util.concurrent.ExecutorService
 import java.util.concurrent.Executors
-import java.util.regex.Pattern
 import kotlin.io.path.createDirectories
 import kotlin.io.path.outputStream
 import kotlin.io.path.writeText
@@ -947,8 +946,8 @@ abstract class AbstractServerTest {
     val message =
       CreateEvaluatorRequest(
         123,
-        listOf(Pattern.compile(".*")),
-        listOf(Pattern.compile(".*")),
+        listOf(".*"),
+        listOf(".*"),
         moduleReaders,
         resourceReaders,
         modulePaths,

pkl-server/src/test/kotlin/org/pkl/server/ServerMessagePackCodecTest.kt

@@ -20,16 +20,13 @@ import java.io.PipedOutputStream
 import java.net.URI
 import java.nio.file.Path
 import java.time.Duration
-import java.util.regex.Pattern
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.Test
 import org.msgpack.core.MessagePack
-import org.pkl.core.evaluatorSettings.PklEvaluatorSettings
-import org.pkl.core.evaluatorSettings.PklEvaluatorSettings.ExternalReader
 import org.pkl.core.messaging.Message
 import org.pkl.core.messaging.MessageDecoder
 import org.pkl.core.messaging.MessageEncoder
-import org.pkl.core.messaging.Messages.*
+import org.pkl.core.messaging.Messages
 import org.pkl.core.packages.Checksums
 
 class ServerMessagePackCodecTest {
@@ -52,18 +49,16 @@ class ServerMessagePackCodecTest {
 
   @Test
   fun `round-trip CreateEvaluatorRequest`() {
-    val resourceReader1 = ResourceReaderSpec("resourceReader1", true, true)
-    val resourceReader2 = ResourceReaderSpec("resourceReader2", true, false)
-    val moduleReader1 = ModuleReaderSpec("moduleReader1", true, true, true)
-    val moduleReader2 = ModuleReaderSpec("moduleReader2", true, false, false)
+    val resourceReader1 = Messages.ResourceReaderSpec("resourceReader1", true, true)
+    val resourceReader2 = Messages.ResourceReaderSpec("resourceReader2", true, false)
+    val moduleReader1 = Messages.ModuleReaderSpec("moduleReader1", true, true, true)
+    val moduleReader2 = Messages.ModuleReaderSpec("moduleReader2", true, false, false)
     val externalReader = ExternalReader("external-cmd", listOf("arg1", "arg2"))
     roundtrip(
       CreateEvaluatorRequest(
         requestId = 123,
-        allowedModules = listOf("pkl", "file", "https").map(Pattern::compile),
-        allowedResources =
-          listOf("pkl", "file", "https", "resourceReader1", "resourceReader2")
-            .map(Pattern::compile),
+        allowedModules = listOf("pkl", "file", "https"),
+        allowedResources = listOf("pkl", "file", "https", "resourceReader1", "resourceReader2"),
         clientResourceReaders = listOf(resourceReader1, resourceReader2),
         clientModuleReaders = listOf(moduleReader1, moduleReader2),
         modulePaths = listOf(Path.of("some/path.zip"), Path.of("other/path.zip")),
@@ -99,7 +94,7 @@ class ServerMessagePackCodecTest {
           ),
         http =
           Http(
-            proxy = PklEvaluatorSettings.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),
           ),
         externalModuleReaders = mapOf("external" to externalReader, "external2" to externalReader),