ISPN-14108 StoreMigrator throws NPE with JDBC source store #10285
Conversation
|
Looks fine to me. Will CI cover the NPE if for example the marshaller or factory are an issue? |
|
Unfortunately we don't have any test coverage of the JDBC StoreMigrator use-case, which is why the NPE was not detected earlier. I guess we could add a h2 test that populates the DB with some data using the latest |
If it isn't too much work, sure. Otherwise, I guess I am fine with it the way it is. |
ryanemerson
force-pushed
the
ISPN-14108
branch
from
7b400c8
to
90af7dd
Compare
|
I've added a test and it's a good job I did, as calls to |
ryanemerson
force-pushed
the
ISPN-14108
branch
4 times, most recently
from
8fa5937
to
97ee3d8
Compare
|
There are a couple of JDBC failing tests. Could they be caused by the changes to AbstractTableManager ? |
I don't think so. If the tests somehow passed a null |
|
Looks like we're good 👍 |
| ps.setLong(1, System.currentTimeMillis()); | ||
| rs = ps.executeQuery(); | ||
| rs.next(); | ||
| numberOfRows = rs.getInt(1); |
There was a problem hiding this comment.
We need to close the ps and rs variables.
There was a problem hiding this comment.
They're already closed by #close in normal circumstances. I have added an explicit call to #close in the event of an exception.
There was a problem hiding this comment.
No, they aren't closed in close, because they are reassigned in the lines below. Sorry I wasn't more explicit as to what was exactly wrong.
There was a problem hiding this comment.
Argh of course 🤦♂️, updated!
ryanemerson
force-pushed
the
ISPN-14108
branch
from
97ee3d8
to
99ed76b
Compare
ryanemerson
force-pushed
the
ISPN-14108
branch
from
99ed76b
to
762be59
Compare
|
Integrated into main, thanks @ryanemerson ! |
https://issues.redhat.com/browse/ISPN-14108