Keep fixture annotations out of ERB blocks (#345)#353
Merged
Conversation
ERB fixtures cannot be parsed by Psych, so the parser used to evaluate the ERB and read Psych line numbers off the *evaluated* output. Those line numbers were then used as indices into the *original* file. When an ERB tag spans multiple lines the offsets no longer line up, so the schema annotation was written inside an ERB tag (e.g. between `<%` and `%>`, or inside a `<% ... do %>` loop). Evaluating the ERB also runs arbitrary code, which could fail outright. Instead, derive the content bounds straight from the original lines: the first non-blank, non-comment line is the start, so annotations land above the ERB body (and after any leading comments), never inside a tag. Fixes drwl#345
OdenTakashi
reviewed
Jul 4, 2026
Comment on lines
+46
to
+54
| # "Dynamic fixtures with ERB" exist in Rails and cause Psych.parser to error. | ||
| # | ||
| # We deliberately do not evaluate the ERB and read line numbers off the | ||
| # result: evaluating runs arbitrary code, and the line numbers from the | ||
| # evaluated output do not map back to the original file (ERB tags spanning | ||
| # multiple lines shift the offsets), which would place annotations inside | ||
| # an ERB tag. Instead we derive the content bounds straight from the | ||
| # original lines so annotations land around the ERB body. | ||
| return record_erb_positions |
Collaborator
There was a problem hiding this comment.
One concern: this now rescues any Psych::SyntaxError and falls back to record_erb_positions, so invalid YAML without ERB.
e.g.
user_one: [would no longer raise and could still be annotated.
Personally, I think surfacing a broken file is friendlier. Ruby model files generally fail while resolving/loading the model, so I wonder if YAML should keep similar behavior unless the file actually looks like an ERB fixture.
What do you think about re-raising when there are no ERB tags?
Contributor
Author
There was a problem hiding this comment.
@OdenTakashi I agree, I added the update
The ERB fallback rescued any Psych::SyntaxError, so a genuinely malformed YAML fixture with no ERB (e.g. `user_one: [`) would no longer surface the parse error and could still be annotated. Only fall back to the line-based content bounds when the file actually contains an ERB tag; otherwise re-raise so the broken file surfaces, matching prior behavior.
OdenTakashi
approved these changes
Jul 4, 2026
OdenTakashi
left a comment
OdenTakashi
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the quick fix. Looks great to me.
Comment on lines
+107
to
+109
| def erb_fixture? | ||
| @input.match?(/<%.*?%>/m) | ||
| end |
Collaborator
There was a problem hiding this comment.
I also like the erb_fixture? helper here. Even though the condition is small, it makes the guard really easy to read.
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
2 participants
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.
Summary
Fixes #345.
Schema annotations were being written inside ERB tags in dynamic fixture files. For example, given a fixture that starts with a multi-line ERB block:
running
annotaterb modelsproduced:i.e. the annotation landed in the middle of the ERB block, corrupting the file.
Root cause
ERB fixtures cannot be parsed by Psych, so
YmlParserevaluated the ERB (ERB.new(input).result) and read Psych's line numbers off the evaluated output. Those line numbers were then used as indices into the original file.When an ERB tag spans multiple lines, the evaluated output has a different line count than the source, so the offsets no longer line up and the annotation is inserted at the wrong position — inside an ERB tag, or inside a
<% ... do %>loop. Evaluating the ERB also executes arbitrary Ruby, which can fail outright.This affected every ERB fixture, not just files starting with
<%— the existing<% 1.upto(100) do |i| %>case also had its annotation written inside the loop.Fix
Stop evaluating the ERB. Instead derive the content bounds directly from the original lines: the first non-blank, non-comment line is the start, so annotations are written above the ERB body (and after any leading comments), never inside a tag. The result is also idempotent across repeated runs.
Tests
YmlParserspec for a fixture that starts with a multi-line ERB block.Generatorspecs covering an ERB fixture that starts with<%and one preceded by a comment.YmlParserandSingleFileAnnotatorwhose expectations encoded the buggy placement.Verified these specs fail without the fix and pass with it. Full unit suite is green.