Repository navigation
Conversation
When a template placeholder contains an escaped closing brace like {foo\},
the parser would throw StringIndexOutOfBoundsException because it attempted
to use suffixIndex=-1 in a substring operation.
Added a bounds check after the inner while loop to break out when no valid
(non-escaped) closing brace is found.
|
commit force-pushed due to spotless check |
|
@psxjoy could you review this? |
There was a problem hiding this comment.
Pull request overview
Fixes template placeholder parsing to avoid an out-of-bounds exception when encountering an escaped closing brace (e.g., {foo\}), addressing issue #462.
Changes:
- Adds a post-loop bounds check in
prepareDatato stop parsing when no valid (non-escaped) closing brace is found. - Adds a new regression test class covering escaped-brace placeholder scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/executor/ExcelWriteFillExecutor.java |
Prevents parsing from continuing when the suffix scan ends without finding a non-escaped }. |
fesod-examples/fesod-sheet-examples/src/test/java/org/apache/fesod/sheet/temp/issue462/EscapedPlaceholderTest.java |
Introduces tests for escaped closing/opening brace cases and a normal placeholder control case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| private static void createTemplate(String fileName, String content) throws IOException { | ||
| File templateFile = new File(TEMPLATE_PATH + fileName); |
There was a problem hiding this comment.
createTemplate writes to TEMPLATE_PATH + fileName, but it never ensures that temp/issue462/ exists under the test classpath root. Since src/test/resources/temp/ exists but temp/issue462/ does not, new FileOutputStream(templateFile) will throw FileNotFoundException on a clean checkout. Create the parent directories before opening the stream (or reuse TestFileUtil.createNewFile(...) which already creates parent dirs).
| File templateFile = new File(TEMPLATE_PATH + fileName); | |
| File templateFile = new File(TEMPLATE_PATH + fileName); | |
| File parentDir = templateFile.getParentFile(); | |
| if (parentDir != null && !parentDir.exists() && !parentDir.mkdirs()) { | |
| throw new IOException("Failed to create directory: " + parentDir); | |
| } |
There was a problem hiding this comment.
@psxjoy I think this feedback doesn't make sense, what do you think?
|
@roeniss Hi, can you update the branch and let code in right place ? I think we can merge this PR if everything is already. |
Per CONTRIBUTING.md, tests should live in the corresponding module's test directory. The bug under test is in fesod-sheet's ExcelWriteFillExecutor, so move the test from fesod-examples to fesod-sheet/src/test/java/.../fill/. Also use TestFileUtil.createNewFile() for path resolution so parent directories are created automatically, instead of relying on raw FileOutputStream which fails on a clean checkout.
|
Hi @roeniss — checking in on this one. psxjoy mentioned back in April that it would be good to merge once the branch is updated and the code moved into the right place, and the PR has been quiet since July. Are you still planning to carry it forward? If you don't have the bandwidth for it right now, I'd be happy to pick it up — I'd re-apply the |
|
@Mikkey-f Hi Mikkey, you can carry the issue instead of me. Thanks! |
|
@roeniss Thanks for the go-ahead — carrying it forward as promised. Re-applied the New PR: #1218 (credits this PR and #462 in its description). Once it is merged, |
Purpose of the pull request
Closed: #462
What's changed?
Added a bounds check after the inner while loop to break out when no valid (non-escaped) closing brace is found.
Checklist