Fix 2701 too may files found #2732

sauliusg · 2017-04-12T18:45:47Z

I would like to offer my fix for the issue 2701: JabRef would find too many files when they are search with the [year]_[auth].* pattern. The reason seems to be a 'null' database passed to the BibtexKeyPatternUtil.makeLabel(BibEntry, String, Character, BibDatabase) method, and a null pointer exception is triggered. As a result, the '[auth]' part of the pattern was erased and ignored. With the suggested fix, JabRef uses the '[auth]' part of the pattern and finds just the files with the matching author, as expected. A relevant unit test was added; no other tests were broken.

Change in CHANGELOG.md described -- no functionality changes, restored the previous state
[x ] Tests created for changes -- 1 unit test added;
Screenshots added (for bigger UI changes) -- no GUI changes;
Manually tested changed features in running JabRef -- none
[x ] Check documentation status (Issue created for outdated help page at help.jabref.org?) -- the issue "Get fulltext" finds too many files when using regexp **/[year]_[auth].*\\.[extension] #2701 still open.
If you changed the localization: Did you run gradle localizationUpdate? -- no localisation changes.

-- this fixes incorrect match of multiple files caused by a (masked) "null pointer exception".

Siedlerchr · 2017-04-12T18:50:24Z

Thank you very much for your contribution 👍 . Codewise it looks very good to me. If the test passes and another dev gives his okay, we can merge it.

LinusDietz · 2017-04-12T23:26:21Z

Thanks for the contribution @sauliusg . Great, when users find an issue and even fix it themselves (;

The reason seems to be a 'null' database passed to the BibtexKeyPatternUtil.makeLabel(BibEntry, String, Character, BibDatabase) method, and a null pointer exception is triggered

I will investigate this tomorrow, because I'm not entirely sure if your fix solves the symptoms or we should dig a bit deeper and eliminate the main problem, which could be that bloody 'null' database.

second 'if' branch in the 2701 fix.

sauliusg · 2017-04-13T05:50:29Z

Dear Siedlerchr, dear Lynyus,

thank you for the instant reply!

I have got feedback from Codacy and fixed: removed an unused variable from the new unit test. Also, I added another second unit test to cover the second 'if' statement added by me. Sorry if it breaks the "changes approved" status...

I will investigate this tomorrow, because I'm not entirely sure if your fix solves the symptoms or we
should dig a bit deeper and eliminate the main problem, which could be that bloody 'null' database.

Please have a look. It seems to me that the 'null' can be traced to the explicit 'null' argument passed to it from the 'private static List findFile(BibEntry entry, File directory, String file, String extensionRegExp, Character keywordDelimiter)' method (line 250, String filenameToLookFor = expandBrackets(filePart, entry, null, keywordDelimiter).replaceAll(EXT_MARKER, extensionRegExp);).

Regards,
Saulius

LinusDietz

I would say, that the call structure of this functionality should be refactored.

There are two calls in https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/logic/util/io/RegExpFileSearch.java
that use null as argument for the database in expandBrackets(). (Line 195 and 250)

I would argue that we thus need two expandBrackets() methods, one that uses a database argument, and one that doesn't.
Naturally, this cascades and several called methods have to be changed as well, but it would improve the code w.r.t. extensibility and we would not need to write such workarounds as @sauliusg did. (Which indeed would fix the problem, though)

@tobiasdiez What do you think about this?

tobiasdiez · 2017-04-13T15:10:40Z

@lynyus your suggestions sounds reasonable. We should try to not pass null as an argument. Moreover, the label generation code probably needs a complete refactoring (especially, get rid of the huge amount of static methods). But this will be probably a bigger project. While I would welcome PRs from you or @sauliusg in this direction, I think the current PR is an acceptable workaround within the boundaries of the current code infrastructure.

tobiasdiez

Just two minor remarks and then this can be merged in my opinion.

tobiasdiez · 2017-04-13T15:12:21Z

src/test/java/org/jabref/logic/bibtexkeypattern/MakeLabelWithoutDatabaseTest.java

+    @Test
+    public void makeLabelForFileSearch() {
+        String label = BibtexKeyPatternUtil.makeLabel(entry,
+                /*value=*/ "auth",


I think we never used comments to indicate the name of the argument and thus would prefer that you remove them.

Hmmm, indicating “Named Function Parameter” Comments is a recommended practice from the Dustin Boswell, Trevor Foucher "The Art of Readable Code", for languages that do not support named parameters. I personally find such code easier to read. But if this is not used on JabRef and you do not find them useful, then, sure, please just remove them. Should I make the requested changes and recommit, or will you handle these issues?

Yes please, IntelliJ automatically displays the parameter names.

OK, both issues fixed, please find a new push.

tobiasdiez · 2017-04-13T15:12:42Z

src/test/java/org/jabref/logic/bibtexkeypattern/MakeLabelWithoutDatabaseTest.java

+
+public class MakeLabelWithoutDatabaseTest {
+
+    // private GlobalBibtexKeyPattern pattern;


Use remove the unused variable completely.

Agreed, this var will probably never be needed.

Obsolete, cf #2732 (comment)

MakeLabelWithoutDatabaseTest.java, since IntelliJ handles them automagically.

Siedlerchr · 2017-04-13T18:11:15Z

Okay, lgtm! I merge it in! Thank you very much for your contribution! 👍 Feel free to take a look at other issues

sauliusg · 2017-04-13T18:14:28Z

Thanks!

* upstream/master: Speedup start by improving Application Insights configuration (#2737) Localization: French: Menu: Translation of new entries (#2735) Localization: French: General: Translation of new entries (#2736) Update Chinese translation. (#2734) Fix 2701 too may files found (#2732) Update errorprone from 0.0.8 to 0.0.10, because of update of Gradle to 3.5 Update gradle from 3.4.1 to 3.5

Saulius Gražulis added 4 commits April 11, 2017 21:44

Splitting up several variables for debugging.

1cd40ff

Checking if the 'database' value is not 'null' before invokig a method

cc7045b

-- this fixes incorrect match of multiple files caused by a (masked) "null pointer exception".

Restoring the previous (more compact) code style.

c34440f

Adding a unit test for the new fix.

4936cd1

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

Siedlerchr approved these changes Apr 12, 2017

View reviewed changes

LinusDietz self-assigned this Apr 12, 2017

LinusDietz added [outdated] type: bug Confirmed bugs or reports that are very likely to be bugs component: search labels Apr 12, 2017

Saulius Gražulis added 2 commits April 13, 2017 08:30

Removing unused member 'private String patternString'.

e81a689

Adding another test to MakeLabelWithoutDatabaseTest.java, to cover the

7897e51

second 'if' branch in the 2701 fix.

LinusDietz previously requested changes Apr 13, 2017

View reviewed changes

tobiasdiez requested changes Apr 13, 2017

View reviewed changes

Saulius Gražulis and others added 2 commits April 13, 2017 20:39

Removing parameter annotation comments from the tests in

d729e79

MakeLabelWithoutDatabaseTest.java, since IntelliJ handles them automagically.

Removing unused commented-out class member.

ed5e96d

Siedlerchr merged commit e7fc94c into JabRef:master Apr 13, 2017

sauliusg deleted the fix-2701-too-may-files-found branch April 13, 2017 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 2701 too may files found #2732

Fix 2701 too may files found #2732

sauliusg commented Apr 12, 2017

Siedlerchr commented Apr 12, 2017

LinusDietz commented Apr 12, 2017

sauliusg commented Apr 13, 2017 •

edited

Loading

LinusDietz left a comment •

edited

Loading

tobiasdiez commented Apr 13, 2017 •

edited

Loading

tobiasdiez left a comment

tobiasdiez Apr 13, 2017

sauliusg Apr 13, 2017 •

edited

Loading

LinusDietz Apr 13, 2017 •

edited

Loading

sauliusg Apr 13, 2017 •

edited

Loading

tobiasdiez Apr 13, 2017

sauliusg Apr 13, 2017

Siedlerchr commented Apr 13, 2017

sauliusg commented Apr 13, 2017


		public class MakeLabelWithoutDatabaseTest {

		// private GlobalBibtexKeyPattern pattern;

Fix 2701 too may files found #2732

Fix 2701 too may files found #2732

Conversation

sauliusg commented Apr 12, 2017

Siedlerchr commented Apr 12, 2017

LinusDietz commented Apr 12, 2017

sauliusg commented Apr 13, 2017 • edited Loading

LinusDietz left a comment • edited Loading

Choose a reason for hiding this comment

tobiasdiez commented Apr 13, 2017 • edited Loading

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez Apr 13, 2017

Choose a reason for hiding this comment

sauliusg Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

LinusDietz Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

sauliusg Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

tobiasdiez Apr 13, 2017

Choose a reason for hiding this comment

sauliusg Apr 13, 2017

Choose a reason for hiding this comment

Siedlerchr commented Apr 13, 2017

sauliusg commented Apr 13, 2017

sauliusg commented Apr 13, 2017 •

edited

Loading

LinusDietz left a comment •

edited

Loading

tobiasdiez commented Apr 13, 2017 •

edited

Loading

sauliusg Apr 13, 2017 •

edited

Loading

LinusDietz Apr 13, 2017 •

edited

Loading

sauliusg Apr 13, 2017 •

edited

Loading