Fix substitution keys with characters invalid in a Jinja2 identifier#1172
Open
agu2347 wants to merge 1 commit into
Open
Fix substitution keys with characters invalid in a Jinja2 identifier#1172agu2347 wants to merge 1 commit into
agu2347 wants to merge 1 commit into
Conversation
render_substitution() builds a Jinja2 expression by directly
interpolating the raw substitution content into a template string:
env.from_string(f"{{{{{token.content}}}}}"). For a key like
`foo-bar`, this produces the template "{{foo-bar}}", which Jinja2
parses as the *expression* `foo - bar` (subtraction of two separate
names) rather than as a lookup of a single variable named "foo-bar".
Since neither `foo` nor `bar` exist in the substitution context, this
raised `UndefinedError: 'foo' is undefined`, matching the error in
the issue -- notice it never even mentions the `-bar` part, because
Jinja evaluates left-to-right and raises on the first undefined name.
MyST substitutions deliberately support full Jinja2 expression syntax
(filters, dotted attribute access, etc. -- confirmed by the existing
circular-reference detection walking the full parsed AST for *all*
Name nodes, not just a single key), so a blanket fix escaping all
non-identifier-safe characters isn't viable without breaking that.
Fix: when the substitution's content is *exactly* one key already
present in the variable context, look it up directly via dict
subscript, bypassing Jinja2 expression parsing entirely for that
case. Anything else -- arbitrary expressions, filters, dotted access,
keys that aren't a literal match -- still goes through the existing
Jinja2 rendering path, completely unaffected. The special `env` name
is excluded from the fast path's reference-tracking, matching the
existing exclusion in the general path.
Verified end-to-end with a real Sphinx build (a shallow test-fixture
based reproduction wasn't enough on its own, so I also built a
throwaway Sphinx project on disk mirroring the issue's exact repro):
before the fix, `{{foo-bar}}` renders empty with the exact
'foo' is undefined warning from the issue; after the fix, it renders
correctly. Also verified normal plain keys, Jinja2 filter expressions
(e.g. `{{ value | upper }}`), and circular-reference detection all
continue to work exactly as before.
Added an integration test (test_substitutions_dashed_key) with a new
sourcedirs fixture covering: a dashed key, a plain key, and a circular
reference (to confirm circular-reference detection still triggers
correctly for a dashed key too, exercising the fast path's reference
tracking). Confirmed the test fails with the original code (produces
the exact reported UndefinedError and leaves the dashed key
unrendered) and passes with the fix.
Ran the full existing test suite: 1241 passed (1240 baseline + 1 new),
12 skipped. The 4 remaining failures (missing linkify-it-py optional
dependency, and pygments/docutils version-related rendering diffs)
are present identically on a clean checkout of master, unrelated to
this change.
Fixes executablebooks#1007
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1007.
render_substitution()builds a Jinja2 expression by directly interpolating the raw substitution content into a template string:env.from_string(f"{{{{{token.content}}}}}"). For a key likefoo-bar, this produces the template"{{foo-bar}}", which Jinja2 parses as the expressionfoo - bar(subtraction of two separate names) rather than as a lookup of a single variable namedfoo-bar. Since neitherfoonorbarexist in the substitution context, this raisedUndefinedError: 'foo' is undefined, matching the error in the issue -- notice it never even mentions the-barpart, because Jinja evaluates left-to-right and raises on the first undefined name.MyST substitutions deliberately support full Jinja2 expression syntax (filters, dotted attribute access, etc. -- confirmed by the existing circular-reference detection walking the full parsed AST for all
Namenodes, not just a single key), so a blanket fix escaping all non-identifier-safe characters isn't viable without breaking that.Fix: when the substitution's content is exactly one key already present in the variable context, look it up directly via dict subscript, bypassing Jinja2 expression parsing entirely for that case. Anything else -- arbitrary expressions, filters, dotted access, keys that aren't a literal match -- still goes through the existing Jinja2 rendering path, completely unaffected. The special
envname is excluded from the fast path's reference-tracking, matching the existing exclusion in the general path.Verification: verified end-to-end with a real Sphinx build (a shallow test-fixture based reproduction wasn't enough on its own, so I also built a throwaway Sphinx project on disk mirroring the issue's exact repro): before the fix,
{{foo-bar}}renders empty with the exact'foo' is undefinedwarning from the issue; after the fix, it renders correctly. Also verified normal plain keys, Jinja2 filter expressions (e.g.{{ value | upper }}), and circular-reference detection all continue to work exactly as before.Testing: added an integration test (
test_substitutions_dashed_key) with a newsourcedirsfixture covering: a dashed key, a plain key, and a circular reference (to confirm circular-reference detection still triggers correctly for a dashed key too, exercising the fast path's reference tracking). I confirmed the test fails with the original code (produces the exact reportedUndefinedErrorand leaves the dashed key unrendered) and passes with the fix.Ran the full existing test suite: 1241 passed (1240 baseline + 1 new), 12 skipped. The 4 remaining failures (missing
linkify-it-pyoptional dependency, and pygments/docutils version-related rendering diffs) are present identically on a clean checkout ofmaster, unrelated to this change.