Repository navigation
Don't overwrite but merge keys for pandoc - #13353
jkrumbiegel wants to merge 7 commits into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
After simplifying the mechanism to also handle arrays, some examples with evaluated entries break, but at this point I can't tell anymore how this is supposed to work at all :) |
|
Merging is supposed to be behavior we use, but We are working on fixing some things that will (fix some things that will)* eventually let us reason more precisely about the behavior of YAML configuration. Stay tuned for the behavior of YAML blocks in our new markdown variant! |
|
This problem is not limited to I would appreciate if we could find a quicker intermediate solution than a full overhaul of the markdown parsing system. (The parsing is also not the problem here, all attributes are correctly merged and available at first, and then some info is thrown away). As you say, there are too many special keys, so the only way to avoid problems seems to be to namespace all custom keys under one key that's unlikely to conflict with anything quarto might add. But this you cannot do due to this overwriting bug. |
|
Do you have an MWE of this happening outside of |
|
The MWE I posted contains both, params and no params. Consider the entry |
|
We are about to release 1.8 (hopefully tomorrow), so this isn't a PR we can merge right now. But I agree that we should try to do a faster fix early in 1.9 (the bigger changes I'm alluding to will start coming in at 1.9 as well) |
|
(cc @gordonwoodhull, cf our discussion yesterday) |
|
@cscheid did you have time to form an opinion about this one, yet? |
Metadata handed to pandoc starts from the fully resolved options.format.metadata and then overwrites individual top-level keys with the document's own front matter, re-read after execution so that inline expressions are resolved. For a key defined in both _quarto.yml and the document, that assignment discarded everything the project file contributed, even though options.format.metadata had merged it correctly. Merging the two instead is not enough: options.format.metadata already contains the document's unresolved values, so concatenating the resolved array on top leaves an inline expression in the result next to the value it evaluated to. Treat options.format.metadata as the correct merge and substitute only the leaves that execution actually changed, comparing against the front matter as written, which is now passed through as PandocOptions.unexecutedMetadata. Keys the engine did not touch keep their merged value, so mappings keep the project's keys and arrays keep the project's entries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cderv tagging you as I believe you're the one now mostly handling quarto 1 stuff. I went over this PR again with Claude and tried to fix the test failures from before. Seems to be a design wart mostly due to the ability of knitr to change frontmatter entries. Here's a summary from Claude detailing how the final set of parameters to pass to pandoc is now constructed. This PR is important for us as our typst templates can't currently be controlled from project settings, the keys are all thrown away due to this bug. 🤖: At the point where pandoc metadata is assembled there are two partially intersecting sets: the document's own front matter, with some leaves replaced by evaluation, and the fully merged metadata including Merging them instead is what I tried before, and it is what broke the evaluated entries. The merged value still holds the unevaluated expression, so concatenating the evaluated array on top keeps both: Both versions of the front matter are now available, since Test at One behaviour change worth a look: a top level |
|
🤖: The failing checks here are not caused by this PR. CI installs a dev knitr on top of the lockfile, and that version moved between the last run of this workflow on main and this one:
In between, knitr landed yihui/knitr@5e41f455 "Stash the raw chunk body as original.code for Quarto (#2239)", which adds the chunk source as a cell div attribute. That is exactly the snapshot failure in The three For what it is worth, rendering both |
|
CI is now fully green |
|
Thanks @jkrumbiegel - I need to take time to look into the problem. It may seem like an oversight at first, but initial design was not to have all YAML metadata (from doc or project) be exposed as is to Lua fitlers and templates through pandoc. I need to find back a bit of history or that to be sur we don't break anything we may not tests. For some of the metadata, we do expose them through scoped variables (ex: https://quarto.org/docs/authoring/language.html#localized-strings-in-templates) and not broader metadata merging. I am not saying we should not improve it the way you did it, we just need to be careful on this. New quarto-dev/q2 is a better place to improve how we do handle metadata and expose them too. Sorry for the wait. |
|
@cderv thanks for looking into it! Yes could be that it was not originally intended to send all data to pandoc. I would argue however that it is already being done, and given that fact it's probably better to have that data be correct rather than half-correct. As said in the PR description, it's already happening, just in a buggy way:
Everywhere in quarto it works such that you can define settings either in _quarto.yml or in frontmatter, and these are merged. It's only due to this old code that tries to update keys that might have been defined in that special knitr format and which need R evaluation that the already correctly merged data becomes broken again. |
|
Thanks for your patience on this, and for the detailed MWE. I looked into that more closely (also with 🤖 help) I need to correct what I said earlier. I wrote that the initial design was not to have all YAML metadata exposed to Lua filters and templates. Looking at the history this is not right. The fully merged metadata (project, directory, document, format) has been passed to Pandoc for a long time, and only a few keys are removed before that ( quarto-cli/src/command/render/pandoc.ts Lines 1318 to 1322 in 9b307f7 The loop you found was added later, only so that inline R expressions in YAML (a knitr feature kept for R Markdown compatibility) reach Pandoc evaluated. It replaces the whole merged value of every key that the document also sets, so project values under a shared key are lost, as you described: quarto-cli/src/command/render/pandoc.ts Lines 1156 to 1184 in 9b307f7 So you are right, this seems to be a bug and not a design choice. And I think this is the same issue as which is still open, so we can link this PR to it. I'll do a review with suggestions for a simpler change. I'll explain the reasoning there. |
cderv
left a comment
There was a problem hiding this comment.
Thanks for the PR. I reproduced the issue with your MWE, also without the Julia engine, and I agree with the direction: the merged metadata should not be thrown away.
I would like to suggest a simpler change than the recursive substitution. Only knitr can change front matter values during execution (inline R expressions in YAML). For every other engine, and for any key knitr did not touch, the executed front matter is the same as the front matter as written. So the check can be per key: if execution did not change the key, keep the merged value; if it did, use the executed value like today.
The reason I would not keep withExecutedValues is that matching evaluated entries back into an already merged array is fragile, and I found a few cases where it gives a wrong result:
-
With project
keywords: [project-kw]and a document entry`{r} "project-kw"`, the result is[project-kw, project-kw, static-kw]. knitr escapes Markdown characters in{r}results, so the executed value isproject\-kwand the dedupe does not match it. Youralicecase works because it has nothing to escape. -
With project
items: [project-entry]and a document scalaritems: ``{r} "evaluated-entry"`` , the merge first turns this into an array, and the helper then returns onlyevaluated-entry`. -
With
execute: freeze: true, the executed front matter comes from the frozen result whileunexecutedMetadatais the current source. After editing the document, a newly added nested inline expression reaches the template unevaluated. Main has the same problem for new top-level keys, but the helper extends it to nested keys.
With the per-key check, I rendered your MWE (with and without Julia), shared nested params, shared maps and arrays, and they all keep the project values. It also keeps the RevealJS controls: auto / previewLinks: auto conversion that you noticed, which on main ends up as a bare auto in the Reveal config and breaks initialisation. We probably want to add a smoke-all test for that.
The trade off is that a shared key containing an inline R expression behaves like today: the document's evaluated value replaces the whole key. Your current test fixture is exactly this case, so the test would need to change. I put a suggestion inline. I think this is acceptable as inline R in YAML is specific to knitr, and it does not regress anything compared to now. If we want more later, recursing only into mappings would be the next step, but it brings back some of the complexity.
One behaviour change to be aware of: a document's own metadata-files: and --metadata-file on the command line now take precedence over the document's front matter for keys execution did not change, because that is the order in which Quarto already merges them. Main hides this through the same loop. I am still checking what the intended precedence is there.
Could you also link #11139 in the PR description, and use it in the changelog entry? I do believe this PR is about this issue.
Also, I did suggestion as this is your PR and branch, but I can take over based on your branch if you prefer. Just tell me.
Happy to discuss further if you have any question or if I missed anything.
| // perform the override | ||
| pandocMetadata[key] = engineMetadata[key]; | ||
| pandocMetadata[key] = withExecutedValues( | ||
| pandocMetadata[key], | ||
| options.unexecutedMetadata?.[key], | ||
| engineMetadata[key], | ||
| ); |
There was a problem hiding this comment.
| // perform the override | |
| pandocMetadata[key] = engineMetadata[key]; | |
| pandocMetadata[key] = withExecutedValues( | |
| pandocMetadata[key], | |
| options.unexecutedMetadata?.[key], | |
| engineMetadata[key], | |
| ); | |
| // only take the executed value when execution changed it (e.g. knitr | |
| // inline R expressions in YAML), otherwise keep the merged value | |
| if ( | |
| !ld.isEqual(options.unexecutedMetadata?.[key], engineMetadata[key]) | |
| ) { | |
| pandocMetadata[key] = engineMetadata[key]; | |
| } |
| // options.format.metadata already merged the project's and the document's | ||
| // values for this key; engineMetadata only re-resolved the leaves that held | ||
| // inline expressions. Swap in exactly those leaves so the merge survives, | ||
| // rather than replacing the whole key with the document's own values. | ||
| function withExecutedValues( | ||
| resolved: unknown, | ||
| unexecuted: unknown, | ||
| executed: unknown, | ||
| ): unknown { | ||
| if (ld.isEqual(unexecuted, executed)) { | ||
| return resolved; | ||
| } | ||
|
|
||
| if ( | ||
| ld.isPlainObject(resolved) && ld.isPlainObject(unexecuted) && | ||
| ld.isPlainObject(executed) | ||
| ) { | ||
| const result = { ...(resolved as Metadata) }; | ||
| for (const key of Object.keys(executed as Metadata)) { | ||
| result[key] = withExecutedValues( | ||
| result[key], | ||
| (unexecuted as Metadata)[key], | ||
| (executed as Metadata)[key], | ||
| ); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| // execution rewrites array entries in place, so an entry still matching the | ||
| // front matter as written maps to the entry at that position in the executed | ||
| // array; a differing length means that correspondence no longer holds | ||
| if ( | ||
| Array.isArray(resolved) && Array.isArray(unexecuted) && | ||
| Array.isArray(executed) && unexecuted.length === executed.length | ||
| ) { | ||
| const substituted = resolved.map((item) => { | ||
| const index = unexecuted.findIndex((value) => ld.isEqual(value, item)); | ||
| return index === -1 ? item : executed[index]; | ||
| }); | ||
| // an expression can evaluate to a value the project already contributed, | ||
| // so dedupe as the merge that produced `resolved` would have | ||
| return ld.uniqBy(substituted, (value: unknown) => JSON.stringify(value)); | ||
| } | ||
|
|
||
| return executed; | ||
| } |
There was a problem hiding this comment.
With the per key check above, this helper is no longer needed and can be removed. See the review comment for the cases where the array substitution gives a wrong result.
| // options.format.metadata already merged the project's and the document's | |
| // values for this key; engineMetadata only re-resolved the leaves that held | |
| // inline expressions. Swap in exactly those leaves so the merge survives, | |
| // rather than replacing the whole key with the document's own values. | |
| function withExecutedValues( | |
| resolved: unknown, | |
| unexecuted: unknown, | |
| executed: unknown, | |
| ): unknown { | |
| if (ld.isEqual(unexecuted, executed)) { | |
| return resolved; | |
| } | |
| if ( | |
| ld.isPlainObject(resolved) && ld.isPlainObject(unexecuted) && | |
| ld.isPlainObject(executed) | |
| ) { | |
| const result = { ...(resolved as Metadata) }; | |
| for (const key of Object.keys(executed as Metadata)) { | |
| result[key] = withExecutedValues( | |
| result[key], | |
| (unexecuted as Metadata)[key], | |
| (executed as Metadata)[key], | |
| ); | |
| } | |
| return result; | |
| } | |
| // execution rewrites array entries in place, so an entry still matching the | |
| // front matter as written maps to the entry at that position in the executed | |
| // array; a differing length means that correspondence no longer holds | |
| if ( | |
| Array.isArray(resolved) && Array.isArray(unexecuted) && | |
| Array.isArray(executed) && unexecuted.length === executed.length | |
| ) { | |
| const substituted = resolved.map((item) => { | |
| const index = unexecuted.findIndex((value) => ld.isEqual(value, item)); | |
| return index === -1 ? item : executed[index]; | |
| }); | |
| // an expression can evaluate to a value the project already contributed, | |
| // so dedupe as the merge that produced `resolved` would have | |
| return ld.uniqBy(substituted, (value: unknown) => JSON.stringify(value)); | |
| } | |
| return executed; | |
| } |
There was a problem hiding this comment.
This is really specific to knitr here. Using inline R expression in YAML key was, to my knowledge, added for backward compatibility with some Rmd usage.
However, this is good checking, so we should have one test without R expressions.
With the per key check, each of these keys contains an inline expression, so they would behave like on main and the assertions on project values fail. Could you split this into one document without code, with shared keywords, nested params and custom (your original case), and one knitr document with the same static keys plus a key only the document defines, e.g. evaluated: '{r} paste0("evaluated", "-value")'? That shows both the merge and that inline R still works.
| /* | ||
| * project-metadata-merge.test.ts | ||
| * | ||
| * Copyright (C) 2020-2022 Posit Software, PBC |
There was a problem hiding this comment.
New files get a single current year header:
| * Copyright (C) 2020-2022 Posit Software, PBC | |
| * Copyright (C) 2026 Posit Software, PBC |
| - ([#14775](https://github.com/quarto-dev/quarto-cli/issues/14775)): Fix a crash when the `QUARTO_R` environment variable is set to a malformed path. Quarto now warns and falls back to other R lookup methods. | ||
| - ([#14865](https://github.com/quarto-dev/quarto-cli/issues/14865)): Fix internal links in a preview being treated as external when the preview is reached through a proxy, such as on Posit Workbench. Links are now classified against the origin the browser sees. | ||
| - ([#14878](https://github.com/quarto-dev/quarto-cli/pull/14878)): Add `az` (Azerbaijani) language translation. (author: @abdanar) | ||
| - ([#13353](https://github.com/quarto-dev/quarto-cli/pull/13353)): Fix metadata that both `_quarto.yml` and a document's front matter define being replaced by the document's value instead of merged before it reaches Pandoc templates. (author: @jkrumbiegel) |
There was a problem hiding this comment.
Could you reference #11139 here instead of the PR?
I think this PR solves the issue so let's use the issue number.
Also it covers Lua filters too, not only templates.
|
Thank you @cderv that sounds reasonable. Please just modify my branch as you see fit, I think that's easier than me implementing it at this point :) |
|
Ok great. It seems I can't modify your PR branch, so I'll branch of it and finish the work. Thanks a gain for this finding ! |
|
ok thank you, yeah I hadn't considered that this fork being in the PumasAI org can't be edited by maintainers like my personal ones |
|
I opened #14996 to continue this work. It keeps your commits with authorship and builds on your finding. Thanks again for the report and the MWE. |
|
Thanks, closing this one then :) |
Description
This PR fixes an issue I noticed when writing a typst template that was supposed to use metadata defined under the
paramskey, so that both the template and some julia code in the notebook could inspect the relevant values. I could not access values defined in_quarto.ymland first thought it was an issue aboutparamsitself, which turned out not to be the case.Here's a repo where I pushed an MWE with a basic qmd file and typst template https://github.com/jkrumbiegel/quarto-mwe-pandoc-metadata
When rendering this with quarto, I got the following result:
You can see that only the data from
_quarto.ymlthat does not share keys with the file's frontmatter actually reaches the template. I logged two relevant bits of data frompandoc.tsand you can see that this would be the behavior when replacing the first object's keys with those present in the second object:I don't really understand what problem the code that does the overwriting was supposed to solve, but I thought if data from the
engineMetadatais needed, maybe we should at least just merge it in, not overwrite. I assume the nested case was not considered when writing this.With the change from this PR, I get the following render:
I need help in adding a minimal test, as the MWE I have seems too extensive and a more targeted unit test might be better suited.
I would also ask for this fix to be considered for backporting to 1.7, as we cannot update to the 1.8 series quickly and the extent of the fix is very small while its impact is larger.
Checklist
I have (if applicable):