Instances must either be null or have the exact same type.

[garpinc]

I have 2 objects that implement the same interface but are different types that i'm trying to diff and i'm getting error "Instances must either be null or have the exact same type.". Is this intentional?

[SQiShER]

Yes, this is intentional. Different types are currently only allowed for Maps and Collections. Treating different types properly is pretty tricky so I didn't want to focus on it until people actually start asking for it. I think the requirements for this could vary dramatically from developer to developer, so I'd like to make it as flexible as possible. One may want to specify an interface or base class that should be used to get and set properties, others may want to ignore the type completely and simply look at matching properties; the next one may actually need exact type matching. That's why I don't think there is a one-size-fits-all solution.

[garpinc]

I agree with you. In my case ahead of time I decided on the types I wanted to use since I have the ability to control that. Other people might not be so lucky. I think the most flexible (one size fits all scenario) is the ignore type entirely.. If for instance you're comparing A and B and A and B have completely different properties then everything in A is deleted and everything in B is added.

Then the other things you have for configuration kick in to ignore columns etc. And for this I think adding the common interface or base class can help so you don't have to specify every column you want to ignore.

For the ones who want exact type matching what you're doing now is simply saying the types have to match. I'd do what you do for Collections instead and allow it to be configurable.

I agree there are a few use cases but I think it's a constrained enough use case. For my part I have a work around already so there is no immediate need but I just wanted to acknowledge that I did run into the issue and could foresee an issue with the functionality being absent..

[SQiShER]

I fully agree with all you're saying. This is really valuable feedback. Thank you for that. I'm glad you found a workaround and I will definitely put it on the list of must-have features.

[SQiShER]

This issue has been tackled in release 0.12. The solution could work for your scenario, but I'm not entirely sure. I'm closing this issue for now, but feel free to reopen it, if the change doesn't help.

[jlsalmon]

Consider the following code which throws this error (with 0.93):

Map<String, Object> original = new HashMap<>(Collections.singletonMap("x", "string"));
Map<String, Object> modified = new HashMap<>(Collections.singletonMap("x", 0));
ObjectDifferBuilder.buildDefault().compare(modified, original);

How would you suggest tackling this kind of scenario? I was expecting this to simply resolve to CHANGED where base happens to be a string and working happens to be an integer. Would it be possible to implement a fallback Differ that doesn't care what the type is and just replaces the entire tree at that node?

If you'd like me to open a separate issue for this I'd be happy to. Thanks for the great library, by the way.

[SQiShER]

Hi @jlsalmon, yes, I think what you expect is reasonable and should be the result of the example you've given. It's the path of least surprise and makes total sense. Unfortunately it's not that easy to make it work that way, because the type-check is located in a pretty low-level place and a lot of code around it relies on it behaving the way it behaves right now.

The only idea (and I'm not sure it's a good one) that doesn't involve a huge rewrite of the affected code would be to modify de.danielbechler.diff.access.Instances#getType so it returns Object.class instead of throwing an exception when base and working have different types. Come to think of it, that could actually be a pretty good solution, which has a low risk of breaking anything for existing users.

I may give it a try this weekend, but can't make any promises. If you'd like to give it a shot instead, I'd be happy to work with you on a pull request.

[jlsalmon]

@SQiShER thanks for your response. Sounds like a good solution to me. I may have some time to work on a PR next week, so I will try to do that if you don't manage before. Thanks!

[jlsalmon]

Hi @SQiShER , did you manage to give this a shot?

[SQiShER]

@jlsalmon Not yet. You?

[jlsalmon]

@SQiShER I've started work on this, and I wanted to get your opinion on the best way to implement it before I make a naive PR.

Assuming that we return Object.class from de.danielbechler.diff.access.Instances#getType when the types are different, I see two options:

1. Add a new ComparisonStrategy to the existing BeanDiffer, as BeanDiffer#accepts would already return true for Object.class. The new comparison strategy (IncompatibleTypeComparisonStrategy or something) would set CHANGED state and set the node type to the working type in the compare method.
2. Implement an entirely new Differ whose compare method would do the same as above. Although I don't really have a good idea of a name for that differ.

You are better suited to make this decision since I don't know the possible ramifications of the two options. Or maybe I am missing other possible options entirely. Let me know! :relaxed:

[SQiShER]

@jlsalmon Cool, thanks for looking into it! I think both solutions would be fine. The first one is probably a little cleaner, because the BeanDiffer is technically the generic Differ for common objects. Adding a comparison strategy for the special case that the object type is actually Object.class is reasonable. It also makes it easy for users to provide their own Differ implementations to handle that case in a more complex manner, without having to consider the order in which the Differs are chosen.

However, there should be no need for a IncompatibleTypeComparisonStrategy. The existing EqualsOnlyComparisonStrategy should already get the job done. Maybe you can just return it as a fallback, when the type is Object and no user-provided ComparisonStrategy was found.

[jlsalmon]

Gotcha. PR incoming.