Improve KeywordList parsing to support escaped characters and nested structures #12888
Conversation
|
@trag-bot didn't find any issues in the code! ✅✨ |
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
|
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 ? |
|
Seems like a false positive from the bot @koppor |
|
@krishnagjsForGit please change the PR title to contain text summarizing the fix. |
krishnagjsForGit
changed the title
Done. is this fine @koppor ? |
| } else if (currentChar == '\\') { | ||
| isEscaping = true; | ||
| } else if (currentChar == delimiter) { | ||
| tokens.add(currentToken.toString().trim()); |
There was a problem hiding this comment.
Maybe we should add a check here to ensure the token isn't empty. This would make the implementation more reliable and error-tolerant. For example, consider a .bib file that contains a token with only whitespace, like , ,. The current implementation would trim the string to an empty one and still add it, which might not be the desired behavior.
| currentToken.append(currentChar); | ||
| isEscaping = false; | ||
| } else if (currentChar == '\\') { | ||
| isEscaping = true; |
There was a problem hiding this comment.
I'm not sure whether a backslash (\) is a valid character in a keyword. The current implementation would remove the backslash, which might not be the intended behavior.
There was a problem hiding this comment.
Solution escaped delimiters and escaped hierarchical delimiters.
| keywordList.add(chainRoot); | ||
| List<String> tokens = splitRespectingEscapes(keywordString, delimiter); | ||
|
|
||
| for (String token : tokens) { |
There was a problem hiding this comment.
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;
}
Closes #12810
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 bug #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
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)