search

kitodo / kitodo-production

Kitodo.Production is a workflow management tool for mass digitization and is part of the Kitodo Digital Library Suite.
http://www.kitodo.org/software/kitodoproduction/
GNU General Public License v3.0
60 stars 65 forks source link

PDF-Download fails for volumes within Kitodo.Production 2.0.0 #653

Closed frank-ulrich-weber closed 7 years ago

frank-ulrich-weber commented 7 years ago

If you try to download a pdf-file of a volume within Kitodo.Production 2.0.0 the following errors occur and no pdf is created:

DEBUG 2017-02-02 08:43:12,024 (METSParser.java:loadMETS:170) load METS file http://www.example.com/?docid=12345 WARN 2017-02-02 08:43:12,188 (SimplePDFMetadataExtractor.java:parseParentMETSFile:737) Reading parent-METS file NOT successful:http://www.example.com/?docid=12345 java.io.FileNotFoundException: http://www.example.com/?docid=12345 ERROR 2017-02-02 08:43:12,189 (GetMetsPdfAction.java:run:330) error during pdf generation (java.lang.NullPointerException) java.lang.NullPointerException: Nullpointer occured before pdf-generation at org.goobi.presentation.contentservlet.controller.GetMetsPdfAction.run(GetMetsPdfAction.java:323) at de.unigoettingen.sub.commons.contentlib.servlet.controller.ContentServer.doGet(ContentServer.java:156)

It looks like the reason is that the integrated content server cannot load the parent-METS file by using the mptr. I think that would fail in the most cases, because at this stage of the workflow the URLs are often not available.

A temporary repair of this issue is to replace the new content server library with the one from the last release.

henning-gerhardt commented 7 years ago

Good news is that I can reproduce this error.

henning-gerhardt commented 7 years ago

Maybe I found the location of the bug:

https://github.com/kitodo/kitodo-contentserver/blob/kitodo-contenserver-2.0.0/src/de/unigoettingen/sub/commons/simplemets/SimplePDFMetadataExtractor.java#L209

If I negate the condition in the line, which made more sense, then I get the old behaviour.

Changing code without test suite open chance to introduce some other bugs.

henning-gerhardt commented 7 years ago

Sometimes old code is helping:

http://bazaar.launchpad.net/~slub.team/goobi-contentserver/1.0/view/head:/src/de/unigoettingen/sub/commons/simplemets/SimplePDFMetadataExtractor.java#L207

So the change to negate the condition could be correct.

Changing from metspointer.size() > 0 to metspointer.isEmpty() looks like a "optimisation" with a forgotten negation.

stweil commented 7 years ago

That's a good example why a more complete repository is useful. See it here on GitHub.

Your conclusion is correct. If you send a PR to fix it you can also fix the related comment (its should be it is)

henning-gerhardt commented 7 years ago

Missing tests are the main problem not an incomplete history. With existing tests this wrong optimisation was detected earlier and not now.

stweil commented 7 years ago

I agree that tests are a good thing and that we need better tests. That's independent of the point addressed by my comment: as you have written, old code is helping. Therefore we should not rely on the old bazaar repository but add all available history to the git repository.

henning-gerhardt commented 7 years ago

@frank-ulrich-weber : it is possible for to to test the changes from https://github.com/kitodo/kitodo-contentserver/pull/9 or need you a compiled jar file for testing?