diff --git a/NEWS.md b/NEWS.md index 4324cfddb6..d64ff75676 100644 --- a/NEWS.md +++ b/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 diff --git a/R/kaleido.R b/R/kaleido.R index 0f0224be14..03c0703a61 100644 --- a/R/kaleido.R +++ b/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 #' diff --git a/tests/testthat/test-kaleido.R b/tests/testthat/test-kaleido.R new file mode 100644 index 0000000000..3c7c0eee62 --- /dev/null +++ b/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") +})