Fix OutOfBoundsException when importing multiple entries in medline format #1611

tschechlovdev · 2016-07-21T15:09:43Z

@zellerdev found out a bug when importing more than one entry in the medline format into JabRef.
JabRef version
JabRef 3.5
windows 8.1 6.3 amd64
Java 1.8.0_25

Steps to reproduce:

Select more than entry from here .
Select "Send to File" and then choose "Medline" Format.
Open Jabref and select "Import into new database".
I get an IndexOutOfBoundException shown on the console and no entries are imported.

Alternatively use the testfiles I added in this PR.

Quick fix: Add an if clause around the substring method, that causes the Exception.

Tests created for changes

boceckts · 2016-07-21T16:03:37Z

I can confirm the bug but next time please create an issue for this and assign it to yourself if you want to fix it. Also please use the "copy version to clipboard" functionallity in the About dialog in the help menu to easily paste your JabRef-, OS- and Java-version.

boceckts · 2016-07-21T16:26:30Z

src/main/java/net/sf/jabref/importer/fileformat/MedlinePlainImporter.java

@@ -127,91 +126,93 @@ public ParserResult importDatabase(BufferedReader reader) throws IOException {
                    }
                }
                String entry = current.toString();
+                if (entry.contains("-")) {


I think the while-loop above this is already trying to check the current line for validity and tries to skip all the invalid ones. I would propose to extract a new method called private boolean checkLineValidity(String line) and do all the checks in there such as: empty line, line doesn't contain "-", "-" symbol not at 4th position, etc...
then the while loop above can also be replaced and you don't need to modify the counter variable "j". You can then simply skip any invalid lines with:

if (!checkLineValidity(lines[j])) { continue; }

This would also mean you don't need to modify as much code as you did now because then you don't have the if-clause around all the code.

But I wouldn't extract the while loop above, because in the while loop are also correcctions, like adding space if '-' is at third position. And putting the while loop into a method that returns a boolean could have side effects, since you don't expect, that this method is changing something.

You could also extract to multiple methods, private boolean first checkLineValidity(String line), if false try to correct it by calling another method that tries to insert spaces or similar things.

boceckts · 2016-07-25T15:20:28Z

src/main/java/net/sf/jabref/importer/fileformat/MedlinePlainImporter.java

        }

        return new ParserResult(bibitems);

    }

+    private boolean checkLineValidity(String line) {
+        if (line.length() >= 4) {
+            return line.contains("-") || !(line.charAt(4) == '-') ? true : false;


!(line.charAt(4) == '-') this will throw an OutOfBoundsException if the length of the line is exactly 4. Please cover this case.

Also please check the if statement again, I can't find a case where this will return false.

? true : false is unnecessary.

boceckts · 2016-07-26T09:18:51Z

src/main/java/net/sf/jabref/importer/fileformat/MedlinePlainImporter.java

-        } else {
-            return false;
-        }
+        return (line.length() >= 5) ? line.contains("-") && (line.charAt(4) == '-') : false;


I think line.contains("-") is unnecessary when you also check for it's position.

tschechlovdev · 2016-08-01T10:30:38Z

No remarks?

Siedlerchr · 2016-08-01T10:47:55Z

src/test/java/net/sf/jabref/importer/fileformat/MedlinePlainImporterTest.java

+    public void testWithMultipleEntries() throws IOException, URISyntaxException {
+        Path file = Paths
+                .get(MedlinePlainImporter.class.getResource("MedlinePlainImporterStringOutOfBounds.txt").toURI());
+        List<BibEntry> entries = importer.importDatabase(file, Charset.defaultCharset()).getDatabase().getEntries();


When the bib file is in UTF8, then please set the Charset explicit to UTF8, too. DefaultCharsets.UTF8 (or similar), otherwise test will fail on windows in gradle.

Siedlerchr · 2016-08-01T10:52:58Z

In general good. 👍 But I would recommend adding some test with invalid formats, e.g. if an exceptions is thrown (expected) or whatever.

tschechlovdev · 2016-08-01T12:43:33Z

Hm an Excpetion should be not thrown, except if the reader would be null. If the format is invalid then the entries list will be empty, but no exception will be thrown. I'll added tests for both.

tschechlovdev · 2016-08-01T20:35:57Z

Any other remarks?

stefan-kolb · 2016-08-02T07:05:11Z

LGTM, just one remark: can you keep the test files as minimal as needed inside the tests? No need for 300 attributes when you can recreate the test issue with one or two.

tschechlovdev · 2016-08-02T10:33:50Z

Ok, I've minimized the testfile for the issue.

stefan-kolb · 2016-08-03T07:47:44Z

Maybe fix the two Codacy warnings (https://www.codacy.com/app/simonharrer/jabref/pullRequest?prid=289709) and add a key to the @misc entries. Otherwise im 👍 We need a second review from a @JabRef/developers tho.

Siedlerchr · 2016-08-03T08:52:24Z

src/test/java/net/sf/jabref/importer/fileformat/MedlinePlainImporterTest.java

+        Path file = Paths
+                .get(MedlinePlainImporter.class.getResource("MedlinePlainImporterTestInvalidFormat.xml").toURI());
+        List<BibEntry> entries = importer.importDatabase(file, Charsets.UTF_8).getDatabase().getEntries();
+        assertEquals(Collections.EMPTY_LIST, entries);


Use the method Collections,emptyList() rather than the constant, because the constant does not return a generic list and is therefore not typesafe.
(In this case not so important, but could possinly produce an unchecked warning)

Siedlerchr · 2016-08-03T08:54:39Z

Just the collections,emptyList and the codacy and it's fine 👍 ( I already reviewd the main parts before)

* master: Fixed OO/LO manual connection dialog on Linux Removed thrown Exception declarations (JabRef#1673) Fix JabRef#1288 Newly opened bib-file is not focused (JabRef#1671) Refactor DB loading Fix OutOfBoundsException when importing multiple entries in medline format (JabRef#1611) Removed the possibility to auto show or hide the groups interface (JabRef#1668) Add test to describe workaround for JabRef#1633 Fixed JabRef#1643: Searching with double quotes in a specific field ignores the last character fix build Fixes JabRef#1554: JabRefFrame is set as owner for ImportInspectionDialog Fixed most of the ErrorProne warnings Replaced output of getResolvedField to Optional<String> (JabRef#1650) PushToApplication cleanup and refactoring (JabRef#1659) Replaced Object with appropriate class where possible (JabRef#1660) Replaced some array return types (JabRef#1661) Fix XMP test Localization Moved the main part of XMPUtil to jabref.XMPUtilMain and injected a b… (JabRef#1642) Made possible to make the OO/LO panel a bit more narrow (JabRef#1652) French localization: Jabref_fr: empty strings + some cleaning

tschechlovdev added stupro labels Jul 21, 2016

boceckts reviewed Jul 21, 2016
View reviewed changes

boceckts removed the stupro-ready-for-internal-review label Jul 21, 2016

boceckts mentioned this pull request Jul 21, 2016

MedlinePlain Importer throws StringIndexOutOfBoundsException #1612

Closed

tschechlovdev force-pushed the MedlinePlainStringOutOfBounds branch 3 times, most recently from b92d43e to f37f8e0 Compare July 24, 2016 14:50

tschechlovdev added the stupro-ready-for-internal-review label Jul 25, 2016

boceckts reviewed Jul 25, 2016
View reviewed changes

boceckts added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Jul 26, 2016

boceckts reviewed Jul 26, 2016
View reviewed changes

fix OutOfBoundsException and add test case

e3fd748

tschechlovdev force-pushed the MedlinePlainStringOutOfBounds branch from 0b4cd56 to e3fd748 Compare July 26, 2016 10:03

tschechlovdev changed the title ~~[WIP] Fix OutOfBoundsException when importing multiple entries in medline format~~ Fix OutOfBoundsException when importing multiple entries in medline format Jul 26, 2016

Siedlerchr reviewed Aug 1, 2016
View reviewed changes

tschechlovdev added 2 commits August 1, 2016 14:38

add test for invalid format and null reader

b5026c0

change encoding to utf-8

d369a88

minimize tesfile

89092aa

Siedlerchr reviewed Aug 3, 2016
View reviewed changes

add comments

ca5825b

stefan-kolb merged commit f6ccf27 into JabRef:master Aug 3, 2016

tschechlovdev deleted the MedlinePlainStringOutOfBounds branch August 3, 2016 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OutOfBoundsException when importing multiple entries in medline format #1611

Fix OutOfBoundsException when importing multiple entries in medline format #1611

tschechlovdev commented Jul 21, 2016 •

edited

Loading

boceckts commented Jul 21, 2016

boceckts Jul 21, 2016

boceckts Jul 21, 2016

tschechlovdev Jul 24, 2016

boceckts Jul 25, 2016

boceckts Jul 25, 2016

boceckts Jul 25, 2016

boceckts Jul 25, 2016

boceckts Jul 26, 2016

tschechlovdev commented Aug 1, 2016

Siedlerchr Aug 1, 2016

tschechlovdev Aug 1, 2016

Siedlerchr commented Aug 1, 2016

tschechlovdev commented Aug 1, 2016

tschechlovdev commented Aug 1, 2016

stefan-kolb commented Aug 2, 2016

tschechlovdev commented Aug 2, 2016

stefan-kolb commented Aug 3, 2016

Siedlerchr Aug 3, 2016

tschechlovdev Aug 3, 2016

Siedlerchr commented Aug 3, 2016

Fix OutOfBoundsException when importing multiple entries in medline format #1611

Fix OutOfBoundsException when importing multiple entries in medline format #1611

Conversation

tschechlovdev commented Jul 21, 2016 • edited Loading

boceckts commented Jul 21, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschechlovdev commented Aug 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Aug 1, 2016

tschechlovdev commented Aug 1, 2016

tschechlovdev commented Aug 1, 2016

stefan-kolb commented Aug 2, 2016

tschechlovdev commented Aug 2, 2016

stefan-kolb commented Aug 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Aug 3, 2016

tschechlovdev commented Jul 21, 2016 •

edited

Loading