Fix NPE when importing newspaper course with metadata #6390
Conversation
matthias-ronge
force-pushed
the
issue-6069
branch
2 times, most recently
from
b11a6ba to
ef97acb
Compare
|
As code changes are similar, see my comments in #6393 |
There was a problem hiding this comment.
@matthias-ronge please apply the additional commits from #6393 to this pull request as well as @henning-gerhardt requested!
Kitodo/src/main/java/org/kitodo/production/forms/createprocess/ProcessSelectMetadata.java
Outdated
Show resolved
Hide resolved
| @@ -28,10 +29,10 @@ | |||
| public class CourseTest { | |||
|
|
|||
| @Test | |||
| public void testCloneMethod() throws IOException, ParserConfigurationException, SAXException { | |||
| public void testCloneMethod() throws Exception { | |||
There was a problem hiding this comment.
In all other use cased you added the thrown exception class InvalidMetadataValueException but here you replaced all exceptions through the most general exception class at all.
There was a problem hiding this comment.
True. I don't think we have any specific rules in this regard. It only needs to be declared to satisfy the compiler. JUnit handles the exceptions in the test execution anyway. This, in turn, argues more in favor of a general declaration of throws Exception. I've adapted the other two tests.
There was a problem hiding this comment.
With my understanding going to the general exception class is the wrong way as you even hiding other exceptions which extending the general exception class. No one - even Junit - can not decide any more with using general exception class is the thrown exception a valid exception for this method or not. So far as I know Junit5 did not handle any exception except you write an assertion to test for throwing an exception.
There was a problem hiding this comment.
Thanks. I was wrong there. I added it.
| * | ||
| * @param value | ||
| * value to be set | ||
| */ | ||
| public void setValue(String value) { |
There was a problem hiding this comment.
Should this method not got a @Override annotation like in the other places?
There was a problem hiding this comment.
The method getValue() of type ProcessTextMetadata does not override or implement a supertype method.
There was a problem hiding this comment.
But this is the setValue method. Class ProcessTextMetadata is extending class ProcessSimpleMetadata and ProcessSimpleMetadata is a abstract class with abstract method setValue. In the other implementations of the setValue method in class ProcessBooleanMetadata, class ProcessDateMetadata and ProcessSelectMetadata is always the method setValue defined with the @Override annotation but not in class ProcessTextMetadata.
Restores functionality for uploading newspaper courses containing metadata.
Fixes #6069