Fix #2739 null pointer exception on get fulltext #2744

sauliusg · 2017-04-15T18:45:35Z

Fixing issue #2739: a check in the BibDatabaseContext.getFileDirectories(String, FileDirectoryPreferences) method is added (BibDatabaseContext.java:203) to make sure that "dbPath.getParent()" is not null before its toAbsolutePath() method is called. This fixes 'null pointer exception' when searching files for databases opened without explicit path, as in the CLI 'jabref bibliography.bib' command.

Asserts were added to make sure that dbPath.getParent() and other variables are not null before invoking methods.

Change in CHANGELOG.md described -- internal bug fix, no functionality added -- CHANGELOG not changed.
Tests created for changes -- 1 test added to cover new code; 3 regression tests added to make sure old functionality works as before.
Screenshots added (for bigger UI changes) -- no UI changes
Manually tested changed features in running JabRef -- manually tested on the LinuxMint 18.1 installation from the build/distributions/JabRef-4.0.0-dev.tar file.
Check documentation status (Issue created for outdated help page at help.jabref.org?) -- help pages need not be changed,
If you changed the localization: Did you run gradle localizationUpdate? -- no localisation changes.

'null pointer exception'.

pointer exception' when a databse is opened in a current directory.

re-create the necessary run environment.

also three regression tests for the old functionality.

the previous bug. Indeed, the new unit test fails both in Eclipse and in the CLI './gradlew build' run.

pointer exception' if a database is passed as a local file without explicite path. The tests in 'BibDatabaseContextTest.java' pass again.

'BibDatabaseContextTest'.

tobiasdiez

Thank for your PR. I've no experience with the file/path - related code, so I can't say anything about the actual fix (but it looks good in principle). So I just made a few comments regarding the test code.
A big thumps up that you create tests for your fixes. This is really appreciated! I look forward to more PRs from you 😄

tobiasdiez · 2017-04-15T20:57:37Z

src/main/java/org/jabref/model/database/BibDatabaseContext.java

+            assert dbPath != null : "dbPath is null";
+            Path parentPath = dbPath.getParent();
+            if( parentPath == null ) {
+                parentPath = Paths.get(System.getProperty("user.dir"));


I'm not sure about the fallback to the user directory and would have just encapsulated everything in an if (parentPath != null) statemant. @Siedlerchr what do you think?

Not sure whether that helps: The directory lookup is explained at #459 (comment). The addition to help is still a TODO (https://github.com/JabRef/help.jabref.org/issues/160).

I also wait for @Siedlerchr s feedback 😇

Hi, folks, thank you for the comments!
I think the
if (parentPath != null){parentPath = Paths.get(System.getProperty("user.dir"));}

fix is essential. It expresses a usual Unix/Windows CLI convention that invoking "jabref biblio.bib" is actually equivalent to "jabref ./biblio.bib" and "jabref $(pwd)/biblio.bib". Without this fix, running "jabref biblio.bib" and clicking the "Get fulltext" button in the "General" tab throws a "null pointer exception" in the searcher thread, and the main GUI thread just "hangs" forever without yielding any results (and without any error messages...). IMHO, this is a show stopper.

With the suggested fix, JabRef behaves as most users would expect -- it reads 'biblio.bib' in the current directory, and, if a database directory has to be used as a base for searching files, it uses the same current directory as a 'biblio.bib' base. Please note that when the directory is specified explicitely, as in 'jabref ~/papers/bibliography.bib', this workaround is not used.

The behaviour of the fix is totally orthogonal to the policies in #459 (comment): if any other policy of determining a search directory is used, the fix is inactive; also, when a database has explicit directory name, it is inactive as well. It is only when the directory is not specified that I suggest using System.getProperty("user.dir"); BTW, this seems to be the same policy as used when opening a database for the command 'jabref biblio.bib'.

tobiasdiez · 2017-04-15T21:00:13Z

src/test/java/org/jabref/model/database/BibDatabaseContextTest.java

+    public void setUp() {
+        Map<String, String> mapFieldDirs = new HashMap<>();
+        preferences = new FileDirectoryPreferences("saulius", mapFieldDirs, true);
+        currentWorkingDir = Paths.get(System.getProperty("user.dir")).toString();


I think it is preferable to use temporary folder rule of junit here, so that the test files are automatically deleted afterwards.

The directory named in the 'currentWorkingDir' value is not intended to hold any created files; actually, these tests should not create any files (I'll double check that).

The 'currentWorkingDir' variable is used to re-create the part of the JabRef internal state that we get when the 'jabref biblio.bib' command is invoked from a command line (on Unix/Linux, but I guess on Windows clones as well). Since the directory is different on various computers that invoke the test, I can not hard-code it into the test but must determine it at run-time.

Should I comment this in the code, or find a better name for the 'currentWorkingDir' variable?

+1 for commenting in the code.

Checked: the tests do not leave the 'biblio.bib' file behind. The File object, as I understand, holds an abstract system-independent path to a file, but does not open or create the file unless exlicitely requested.

+1 for commenting in the code.

Commenting the use of the variables in the test.

You can influence the fileDirectoryPreferences by mocking them, otherwise you always depend on the the settings of the user and can't have a reliable value to test.
In the CleanupTests for Renaming/Moving Files I mocked the relevant options like this:
fileDirPrefs = mock(FileDirectoryPreferences.class); when(fileDirPrefs.isBibLocationAsPrimary()).thenReturn(false);

Have a look at MoveFilesCleanupTest

tobiasdiez · 2017-04-15T21:00:46Z

src/test/java/org/jabref/model/database/BibDatabaseContextTest.java

+    private String currentWorkingDir;
+
+    @Rule
+    public ExpectedException thrown = ExpectedException.none();


This rule is not used, right?

As I understand, it "Returns a rule that expects no exception to be thrown" (http://junit.org/junit4/javadoc/latest/org/junit/rules/ExpectedException.html). Although this is the default behaviour, it explicitly states that these tests MUST NOT through an exception, since this is what we test for. The same construct is used in BibDatabaseTest.java, and make IMHO the intent of the test more explicit and easier to read. I suggest leaving it.

tobiasdiez · 2017-04-15T21:02:48Z

src/test/java/org/jabref/model/database/BibDatabaseContextTest.java

+        BibDatabaseContext dbContext = new BibDatabaseContext();
+        dbContext.setDatabaseFile(new File("biblio.bib"));
+        List<String> fileDirectories = dbContext.getFileDirectories( "file", preferences );
+        assertTrue(fileDirectories.get(0).equals(currentWorkingDir));


please use assertEquals so that you get a better error message in case the test fails. For the same reason, assertEquals(Collections.singletonList(expected item), actual list) is the preferred way for lists.

You are right. assertEquals(Collections.singletonList(... will check also the length of the list. Fixed.

koppor · 2017-04-16T06:27:56Z

src/main/java/org/jabref/model/database/BibDatabaseContext.java

        // 4. BIB file directory
        getDatabasePath().ifPresent(dbPath -> {
-            String parentDir = dbPath.getParent().toAbsolutePath().toString();
+            assert dbPath != null : "dbPath is null";


We do not use assert statements in our code. We always do Objects.requireNonNull(...). Is it possible to change that?

Sure it is :). IMHO, assert seems a bit more applicable here, since I see asserts intended for developers, and Objects.requireNonNull() will probably end up with the message for the end user.
The assert statements are off by default but can be switched on 'in the field', unless the they were deliberately optimised away. As I understand, Objects.requireNonNull() will be always on and will always run. If this is what your policy is, I can change it, for sure.

Fixed 'asserts' in the ee4e023 commit.

koppor · 2017-04-16T06:28:27Z

src/main/java/org/jabref/model/database/BibDatabaseContext.java

-            String parentDir = dbPath.getParent().toAbsolutePath().toString();
+            assert dbPath != null : "dbPath is null";
+            Path parentPath = dbPath.getParent();
+            if( parentPath == null ) {


Can you adjust formatting? In JabRef we do if (...) {

Fixed in e56e68c. Sorry, I've overlooked that (and Eclipse overlooked it as well ;)

koppor · 2017-04-16T06:34:28Z

src/main/java/org/jabref/model/database/BibDatabaseContext.java

+            assert dbPath != null : "dbPath is null";
+            Path parentPath = dbPath.getParent();
+            if( parentPath == null ) {
+                parentPath = Paths.get(System.getProperty("user.dir"));


Not sure whether that helps: The directory lookup is explained at #459 (comment). The addition to help is still a TODO (https://github.com/JabRef/help.jabref.org/issues/160).

I also wait for @Siedlerchr s feedback 😇

koppor · 2017-04-16T06:36:55Z

Thank you for the PR! I'm not sure whether the Codacy coplaint can be fixed.

Unfortunately, we want to release a beta version today. Since it's a beta version, I'd like to postpone your improvement. Please forgive me for that 😄 I hope, we get the improvment in the next days.

preferences'.

sauliusg · 2017-04-16T09:07:24Z

The Codacy complaint should be fixed by now (commit 2deedc7).

BibDatabaseContextTest unit test.

sauliusg · 2017-04-16T10:00:19Z

Unfortunately, we want to release a beta version today. Since it's a beta version, I'd like to postpone
your improvement. Please forgive me for that 😄 I hope, we get the improvement in the next days.

No problem. It would be great, though, if the PR is not lost 😄. We essentially depend on its behaviour in our use of JabRef, but I can easily install my local branch on our computers, and install the next release as it comes out.

Regards,
Saulius

Siedlerchr · 2017-04-16T12:25:35Z

src/test/java/org/jabref/model/database/BibDatabaseContextTest.java

+    public void getFileDirectoriesWithRelativeDbParent() {
+        String dbDirectory = "relative/subdir";
+        BibDatabaseContext dbContext = new BibDatabaseContext();
+        dbContext.setDatabaseFile(new File(dbDirectory + "/" + "biblio.bib"));


Please use the methods of the Paths class. You can then just call Paths.get(dbDirectory).resolve("bib")

Siedlerchr · 2017-04-16T12:29:47Z

src/test/java/org/jabref/model/database/BibDatabaseContextTest.java

+    @Test
+    public void getFileDirectoriesWithEmptyDbParent() {
+        BibDatabaseContext dbContext = new BibDatabaseContext();
+        dbContext.setDatabaseFile(new File("biblio.bib"));


And here also please use Paths.get().toFile()

Siedlerchr

Please mock the prefs and use methods of the Paths. class.
Otherwise looks good!

Siedlerchr · 2017-04-16T12:31:59Z

Thank you very much for your PR! I only have a few small remarks. The rest looks very good to me!
As soon you fixed the things and the beta is out, we can merge it

sauliusg added 10 commits April 14, 2017 22:37

Adding a bunch of debug prints and assert's to find a reason for the

fa84e4e

'null pointer exception'.

Removing the 'HERE' debug print.

9e62873

Removing the rest of my debug prints.

ab8dd42

Adding a fix for the [WIP] Clicking 'Get fulltext' triggers a 'null

cb89c73

pointer exception' when a databse is opened in a current directory.

Attempting to create a unit test for the new fix, but so far could not

89493e3

re-create the necessary run environment.

Creating a working unit test for the new relative path fix, and adding

30afedd

also three regression tests for the old functionality.

Fixing the order of imports in the newly created test file.

05cdd9a

Temporarily commenting out of the new fix, to see if the new tests catch

2c6a209

the previous bug. Indeed, the new unit test fails both in Eclipse and in the CLI './gradlew build' run.

Restoring my fix in the 'BibDatabaseContext.java' file, fixing the 'null

54b2ada

pointer exception' if a database is passed as a local file without explicite path. The tests in 'BibDatabaseContextTest.java' pass again.

Removing the test line that is not needed for the tests to work in

9ee768c

'BibDatabaseContextTest'.

tobiasdiez requested changes Apr 15, 2017

View reviewed changes

koppor requested changes Apr 16, 2017

View reviewed changes

sauliusg added 2 commits April 16, 2017 11:48

Adding explicite scope ('private') to 'FileDirectoryPreferences

2deedc7

preferences'.

Fixed the coding style according to the JabRef conventions.

e56e68c

Switcing from 'assertTrue' to 'assertEquals' in the

bf24bbd

BibDatabaseContextTest unit test.

sauliusg added 2 commits April 16, 2017 13:22

Changing 'assert ...path != null ...' to 'Objects.requireNonNull(...)'.

ee4e023

Commenting variables used in the 'BibDatabaseContextTest' unit test.

f76ecba

Siedlerchr reviewed Apr 16, 2017

View reviewed changes

Siedlerchr requested changes Apr 16, 2017

View reviewed changes

Siedlerchr closed this Apr 16, 2017

Siedlerchr mentioned this pull request Apr 16, 2017

Sauliusg fix 2700 null pointer exception on get fulltext #2751

Merged

Uh oh!

Fix #2739 null pointer exception on get fulltext #2744

Fix #2739 null pointer exception on get fulltext #2744

Uh oh!

Conversation

sauliusg commented Apr 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sauliusg Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sauliusg Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sauliusg Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sauliusg Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Apr 16, 2017

Uh oh!

sauliusg commented Apr 16, 2017

Uh oh!

sauliusg commented Apr 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Apr 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sauliusg commented Apr 15, 2017 •

edited

Loading

sauliusg Apr 16, 2017 •

edited

Loading

sauliusg Apr 16, 2017 •

edited

Loading

sauliusg Apr 16, 2017 •

edited

Loading

sauliusg Apr 16, 2017 •

edited

Loading