#296 NPE when setting properties

[wujek]

The method: DatabaseConfig.convertIfNeeded is not NPE safe:

private Object convertIfNeeded(String property, Object value)
{
logger.trace("convertIfNeeded(property={}, value={}) - start", property, value);

ConfigProperty prop = findByName(property);
Class allowedPropType = prop.getPropertyType(); // HERE

if(allowedPropType == Boolean.class || allowedPropType == boolean.class)
{
// String -> Boolean is a special mapping which is allowed
if(value instanceof String)
{
return Boolean.valueOf((String)value);
}
}

return value;
}

When a property that is not supported is set, NPE happens. It would be much better to simply ignore such a property and maybe issue a warning.

[matthias g]

fixed by throwing a proper NPE with a nice exception text (svn rev 1192). I believe it is not sensible to ignore properties not known to dbunit. this would lead to more traffic in the forums in case of wrong spellings etc. hope this is okay, if not please let me know.
rgds
matthias

[wujek]

I wanted that because I build a framework that uses DbUnit in tandem with JPA and TestNG, and wanted to be able to set properties in pesristence.xml, which then would simply be all issued to DbUnit. JPA ignores properties that it doesn't know, so I thought that maybe it would make sense to have that in DbUnit as well. For now, I can just think of something smarter in my framework, no biggie.

I can understand your point in not ignoring the faulty property; however, I don't like that it is a NPE that reports the property problem - I think it should be a IllegalArgumentException or even a custom exception maybe (runtime). NPE is misleading here and it kind of exposes the underlying implementation. What if you decide to change the convertIfNeeded method to look up the property in a known repository, before you try to look it up? You might not do this, ever, but throwing a more meaningful exception would be better. Not sure about backwards-compatibility, though, but if users catch NPE to deal with faulty properties, this also doesn't look good. What do you think?
Wuj

[matthias g]

Hi Wuj,
you're right. I just changed it to an IllegalArgumentException which is definitely more appropriate here. I also thought about throwing a Dbunit runtime exception but I think IAE is ok here.
thanks for your feedback!
rgds,
matthias

[wujek]

I just realized there is another method for ant and maven: DatabaseConfig.setPropertiesByString() that _does_ check property names (the result of findByName()), and simply issues a warning when a property doesn't exist. This makes the two methods inconsistent.

I humbly reopen the bug with a pledge that you maybe rethink the original problem. For now my framework doesn't allow to set DbUnit properties (DbUnit is only one thing we integrate with) simply because of this bug. It would be much easier to take all test properties and feed them to DbUnit, which in turn would itself ignore faluty ones.

At the very least, the two methods' behaviour should be consistent - either both allow any property, ignore and warn, or both throw the NPE.

There are three workarounds:
1. catch the NPE - blah!
2. construct a string from all parameters and feed it to setPropertiesByString() to let DbUnit filter it out
3. instead of calling setProperty, cal setPropertiesByString() for each property - very similar to #2, but doesn't require creating a long string

[wujek]

Please forget about what I said in workarounds #2 and #3 - you don't build any string of course. You create Properties and put the entries there, as simple as that. Works like a charm.