Skip to content

Always sanitize braces on save and remove InvalidFieldValueException#15559

Merged
calixtus merged 34 commits intomainfrom
braces
Apr 19, 2026
Merged

Always sanitize braces on save and remove InvalidFieldValueException#15559
calixtus merged 34 commits intomainfrom
braces

Conversation

@calixtus
Copy link
Copy Markdown
Member

@calixtus calixtus commented Apr 15, 2026

Related issues and pull requests

Fixes #8730
Refs #9833

PR Description

See qodo

Bonus: Replaced proper StringUtil repeat method with new JDK repeat method

Steps to test

  • Insert unbalanced brace in entry field
grafik
  • Run integrity check
grafik
  • Save and read with text editor
grafik
  • Bonus: Notice little warning sign is also shown. JabRef automatically adapts. How nice.
grafik

Checklist

  • I own the copyright of the code submitted and I license it under the MIT license
  • I manually tested my changes in running JabRef (always required)
  • I added JUnit tests for changes (if applicable)
  • I added screenshots in the PR description (if change is visible to the user)
  • [.] I added a screenshot in the PR description showing a library with a single entry with me as author and as title the issue number
  • [.] I described the change in CHANGELOG.md in a way that can be understood by the average user (if change is visible to the user)
  • [.] I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Always sanitize unbalanced braces on save and remove InvalidFieldValueException

🐞 Bug fix ✨ Enhancement

Grey Divider

Walkthroughs

Description
• Remove InvalidFieldValueException and always sanitize unbalanced braces on save
• Refactor brace validation to return error list instead of throwing exceptions
• Replace custom StringUtil.repeatSpaces() with native JDK String.repeat()
• Relax validation in SourceTab and BackupManager to handle malformed input gracefully
Diagram
flowchart LR
  A["Field Writer"] -->|sanitizeUnbalancedBraces| B["Escaped Braces"]
  A -->|checkBalancedBraces| C["Error List"]
  D["Source Tab"] -->|validate| C
  E["Backup Manager"] -->|catch IOException| F["Log Error"]
  G["Removed Exception"] -.->|replaced by| C
Loading

Grey Divider

File Changes

1. jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java ✨ Enhancement +79/-53

Refactor brace checking and add sanitization

jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java


2. jablib/src/main/java/org/jabref/logic/bibtex/InvalidFieldValueException.java 🐞 Bug fix +0/-12

Remove deprecated exception class

jablib/src/main/java/org/jabref/logic/bibtex/InvalidFieldValueException.java


3. jabgui/src/main/java/org/jabref/gui/entryeditor/SourceTab.java ✨ Enhancement +81/-76

Relax validation and use error list checking

jabgui/src/main/java/org/jabref/gui/entryeditor/SourceTab.java


View more (13)
4. jabgui/src/main/java/org/jabref/gui/autosaveandbackup/BackupManager.java ✨ Enhancement +1/-15

Remove exception handling and simplify logging

jabgui/src/main/java/org/jabref/gui/autosaveandbackup/BackupManager.java


5. jablib/src/main/java/org/jabref/logic/integrity/BracketChecker.java ✨ Enhancement +5/-22

Simplify bracket validation using FieldWriter

jablib/src/main/java/org/jabref/logic/integrity/BracketChecker.java


6. jablib/src/main/java/org/jabref/logic/bibtex/BibEntryWriter.java ✨ Enhancement +5/-10

Remove exception handling for field writing

jablib/src/main/java/org/jabref/logic/bibtex/BibEntryWriter.java


7. jablib/src/main/java/org/jabref/logic/exporter/BibDatabaseWriter.java ✨ Enhancement +5/-10

Remove exception handling and use native repeat

jablib/src/main/java/org/jabref/logic/exporter/BibDatabaseWriter.java


8. jablib/src/main/java/org/jabref/logic/bst/BstVMVisitor.java ✨ Enhancement +14/-18

Remove exception handling for field writing

jablib/src/main/java/org/jabref/logic/bst/BstVMVisitor.java


9. jablib/src/main/java/org/jabref/logic/util/strings/StringUtil.java ✨ Enhancement +1/-24

Remove repeatSpaces and repeat utility methods

jablib/src/main/java/org/jabref/logic/util/strings/StringUtil.java


10. jabgui/src/main/java/org/jabref/gui/collab/entrychange/PreviewWithSourceTab.java ✨ Enhancement +1/-1

Use direct constructor instead of factory method

jabgui/src/main/java/org/jabref/gui/collab/entrychange/PreviewWithSourceTab.java


11. jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java ✨ Enhancement +1/-1

Use direct constructor instead of factory method

jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java


12. jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java ✨ Enhancement +1/-1

Use direct constructor instead of factory method

jabgui/src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java


13. jablib/src/main/java/org/jabref/model/entry/BibEntry.java ✨ Enhancement +1/-1

Use direct constructor instead of factory method

jablib/src/main/java/org/jabref/model/entry/BibEntry.java


14. jablib/src/test/java/org/jabref/logic/bibtex/FieldWriterTest.java 🧪 Tests +46/-45

Update tests for sanitization and error list checking

jablib/src/test/java/org/jabref/logic/bibtex/FieldWriterTest.java


15. jablib/src/test/java/org/jabref/logic/util/strings/StringUtilTest.java 🧪 Tests +0/-25

Remove tests for deleted utility methods

jablib/src/test/java/org/jabref/logic/util/strings/StringUtilTest.java


16. jablib/src/main/java/org/jabref/logic/exporter/EmbeddedBibFilePdfExporter.java Additional files +1/-1

...

jablib/src/main/java/org/jabref/logic/exporter/EmbeddedBibFilePdfExporter.java


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

qodo-free-for-open-source-projects Bot commented Apr 15, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (2) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Brace errors not localized📘 Rule violation ⚙ Maintainability
Description
BracketChecker returns raw English strings (from FieldWriter.checkBalancedBraces) that are shown
to users during integrity checking instead of using Localization.lang(...). This violates the
requirement that all user-facing text be localized and can lead to inconsistent UX across locales.
Code

jablib/src/main/java/org/jabref/logic/integrity/BracketChecker.java[R11-15]

public Optional<String> checkValue(String value) {
-        if (StringUtil.isBlank(value)) {
-            return Optional.empty();
-        }
-
-        // metaphor: integer-based stack (push + / pop -)
-        int counter = 0;
-        for (char a : value.trim().toCharArray()) {
-            if (a == '{') {
-                counter++;
-            } else if (a == '}') {
-                if (counter == 0) {
-                    return Optional.of(Localization.lang("unexpected closing curly bracket"));
-                } else {
-                    counter--;
-                }
-            }
-        }
-
-        if (counter > 0) {
-            return Optional.of(Localization.lang("unexpected opening curly bracket"));
+        List<String> errors = FieldWriter.checkBalancedBraces(value);
+        if (!errors.isEmpty()) {
+            return Optional.of(String.join("\n", errors));
}
Evidence
PR Compliance ID 25 requires all user-facing text to be localized; the changed integrity checker now
returns a joined, non-localized error string. The underlying brace-checker also constructs hardcoded
English messages.

AGENTS.md
jablib/src/main/java/org/jabref/logic/integrity/BracketChecker.java[11-15]
jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[56-75]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`BracketChecker` now surfaces non-localized, hardcoded English brace-balance errors to the UI.
## Issue Context
Integrity-check messages are user-facing and must be localized via `Localization.lang(...)`.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/integrity/BracketChecker.java[11-15]
- jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[56-75]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Legacy Stack used📘 Rule violation ⚙ Maintainability
Description
New brace validation/sanitization code uses java.util.Stack, which is a legacy collection type and
not preferred in modern Java. This conflicts with the requirement to use modern Java APIs/idioms
where applicable.
Code

jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[R36-38]

+    public static List<String> checkBalancedBraces(String text) {
+        Stack<FieldWriter.BracePosition> openBraces = new Stack<>();
+        List<String> errors = new ArrayList<>();
Evidence
PR Compliance ID 4 requires preferring modern Java idioms/APIs; Stack is legacy and typically
replaced with Deque (e.g., ArrayDeque) for stack semantics. The new code introduces Stack for
both brace checking and sanitization.

AGENTS.md
jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[3-5]
jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[36-38]
jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[84-86]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`FieldWriter` uses `java.util.Stack` for brace tracking, which is a legacy API.
## Issue Context
Modern Java code typically uses `Deque` implementations (e.g., `ArrayDeque`) for stack behavior.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[3-5]
- jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[36-38]
- jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[84-109]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Passing null to UndoableFieldChange 📘 Rule violation ☼ Reliability
Description
storeSource passes null as the new field value when recording an undoable edit. This
introduces/retains a null-argument call path outside the documented exception and can hide
null-handling bugs.
Code

jabgui/src/main/java/org/jabref/gui/entryeditor/SourceTab.java[R333-335]

+            if (!newEntry.hasField(fieldName)) {
+                compound.addEdit(new UndoableFieldChange(outOfFocusEntry, fieldName, fieldValue, null));
+                outOfFocusEntry.clearField(fieldName);
Evidence
PR Compliance ID 13 disallows passing null as an argument (outside the stated exception). The
modified code calls new UndoableFieldChange(..., null) when clearing a field.

AGENTS.md
jabgui/src/main/java/org/jabref/gui/entryeditor/SourceTab.java[333-335]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SourceTab.storeSource` passes `null` into `UndoableFieldChange` to represent clearing a field.
## Issue Context
Compliance requires avoiding `null` arguments; consider an explicit API for field-clear edits (e.g., dedicated constructor/factory method) or an Optional-based representation.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/entryeditor/SourceTab.java[333-335]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Weak checkBalancedBraces assertions 📘 Rule violation ☼ Reliability
Description
The new test only asserts the result is not an empty list, rather than asserting the exact expected
error output. This weak predicate assertion can allow near-miss regressions to pass.
Code

jablib/src/test/java/org/jabref/logic/bibtex/FieldWriterTest.java[R137-146]

+    @ParameterizedTest
+    @ValueSource(strings = {
+            "{",
+            "}",
+            "{{",
+            "{}",
+            "\\{}"})
+    void checkUnbalancedBraces(String input) {
+        assertNotEquals(List.of(), FieldWriter.checkBalancedBraces(input));
}
Evidence
PR Compliance ID 39 requires tests to assert exact expected values instead of weak predicates. The
added test uses assertNotEquals(List.of(), ...), which does not validate specific error content.

jablib/src/test/java/org/jabref/logic/bibtex/FieldWriterTest.java[137-146]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`checkUnbalancedBraces` asserts only that errors are non-empty, which is a weak predicate.
## Issue Context
To prevent regressions, the test should assert the exact expected list (or at minimum exact expected message(s) for each input).
## Fix Focus Areas
- jablib/src/test/java/org/jabref/logic/bibtex/FieldWriterTest.java[137-146]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (2)
5. Brace locations misreported🐞 Bug ≡ Correctness
Description
FieldWriter.checkBalancedBraces reports incorrect positions: it sets lastLineIndex to the newline
index (making columns after a newline off by one) and prints the opening brace character index as
the "line" for unbalanced '{'. This causes integrity/validation messages to point users to the wrong
location in the field value.
Code

jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[R45-74]

if (item == '\n') {
Evidence
In checkBalancedBraces, newline handling uses lastLineIndex = i but column is calculated as `i -
lastLineIndex + 1`, which makes the first character after a newline report column 2 (should be 1).
For unbalanced opening braces, the error message uses lastOpenBrace.index() for the line
placeholder, so the message prints the character index instead of the tracked line number;
BracketChecker returns these messages to the user during integrity checks.

jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[45-75]
jablib/src/main/java/org/jabref/logic/integrity/BracketChecker.java[10-16]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`FieldWriter.checkBalancedBraces` produces incorrect user-facing locations for brace issues:
- Column computation becomes off-by-one after a newline because `lastLineIndex` is set to the newline index.
- Unbalanced opening-brace errors print the character index where a 1-based line number is expected.
### Issue Context
These messages are surfaced to users (e.g., via integrity checks) and should be accurate to help them fix malformed BibTeX/LaTeX values.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[45-75]
### Expected fix
- When encountering `\n`, set `lastLineIndex` such that the next character’s column becomes 1 (e.g., `lastLineIndex = i + 1`), and keep column calculations consistent.
- For unbalanced `{` messages, use the stored brace `line` (convert to 1-based) instead of `index`.
- Ensure both `{` and `}` errors use consistent 1-based line/column reporting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Brace list in message🐞 Bug ≡ Correctness
Description
SourceTab.storeSource passes a List<String> of brace errors directly to Localization.lang, which
will be rendered via List.toString() (e.g., with brackets/commas) instead of a readable multiline
message. This makes the validation message confusing when brace errors occur.
Code

jabgui/src/main/java/org/jabref/gui/entryeditor/SourceTab.java[R339-350]

+        // Then set all fields that have been set by the user.
+        for (Map.Entry<Field, String> field : newEntry.getFieldMap().entrySet()) {
+            Field fieldName = field.getKey();
+            String oldValue = outOfFocusEntry.getField(fieldName).orElse(null);
+            String newValue = field.getValue();
+            if (!Objects.equals(oldValue, newValue)) {
+                // Test if the field is legally set.
+                List<String> errors = FieldWriter.checkBalancedBraces(newValue);
+                if (!errors.isEmpty()) {
+                    validationMessage.setValue(ValidationMessage.error(Localization.lang("Failed to parse Bib(La)TeX: %0", errors)));
+                    return;
}
-            }
Evidence
The code computes List errors = FieldWriter.checkBalancedBraces(newValue) and then passes errors
as the formatting argument to Localization.lang("... %0", errors), which will format the list
object rather than a joined string.

jabgui/src/main/java/org/jabref/gui/entryeditor/SourceTab.java[339-350]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SourceTab.storeSource` constructs a `List<String>` of brace errors and passes that list directly into `Localization.lang(...)`, causing the UI message to show Java list formatting (e.g., `[err1, err2]`) instead of a readable message.
### Issue Context
This path is hit when `FieldWriter.checkBalancedBraces(newValue)` returns errors during source editing.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/entryeditor/SourceTab.java[339-350]
### Expected fix
- Convert the `List<String>` to a user-friendly string (e.g., `String.join("\n", errors)`) before passing to `Localization.lang`.
- Keep the returned behavior (early return) unchanged, only fix message formatting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

7. Quadratic brace sanitization🐞 Bug ➹ Performance
Description
FieldWriter.sanitizeUnbalancedBraces calls sanitized.toString() inside a per-character loop and
then scans backward in isEscaped, creating avoidable O(n^2) work on long field values. This can slow
export/save when large fields (e.g., abstracts) are written because FieldWriter.write always
sanitizes braces.
Code

jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[R84-104]

+    public static String sanitizeUnbalancedBraces(String text) {
+        Stack<Integer> openBraces = new Stack<>();
+        StringBuilder sanitized = new StringBuilder(text);
+
+        // Fix unbalanced closing braces
+        for (int i = 0; i < sanitized.length(); i++) {
+            char item = sanitized.charAt(i);
+
+            if (!isEscaped(sanitized.toString(), i)) {
+                if (item == '{') {
+                    openBraces.push(i);
+                } else if (item == '}') {
+                    if (openBraces.isEmpty()) {
+                        sanitized.insert(i, '\\');
+                        i++;
+                    } else {
+                        openBraces.pop();
+                    }
+                }
}
-            String errorMessage = "The following unescaped '{' do not have matching closing bracket:\n%s".formatted(joiner);
-            LOGGER.error(errorMessage);
-            throw new InvalidFieldValueException(errorMessage);
}
Evidence
sanitizeUnbalancedBraces invokes isEscaped(sanitized.toString(), i) for every character, which
allocates a new String repeatedly and adds repeated scanning work. FieldWriter.write calls
sanitizeUnbalancedBraces for all field values, so this cost is paid during writing/exporting
entries.

jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[84-104]
jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[144-157]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`sanitizeUnbalancedBraces` repeatedly calls `sanitized.toString()` inside its main loop, causing repeated allocations and quadratic behavior on longer strings.
### Issue Context
`FieldWriter.write(...)` always calls `sanitizeUnbalancedBraces(content)`, so this runs during export/save for every field.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[84-104]
- jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[121-137]
### Expected fix
- Refactor `isEscaped` to accept a `CharSequence` (or overload it) so it can work directly on `StringBuilder` without converting to String each iteration.
- Update `sanitizeUnbalancedBraces` to call the new overload, eliminating `sanitized.toString()` from the loop while keeping behavior the same.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@koppor
Copy link
Copy Markdown
Member

koppor commented Apr 17, 2026

JBang works locally on branch braces. Thus, just ignore the jbang errors.

"Issue" gradlex-org/extra-java-module-info#237

Longer story: The modified classes are using @ADR, which is loaded for compile time only, not for runtime (in our gradle build configuration - #15194). Since it is a dependency required by jablib - and not by the JBang scripts themselves-

I updated the scripts in this PR at f7b15d3

@calixtus calixtus enabled auto-merge April 17, 2026 11:40
@calixtus calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 17, 2026
Comment thread jablib-examples/jbang/doi_to_bibtex.java Outdated
Comment thread jablib-examples/jbang/ieee_pdf_references_to_bibtex.java Outdated
@Siedlerchr
Copy link
Copy Markdown
Member

Tested with

@Article{1,
  author  = {1},
  journal = {1},
  title   = {1},
  year    = {1},
  pages   = {
  volume  = {1},
}

in source code editor, when pressing cmd + s to save I get two poups after each other

Error occurred when parsing entry: 'Error in line 9: EOF in mid-string'. 

JabRef skipped the entry

@calixtus
Copy link
Copy Markdown
Member Author

calixtus commented Apr 18, 2026

The latter is a separate issue with the source code editor, who is responsible for parsing its contents.
This pr is only concerned with the inner string entered in the field editors.
Fixing the latter issue would escalate this pr way beyond scope.

@calixtus
Copy link
Copy Markdown
Member Author

Sanitizing source editor source is actually covered by #15521 imho

Comment thread jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java Outdated
Comment thread jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java Outdated
Comment thread jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java
Comment thread jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java Outdated
calixtus and others added 2 commits April 19, 2026 09:48
@calixtus
Copy link
Copy Markdown
Member Author

Thx for review

Copy link
Copy Markdown
Member

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

Lgtm!

@calixtus calixtus added this pull request to the merge queue Apr 19, 2026
@github-actions github-actions Bot added the status: to-be-merged PRs which are accepted and should go into the merge-queue. label Apr 19, 2026
Merged via the queue into main with commit bebe6a3 Apr 19, 2026
56 checks passed
@calixtus calixtus deleted the braces branch April 19, 2026 10:29
@calixtus calixtus mentioned this pull request Apr 20, 2026
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: export-or-save component: integrity-checker dev: code-quality Issues related to code or architecture decisions status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers status: to-be-merged PRs which are accepted and should go into the merge-queue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Saving bug after import - braces don't match

4 participants