Skip to content

Don't overwrite but merge keys for pandoc - #13353

Closed
jkrumbiegel wants to merge 7 commits into
quarto-dev:mainfrom
PumasAI:jk/fix-nested-parameter-overwriting
Closed

jkrumbiegel wants to merge 7 commits into
quarto-dev:mainfrom
PumasAI:jk/fix-nested-parameter-overwriting

Conversation

@jkrumbiegel

@jkrumbiegel jkrumbiegel commented Sep 9, 2025 •

Copy link
Copy Markdown
Contributor

Description

This PR fixes an issue I noticed when writing a typst template that was supposed to use metadata defined under the params key, 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.yml and first thought it was an issue about params itself, 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:

image

You can see that only the data from _quarto.yml that does not share keys with the file's frontmatter actually reaches the template. I logged two relevant bits of data from pandoc.ts and you can see that this would be the behavior when replacing the first object's keys with those present in the second object:

// options.format.metadata

{
  "revealjs-plugins": [],
  params: { nested: { B: "quarto.yml", A: "frontmatter" } },
  nested_merged: { B: "quarto.yml", A: "frontmatter" },
  nested_unmerged_B: { B: "quarto.yml" },
  unnestedB: "quarto.yml",
  project: {},
  format: { "nested-typst": {} },
  nested_unmerged_A: { A: "frontmatter" },
  unnestedA: "frontmatter",
  "toc-title": "Table of contents",
  logo: { light: undefined, dark: undefined },
  "font-paths": []
}


// engineMetadata

{
  engine: "julia",
  format: { "nested-typst": "default" },
  params: { nested: { A: "frontmatter" } },
  nested_merged: { A: "frontmatter" },
  nested_unmerged_A: { A: "frontmatter" },
  unnestedA: "frontmatter"
}

I don't really understand what problem the code that does the overwriting was supposed to solve, but I thought if data from the engineMetadata is 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:

image

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):

  • filed a contributor agreement.
  • referenced the GitHub issue this PR closes
  • updated the appropriate changelog in the PR
  • ensured the present test suite passes
  • added new tests
  • created a separate documentation PR in Quarto's website repo and linked it to this PR

@posit-snyk-bot

posit-snyk-bot commented Sep 9, 2025 •

Copy link
Copy Markdown
Collaborator

✅ Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@jkrumbiegel

Copy link
Copy Markdown
Contributor Author

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 :)

@cscheid

cscheid commented Sep 9, 2025

Copy link
Copy Markdown
Member

Merging is supposed to be behavior we use, but params is a special yaml key (and yes, it's a problem that we have too many of these).

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!

@jkrumbiegel

jkrumbiegel commented Sep 9, 2025 •

Copy link
Copy Markdown
Contributor Author

This problem is not limited to params though, it applies to all nested keys. The params key was just what I found it first with, as you can only meaningfully use it in a nested way.

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.

@cscheid

cscheid commented Sep 9, 2025

Copy link
Copy Markdown
Member

Do you have an MWE of this happening outside of params? That would make it easier for us to discuss.

@jkrumbiegel

jkrumbiegel commented Sep 9, 2025 •

Copy link
Copy Markdown
Contributor Author

The MWE I posted contains both, params and no params. Consider the entry nested_merged: { B: "quarto.yml", A: "frontmatter"}, this is still correctly merged in options.format.metadata but then gets overwritten with nested_merged: { A: "frontmatter" } from engineMetadata which contains only the key A from the frontmatter and not B from _quarto.yml. I also checked that this is not engine specific, it also works incorrectly when engine: julia and the executable cell are removed.

@cscheid cscheid self-assigned this Sep 9, 2025
@cscheid cscheid added this to the v1.9 milestone Sep 9, 2025
@cscheid

cscheid commented Sep 9, 2025

Copy link
Copy Markdown
Member

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)

@cscheid

cscheid commented Sep 30, 2025

Copy link
Copy Markdown
Member

(cc @gordonwoodhull, cf our discussion yesterday)

@cscheid cscheid modified the milestones: v1.9, v1.10 Mar 11, 2026
@jkrumbiegel

Copy link
Copy Markdown
Contributor Author

@cscheid did you have time to form an opinion about this one, yet?

@mcanouil mcanouil modified the milestones: v1.10, v1.11 Jul 30, 2026
jkrumbiegel and others added 3 commits September 21, 2026 16:08
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>
@jkrumbiegel

jkrumbiegel commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor Author

@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 _quarto.yml, which still holds the unevaluated entries. The old line took the first and dropped everything the project had contributed.

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:

_quarto.yml   keywords: [project-kw]
doc.qmd       keywords: ["`r f()`", static-kw]

main          [eval-kw, static-kw]                        project-kw dropped
mergeConfigs  [project-kw, `r f()`, static-kw, eval-kw]   expression and its own result
this PR       [project-kw, eval-kw, static-kw]

Both versions of the front matter are now available, since context.target.metadata is passed to runPandoc as unexecutedMetadata, so only the leaves evaluation actually changed get substituted. Where a key's unexecuted and executed values are equal the merged value stays untouched, which is what makes arrays and nested mappings work.

Test at tests/smoke/project/project-metadata-merge.test.ts: a mapping and an array defined in both places, an inline expression inside an array, and an expression evaluating to a value the project already contributed (that one has to dedupe).

One behaviour change worth a look: a top level controls: auto or previewLinks: auto on revealjs currently reaches the reveal config as the string auto, because the loop overwrites the metadataOverride from format-reveal.ts. With this change the override survives and reveal gets false plus controlsAuto: true.

@jkrumbiegel

Copy link
Copy Markdown
Contributor Author

🤖:


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:

  • main at 19a7da9 (2026-09-21): knitr 1.52.3, these tests passed
  • this PR (2026-09-22): knitr 1.52.4, they fail

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 smoke/engine/intermediate-output-markdown.test.ts:

-::: {.cell}
+::: {.cell original.code='1 + 1'}

The three execution-env-var.qmd failures look like the same cause one step removed: those chunks are echo: false and their source contains the literal 'UNDEFINED' fallback of Sys.getenv(), while the test asserts that string must not appear in the output. Stashing the raw chunk body into the document trips that check. I could not confirm this one directly, since the job logs carry render output rather than the resulting .md.

For what it is worth, rendering both execution-env-var.qmd documents on this branch and on main gives byte identical output, so nothing in this change is involved.

@jkrumbiegel

Copy link
Copy Markdown
Contributor Author

CI is now fully green

@cderv
cderv self-requested a review September 30, 2026 08:52
@cderv

cderv commented Sep 30, 2026

Copy link
Copy Markdown
Member

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.

@jkrumbiegel

Copy link
Copy Markdown
Contributor Author

@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:

You can see that only the data from _quarto.yml that does not share keys with the file's frontmatter actually reaches the template

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.

@cderv

cderv commented Oct 2, 2026

Copy link
Copy Markdown
Member

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 (format, project, website, about):

const pandocPassedMetadata = safeCloneDeep(pandocMetadata);
delete pandocPassedMetadata.format;
delete pandocPassedMetadata.project;
delete pandocPassedMetadata.website;
delete pandocPassedMetadata.about;

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:

// selectively overwrite some resolved metadata (e.g. ensure that metadata
// computed from inline r expressions gets included @ the bottom).
const pandocMetadata = safeCloneDeep(options.format.metadata || {});
for (const key of Object.keys(engineMetadata)) {
const isChapterTitle = key === kTitle && projectIsBook(options.project);
if (!isQuartoMetadata(key) && !isChapterTitle && !isIncludeMetadata(key)) {
// if it's standard pandoc metadata and NOT contained in a format specific
// override then use the engine metadata value
// don't do if they've overridden the value in a format
const formats = engineMetadata[kMetadataFormat] as Metadata;
if (ld.isObject(formats) && metadataGetDeep(formats, key).length > 0) {
continue;
}
// don't process some format specific metadata that may have been processed already
// - theme is handled specifically already for revealjs with a metadata override and should not be overridden by user input
if (key === kTheme && isRevealjsOutput(options.format.pandoc)) {
continue;
}
// - categories are handled specifically already for website projects with a metadata override and should not be overridden by user input
if (key === kFieldCategories && projectIsWebsite(options.project)) {
continue;
}
// perform the override
pandocMetadata[key] = engineMetadata[key];
}
}

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 cderv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 is project\-kw and the dedupe does not match it. Your alice case works because it has nothing to escape.

  • With project items: [project-entry] and a document scalar items: `` {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 while unexecutedMetadata is 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.

Comment on lines -1181 to +1185
// perform the override
pandocMetadata[key] = engineMetadata[key];
pandocMetadata[key] = withExecutedValues(
pandocMetadata[key],
options.unexecutedMetadata?.[key],
engineMetadata[key],
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// 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];
}

Comment on lines +1886 to +1931
// 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;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Suggested change
// 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;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

New files get a single current year header:

Suggested change
* Copyright (C) 2020-2022 Posit Software, PBC
* Copyright (C) 2026 Posit Software, PBC

Comment thread news/changelog-1.11.md
- ([#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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@jkrumbiegel

Copy link
Copy Markdown
Contributor Author

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 :)

@cderv

cderv commented Oct 5, 2026

Copy link
Copy Markdown
Member

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 !

@jkrumbiegel

Copy link
Copy Markdown
Contributor Author

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

@cderv

cderv commented Oct 5, 2026

Copy link
Copy Markdown
Member

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.

@jkrumbiegel

Copy link
Copy Markdown
Contributor Author

Thanks, closing this one then :)

@jkrumbiegel jkrumbiegel closed this Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants