From dd16f7469e73212b4caaafaed70ae1ba183468e7 Mon Sep 17 00:00:00 2001
From: Josh B <421772+HT154@users.noreply.github.com>
Date: Tue, 29 Oct 2024 23:29:19 -0700
Subject: [PATCH] pkl-executor: fix loading projects and fix incorrectly thrown
 PklException

* Load `PklProject` using the same security settings and env vars passed provided via ExecutorSPIOptions
* Handle thrown `PklException` from loading `PklProject` and throw `ExecutorSPIException`
* Handle thrown `PklException` of older Pkl distributions, converting them into `ExecutorSpiException`
---
 .../org/pkl/core/service/ExecutorSpiImpl.java | 21 +++++--
 .../org/pkl/executor/EmbeddedExecutor.java    |  8 +++
 .../org/pkl/executor/EmbeddedExecutorTest.kt  | 60 ++++++++++++++++++-
 3 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java b/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java
index a4fd373c..1bd91482 100644
--- a/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java
+++ b/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java
@@ -116,13 +116,22 @@ public final class ExecutorSpiImpl implements ExecutorSpi {
             .setTimeout(options.getTimeout())
             .setOutputFormat(options.getOutputFormat())
             .setModuleCacheDir(options.getModuleCacheDir());
-    if (options.getProjectDir() != null) {
-      var project = Project.loadFromPath(options.getProjectDir().resolve(PKL_PROJECT_FILENAME));
-      builder.setProjectDependencies(project.getDependencies());
-    }
 
-    try (var evaluator = builder.build()) {
-      return evaluator.evaluateOutputText(ModuleSource.path(modulePath));
+    try {
+      if (options.getProjectDir() != null) {
+        var project =
+            Project.loadFromPath(
+                options.getProjectDir().resolve(PKL_PROJECT_FILENAME),
+                securityManager,
+                null,
+                transformer,
+                options.getEnvironmentVariables());
+        builder.setProjectDependencies(project.getDependencies());
+      }
+
+      try (var evaluator = builder.build()) {
+        return evaluator.evaluateOutputText(ModuleSource.path(modulePath));
+      }
     } catch (PklException e) {
       throw new ExecutorSpiException(e.getMessage(), e.getCause());
     } finally {
diff --git a/pkl-executor/src/main/java/org/pkl/executor/EmbeddedExecutor.java b/pkl-executor/src/main/java/org/pkl/executor/EmbeddedExecutor.java
index c1296778..5eb84682 100644
--- a/pkl-executor/src/main/java/org/pkl/executor/EmbeddedExecutor.java
+++ b/pkl-executor/src/main/java/org/pkl/executor/EmbeddedExecutor.java
@@ -227,6 +227,14 @@ final class EmbeddedExecutor implements Executor {
         return executorSpi.evaluatePath(modulePath, options.toSpiOptions());
       } catch (ExecutorSpiException e) {
         throw new ExecutorException(e.getMessage(), e.getCause(), executorSpi.getPklVersion());
+      } catch (RuntimeException e) {
+        // This branch would ideally never be hit, but older Pkl releases (<0.27) erroneously throw
+        // PklException in some cases.
+        // Can't catch PklException directly because pkl-executor cannot depend on pkl-core.
+        if (e.getClass().getName().equals("org.pkl.core.PklException")) {
+          throw new ExecutorException(e.getMessage(), e.getCause());
+        }
+        throw e;
       } finally {
         currentThread.setContextClassLoader(prevContextClassLoader);
       }
diff --git a/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt b/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt
index 11de05d3..317faa9f 100644
--- a/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt
+++ b/pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt
@@ -415,6 +415,64 @@ class EmbeddedExecutorTest {
       .doesNotContain(tempDir.toString())
   }
 
+  @ParameterizedTest
+  @MethodSource("getAllTestExecutors")
+  fun `evaluate a module whose project evaluation fails`(
+    executor: TestExecutor,
+    @TempDir tempDir: Path
+  ) {
+    // the toRealPath is important here or the failure reason can change
+    // this happens on macOS where /tmp is a symlink to /private/tmp
+    // it's related to how SecurityManagers.Standard handles canonicalizing paths that don't exist
+    val rootDir = tempDir.toRealPath()
+
+    val innerDir = rootDir.resolve("inner").createDirectories()
+    innerDir
+      .resolve("PklProject")
+      .toFile()
+      .writeText(
+        """
+      amends "pkl:Project"
+      dependencies {
+        ["myDep"] = import("../nonexistent/PklProject")
+      }
+    """
+          .trimIndent()
+      )
+
+    val pklFile = innerDir.resolve("test.pkl")
+    pklFile
+      .toFile()
+      .writeText(
+        """
+      @ModuleInfo { minPklVersion = "0.11.0" }
+      module test
+
+      foo = "bar"
+    """
+          .trimIndent()
+      )
+
+    val e =
+      assertThrows<ExecutorException> {
+        executor.evaluatePath(pklFile) {
+          allowedModules("file:", "pkl:")
+          allowedResources("prop:")
+          projectDir(innerDir)
+          rootDir(rootDir)
+        }
+      }
+
+    assertThat(e.message).contains("Cannot find module").contains("/nonexistent/PklProject")
+
+    // ensure module file paths are relativized
+    // legacy distribution does not handle these errors with the correct stack frame transformer
+    // only assert on this for newer distributions
+    if (!executor.toString().contains("Distribution1")) {
+      assertThat(e.message).doesNotContain(innerDir.toString())
+    }
+  }
+
   @ParameterizedTest
   @MethodSource("getAllTestExecutors")
   fun `time out a module`(executor: TestExecutor, @TempDir tempDir: Path) {
@@ -556,7 +614,7 @@ class EmbeddedExecutorTest {
       )
     val result =
       executor.evaluatePath(pklFile) {
-        allowedModules("file:", "package:", "projectpackage:", "https:")
+        allowedModules("file:", "package:", "projectpackage:", "https:", "pkl:")
         allowedResources("file:", "prop:", "package:", "projectpackage:", "https:")
         moduleCacheDir(cacheDir)
         projectDir(projectDir)