Improve KeywordList parsing to support escaped characters and nested structures #12888

krishnagjsForGit · 2025-04-06T03:00:57Z

What I changed
Implemented escape handling in KeywordList.parse(String, Character, Character) to support escaping the keyword separator using backslash (). This prevents incorrect splitting of keywords when delimiters appear within the keyword itself.

Refactored the parsing logic for clarity, including renaming loop variables for better readability and intent.

Added test cases in KeywordListTest to cover escape scenarios such as:

Escaped delimiter: "one\,two" → ["one,two"]

Escaped backslash: "one\\two" → ["one\two"]

Mixed escaped and unescaped delimiters.

Where the changes are
KeywordList.java: Modified the parse method to include escape handling logic using a character-by-character loop.

KeywordListTest.java: Added JUnit test cases to ensure parsing behaves correctly with escaped delimiters and backslashes.

Why I made these changes
Fixes issue #12810: Current parsing breaks when delimiters appear inside keywords (e.g., in MeSH terms). There was no way to escape the delimiter character, leading to incorrect keyword splitting.

Improves consistency: Provides a more robust, predictable behavior for keyword parsing, especially when users or importers use delimiters in keyword values.

Next Steps
Awaiting review and feedback.

If the community prefers a different escape character or behavior, I’m happy to adjust the implementation.

After this is merged, similar logic could be extracted or reused where keyword parsing happens elsewhere (e.g., importers).

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
[/] Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
[/] Screenshots added in PR description (if change is visible to the user)
[/] Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

src/test/java/org/jabref/model/entry/KeywordListTest.java

krishnagjsForGit · 2025-04-06T04:03:33Z

can someone help me on this. is this a valid comment from Bot ? I have never used DisplayName annotation and I changed method name to be more comprehensive. Am I missing anything here ?

Siedlerchr · 2025-04-06T08:05:35Z

Seems like a false positive from the bot @koppor

koppor · 2025-04-06T08:27:16Z

@krishnagjsForGit please change the PR title to contain text summarizing the fix.

krishnagjsForGit · 2025-04-06T10:55:16Z

@krishnagjsForGit please change the PR title to contain text summarizing the fix.

Done. is this fine @koppor ?

src/main/java/org/jabref/model/entry/KeywordList.java

ungerts · 2025-04-06T20:15:25Z

src/main/java/org/jabref/model/entry/KeywordList.java

-            keywordList.add(chainRoot);
+        List<String> tokens = splitRespectingEscapes(keywordString, delimiter);
+
+        for (String token : tokens) {


Maybe there's an opportunity to eliminate this loop. In the _splitRespectingEscapes_ method, all tokens are already available and accessible. It feels inefficient to first collect all tokens in a List and then iterate over it.

Sketch:

public static KeywordList parse(String keywordString, Character delimiter, Character hierarchicalDelimiter) { if (StringUtil.isBlank(keywordString)) { return new KeywordList(); } Objects.requireNonNull(delimiter); Objects.requireNonNull(hierarchicalDelimiter); KeywordList keywordList = new KeywordList(); List<String> hierarchy = new ArrayList<>(); StringBuilder currentToken = new StringBuilder(); boolean isEscaping = false; for (int i = 0; i < keywordString.length(); i++) { char currentChar = keywordString.charAt(i); if (isEscaping) { currentToken.append(currentChar); isEscaping = false; } else if (currentChar == '\\') { isEscaping = true; } else if (currentChar == hierarchicalDelimiter) { hierarchy.add(currentToken.toString().trim()); currentToken.setLength(0); } else if (currentChar == delimiter) { hierarchy.add(currentToken.toString().trim()); keywordList.add(Keyword.of(hierarchy.toArray(new String[0]))); hierarchy.clear(); currentToken.setLength(0); } else { currentToken.append(currentChar); } } // Handle final token if (currentToken.length() > 0 || !hierarchy.isEmpty()) { hierarchy.add(currentToken.toString().trim()); keywordList.add(Keyword.of(hierarchy.toArray(new String[0]))); } return keywordList; }

@krishnagjsForGit I see this marked as resolved. Was this discussed further?

(I see this loop still present)

koppor · 2025-04-16T14:56:23Z

src/main/java/org/jabref/model/entry/KeywordList.java

-            String chain = tok.nextToken();
-            Keyword chainRoot = Keyword.of(chain.split(hierarchicalDelimiter.toString()));
-            keywordList.add(chainRoot);
+        List<String> tokens = splitRespectingEscapes(keywordString, delimiter);


@Yubo-Cao What do you think about this? Maybe an ANTLR grammar would help here, too?

Yubo-Cao · 2025-04-20T20:03:11Z

I'm sorry for not getting back to you sooner. I have two suggestions:

ANTLR probably won't help in this use case, since the delimiters are dynamically specified
I am concerned regarding the Unicode handling of the code, since Java char is finicky, and potentially you want to use codepoints instead of .charAt
One way to address 2 and increase readability without necessarily using ANTLR is to dynamically construct a Regular Expression with a lookbehind and use only .split and .replace. Take a look at this: https://regex101.com/r/O1kLWF/1

krishnagjsForGit · 2025-05-01T06:55:49Z

I'm sorry for not getting back to you sooner. I have two suggestions:

ANTLR probably won't help in this use case, since the delimiters are dynamically specified

I am concerned regarding the Unicode handling of the code, since Java char is finicky, and potentially you want to use codepoints instead of .charAt

One way to address 2 and increase readability without necessarily using ANTLR is to dynamically construct a Regular Expression with a lookbehind and use only .split and .replace. Take a look at this: https://regex101.com/r/O1kLWF/1

@koppor you want me to change the code accordingly ?

koppor · 2025-05-01T07:10:54Z

I think the current code is good.

Is a result of current JabRefs way of parsing. If it was done "correctly", the Bib parser would do the escape handling - and all other data structures would use tokens. But this is much much effort to change.

koppor · 2025-05-01T07:11:18Z

@krishnagjsForGit please add a CHANGELOG.md entry

subhramit · 2025-05-02T09:37:44Z

@ungerts thank you for helping with reviews!

koppor

Changes requested at #12888 (comment) need to be addressed.

krishnagjsForGit · 2025-05-02T17:12:26Z

@krishnagjsForGit please add a CHANGELOG.md entry

Can you please check if I updated correctly ?

subhramit · 2025-05-02T17:20:08Z

Seems like it was not a clean merge.
@koppor may be able to help with some git trick
Otherwise, you'll have to git reset and push a fresh commit.

subhramit · 2025-05-02T17:21:06Z

How did you merge, by the way?
Did you do git pull upstream main?

koppor · 2025-05-02T22:22:09Z

Seems like it was not a clean merge. @koppor may be able to help with some git trick Otherwise, you'll have to git reset and push a fresh commit.

Just checking the output of GitHub here, I also suggest git reset origin/main after a git merge origin/main. - A force push afterwards is OK. Please use gitk --all& as a double check - and run this command before any action and leave the window open. You can press F5 to refresh.

koppor · 2025-05-02T22:22:48Z

Alternatively, one could work with git cherry-pick on a new branch - and just cherry-pick the "good" commits.

trag-bot · 2025-05-03T03:45:12Z

jabgui/src/main/java/org/jabref/gui/frame/JabRefFrame.java

@@ -2,6 +2,7 @@

 import java.io.IOException;
 import java.nio.file.Path;
+import java.util.Collections;


The import statement for Collections is unnecessary because the patch replaces its usage with List.of(), which is preferred in JabRef for creating empty lists.

trag-bot · 2025-05-03T03:45:14Z

jabgui/src/main/java/org/jabref/gui/frame/JabRefFrame.java

@@ -431,7 +432,7 @@ private void initBindings() {
                // All databases are closed or an unknown tab is selected
                stateManager.setActiveDatabase(null);
                stateManager.activeTabProperty().set(Optional.empty());
-                stateManager.setSelectedEntries(List.of());
+                stateManager.setSelectedEntries(Collections.emptyList());


In JabRef, to create an empty list, List.of() should be used instead of Collections.emptyList() for consistency with the project's coding standards.

trag-bot · 2025-05-03T03:50:34Z

jabgui/src/main/java/org/jabref/gui/shared/SharedDatabaseUIManager.java

@@ -151,7 +151,7 @@ public void listen(SharedEntriesNotPresentEvent event) {
                            + "\n"
                            + Localization.lang("You can restore the entry using the \"Undo\" operation."));

-            stateManager.setSelectedEntries(List.of());
+            stateManager.setSelectedEntries(Collections.emptyList());


In JabRef, to create an empty list we use List.of() instead of Collections.emptyList(). This aligns with the project's conventions and ensures consistency across the codebase.

krishnagjsForGit · 2025-05-03T03:51:30Z

Something went amiss and PR got closed. Can I create a separate Clean PR and fix this ? @koppor

trag-bot · 2025-05-03T03:51:32Z

jabgui/src/test/java/org/jabref/gui/entryeditor/SourceTabTest.java

@@ -51,7 +53,7 @@ public void onStart(Stage stage) {
        when(stateManager.activeSearchQuery(SearchType.NORMAL_SEARCH)).thenReturn(OptionalObjectProperty.empty());
        when(stateManager.searchQueryProperty()).thenReturn(mock(StringProperty.class));
        when(stateManager.activeTabProperty()).thenReturn(OptionalObjectProperty.empty());
-        KeyBindingRepository keyBindingRepository = new KeyBindingRepository(List.of(), List.of());
+        KeyBindingRepository keyBindingRepository = new KeyBindingRepository(Collections.emptyList(), Collections.emptyList());


In JabRef, to create an empty list, List.of() should be used instead of Collections.emptyList() to adhere to the project's coding standards.

trag-bot · 2025-05-03T03:52:37Z

jabkit/src/main/java/org/jabref/cli/LtwaListMvGenerator.java

+    private static Path downloadLtwaFile() throws IOException, URISyntaxException {
+        LOGGER.info("Downloading LTWA file from {}.", LtwaListMvGenerator.LTWA_URL);
+        var in = new URI(LTWA_URL).toURL().openStream();
+        var path = Files.writeString(


The use of 'Files.createTempFile' should be replaced with '@tempdir' JUnit5 annotation for better test management and cleanup.

trag-bot · 2025-05-03T03:52:39Z

jabkit/src/main/java/org/jabref/cli/LtwaListMvGenerator.java

+                                      if (word.startsWith("-")) {
+                                          String key = word.substring(1);
+                                          suffixMap.computeIfAbsent(key, k ->
+                                                  Stream.<LtwaEntry>builder().build().collect(Collectors.toList())


The use of 'collect(Collectors.toList())' should be replaced with 'toList()' for simplicity and modern Java practices.

trag-bot · 2025-05-03T03:53:38Z

jablib/src/jmh/java/org/jabref/benchmarks/Benchmarks.java

@@ -80,7 +82,7 @@ private StringWriter getOutputWriter() throws IOException {
        StringWriter outputWriter = new StringWriter();
        BibWriter bibWriter = new BibWriter(outputWriter, OS.NEWLINE);
        SelfContainedSaveConfiguration saveConfiguration = new SelfContainedSaveConfiguration(SaveOrder.getDefaultSaveOrder(), false, BibDatabaseWriter.SaveType.WITH_JABREF_META_DATA, false);
-        FieldPreferences fieldPreferences = new FieldPreferences(true, List.of(), List.of());
+        FieldPreferences fieldPreferences = new FieldPreferences(true, Collections.emptyList(), Collections.emptyList());


Use List.of() instead of Collections.emptyList() for creating empty lists, as per the project's conventions.

koppor · 2025-05-03T07:28:39Z

Something went amiss and PR got closed. Can I create a separate Clean PR and fix this ? @koppor

Yes, please.

trag-bot · 2025-05-09T21:29:41Z

@trag-bot didn't find any issues in the code! ✅✨

koppor · 2025-05-09T21:30:03Z

I restored the state as of April, 16th - commit 081b9dc. Mayb,e this helps.

If you do a git fetch origin and then git reset --hard origin/fix-for-issue-12810 you should be on the same page.

Attention: The command git reset --hard .... throws away all your work you did inbetween. Not sure if you did something...

jabref-machine · 2025-05-09T21:36:28Z

Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Tests / Checkstyle (pull_request)" and click on it.

In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues, commit, and push.

jabref-machine · 2025-05-09T21:36:29Z

Hey, we noticed that you force-pushed your changes. Force pushing is a bad practice when working together on a project (mainly because it is not supported well by GitHub itself). Commits are lost and comments on commits lose their context, thus making it harder to review changes. At the end, all commits will be squashed anyway before being merged into the main branch.

In future, please avoid that. For now, you can continue working.

Krishna Kumar Parthasarathy added 7 commits April 6, 2025 08:19

Fix for issue JabRef#12810

ddffe9b

Remove NotNull Import

9997f30

Update test names

eaace58

Fix Styling Issues

664b93c

Update test names

9c00761

Update test names

de3a84b

Update test names

923acfc