Backslashes in group search are erased from bib file after saving

[douglasrizzo]

JabRef version 5.0

• I have tested the latest development version from http://builds.jabref.org/master/ and the problem persists

Steps to reproduce the behavior:

1. Create a new dynamic group using a free-form search expression.
2. Use backslashes in the search, for example, by using word boundaries in the search (e.g. title = "\bJabRef\b").
3. Save the group.
4. Close JabRef and open it again, loading the library where the aforementioned group was created. At this point, it can already be noticed that search results are not the same as before JabRef was closed.
5. Edit the group. Notice that the backslashes in the search are gone, that is, it is now title = "bJabRefb".
6. If this database is saved, it will be saved without the missing backslashes.

Expected behavior: after loading the database, the group should contain the same entries as when the database was closed.

Additional information: I made sure to disable save actions when testing this.

When I first create my group it looks like this in the database:

1 SearchGroup:JabRef\;1\;title ="\\\\bJabRef\\\\b"\;0\;1\;1\;\;\;\;;

After I close JabRef and reopen it, the backslashes are already missing from my search in the GUI. If I save this database, my group looks like this in the database:

1 SearchGroup:JabRef\;1\;title ="bJabRefb"\;0\;1\;1\;\;\;\;;

In order for JabRef to load my database and include the single backslash I want, I had to include 8 backslashes in my search, like so:

1 SearchGroup:JabRef\;1\;title ="\\\\\\\\bJabRef\\\\\\\\b"\;0\;1\;1\;\;\;\;;

However, if I save my database once, the 8 backslashes become 4. If I open it and save it again, the 4 backslashes become none. I imagine at some point when the database is being loaded ,\\ is being interpreted as a single escaped backslash and is loaded as so.

[Siedlerchr]

Thanks for your report. For anyone interested in fixing this, the code for parsing groups is here.
There should also be a unit test.

jabref/src/main/java/org/jabref/logic/importer/util/GroupsParser.java

Lines 256 to 273 in a051e1b

	private static AbstractGroup searchGroupFromString(String s) {
	if (!s.startsWith(MetadataSerializationConfiguration.SEARCH_GROUP_ID)) {
	throw new IllegalArgumentException("SearchGroup cannot be created from \"" + s + "\".");
	}
	QuotedStringTokenizer tok = new QuotedStringTokenizer(s.substring(MetadataSerializationConfiguration.SEARCH_GROUP_ID.length()),
	MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR, MetadataSerializationConfiguration.GROUP_QUOTE_CHAR);
	String name = tok.nextToken();
	int context = Integer.parseInt(tok.nextToken());
	String expression = tok.nextToken();
	boolean caseSensitive = Integer.parseInt(tok.nextToken()) == 1;
	boolean regExp = Integer.parseInt(tok.nextToken()) == 1;
	// version 0 contained 4 additional booleans to specify search
	// fields; these are ignored now, all fields are always searched
	return new SearchGroup(StringUtil.unquote(name, MetadataSerializationConfiguration.GROUP_QUOTE_CHAR),
	GroupHierarchyType.getByNumberOrDefault(context), StringUtil.unquote(expression, MetadataSerializationConfiguration.GROUP_QUOTE_CHAR), caseSensitive, regExp
	);
	}

[douglasrizzo]

I believe the problem is in the inverse function, the one that transforms a group into a string, no? Because the backslashes disappear when the user saves the database to a file, not when the group is read from the file.

[tobiasdiez]

The serialization happens here:

jabref/src/main/java/org/jabref/logic/exporter/GroupSerializer.java

Lines 63 to 80 in 701e73d

	private String serializeSearchGroup(SearchGroup group) {
	StringBuilder sb = new StringBuilder();
	sb.append(MetadataSerializationConfiguration.SEARCH_GROUP_ID);
	sb.append(StringUtil.quote(group.getName(), MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR, MetadataSerializationConfiguration.GROUP_QUOTE_CHAR));
	sb.append(MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR);
	sb.append(group.getHierarchicalContext().ordinal());
	sb.append(MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR);
	sb.append(StringUtil.quote(group.getSearchExpression(), MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR, MetadataSerializationConfiguration.GROUP_QUOTE_CHAR));
	sb.append(MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR);
	sb.append(StringUtil.booleanToBinaryString(group.isCaseSensitive()));
	sb.append(MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR);
	sb.append(StringUtil.booleanToBinaryString(group.isRegularExpression()));
	sb.append(MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR);
	appendGroupDetails(sb, group);
	return sb.toString();
	}

On a first look, the code looks fine. There are quote/unquote commands at the correct places, which should take care of escaping.

[abepolk]

Hi all,

I am working on this issue.

I have a question about both the code and Git. In the process of looking at this issue, I realized that the create group dialog goes off the screen (so I can’t see the OK buttons no matter what I do). So I just put the mainPane into a ScrollPane so I could scroll down and see the whole dialog. In addition, I am creating a number of log statements instead of the debugger because I want to try out that way of debugging. Finally, I plan on making changes that directly solve the problem.

Since I’m doing more than three things, I was wondering if I should just create a branch for each. I could also submit the ScrollPane fix as a PR. But I need to put both changes on top of each other to be able to create the group and test whatever I put in the logs. I thought I might use rebase, but I don’t want to mess up the commit history. I could also use merge and stash, with a parallel scrollPane branch that continually merges on top of the commits for the main issue.

The git diff for the scrollPane looks like this:

@@ -201,8 +206,10 @@ class GroupDialog extends BaseDialog<AbstractGroup> {
         descriptionPane.setVbarPolicy(ScrollBarPolicy.AS_NEEDED);
 
         // create layout
+        ScrollPane mainScrollPane = new ScrollPane();
+        getDialogPane().setContent(mainScrollPane);
         VBox mainPanel = new VBox(15);
-        getDialogPane().setContent(mainPanel);
+        mainScrollPane.setContent(mainPanel);
         mainPanel.setPadding(new Insets(5, 15, 5, 15));
         mainPanel.getChildren().setAll(
                 generalPanel,

Finally, I created another fix in GroupDialog that removes an error but doesn’t change the issue but solves another problem. I could create a PR for that.

What is the best way to use Git here? Also, should I send a PR for the scrollPane?

[Siedlerchr]

Hi,
thanks for the info. As all is somehow group related you can put in one PR, just make a commit for each fix.
Checkout master
create a branch which fixes the scrollPane issue. Just stage the single file for commit
Push that as PR.
Then we merge this into master and you can pull that changes into your code.

[abepolk]

I'm not sure the ScrollPane is the final fix. It just makes it go from "not working" to "working well enough to use." But I can PR it anyway.

[tobiasdiez]

The issue with the scrollpane should be fixed now in the master as a result of #4660.

[douglasrizzo]

Thanks for looking into the issue. In case a development version implements this, I can test it, as I currently have a ton of groups made with searches containing regex and backslashes.

[abepolk]

I'm still looking into this issue. I think I have found the line that adds the extra quote character \. Besides the original one pointed out by Tobias above, also see line 121 below:

jabref/src/main/java/org/jabref/logic/exporter/MetaDataSerializer.java

Lines 116 to 126 in 6c61664

	private static String serializeGroups(GroupTreeNode root) {
	StringBuilder stringBuilder = new StringBuilder();
	stringBuilder.append(OS.NEWLINE);
	for (String groupNode : new GroupSerializer().serializeTree(root)) {
	stringBuilder.append(StringUtil.quote(groupNode, MetaData.SEPARATOR_STRING, MetaData.ESCAPE_CHARACTER));
	stringBuilder.append(MetaData.SEPARATOR_STRING);
	stringBuilder.append(OS.NEWLINE);
	}
	return stringBuilder.toString();
	}

This quotes it again once the group tree has been serialized into one string. I could remove this, and to keep the class balanced, I might also want to remove the quote call in line 78 below:

jabref/src/main/java/org/jabref/logic/exporter/MetaDataSerializer.java

Lines 73 to 95 in 6c61664

	private static Map<String, String> serializeMetaData(Map<String, List<String>> stringyMetaData) {
	Map<String, String> serializedMetaData = new TreeMap<>();
	for (Map.Entry<String, List<String>> metaItem : stringyMetaData.entrySet()) {
	StringBuilder stringBuilder = new StringBuilder();
	for (String dataItem : metaItem.getValue()) {
	stringBuilder.append(StringUtil.quote(dataItem, MetaData.SEPARATOR_STRING, MetaData.ESCAPE_CHARACTER)).append(MetaData.SEPARATOR_STRING);
	//in case of save actions, add an additional newline after the enabled flag
	if (metaItem.getKey().equals(MetaData.SAVE_ACTIONS)
	&& (FieldFormatterCleanups.ENABLED.equals(dataItem)
	|| FieldFormatterCleanups.DISABLED.equals(dataItem))) {
	stringBuilder.append(OS.NEWLINE);
	}
	}
	String serializedItem = stringBuilder.toString();
	// Only add non-empty values
	if (!serializedItem.isEmpty() && !MetaData.SEPARATOR_STRING.equals(serializedItem)) {
	serializedMetaData.put(metaItem.getKey(), serializedItem);
	}
	}
	return serializedMetaData;
	}

This way, the quoting of both the groups and the other metadata could be done at a more granular level in other classes rather than where the serialized metadata is being aggregated in getSerializedStringMap. I would need to know whether saveOrderConfig.getAsStringList() and saveActions.getAsStringList(OS.NEWLINE)below return lists of Strings with ;. Those classes looked a little complicated, but I doubt they would return something with a ;.

jabref/src/main/java/org/jabref/logic/exporter/MetaDataSerializer.java

Lines 32 to 35 in 6c61664

	metaData.getSaveOrderConfig().ifPresent(
	saveOrderConfig -> stringyMetaData.put(MetaData.SAVE_ORDER_CONFIG, saveOrderConfig.getAsStringList()));
	metaData.getSaveActions().ifPresent(
	saveActions -> stringyMetaData.put(MetaData.SAVE_ACTIONS, saveActions.getAsStringList(OS.NEWLINE)));

What do the save actions and save order configuration represent? Could they contain ;? If not, I am just going to move on and remove those StringUtil.quote calls before working on the rest of this bug.

[Siedlerchr]

Thanks for digging into this! Much appreciated!

The save sort order define in which sort order the entries are exported and stored into the bib file. You can see them in preferences dialog and in library properties. They contain only ascending /descending order and the bib field. So nothing special. Not fully sure but they are stored as key value in the bib file, with semicolon.

Save actions are the same as cleanup. You can automatically run cleanup ops (latex to Unicode for examplel) always on saving entries.

Just configure a simple save action in the library properties to see how they are serialized

[abepolk]

Okay, I will do that soon, and see what happens when I remove the StingUtil.quote calls.

[abepolk]

The library properties dialog goes off the screen for me so I can't click OK. Is there a quick fix for this? My monitor is 1366 x 768.

[douglasrizzo]

Isn't there an OS-specific shortcut which you can use to move a window with the mouse or arrow keys? I believe on Windows and Ubuntu it is Alt + Spacebar, then M.

[abepolk]

I think I found and fixed the problem, which seems to be in class org.jabref.logic.util.strings.QuotedStringExporter.

I found the following. During serialization, it is quoted twice, once when it is being serialized to the group string in class org.jabref.logic.exporter.GroupSerializer, and once when the group string is being combined with other metadata in org.jabref.logic.exporter.MetaDataSerializer, hence 2^2 = 4 quotes in the database after saving. It is also unquoted twice, once when the group string is being separated from the metadata string in method getNextUnit in class org.jabref.logic.importer.MetaDataParser, and again in org.jabref.logic.importer.GroupParser in each of the ...fromString methods using StringUtil.unquote. However, it is also unquoted a third time (between the other two) in QuotedStringTokenizer. QuotedStringTokenizer splits the group into parts, which are then given back to GroupsParser and unquoted using StringUtil.unquote. The change I made allows QuotedStringTokenizer only to split the parts of the group string, rather than also unquoting them.

I will put this into a PR. @douglasrizzo , feel free to test this more thoroughly and let me know if it breaks anything else.

[douglasrizzo]

OK, just a small clarification. In order to test this, I have to download @NorwayMaple 's fork, checkout branch fix_escape and build JabRef using Gradle, correct? Do I need to wait for the pull request to pass the checks or can I try it right now?

[abepolk]

You can do that. I can also just add StringUtil.unquote in a couple of places and it will pass the checks. I'm only 75% sure that that is the right answer, and I was hoping to do a little bit more investigation just to be sure.

[Siedlerchr]

@douglasrizzo Yes, you can simply run ./gradlew run in the code. You need jdk8 installed.

[tobiasdiez]

You can also wait until @NorwayMaple fixed the tests, then the build can be found under http://builds.jabref.org/ 😉

[Siedlerchr]

Thanks to @NorwayMaple this should be fixed now.
Please use the latest development version from https://builds.jabref.org

[douglasrizzo]

I have tested with my database, which contains quite a few backslashes in group searches, and I can confirm that this is fixed for me. Thank you everyone, great work.

[douglasrizzo]

OK, one last question. The only version of JabRef that has this bugfix is version 5, correct? More specifically, it's present in all the development snapshots after this PR was accepted, right? These fixes are not applied to version 4.3.1?

[tobiasdiez]

Right on all points. We don't have the resources to backport fixes to older versions. Usually, we try to release more often, the longer cycle for 5.0 comes from the many changes that were made.