Skip to content

fix: handle escaped closing brace in template placeholder parsing (#462) - #1218

Open
Mikkey-f wants to merge 5 commits into
apache:mainfrom
Mikkey-f:fix/462-escaped-placeholder
Open

Mikkey-f wants to merge 5 commits into
apache:mainfrom
Mikkey-f:fix/462-escaped-placeholder

Conversation

@Mikkey-f

@Mikkey-f Mikkey-f commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Purpose of the pull request

Fixes #462. Supersedes #813 by @roeniss — the diagnosis, the fix and the original test scenarios are his; I re-applied the change on current main and rewrote the test to follow the post-#901 testkit conventions.

Filling a template whose placeholder has an escaped closing brace (a cell
containing {foo\}) aborts with StringIndexOutOfBoundsException:

java.lang.StringIndexOutOfBoundsException: begin 1, end -1, length 6

The inner loop that looks for a valid (non-escaped) closing brace can exit
because startIndex ran past the end of the value, leaving suffixIndex == -1;
only the other exit path (indexOf returning -1) was handled, so
value.substring(prefixIndex + 1, -1) blows up. A placeholder with an escaped
closing brace has no valid closing brace to parse and should be kept as-is.

A related edge case remains when a normal placeholder follows: in
{foo\} {bar} the scan used to consume {bar}'s closing brace, parse
foo\} {bar as the variable name and drop the row. An escaped closing brace
must only end the literal placeholder, so parsing now resumes at the next {.

What's changed?

  • ExcelWriteFillExecutor.prepareData: an escaped closing brace no longer
    ends or derails the placeholder scan. When another { starts before the
    next non-escaped }, the opener is kept as literal text and parsing
    resumes at that next placeholder, so {foo\} {bar} fills to
    {foo\} second_value. The closing-brace scan now also starts after the
    current opener instead of possibly scanning past it.
  • New EscapedPlaceholderTest (5 cases): a one-cell template is filled with
    foo=replaced_value and the cell is read back —
    • {foo\} (escaped closing brace) → cell stays {foo\}
    • prefix {foo\} suffix → cell stays as-is
    • \{foo} (escaped opening brace) → cell stays as-is
    • {foo} → still replaced with replaced_value
    • {foo\} {bar} → {foo\} second_value (the escaped placeholder is kept,
      the following placeholder is still filled)

Verification

  • Red: with the parser fix reverted, the mixed case reads back no rows at all
    (the fill drops the row); before the earlier guard, {foo\} threw
    StringIndexOutOfBoundsException: begin 1, end -1, length 6.
  • Green: 5/5 in EscapedPlaceholderTest.
  • Full suite: mvn clean package -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet → 60 + 1090 tests, 0 failures.
    spotless:check and javadoc:javadoc pass.
  • CI: 9/9 green on bbe472d4.

Notes

Checklist

  • I have read the Contributor Guide.
  • I have added the necessary unit tests and all cases have passed.

@skytin1004 skytin1004 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix @Mikkey-f. I found one remaining edge case: with {foo} {bar}, the scan consumes {bar}'s closing brace as part of the first placeholder, causing the output row to disappear. Could you update the parser to resume at the next placeholder and add this regression case?

@Test
void escapedPlaceholderInsideTextIsKeptAsLiteralText() throws IOException {
Assertions.assertEquals("prefix {foo\\} suffix", fillTemplateWith("prefix {foo\\} suffix"));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add the mixed case that exposes the remaining parser error.

Suggested change
}
}
@Test
void escapedPlaceholderBeforeNormalPlaceholderIsHandledIndependently() throws IOException {
Assertions.assertEquals("{foo\\} second_value", fillTemplateWith("{foo\\} {bar}"));
}

File template = createTemplate(cellText);
File output = createTempFile("escaped-placeholder-out", ExcelFormat.XLSX);
Map<String, Object> data = new HashMap<>();
data.put(VARIABLE, VALUE);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This supplies the normal placeholder used by the regression case.

Suggested change
data.put(VARIABLE, VALUE);
data.put(VARIABLE, VALUE);
data.put("bar", "second_value");

@Mikkey-f

Copy link
Copy Markdown
Contributor Author

Thanks @skytin1004 — fixed in bbe472d.

The parser now treats an opener whose closing brace is escaped as literal text
and resumes at the next {, so {foo\} {bar} fills to {foo\} second_value
instead of consuming {bar}'s closing brace. I added the mixed regression
case and the bar data entry you suggested, and made the closing-brace scan
start after the current opener (it could previously scan past it — the
existing \{name\} … {name} templates exercise this path).

Verification: EscapedPlaceholderTest 5/5; full suite 1150 (60 + 1090) with 0
failures; spotless and javadoc pass; CI is 9/9 green on bbe472d. Could you
take another look when you have a moment?

This branch has not been deployed

No deployments
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.

[Bug] When filling the template, an exception is reported when the placeholder content is '{foo\}'

3 participants