PojomaticAssert.assertEqualsWithDiff should identify nested

[irobertson]

(Reported initially on the old google-code site by qualidafial, aka Matt Hall)

class Foo { @Property Bar bar; }
class Bar { @Property String baz; }

Foo expected = new Foo().withBar( new Bar().withBaz("abc") );
Foo actual = new Foo().withBar( new Bar().withBaz("123") );

In the above case, calling PojomaticAssert.assertEqualsWithDiff produces a message to the effect that Foo.bar is different for each object.

java.lang.AssertionError: differences between expected and actual:
bar: {Bar{baz: {"abc"}} versus
{Bar{baz: {"123"}}
(expected:
but was:)

Most of the time the differences can be sorted out by looking at toString().

However tonight I was bitten Hibernate's PersistentBag implementation, which uses instance equals despite the List interface contract. Thus the differences between my two instances was not evident from the toString() of expected and actual--they were both empty lists.

I propose enhancing Pojomatic.diff so that the description of any property difference will recurse on Pojomatic.diff, provided both property values are equals-compatible, and from pojomated classes.

I believe this change will help cut to the chase when debugging equals-related testing problems.

[qualidafial]

Mind if I take a crack at this issue?

[irobertson]

Go for it :).

[qualidafial]

In the interest of performance, I'm confining the nested diff to only properties with a known pojomated type.

The plan is to modify PojomatorByteCodeGenerator around line 774. If the property type is equals-compatible according to ClassProperties.get(), then instead of adding a single difference for that property, add bytecode to call Pojomatic.diff() on the property values, and add a ValueDifference with the property name prepended for each difference in the result.

One concern I have is ClassProperties.get() throws an exception for any non-pojomated type, and this could have performance implications on just about every class. Adding a java.* package exemption might help, but that feels like a hack.

Edit: performance implications are confined to when the class is first pojomated.

[qualidafial]

I was hoping for feedback on the plan I laid out in my last comment.

I'm a little concerned about the performance implications of flattening the nested diffs, in the event that the nested objects are deep graphs of pojomated objects. It's especially a problem if there are tons of different properties. e.g. Meeting.organizer changed from person A to person B. If the Person object has a deep tree of pojomated properties, the performance can get out of hand quickly.

Currently the diff generation is quick because we're just using equals, which fails fast. But by performing nested comparisons, we start to look at O(n^2) performance depending on the number of layers in the object graph.

Based on the above, I think a different approach is warranted. My current thinking is to add a Differences.flatten() method, which returns a new Differences instance with all the nested differences flattened out. I'm betting it can be implemented lazily too.

Is there a place in the internal API where I can query whether an object's class is pojomated? (without a try/catch?)

[irobertson]

Matt,

I appologize for not getting back to you sooner on this. A few thoughts:

• Performance may not be as critical here - I think of diff as being used in unit tests, and then only on failure.
• We should consider offering some way to select whether to recurse or not. A conservative approach would be to recurse into a property only if that property is annotated with something like @RecursiveDiff. Passing a recursive flag to diff would be another option, but would greatly complicate the bytecode generation process.
• If we go with the annotation-per-property approach, then it should be (relatively) safe to assume that we can recurse on a property value. Another option would be to only recurse if the property's static type is a pojomated class; the latter case would guarantee that any NoPojomaticPropertiesException would be thrown only at bytecode generation time, instead of at "runtime".
• There is no place in the API to determine if a class is pojomated without incuring an exception. However, given the above comments, I'm not sure it's worth replumbing the internals to avoid the exception.
• When reporting differences, do we want to add OGNL-type property names (bar.baz), or have a recursive structure - i.e. a ValueDifference with a getSubDifference method?

This last question also has me wondering if we should tackle this from a completely different angle. Currently, AssertUtils does an equals check, and if it fails, adds the result of Pojomatic.diff(...).toString() to the message. Why not beef up AssertUtils.assertEquals to recurse into the results of Pojomatic.diff, calling diff on differing property values which themselves are instances of pojomatized classes? It would be a whole lot easier than messing with byte code.