I am trying to update the (~11 year old!) apache commons-beanutils and all tests pass except for this one:
@Test
public void isDirty_shouldReturnFalseWhenNoChangeHasBeenMade() throws Exception {
assertFalse(new Obs().isDirty());
//Should also work if setters are called with same values as the original
Obs obs = createObs(2);
obs.setGroupMembers(new LinkedHashSet<>());
obs.getConcept().setDatatype(new ConceptDatatype());
assertFalse(obs.isDirty());
BeanUtils.copyProperties(obs, BeanUtils.cloneBean(obs));
assertFalse(obs.isDirty());
it fails on the last line I am showing above. If I comment out the BeanUtils line all is fine. All other tests regarding the isDirty logic pass as well. Does not seem right to me to do this BeanUtils manipulation and assert anything afterwards since this ObsTest is a unit test it should be concerned with the Obs logic and not assert anything on a 3rd party library’s workings.
I wonder what the purpose of the BeanUtils logic is in there? Hope someone can shed some light no this for me so we can find another way
The test is meant to ensure that if you have an Obs and you call setters on it with values that are the same as the old ones then the Obs should not be marked as dirty, we use BeanUtils to have the setters invoked.
My point is that this relies on specifics of the BeanUtils that are rather obscure and obviously have changed in this 11 years difference. Why use BeanUtils if we simply need to invoke setters to assert that Obs.isDirty() or not dirty? Its not clear what this test is trying to achieve due to this clone/copy logic.
Does anything speak against removing the use of BeanUtils here and replacing it with calls to setters?
The test doesn’t have to use BeanUtils, I believe there is other libraries with similar utility methods that one can call, it doesn’t matter what you use as long as the test still passes.
I wouldn’t use BeanUtils.copyProperties for two reasons:
Order of setters invocation: in Obs, order of setters is important (some setters depend on the result of previous ones, for instance setValueAsString must be called after setConcept). BeanUtils.copyProperties uses reflection and order is not guaranteed, can vary even between JVM versions.
Hibernate shared references: BeanUtils.copyProperties makes shallow copy of maps and collections, provoking “org.hibernate.HibernateException: Found shared references to a collection” when flushing the session (I recently fixed a bug precisely for this reason).
Setters are invoked in alphabetical order in commons-beanutils.1.7:
But setConcept comes before setValueXXX therefore it works … by luck. If they changed the implementation of the properties map (see Introspector class) it’ll work or not
I presume the reason someone used BeanUtils was to automagically call every setter on the bean, for fear that if we called them all manually someone would forget to update the test when more properties were added sometime in the future.
That seems like a valid fear to me, so I would vote to keep doing things dynamically, rather than just replacing this with 20 lines of obs.setXyz(obs.getXyz()). (In other words: the danger/cost of someone adding a new property to obs, getting the dirty behavior wrong, and forgetting to update this test is larger than the danger/cost of having to change the test implementation again 10 years from now.)
Darius is correct, in that test you can’t call the setters manually, if you want you can replace the call to BeanUtils but it should be with something that automatically calls all setters.
@lluismf this is used in a unit test, order doesn’t matter and no need to worry about hibernate flushes.
I highly doubt that order does not matter, I think @lluismf is on to the root cause with his explanation (thanks a lot @lluismf ). @wyclif try for yourself. Update the BeanUtils and see that the assumptions we make in this test about the implementation of BeanUtils is not met anymore with the update. Thats why I think its dangerous. I do get @darius point that there is also danger in not using such a dynamic approach if properties are added in the future. However, the current test falls short of stating its assumptions on what it expects this “magic” clone/copy from BeanUtils to do.
I strongly disagree. The way the setters of Obs work, they should be invoked in a certain order. That’s why you can’t use a method that invokes them in a particular (alphabetical) order. I don’t buy the argument that in the future a new attribute can be added and the test should be changed (yes it should, so?).
This is the reason why we have unit tests, if the logic in the library we are using has changed, the test is still able to catch the fact the it has changed. As I said initially, i have no objection to getting rid of the use of BeanUtils, what matters to me is that we should avoid manual calling of the setters.
@lluismf what am saying is that order in this particular case should not really matter and the test has always passed regardless of the order of their execution, setValueAsString isn’t a setter because there is no property named valueAsString, am sure logic around bean property accessors is smart enough to skip setXXX and getXXX methods that aren’t true property accessors.
If the unit test depends on a specific implementation of the library (or a given JVM version), the unit test is wrong IMHO.
I agree that invoking the setters manually makes no sense, because the getters come from the same instance. But invoking copyProperites makes no sense either for the same reason (specially after doing a cloneBean)
If you check the image I attached, setValueAsString is invoked, even if it’s not a “real” setter.
@teleivo’s upgrade simply exposed a bug in the Obs class.
In summary, a non boolean obs with a valueCoded would get overwritten on calling setValueBoolean and hence make it dirty. This bug exposure happened because of the change of the order in which the properties are called in the upgraded apache commons-beanutils. If you called obs.saveValueBoolean(null) before obs.setValueCode(concept), all is well. But reverse the order and you get to this bug.
I don’t think it’s a bug in Obs. Obs requires the setters to be invoked in a certain way, setValueCoded should NOT be called if it’s a boolean Obs.
It’s a bug in the test
Calling setValueBoolean shouldn’t be a problem if the value hasn’t changed, if a value has changed then it should be fine for the obs to be dirty regardless of the order of invocation of the setters.
@wyclif is that in reference to your comment above which says “The test is meant to ensure that if you have an Obs and you call setters on it with values that are the same as the old ones then the Obs should not be marked as dirty”
