Skip to content

Commit 44fd783

Browse files
cpsievertcpsievert
authored
fix: support NULL as the canonical root namespace for session$destroy(), etc (#4395)
* Refactor session destroy to treat character(0) as the root namespace Pairing-session work with colleague: make character(0) the canonical user-facing value for the root scope (replacing ""), rename the destroy `id` argument to `namespace`, fold id validation into invokeDestroyCallbacks via an `allowRoot` flag, and drop the standalone validateDestroyId helper. Committed verbatim as a record of the pairing session before applying fixes. * Fix session destroy regressions from character(0) refactor - Real ShinySession scope `destroy()` recursed infinitely: the renamed `namespace` parameter shadowed the scope's own captured namespace, and the self-destroy branch was replaced by a recursive makeScope() call that never reached invokeDestroyCallbacks(). Restore the self-vs-child branch and capture the scope's namespace as `selfNamespace`. - MockShinySession was left on the old nzchar()/"" logic, so it still crashed with "argument is of length zero" on callModule(server, character(0)) (the teal failure path via testServer). Mirror the character(0) handling so mock and real sessions behave identically. - Validate the user-supplied destroy `namespace` at the public boundary (before makeScope) via validateDestroyNamespace(), instead of inside invokeDestroyCallbacks() which only runs after makeScope() has already choked on bad input. - Restore the reserved "..root" namespace guard in real makeScope(), and map "" to root in destroyNsKey()/getOrCreateDestroyCallbacks() (fastmap can't key on an empty string). Full test suite passes (0 failures). * Extract '..root' destroy sentinel into a documented constant Replace the private ShinySession$destroyNsRoot field and the hardcoded "..root" literals in MockShinySession with a single package-level `destroyNsRoot` constant, documenting that it must be a non-zero-length string because it is used as a fastmap key (fastmap disallows ""/NA keys). * Apply suggestion from @cpsievert * Apply suggestion from @cpsievert * Address PR review feedback - validateDestroyNamespace(): quote/coerce the offending value in the error (clear output for "", numeric, NA, and multi-element inputs). - MockShinySession$onDestroy(): use getOrCreateDestroyCallbacks() instead of hand-managing the callback map, keeping root-key handling consistent. - Align the mock's root-teardown error messages with ShinySession (direct callers to close(), add call. = FALSE). - Restore the original two-line formatting of the exportTestValues guard. * Use a single root-detection criterion (length 0) for destroy keys Drop the `|| !nzchar(ns)` folding I had added to destroyNsKey() and the mock's getOrCreateDestroyCallbacks(). It made registration treat "" as root while invokeDestroyCallbacks() (length-0) did not, so an ""-namespaced scope would register onDestroy callbacks under the root key that teardown could never find. character(0) is now the sole root spelling everywhere. * Reject empty-string namespaces at makeScope() Addresses Copilot feedback: with character(0) as the sole root spelling, an empty-string `id` (e.g. callModule(server, "")) would otherwise fall through to an empty-string fastmap key and error cryptically. Reject "" early with a clear message in both the real and mock makeScope(); root (character(0)) is still accepted. NS("") also yields a stray "-" prefix, so "" was never a valid namespace. * Forbid NA namespaces too, and document allowed namespace values Widen the makeScope() guard (real + mock) to reject NA as well as "", since both are unusable as fastmap keys. Document the contract in the destroy() docstrings: a module namespace must be a non-empty, non-NA string, and the root scope is identified by character(0). * Promote NULL as the documented root-scope sentinel Default the public destroy()/onDestroy() entry points and the internal invokeDestroyCallbacks() to NULL instead of character(0), and lead with NULL in the docstrings and guard messages. NULL is the idiomatic R 'absence' value and was the original #4372 public default. character(0) remains accepted (both are length 0, which is how root is detected everywhere), so callers like teal that pass character(0) are unaffected; '' and NA are still rejected. * Extract makeScope() namespace validation into a shared helper The reserved-sentinel and empty/NA guards were duplicated byte-for-byte in ShinySession$makeScope() and MockShinySession$makeScope(). Pull them into a package-level validateScopeNamespace(), mirroring the existing validateDestroyNamespace() helper. Behavior-preserving. * Merge namespace validators into one validateNamespace(allow_root=) validateScopeNamespace and validateDestroyNamespace differed only in whether the root (length-0) namespace is allowed -- the apparent leniency of the scope validator was illusory (numeric/multi-element namespaces crashed cryptically downstream rather than being permitted). Collapse them into a single validateNamespace(namespace, allow_root) helper: makeScope() passes allow_root = TRUE, the destroy paths use the default. As a side benefit, malformed makeScope() namespaces now get the clean validation error instead of a cryptic fastmap/if-length crash. * Use ASCII hyphens in destroy() docstring to fix R CMD check R6 method docstrings are string literals (unlike #' roxygen comments), so the em-dashes I used tripped 'checking code files for non-ASCII characters', which CI runs with error_on = 'warning'. Replace them with '--'. * Handle the root scope in makeScope() bookmark/restore helpers With character(0)/NULL now a valid root namespace, the bookmark/restore helpers' scalar-string assumptions broke for root scopes: filterNamespace() treated the prefix as "-" (dropping all root-level names), and the save/restore dir handling did file.path(dir, character(0)) -> dir.exists(character(0)) -> "argument is of length zero". Treat a length-0 namespace as the root: all names in scope, identity unNamespace(), and share the top-level bookmark dir (no subdir). Pre-existing in v1.13.0; re-exposed by re-enabling root scopes. Verified the root vs named behavior in isolation; full test suite passes.
1 parent 7d50304 commit 44fd783

4 files changed

Lines changed: 208 additions & 97 deletions

File tree

‎R/mock-session.R‎

Lines changed: 49 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -321,24 +321,24 @@ MockShinySession <- R6Class(
321321
#' callback.
322322
#' @param callback The callback to invoke on destroy.
323323
onDestroy = function(callback) {
324-
# Use sentinel key since fastmap disallows empty string keys
325-
ns <- "..root"
326-
if (!private$destroyCallbacksByNs$containsKey(ns)) {
327-
private$destroyCallbacksByNs$set(ns, Callbacks$new())
328-
}
329-
private$destroyCallbacksByNs$get(ns)$register(callback)
330-
},
331-
#' @description Destroys a module session scope. On the root session, an
332-
#' `id` is required: `session$destroy(id)` tears down the child module
333-
#' scope of that `id`. Calling `destroy()` with no `id` on the root
334-
#' session is an error.
335-
#' @param id Optional module `id` whose scope should be destroyed.
336-
destroy = function(id = NULL) {
337-
if (is.null(id)) {
338-
stop("`$destroy()` cannot be called on the root session without an `id`. Pass a module `id` to tear down that scope (e.g. `session$destroy(\"my_module\")`), or call `$destroy()` on a module session.")
324+
private$getOrCreateDestroyCallbacks(NULL)$register(callback)
325+
},
326+
#' @description Destroys a module session scope. `namespace` must be a
327+
#' non-empty, non-NA string naming a child module scope. The root scope is
328+
#' the absence of a namespace -- `NULL` (the default) or `character(0)` --
329+
#' and cannot be destroyed this way: calling `destroy()` with no
330+
#' `namespace` on the root session is an error.
331+
#' @param namespace Module `namespace` (a non-empty, non-NA string) whose
332+
#' scope should be destroyed.
333+
destroy = function(namespace = NULL) {
334+
if (length(namespace) == 0) {
335+
stop(
336+
"`$destroy()` cannot be called on the root session without a `namespace`. Pass a module `namespace` to tear down that scope (e.g. `session$destroy(\"my_module\")`), or call `close()` to tear down the whole session.",
337+
call. = FALSE
338+
)
339339
}
340-
validateDestroyId(id)
341-
self$makeScope(id)$destroy()
340+
validateNamespace(namespace)
341+
self$makeScope(namespace)$destroy()
342342
},
343343

344344
#' @description Returns `FALSE` if the session has not yet been closed
@@ -353,7 +353,7 @@ MockShinySession <- R6Class(
353353
withReactiveDomain(self, {
354354
private$endedCBs$invoke(onError = printError, ..stacktraceon = TRUE)
355355
})
356-
private$invokeDestroyCallbacks("")
356+
private$invokeDestroyCallbacks(allowRoot = TRUE)
357357
private$was_closed <- TRUE
358358
},
359359

@@ -554,13 +554,11 @@ MockShinySession <- R6Class(
554554
#' @param namespace Character vector indicating a namespace.
555555
#' @return A new session proxy.
556556
makeScope = function(namespace) {
557-
if (identical(namespace, "..root")) {
558-
stop(
559-
"The module namespace '..root' is reserved for internal use.",
560-
call. = FALSE
561-
)
562-
}
557+
validateNamespace(namespace, allow_root = TRUE)
563558
ns <- NS(namespace)
559+
# The scope's own namespace, captured because the proxy `destroy()` below
560+
# has a `namespace` parameter that would otherwise shadow it.
561+
selfNamespace <- namespace
564562

565563
bookmarkExclude <- character(0)
566564

@@ -582,12 +580,14 @@ MockShinySession <- R6Class(
582580
onDestroy = function(callback) {
583581
private$getOrCreateDestroyCallbacks(namespace)$register(callback)
584582
},
585-
destroy = function(id = NULL) {
586-
if (is.null(id)) {
587-
private$invokeDestroyCallbacks(namespace)
583+
destroy = function(namespace = NULL) {
584+
if (length(namespace) == 0) {
585+
# Tear down this scope itself.
586+
private$invokeDestroyCallbacks(selfNamespace)
588587
} else {
589-
validateDestroyId(id)
590-
self$makeScope(ns(id))$destroy()
588+
# Tear down a named child scope.
589+
validateNamespace(namespace)
590+
self$makeScope(ns(namespace))$destroy()
591591
}
592592
}
593593
)
@@ -763,19 +763,32 @@ MockShinySession <- R6Class(
763763
# @param ns The namespace key.
764764
# @return A Callbacks object.
765765
getOrCreateDestroyCallbacks = function(ns) {
766-
if (!nzchar(ns)) ns <- "..root"
766+
# The root scope (length 0: `NULL` or `character(0)`) maps to the sentinel
767+
# key; fastmap can't use an empty-string key.
768+
if (length(ns) == 0) ns <- destroyNsRoot
767769
if (!private$destroyCallbacksByNs$containsKey(ns)) {
768770
private$destroyCallbacksByNs$set(ns, Callbacks$new())
769771
}
770772
private$destroyCallbacksByNs$get(ns)
771773
},
772774

773-
# @description Invoke destroy callbacks for the given namespace prefix
774-
# and all child namespaces, deepest-first.
775-
# @param nsPrefix The namespace prefix to match.
776-
invokeDestroyCallbacks = function(nsPrefix = "") {
775+
# @description Invoke destroy callbacks for the given namespace
776+
# and all child namespaces, deepest-first. The root (length 0: `NULL` or
777+
# `character(0)`) may only be torn down with `allowRoot = TRUE` (via `close()`).
778+
# @param namespace The namespace to match (length 0 is the root).
779+
# @param allowRoot Whether tearing down the root scope is permitted.
780+
invokeDestroyCallbacks = function(namespace = NULL, allowRoot = FALSE) {
781+
isRoot <- length(namespace) == 0
782+
# The root scope can only be torn down via `close()` (allowRoot = TRUE).
783+
if (isRoot && !allowRoot) {
784+
stop(
785+
"`$destroy()` cannot be called on the root session without a `namespace`. Pass a module `namespace` to tear down that scope (e.g. `session$destroy(\"my_module\")`), or call `close()` to tear down the whole session.",
786+
call. = FALSE
787+
)
788+
}
789+
790+
nsPrefix <- namespace
777791
allNs <- private$destroyCallbacksByNs$keys()
778-
isRoot <- !nzchar(nsPrefix)
779792

780793
if (!isRoot) {
781794
nsPrefixWithSep <- paste0(nsPrefix, ns.sep)
@@ -787,7 +800,7 @@ MockShinySession <- R6Class(
787800
if (length(matching) > 0L) {
788801
# Sort deepest-first (most separators first); root sentinel always last
789802
depths <- nchar(gsub(paste0("[^", ns.sep, "]"), "", matching))
790-
isRootSentinel <- matching == "..root"
803+
isRootSentinel <- matching == destroyNsRoot
791804
matching <- matching[order(-depths, isRootSentinel, matching)]
792805

793806
for (ns in matching) {

‎R/shiny.R‎

Lines changed: 92 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -438,15 +438,38 @@ NS <- function(namespace, id = NULL) {
438438
#' @export
439439
ns.sep <- "-"
440440

441-
442-
# Validate the `id` passed to `session$destroy(id)`. Must be a single,
443-
# non-empty, non-NA string. The reserved `..root` sentinel is additionally
444-
# rejected by `makeScope()`.
445-
validateDestroyId <- function(id) {
446-
if (!is.character(id) || length(id) != 1L || is.na(id) || !nzchar(id)) {
447-
stop("`id` must be a single, non-empty string.", call. = FALSE)
441+
# Sentinel key under which root-level (non-namespaced) destroy callbacks are
442+
# stored. It must be a non-zero-length character string because it is used as a
443+
# fastmap key, and fastmap disallows "" (and NA) keys. The leading dots also
444+
# keep it from colliding with a real module namespace (which `makeScope()`
445+
# additionally guards against).
446+
destroyNsRoot <- "..root"
447+
448+
# Validate a module `namespace`. A namespace must be a single, non-empty,
449+
# non-NA string and may not be the reserved sentinel. When `allow_root = TRUE`
450+
# (e.g. `makeScope()`), the root scope -- a length-0 `NULL` / `character(0)` --
451+
# is also accepted; the destroy entry points pass `allow_root = FALSE` because a
452+
# child scope must be named explicitly.
453+
validateNamespace <- function(namespace, allow_root = FALSE) {
454+
if (allow_root && length(namespace) == 0) {
455+
return(invisible(namespace))
456+
}
457+
if (!is.character(namespace) || length(namespace) != 1L || is.na(namespace) || !nzchar(namespace)) {
458+
stop(
459+
"Invalid `namespace`: ",
460+
encodeString(paste0(format(namespace), collapse = ", "), quote = '"'),
461+
". A module namespace must be a non-empty, non-NA string; use `NULL` for the root scope.",
462+
call. = FALSE
463+
)
448464
}
449-
invisible(id)
465+
if (identical(namespace, destroyNsRoot)) {
466+
stop(
467+
"The module namespace '", destroyNsRoot,
468+
"' is reserved for internal use.",
469+
call. = FALSE
470+
)
471+
}
472+
invisible(namespace)
450473
}
451474

452475
#' @include utils.R
@@ -808,11 +831,10 @@ ShinySession <- R6Class(
808831
invisible()
809832
},
810833

811-
# Sentinel key for root-level destroy callbacks (fastmap disallows empty string keys)
812-
destroyNsRoot = "..root",
813-
814834
destroyNsKey = function(ns) {
815-
if (!nzchar(ns)) private$destroyNsRoot else ns
835+
# The root scope (length 0: `NULL` or `character(0)`) maps to the sentinel
836+
# key; fastmap can't use an empty-string key.
837+
if (length(ns) == 0) destroyNsRoot else ns
816838
},
817839

818840
getOrCreateDestroyCallbacks = function(ns) {
@@ -822,9 +844,18 @@ ShinySession <- R6Class(
822844
}
823845
private$destroyCallbacksByNs$get(key)
824846
},
825-
invokeDestroyCallbacks = function(nsPrefix = "") {
847+
invokeDestroyCallbacks = function(namespace = NULL, allowRoot = FALSE) {
848+
isRoot <- length(namespace) == 0
849+
# The root scope can only be torn down via `close()` (allowRoot = TRUE).
850+
if (isRoot && !allowRoot) {
851+
stop(
852+
"`$destroy()` cannot be called on the root ShinySession without a `namespace`. Pass a module `namespace` to tear down that scope (e.g. `session$destroy(\"my_module\")`), or call `close()` to tear down the whole session.",
853+
call. = FALSE
854+
)
855+
}
856+
857+
nsPrefix <- namespace
826858
allNs <- private$destroyCallbacksByNs$keys()
827-
isRoot <- !nzchar(nsPrefix)
828859

829860
if (!isRoot) {
830861
nsPrefixWithSep <- paste0(nsPrefix, ns.sep)
@@ -837,7 +868,7 @@ ShinySession <- R6Class(
837868
if (length(matching) > 0L) {
838869
# Sort deepest-first (most separators first); root sentinel always last
839870
depths <- nchar(gsub(paste0("[^", ns.sep, "]"), "", matching))
840-
isRootSentinel <- matching == private$destroyNsRoot
871+
isRootSentinel <- matching == destroyNsRoot
841872
matching <- matching[order(-depths, isRootSentinel, matching)]
842873

843874
for (ns in matching) {
@@ -1005,14 +1036,11 @@ ShinySession <- R6Class(
10051036
self
10061037
},
10071038
makeScope = function(namespace) {
1008-
if (identical(namespace, private$destroyNsRoot)) {
1009-
stop(
1010-
"The module namespace '", private$destroyNsRoot,
1011-
"' is reserved for internal use.",
1012-
call. = FALSE
1013-
)
1014-
}
1039+
validateNamespace(namespace, allow_root = TRUE)
10151040
ns <- NS(namespace)
1041+
# The scope's own namespace, captured because the proxy `destroy()` below
1042+
# has a `namespace` parameter that would otherwise shadow it.
1043+
selfNamespace <- namespace
10161044

10171045
# Private items for this scope. Can't be part of the scope object because
10181046
# `$<-.session_proxy` doesn't allow assignment on overidden names.
@@ -1084,29 +1112,34 @@ ShinySession <- R6Class(
10841112
onDestroy = function(callback) {
10851113
private$getOrCreateDestroyCallbacks(namespace)$register(callback)
10861114
},
1087-
destroy = function(id = NULL) {
1088-
if (is.null(id)) {
1089-
private$invokeDestroyCallbacks(namespace)
1115+
destroy = function(namespace = NULL) {
1116+
if (length(namespace) == 0) {
1117+
# Tear down this scope itself.
1118+
private$invokeDestroyCallbacks(selfNamespace)
10901119
} else {
1091-
validateDestroyId(id)
1092-
self$makeScope(ns(id))$destroy()
1120+
# Tear down a named child scope.
1121+
validateNamespace(namespace)
1122+
self$makeScope(ns(namespace))$destroy()
10931123
}
10941124
}
10951125
)
10961126

10971127
# Given a char vector, return a logical vector indicating which of those
1098-
# strings are names of things in the namespace.
1128+
# strings are names of things in the namespace. For the root scope (a
1129+
# length-0 namespace) everything is in scope, since there is no prefix.
10991130
filterNamespace <- function(x) {
1131+
if (length(namespace) == 0) return(rep_len(TRUE, length(x)))
11001132
nsString <- paste0(namespace, ns.sep)
11011133
substr(x, 1, nchar(nsString)) == nsString
11021134
}
11031135

11041136
# Given a char vector of namespaced names, return a char vector of corresponding
1105-
# names with namespace prefix removed.
1137+
# names with namespace prefix removed. The root scope has no prefix to strip.
11061138
unNamespace <- function(x) {
11071139
if (!all(filterNamespace(x))) {
11081140
stop("x contains strings(s) that do not have namespace prefix ", namespace)
11091141
}
1142+
if (length(namespace) == 0) return(x)
11101143

11111144
nsString <- paste0(namespace, ns.sep)
11121145
substring(x, nchar(nsString) + 1)
@@ -1134,7 +1167,9 @@ ShinySession <- R6Class(
11341167
scopeState$values[[scopedName]] <- state$values[[origName]]
11351168
})
11361169

1137-
if (!is.null(state$dir)) {
1170+
# The root scope shares the top-level bookmark dir; only a real
1171+
# (non-empty) namespace gets a subdir.
1172+
if (!is.null(state$dir) && length(namespace) > 0) {
11381173
dir <- file.path(state$dir, namespace)
11391174
if (dirExists(dir))
11401175
scopeState$dir <- dir
@@ -1153,13 +1188,18 @@ ShinySession <- R6Class(
11531188

11541189
scopeState <- ShinySaveState$new(scope$input, scope$getBookmarkExclude())
11551190

1156-
# Create subdir for this scope
1191+
# Create subdir for this scope. The root scope (length-0 namespace)
1192+
# shares the top-level dir rather than getting its own subdir.
11571193
if (!is.null(state$dir)) {
1158-
scopeState$dir <- file.path(state$dir, namespace)
1159-
if (!dirExists(scopeState$dir)) {
1160-
res <- dir.create(scopeState$dir)
1161-
if (res == FALSE) {
1162-
stop("Error creating subdirectory for scope ", namespace)
1194+
if (length(namespace) == 0) {
1195+
scopeState$dir <- state$dir
1196+
} else {
1197+
scopeState$dir <- file.path(state$dir, namespace)
1198+
if (!dirExists(scopeState$dir)) {
1199+
res <- dir.create(scopeState$dir)
1200+
if (res == FALSE) {
1201+
stop("Error creating subdirectory for scope ", namespace)
1202+
}
11631203
}
11641204
}
11651205
}
@@ -1280,20 +1320,24 @@ ShinySession <- R6Class(
12801320
unregister the callback. For module sessions, use this to register
12811321
cleanup logic that runs when the module's UI is removed and
12821322
`session$destroy()` is called."
1283-
private$getOrCreateDestroyCallbacks("")$register(callback)
1323+
private$getOrCreateDestroyCallbacks(NULL)$register(callback)
12841324
},
1285-
destroy = function(id = NULL) {
1325+
destroy = function(namespace = NULL) {
12861326
"Destroys a module session scope, cleaning up its reactive state and
1287-
invoking its `onDestroy()` callbacks. On the root session, an
1288-
`id` is required: `session$destroy(id)` tears down the
1289-
child module scope of that `id`. Calling `destroy()`
1290-
with no `id` on the root session is an error; the root session
1291-
is torn down via `close()`."
1292-
if (is.null(id)) {
1293-
stop("`$destroy()` cannot be called on the root ShinySession without an `id`. Pass a module `id` to tear down that scope (e.g. `session$destroy(\"my_module\")`), or call `$destroy()` on a module session.")
1327+
invoking its `onDestroy()` callbacks. `namespace` must be a non-empty,
1328+
non-NA string naming a child module scope; `session$destroy(namespace)`
1329+
tears that scope down. The root scope is the absence of a namespace --
1330+
`NULL` (the default) or `character(0)` -- and cannot be destroyed this
1331+
way: calling `destroy()` with no `namespace` on the root session is an
1332+
error, since the root session is torn down via `close()`."
1333+
if (length(namespace) == 0) {
1334+
stop(
1335+
"`$destroy()` cannot be called on the root ShinySession without a `namespace`. Pass a module `namespace` to tear down that scope (e.g. `session$destroy(\"my_module\")`), or call `close()` to tear down the whole session.",
1336+
call. = FALSE
1337+
)
12941338
}
1295-
validateDestroyId(id)
1296-
self$makeScope(id)$destroy()
1339+
validateNamespace(namespace)
1340+
self$makeScope(namespace)$destroy()
12971341
},
12981342
onInputReceived = function(callback) {
12991343
"Registers the given callback to be invoked when the session receives
@@ -1345,7 +1389,7 @@ ShinySession <- R6Class(
13451389
private$closedCallbacks$invoke(onError = printError, ..stacktraceon = TRUE)
13461390
})
13471391
})
1348-
private$invokeDestroyCallbacks("")
1392+
private$invokeDestroyCallbacks(allowRoot = TRUE)
13491393
},
13501394
isClosed = function() {
13511395
return(self$closed)

‎man/MockShinySession.Rd‎

Lines changed: 8 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)