Fix Windows path handling in save_image() (#2487)

* test: capture Windows path regression in kaleido

* fix: avoid embedding Windows paths in kaleido Python code

* docs: note Windows save_image fix in news

* test: tighten kaleido review follow-ups

* fix: harden kaleido context cleanup

Author: cpsievert

NEWS.md

@@ -2,6 +2,10 @@
 
 * Removed the dependency on the `{lazyeval}` package.
 
+## Bug fixes
+
+* Closed #2483: `save_image()` no longer embeds Windows file paths directly into Python source passed to `reticulate`, fixing static image export with Kaleido on Windows.
+
 
 # plotly 4.12.0
 

R/kaleido.R

@@ -133,11 +133,10 @@ newKaleidoScope <- function(kaleido) {
       writeLines(fig, tmp_json)
 
       # Import it as a fig (dict)
-      load_json <- sprintf(
-        "import json; fig = json.load(open('%s'))",
-        tmp_json
+      .py_run_string_with_context(
+        "import json; fig = json.load(open(tmp_json_path))",
+        context = list(tmp_json_path = tmp_json)
       )
-      reticulate::py_run_string(load_json)
 
       # Gather figure-level options
       opts <- list(
@@ -193,8 +192,9 @@ legacyKaleidoScope <- function(kaleido) {
       )
       # Write the base64 encoded string that transform() returns to disk
       # https://github.com/plotly/Kaleido/blame/master/README.md#L52
-      reticulate::py_run_string(
-        sprintf("import sys; open('%s', 'wb').write(%s)", file, transform_cmd)
+      .py_run_string_with_context(
+        sprintf("import sys; open(output_file, 'wb').write(%s)", transform_cmd),
+        context = list(output_file = file)
       )
 
       invisible(file)
@@ -216,6 +216,46 @@ legacyKaleidoScope <- function(kaleido) {
   res
 }
 
+.py_run_string_with_context <- function(code, context = list(), convert = TRUE) {
+  context_names <- names(context)
+  old_values <- vector("list", length(context))
+  had_value <- logical(length(context))
+  was_set <- logical(length(context))
+  
+  if (length(context) > 0) {
+    if (is.null(context_names) || any(context_names == "")) {
+      rlang::abort("`context` must be a named list.")
+    }
+    if (any(!grepl("^[A-Za-z_][A-Za-z0-9_]*$", context_names))) {
+      rlang::abort("`context` names must be valid Python identifiers.")
+    }
+
+    py <- reticulate::py
+    on.exit({
+      for (i in rev(which(was_set))) {
+        name <- context_names[[i]]
+        if (had_value[[i]]) {
+          reticulate::py_set_attr(py, name, old_values[[i]])
+        } else {
+          reticulate::py_del_attr(py, name)
+        }
+      }
+    }, add = TRUE)
+    
+    for (i in seq_along(context)) {
+      name <- context_names[[i]]
+      had_value[[i]] <- reticulate::py_has_attr(py, name)
+      if (had_value[[i]]) {
+        old_values[[i]] <- reticulate::py_get_attr(py, name)
+      }
+      reticulate::py_set_attr(py, name, context[[i]])
+      was_set[[i]] <- TRUE
+    }
+  }
+  
+  reticulate::py_run_string(code, convert = convert)
+}
+
 
 #' Print method for kaleido
 #' 

tests/testthat/test-kaleido.R

@@ -0,0 +1,107 @@
+test_that("newKaleidoScope does not inline Windows temp paths into Python code", {
+  skip_if_not_installed("reticulate")
+  skip_if_not(suppressWarnings(reticulate::py_available(TRUE)))
+  withr::defer({
+    py <- reticulate::py
+    for (name in c("fig", "tmp_json_path")) {
+      if (reticulate::py_has_attr(py, name)) {
+        reticulate::py_del_attr(py, name)
+      }
+    }
+  })
+
+  win_path <- "C:\\users\\name\\AppData\\Local\\Temp\\Rtmp\\file.json"
+  py_calls <- character()
+  write_fig_args <- NULL
+
+  local({
+    testthat::local_mocked_bindings(
+      plotly_build = function(...) {
+        list(x = list(data = list(), layout = list(), config = list()))
+      },
+      to_JSON = function(...) "{}",
+      plotlyMainBundlePath = function() "plotly.min.js",
+      .package = "plotly"
+    )
+
+    testthat::local_mocked_bindings(
+      tempfile = function(...) win_path,
+      writeLines = function(...) NULL,
+      unlink = function(...) 0,
+      .package = "base"
+    )
+
+    testthat::local_mocked_bindings(
+      py_run_string = function(code, ...) {
+        py_calls <<- c(py_calls, code)
+        py_obj <- get("py", envir = asNamespace("reticulate"))
+        py_obj$fig <- "fake-fig"
+        invisible(NULL)
+      },
+      .package = "reticulate"
+    )
+
+    kaleido <- list(write_fig_sync = function(fig, file, opts, kopts) {
+      write_fig_args <<- list(fig = fig, file = file, opts = opts, kopts = kopts)
+      invisible(NULL)
+    })
+
+    scope <- plotly:::newKaleidoScope(kaleido)
+    scope$transform(list(), "figure.png")
+  })
+
+  expect_identical(py_calls[[1]], "import json; fig = json.load(open(tmp_json_path))")
+  expect_true(!grepl(win_path, py_calls[[1]], fixed = TRUE))
+  if (length(py_calls) >= 2) {
+    expect_identical(py_calls[[2]], "del tmp_json_path")
+  }
+  expect_identical(write_fig_args$fig, "fake-fig")
+  expect_identical(write_fig_args$file, "figure.png")
+})
+
+test_that("py_run_string_with_context rejects invalid Python identifiers", {
+  skip_if_not_installed("reticulate")
+
+  expect_error(
+    plotly:::.py_run_string_with_context(
+      "value = 1",
+      context = list("bad name" = 1)
+    ),
+    "`context` names must be valid Python identifiers\\."
+  )
+})
+
+test_that("py_run_string_with_context cleans up partial assignments", {
+  skip_if_not_installed("reticulate")
+  skip_if_not(suppressWarnings(reticulate::py_available(TRUE)))
+
+  deleted <- character()
+
+  testthat::local_mocked_bindings(
+    py_has_attr = function(x, name) FALSE,
+    py_get_attr = function(x, name, silent = FALSE) stop("unexpected py_get_attr call"),
+    py_set_attr = function(x, name, value) {
+      if (identical(name, "second")) {
+        stop("boom")
+      }
+      invisible(NULL)
+    },
+    py_del_attr = function(x, name) {
+      deleted <<- c(deleted, name)
+      invisible(NULL)
+    },
+    py_run_string = function(code, local = FALSE, convert = TRUE) {
+      stop("unexpected py_run_string call")
+    },
+    .package = "reticulate"
+  )
+
+  expect_error(
+    plotly:::.py_run_string_with_context(
+      "value = 1",
+      context = list(first = 1, second = 2)
+    ),
+    "boom"
+  )
+  expect_identical(deleted, "first")
+})