Deployment fails because bundle is wrongly identified as API by `inferAppMode()`

Teebusch commented 1 year ago

Context

We are using a golem-style app-layout and an additional plumber API in inst/plumber/api/plumber.R, i.e.

.
├── app.R
├── DESCRIPTION
├── inst
│   └── plumber
|         └── api
|              └── plumber.R  <----  The culprit
├── R
│   ├── server.R
│   └── ui.R
...

Undesired Behavior

rsconnect:::inferAppMode() infers that the project has appmode: api, not appmode: shiny. This function is called when using rsconnect::writeManifest(), rsconnect::bundleApp() (which is called by rsconnect::deployApp()), as well as when using the deploy button in the RStudio-Interface. Thus, rsconnect::deployApp() and the deployButton will wrongly try to deploy the project as an API instead of as a shiny app.

For example:

> rsconnect::writeManifest()
...
> jsonlite::read_json("manifest.json")$metadata$appmode
# [1] "api"

Creating a manifest.json in the directory and simply changing appmode to shiny in that file doesn't solve the issue, because deployApp() calls bundleApp(), which generates its own manifest.json straight in a temporary bundle directory, so the manifest.json file in the project directory is ignored.

The root of the Problem

rsconnect:::inferAppMode() checks for evidence of a plumber API (and returns) before it checks for evidence of a Shiny App (as can be seen here).

The behavior was introduced in rsconnect version 1.0.0, because

Prior to 1.0.0, inferAppMode() was called with a non-recursive list of the files in the directory, which was obtained via bundleFiles(). Thus, only the top level was searched for evidence of the different app modes and inst/api/plumber.R would not have been part of the list.
Since 1.0.0, the function gets a recursive list of all files in the bundle, which is obtained via listDeploymentFiles(). Thus, in our case, the list now includes the api file in one of the subdirectories 'inst/api/plumber.R`
This is due to a change in the internal function listBundleFIles(), which now calls a function that returns a recursive list of files (old version in 0.8.29 vs. new version in 1.0.0)
This is despite a comment stating that it only checks files in the root dir

Workaround: setting `appPrimaryDoc = "app.R"`

In the community forums, someone suggested to use the argument appPrimaryDoc = "app.R".

Unfortunately, this throws an error:

> rsconnect::writeManifest(appPrimaryDoc = "app.R")`

Error in `checkAppLayout()`:
! Project must not contain both app.R and a single-file Shiny app.
Run `rlang::last_trace()` to see where the error occurred.

This error is thrown by a conditional in rsconnect::checkAppLayout(appDir, appPrimaryDoc) (here), a function that checks whether the app directory has any of the expected layouts. This function is called by both, writeManifest() and bundleApp() via rsconnect:::appMetadata() and it explicitly prevents the user from setting appPrimaryDoc to be app.R:

appFilesBase <- tolower(list.files(appDir))
...
primaryIsRScript <- identical(tolower(tools::file_ext(appPrimaryDoc)), "r")
if (primaryIsRScript && "app.r" %in% appFilesBase) {
  cli::cli_abort("Project must not contain both {.file app.R} and a single-file Shiny app.")
}

A solution is to add an additional condition to test, whether appPrimaryDoc == app.R. Ideally, this would have to be fixed in {rsconnect} itself, but one can temporarily inject a fix like this:

checkAppLayout_modified <- function (appDir, appPrimaryDoc = NULL) 
{
  ...
  if (primaryIsRScript && "app.r" %in% appFilesBase && tolower(appPrimaryDoc) != "app.r") {
    cli::cli_abort("Project must not contain both {.file app.R} and a single-file Shiny app.")
  }
  ...
}

assignInNamespace("checkAppLayout", checkAppLayout_modified, ns = "rsconnect")
rsconnect::writeManifest(appPrimaryDoc = "app.R")

(see below for full functional code)

This allows me to publish to rsconnect succesfully, since the error isn't thrown anymore and the generated manifest.json now has appmode: shiny. However, the solution requires me to modify a function in the rsconnect namespace and it still doesn't allow me to use the deploy Button in RStudio, because there I cannot supply the appPrimaryDoc argument. (Also, this solution won't work if I want to set appPrimaryDoc = "plumber.R" to publish the API.)

It looks like currently there is no straightforward solution to use the project structure we have with the latest version of rsconnect.

Conclusion

Generally, I think, the preference of an API over an App by deployApp() is unexpected and should be removed. If I wanted to deploy the API that's in the package, I'd expect to use deployApi() to do so.

Similarly, when clicking the deploy Button in app.R I'd expect it to deploy the app that's in that file, whereas clicking the deploy button in plumber.R should deploy the API.

It might be a solution to revert to the old behavior of searching only the root directory to infer the app mode. Another option would be to use the user-supplied "manifest.json" in deployApp().

Complete functional code of my 'workaround'

# A solution is to prevent the error by checking whether `tolower(appPrimaryDoc) != "app.r"`

checkAppLayout_original <- rsconnect:::checkAppLayout  # for restoring it later

checkAppLayout_modified <- function (appDir, appPrimaryDoc = NULL) 
{
  cli::cli_alert_warning("Using custom function injected into the rsconnect namespace.")
  appFilesBase <- tolower(list.files(appDir))
  wwwFiles <- tolower(list.files(file.path(appDir, "www/")))
  primaryIsRScript <- identical(tolower(tools::file_ext(appPrimaryDoc)), 
                                "r")

  if (primaryIsRScript && "app.r" %in% appFilesBase && tolower(appPrimaryDoc) != "app.r") {
    cli::cli_abort("Project must not contain both {.file app.R} and a single-file Shiny app.")
  }
  satisfiedLayouts <- c(shinyAndUi = all(c("server.r", "ui.r") %in% 
                                           appFilesBase), shinyAndIndex = "server.r" %in% appFilesBase && 
                          "index.html" %in% wwwFiles, app = primaryIsRScript || 
                          any("app.r" %in% appFilesBase), Rmd = any(grepl(glob2rx("*.rmd"), 
                                                                          appFilesBase)), Qmd = any(grepl(glob2rx("*.qmd"), appFilesBase)), 
                        static = any(grepl("(?:html?|pdf)$", appFilesBase)), 
                        plumber = any(c("entrypoint.r", "plumber.r") %in% appFilesBase))
  if (any(satisfiedLayouts)) {
    return()
  }
  cli::cli_abort(c("Cancelling deployment: invalid project layout.", 
                   i = "Expecting one of the following publication types:", 
                   ` ` = "1. A Shiny app with `app.R` or `server.R` + `ui.R`", 
                   ` ` = "2. R Markdown (`.Rmd`) or Quarto (`.qmd`) documents.", 
                   ` ` = "3. A website containing `.html` and/or `.pdf` files.", 
                   ` ` = "4. A plumber API with `plumber.R` or `entrypoint.R`."))
}

assignInNamespace("checkAppLayout", checkAppLayout_modified, ns = "rsconnect")

rsconnect::writeManifest(appPrimaryDoc = "app.R")

appmode <- jsonlite::read_json("manifest.json")$metadata$appmode
if (appmode == "shiny") {
  rsconnect::deployApp(appPrimaryDoc = "app.R")
} else {
  cli::cli_abort("Something went wrong, `appmode` is `{ appmode }`.")
}

hadley commented 1 year ago

I think we need two fixes:

Fix the bug in inferAppMode() and return to only scanning files in the root dir.
Add an explicit appMode parameter to deployApp() to make future problems easier to work around (@dragonstyle also needs this)

aronatkins commented 1 year ago

We had intended to use top-level files during app-mode inference, but used the wrong set of files:

https://github.com/rstudio/rsconnect/blob/6ae93b211967bf45bcfc02cfe29cdac8b3143d22/R/appMetadata.R#L37-L42

aronatkins commented 1 year ago

@Teebusch - Are you able to try the change in https://github.com/rstudio/rsconnect/pull/949 to see if it resolves what you were seeing?

dragonstyle commented 1 year ago

Just to chime in - even without recursive searching to infer appMode, for Quarto we still need to be able to explicitly provide the appMode, since it is valid for a qmd file to appear in the root of a statically published manuscript project.

If there is someone (other than me :) ) that can take a look at this, it would be immensely appreciated...

aronatkins commented 1 year ago

I've created https://github.com/rstudio/rsconnect/issues/948 to track the app-mode override idea.

rstudio / rsconnect