Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shinyrmd: Safer dependency extraction from pre-rendered HTML #2500

Merged
merged 2 commits into from
Jul 26, 2023

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Jul 20, 2023

This PR provides a fix for #2499 that doesn't address the root cause but will avoid the downstream errors caused by that issue.

In short, under unknown circumstances, the shinyrmd context script tags for dependencies and execution dependencies are written twice into the pre-rendered HTML files. rmarkdown then passes a length-2 character vector to jsonlite::unserializeJSON(), which causes a parsing error.

This PR does two primary things:

  1. I extracted the HTML and JSON parsing of dependencies into a single function shiny_prerendered_extract_context_serialized(). This function ensures a single character string is passed to jsonlite::unserializedJSON(), using the last context in the document if more than one non-unique context is found.

  2. rmarkdown reads both dependency types in shiny_prerendered_prerender() (the function that decides if a pre-render run is required). At this point, we don't need to error if parsing fails and can instead trigger a pre-rendered pass to happen again. Ideally, this new pre-render pass will fix any issues in the pre-rendered HTML file, but if it somehow writes invalid dependency JSON again, shiny_prerendered_html() will still fail (rather than infinitely looping).

    Note that avoiding an infinite loop is hypothetical; the current bug wouldn't trigger a new pre-render pass.

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thanks for this. It makes total sense to do that.

I'd like to know why shiny_prerendered_append_dependencies() is called twice during pre-render

I tried to look into that, but I don't see when we would call render() another time during the same render. shiny_prerendered_append_dependencies() is supposed to append to a file, and this file is supposed to be the knitted file so inermediary results. it would require writing another time to that file and I don't see when we would do that. In general case the intermediary file will be overwritten to a fresh version at each knit() so each render().

So it coulb another render call on the intermediary file but I have check our code base and I don't see that.

I suggest we merge this and this will remain a Mystery

@cderv cderv merged commit 054d735 into rstudio:main Jul 26, 2023
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Aug 16, 2023
* rstudio/main:
  start the next version
  CRAN release v2.24
  shinyrmd: Safer dependency extraction from pre-rendered HTML (rstudio#2500)
  quote the version number per CRAN's request
  Add output_format_dependency() (rstudio#2462)
  file_scope is now correctly merged when creating output_format (rstudio#2488)
  Correctly run some tests only on CI
  start the next version
  CRAN release v2.23
  remove broken links
  suggest cleanrmd for e499bf7
  add news
  comparing version numbers with numbers is no longer allowed: https://bugs.r-project.org/show_bug.cgi?id=18548
  `find_external_resources` works with custom format using `theme` (rstudio#2494)
  start the next version
  CRAN release v2.22
  S3 generic/method consistency
  Change the code-folding button text from "Code" to "Show" (rstudio#2489)
  fix: bump jquery-ui to v1.13.2 to fix multiple CVEs (rstudio#2477)
  detecting external resources needs to consider css argument (rstudio#2486)
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Aug 16, 2023
Merge remote-tracking branch 'rstudio/main' into jg-devel

# By Yihui Xie (13) and others
# Via Yihui Xie
* rstudio/main:
  start the next version
  CRAN release v2.24
  shinyrmd: Safer dependency extraction from pre-rendered HTML (rstudio#2500)
  quote the version number per CRAN's request
  Add output_format_dependency() (rstudio#2462)
  file_scope is now correctly merged when creating output_format (rstudio#2488)
  Correctly run some tests only on CI
  start the next version
  CRAN release v2.23
  remove broken links
  suggest cleanrmd for e499bf7
  add news
  comparing version numbers with numbers is no longer allowed: https://bugs.r-project.org/show_bug.cgi?id=18548
  `find_external_resources` works with custom format using `theme` (rstudio#2494)
  start the next version
  CRAN release v2.22
  S3 generic/method consistency
  Change the code-folding button text from "Code" to "Show" (rstudio#2489)
  fix: bump jquery-ui to v1.13.2 to fix multiple CVEs (rstudio#2477)
  detecting external resources needs to consider css argument (rstudio#2486)

# Conflicts:
#	DESCRIPTION
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Aug 16, 2023
* jg-devel: (21 commits)
  Updated NEWS. Patched `merge_output_format_dependency` to ensure that named elements remain in the correct order.
  start the next version
  CRAN release v2.24
  shinyrmd: Safer dependency extraction from pre-rendered HTML (rstudio#2500)
  quote the version number per CRAN's request
  Add output_format_dependency() (rstudio#2462)
  file_scope is now correctly merged when creating output_format (rstudio#2488)
  Correctly run some tests only on CI
  start the next version
  CRAN release v2.23
  remove broken links
  suggest cleanrmd for e499bf7
  add news
  comparing version numbers with numbers is no longer allowed: https://bugs.r-project.org/show_bug.cgi?id=18548
  `find_external_resources` works with custom format using `theme` (rstudio#2494)
  start the next version
  CRAN release v2.22
  S3 generic/method consistency
  Change the code-folding button text from "Code" to "Show" (rstudio#2489)
  fix: bump jquery-ui to v1.13.2 to fix multiple CVEs (rstudio#2477)
  ...
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shiny prerendered dependencies are sometimes written twice into prerendered HTML
2 participants