Fix not escaping special characters in search pattern #5938

dawidm · 2020-02-11T23:12:47Z

The error when searching for "DOI 10.1210/endrev/bnz006" (or any phrases containing special javascript regular expression characters) is caused by not escaping special characters before passing search pattern to javascript script used for highlighting words in preview view.

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for bigger UI changes)
Checked documentation: Is the information available and up to date? If not: Issue created at https://github.com/JabRef/user-documentation/issues.

fixes JabRef#5892 * add method to get search pattern for searched words with escaped javascript regexp special characters (for search without regular expressions) * in preview viewer use search pattern with escaped javascript regexp special characters

Siedlerchr · 2020-02-12T07:45:49Z

src/main/java/org/jabref/logic/search/SearchQuery.java

+    /* Returns a regular expression pattern in the form (w1)|(w2)| ... wi are escaped if no regular expression search is enabled
+     * @param escapeSpecialCharsForJS whether to escape characters in wi for javascript regexp (escaping all special characters) or for java (using \Q and \E)
+     */
+    private Optional<Pattern> joinWordsToPattern(boolean escapeSpecialCharsForJS) {


Better is to use an enum here, eg.. EscapMode, with java and javascript as values:
https://www.teamten.com/lawrence/programming/prefer-enums-over-booleans.html

Siedlerchr

Thanks for your contribution! In general the fix looks good to me, just some minor code improvements!

Siedlerchr · 2020-02-12T07:47:03Z

src/main/java/org/jabref/logic/search/SearchQuery.java

@@ -18,6 +18,9 @@

 public class SearchQuery implements SearchMatcher {

+    // regexp pattern for escaping special characters in javascript regex
+    public static final String JAVASCRIPT_ESCAPED_CHARS_PATTERN = "[\\.\\*\\+\\?\\^\\$\\{\\}\\(\\)\\|\\[\\]\\\\/]";


You can use Pattern.compile to gain some performace improvements here

Siedlerchr · 2020-02-12T07:47:41Z

src/main/java/org/jabref/logic/search/SearchQuery.java

@@ -133,7 +148,7 @@ public boolean isRegularExpression() {
        // compile the words to a regular expression in the form (w1)|(w2)|(w3)
        StringJoiner joiner = new StringJoiner(")|(", "(", ")");
        for (String word : words) {
-            joiner.add(regularExpression ? word : Pattern.quote(word));
+            joiner.add(regularExpression ? word : (escapeSpecialCharsForJS ? word.replaceAll(JAVASCRIPT_ESCAPED_CHARS_PATTERN, "\\\\$0") : Pattern.quote(word)));


Please extract this to a regular if-else, it's easier to understand on the first look than this chained conditions

* use enum to specify special characters escape mode * use compiled regex pattern instead of string

Siedlerchr

Thanks! Looks good to me!
For external contributions we have the policy, that a second dev looks over your code before it's merged.

calixtus

Got two comments, everything else looks good to me.
A test would be great...

calixtus · 2020-02-12T15:16:28Z

src/main/java/org/jabref/logic/search/SearchQuery.java

+            if (regularExpression) {
+                joiner.add(word);
+            }
+            else {


Isn't checkstyle complaining about putting this in two lines? Has no effect, but looks somewhat odd...

Yeah, it's a mistake, I'll change it. Also looks odd to me. Checkstyle wasn't complaining.

calixtus · 2020-02-12T15:25:38Z

src/main/java/org/jabref/logic/search/SearchQuery.java

+    }
+
+    // Returns a regular expression pattern in the form (w1)|(w2)| ... wi are escaped for javascript if no regular expression search is enabled
+    public Optional<Pattern> getJsPatternForWords() {


IMHO, although "Js" is quite obvious, I would reword that to getJavaScriptPatternForWords, since you are creating a new Method just for the sole purpose to make its use more obvious in it's name. So why not go all the way?

stefan-kolb · 2020-02-12T18:35:49Z

src/main/java/org/jabref/logic/search/SearchQuery.java

+    public static final Pattern JAVASCRIPT_ESCAPED_CHARS_PATTERN = Pattern.compile("[\\.\\*\\+\\?\\^\\$\\{\\}\\(\\)\\|\\[\\]\\\\/]");
+
+    /**
+     * Metod for escaping special characters in regular expressions


dawidm · 2020-02-12T22:10:39Z

A test would be great...

Ok, I will write a test.

dawidm · 2020-02-14T16:14:55Z

src/test/java/org/jabref/logic/search/SearchQueryTest.java

+        //first word contain all javascript special regex characters that should be escaped individually in text based search
+        String queryText = "([{\\\\^$|]})?*+./ word1 word2.";
+        SearchQuery textQueryWithSpecialChars = new SearchQuery(queryText, false, false);
+        String pattern = "(\\(\\[\\{\\\\\\^\\$\\|\\]\\}\\)\\?\\*\\+\\.\\/)|(word1)|(word2\\.)";


I've created tests but I'm not sure about readbility of this approach.

I know this looks totally crazy with those escaping, but it's fine.

Siedlerchr

Thanks again for creating the tests as well!

calixtus · 2020-02-14T19:10:58Z

I think this is really great as well! Thank you!

* upstream/master: followup fix Fixfetcher (#5948) Bump byte-buddy-parent from 1.10.7 to 1.10.8 (#5952) Added MenuButtons to IntegrityCheckDialog (#5955) Reimplement custom entry types dialog (#5799) Bump unirest-java from 3.4.03 to 3.5.00 (#5953) MySQL: Allow public key retrieval (#5909) Restructure and improve docs for setting up IntelliJ (#5960) Change syntax for Oracle multi-row insert SQL statement (#5837) Bump classgraph from 4.8.62 to 4.8.64 (#5954) Squashed 'src/main/resources/csl-styles/' changes from c531528..9e81857 Fix not escaping special characters in search pattern (#5938)

* upstream/master: Bump classgraph from 4.8.62 to 4.8.64 (#5954) Squashed 'src/main/resources/csl-styles/' changes from c531528..9e81857 Fix not escaping special characters in search pattern (#5938) fix missing gson

dawidm marked this pull request as ready for review February 11, 2020 23:13

Siedlerchr reviewed Feb 12, 2020

View reviewed changes

Siedlerchr requested changes Feb 12, 2020

View reviewed changes

Refactoring and performance improvement

5c2925d

* use enum to specify special characters escape mode * use compiled regex pattern instead of string

dawidm requested a review from Siedlerchr February 12, 2020 14:11

Refactoring: braces in if..else

89c72c0

Siedlerchr approved these changes Feb 12, 2020

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 12, 2020

calixtus reviewed Feb 12, 2020

View reviewed changes

stefan-kolb reviewed Feb 12, 2020

View reviewed changes

Refactoring, minor changes: names, comments

6f39cbc

Add tests of escaping special characters in search patterns

266bfc8

dawidm commented Feb 14, 2020

View reviewed changes

dawidm requested review from calixtus and Siedlerchr February 14, 2020 16:16

Siedlerchr approved these changes Feb 14, 2020

View reviewed changes

calixtus approved these changes Feb 14, 2020

View reviewed changes

Siedlerchr merged commit ec93ad3 into JabRef:master Feb 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix not escaping special characters in search pattern #5938

Fix not escaping special characters in search pattern #5938

dawidm commented Feb 11, 2020 •

edited

Loading

Siedlerchr Feb 12, 2020

Siedlerchr left a comment

Siedlerchr Feb 12, 2020

Siedlerchr Feb 12, 2020

Siedlerchr left a comment

calixtus left a comment

calixtus Feb 12, 2020

dawidm Feb 12, 2020

calixtus Feb 12, 2020

stefan-kolb Feb 12, 2020

dawidm commented Feb 12, 2020

dawidm Feb 14, 2020

Siedlerchr Feb 14, 2020 •

edited

Loading

Siedlerchr left a comment

calixtus commented Feb 14, 2020

Fix not escaping special characters in search pattern #5938

Fix not escaping special characters in search pattern #5938

Conversation

dawidm commented Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

calixtus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidm commented Feb 12, 2020

Choose a reason for hiding this comment

Siedlerchr Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

calixtus commented Feb 14, 2020

dawidm commented Feb 11, 2020 •

edited

Loading

Siedlerchr Feb 14, 2020 •

edited

Loading