Skip to content

Fix topmatter clobbering validated config values, and enforce global_only - #1197

Open
dchaudhari7177 wants to merge 1 commit into
executablebooks:masterfrom
dchaudhari7177:fix/1167-topmatter-clobbers-validated-config
Open

Fix topmatter clobbering validated config values, and enforce global_only#1197
dchaudhari7177 wants to merge 1 commit into
executablebooks:masterfrom
dchaudhari7177:fix/1167-topmatter-clobbers-validated-config

Conversation

@dchaudhari7177

Copy link
Copy Markdown

Fixes #1167. Two bugs in merge_file_level, both reachable from ordinary front matter.

1. Validated values were overwritten

The loop called validate_field(new, field, value) and then setattr(new, name, value) with the raw value. Validators that normalise their input write the result back on the instance, so that write was immediately undone.

The issue reports this via heading_slug_func, but it is not specific to that field:

---
myst:
  fence_as_directive: ["mermaid"]
---
global default:   set()      <- the type the field holds
after topmatter:  ['mermaid'] <- a list

check_fence_as_directive converts the sequence to a set and assigns it; the raw list then landed on top. The rest of the code does set operations on that field, so this is a wrong type rather than merely a different one.

Fixed by assigning before validating and letting the validator have the last word — which is the order __post_init__ already uses, where the value is on the instance before validate_fields runs. The consequence is that an invalid value now has to be rolled back explicitly, so a rejected value isn't reported and kept; there are two tests for that, including one asserting it rolls back to the global value rather than the dataclass default.

2. global_only was declared and never checked

Five fields carry the metadata — heading_slug_func, update_mathjax, mathjax_classes, suppress_warnings, inventories — and topmatter could set all of them.

---
myst:
  heading_slug_func: github
---

put the preset name in the field instead of the function it names, so every heading raised 'str' object is not callable [myst.heading_slug] and no slugs were generated. Topmatter now warns and keeps the global value.

Worth noting: the documented contract was already correct. _docs.py:137 filters global_only fields out of the :scope: local table, so configuration.md has never listed these five as front-matter settable. Only the enforcement was missing — which is why I made topmatter reject them rather than making the validation stick.

Tests

Seventeen new tests in tests/test_config_topmatter.py. Eight fail on master:

FAILED TestValidatorNormalisation::test_a_sequence_is_stored_as_the_set_the_field_holds
FAILED TestGlobalOnlyFields::test_a_global_only_field_is_rejected[heading_slug_func-github]
FAILED TestGlobalOnlyFields::test_a_global_only_field_is_rejected[update_mathjax-False]
FAILED TestGlobalOnlyFields::test_a_global_only_field_is_rejected[mathjax_classes-tex2jax_process]
FAILED TestGlobalOnlyFields::test_a_global_only_field_is_rejected[suppress_warnings-value3]
FAILED TestGlobalOnlyFields::test_a_global_only_field_is_rejected[inventories-value4]
FAILED TestGlobalOnlyFields::test_the_rejected_field_keeps_the_global_value
FAILED TestGlobalOnlyFields::test_a_non_global_field_alongside_one_is_still_applied

The other nine are guards for the paths the reordering runs through, and are the ones I'd want if someone later re-simplified this: merge_topmatter still merges rather than replaces, the file-level value still wins on a merged field, one bad key doesn't abandon the rest of the topmatter, an unknown field is still reported, and the global config isn't mutated.

pytest tests/                    1252 passed, 18 skipped
ruff 0.15.20 check / format      clean   (matching .pre-commit-config.yaml)

Three failures in my local run — test_cmdline[40-linkify], test_extended_syntaxes, test_extended_syntaxes_text — are identical on master with this branch stashed, so they aren't from this change.

No CHANGELOG.md entry: entries there carry <gh-pr:NNNN> links and appear to be written at release time. Happy to add one if you'd rather they land with the PR.

…only

Two bugs in `merge_file_level`, both reachable from ordinary front matter.

**Validated values were overwritten.** The loop called
`validate_field(new, field, value)` and then `setattr(new, name, value)`
with the *raw* value. Validators that normalise their input write the
result back on the instance, so that write was immediately undone:

    ---
    myst:
      fence_as_directive: ["mermaid"]
    ---

left a `list` in a field the rest of the code treats as a `set`.

Fixed by assigning before validating and letting the validator have the
last word -- the order `__post_init__` already uses, where the value is on
the instance before `validate_fields` runs. An invalid value now has to be
rolled back explicitly, so a rejected value is not reported *and* kept.

**`global_only` was declared and never checked.** Five fields carry the
metadata -- `heading_slug_func`, `update_mathjax`, `mathjax_classes`,
`suppress_warnings`, `inventories` -- and topmatter could set all of them.

    ---
    myst:
      heading_slug_func: github
    ---

put the preset *name* in the field instead of the function it names, so
every heading raised `'str' object is not callable [myst.heading_slug]` and
no slugs were generated. Topmatter now warns and keeps the global value.

The documented contract was already right: `_docs.py` filters `global_only`
fields out of the `:scope: local` table, so `configuration.md` has never
listed these five as front-matter settable. Only the enforcement was
missing.

Seventeen tests, eight of which fail on master. Beyond the two bugs they
pin the surrounding behaviour the reordering runs through: `merge_topmatter`
still merges rather than replaces, a rejected field falls back to the
*global* value rather than the dataclass default, one bad key does not
abandon the rest of the topmatter, and the global config is not mutated.

tests/: 1252 passed, 18 skipped. The 3 failures in my run
(`test_cmdline[40-linkify]`, `test_extended_syntaxes`,
`test_extended_syntaxes_text`) are identical on master with this branch
stashed. ruff 0.15.20 clean.

Fixes executablebooks#1167
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.

Topmatter clobbers validated config values (heading_slug_func presets crash), and global_only is not enforced

1 participant