Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NPE in Medline fetcher on missing ISSN #2113

Merged
merged 3 commits into from
Oct 4, 2016
Merged

Fix NPE in Medline fetcher on missing ISSN #2113

merged 3 commits into from
Oct 4, 2016

Conversation

Siedlerchr
Copy link
Member

Fix for #2110

When issn was null, calling a method on it resulted in NPE.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 3, 2016
@tobiasdiez
Copy link
Member

tobiasdiez commented Oct 4, 2016

As a quick fix this seems to be ok. Two remarks:

  • Are all other fields always not null (e.g. JournalIssue ?)
  • Can we change the Journal class to return optionals instead of null?

Also adding a test would be nice :)

@Siedlerchr
Copy link
Member Author

@tobiasdiez I will check it if Journal issue is also possible null from the annotation in the Journal class
Changing the code of Journal is not possible because it is autogenerated from the jaxb stuff on base of the xml class

But I wanted to add a test, too

@lenhard
Copy link
Member

lenhard commented Oct 4, 2016

In theory, all fields can be null. JAXB will take whatever is there and will set null for everything that it cannot find. Ultimately, null values depend on how consistent the data from medline are, and I guess we should be rather conservative in our assumptions...

There is already a fair amount of null checking code in the importer and it is quite hard to see what could be missing. @Siedlerchr We will rely on your knowledge and intuition here, but of course it is easy to miss something and then we will have to fix the error when someone opens an issue.

Apart from that I second @tobiasdiez: Please add a test and this is ready to go!

@tobiasdiez
Copy link
Member

Apparently there is a way to let JAXB convert nulls to empty optionals http://stackoverflow.com/a/23113540/873661. Not sure how complicated this is versus just checking for null.

@lenhard
Copy link
Member

lenhard commented Oct 4, 2016

@tobiasdiez If I understand it correctly, this solution is not feasible for us. It requires changing the annotations in the Java class and in our case this Java class is generated from an XSD.

Apart from that I am always happy when we can avoid that darn Optional and do a good ol' null check ;-D

@Siedlerchr
Copy link
Member Author

Apart from codecov having problems with parameterizest test, all works

@tobiasdiez tobiasdiez merged commit 195a172 into master Oct 4, 2016
@tobiasdiez tobiasdiez deleted the medlineIssn branch October 4, 2016 17:54
Siedlerchr added a commit that referenced this pull request Oct 9, 2016
* upstream/master: (102 commits)
  Removed unused test code (#2140)
  Fix main table bug when creating a duplicate (#2135)
  Remove explicit author and add SPDX-License-Identifier
  Remove GPL from README.md and CONTRIBUTING.md
  fix preview update (#2125)
  Remove some UnicodeToLatex uses (#2132)
  Fix mixup in french/farsi localization
  FetcherException should extend JabRefException
  Fix exception when opening preference dialog (#2127)
  Unify ParserException and ParseException (#2124)
  Small refactoring in Importer package (#2053)
  Implement Datepicker "none"-button (#2122)
  revert change from 816d30c
  Change title/tooltip of source panel in biblatex mode (#2120)
  Refactoring: completey typed metadata and add detailed travis output (#2112)
  RTFchars fix (#2097)
  Fix NPE in Medline fetcher on missing ISSN (#2113)
  Ctrl-s parsing error message (#2114)
  Fix bad web search error messages (#2034)
  parse error freeze (#2106)
  ...

# Conflicts:
#	src/main/java/net/sf/jabref/collab/FileUpdateMonitor.java
#	src/main/java/net/sf/jabref/gui/externalfiles/DownloadExternalFile.java
#	src/main/java/net/sf/jabref/gui/externalfiles/DroppedFileHandler.java
#	src/main/java/net/sf/jabref/gui/externalfiles/MoveFileAction.java
#	src/main/java/net/sf/jabref/logic/cleanup/RenamePdfCleanup.java
#	src/main/java/net/sf/jabref/logic/exporter/FileSaveSession.java
#	src/main/java/net/sf/jabref/logic/util/io/FileUtil.java
#	src/main/java/net/sf/jabref/preferences/JabRefPreferences.java
zesaro pushed a commit to zesaro/jabref that referenced this pull request Nov 22, 2016
* Fix NPE in issn, Fix for JabRef#2110

* Add new test for journal with no ISSN
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants