Skip to content

Return Object.class from Instances#getType instead of throwing an exc… #162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 22, 2016
Merged

Return Object.class from Instances#getType instead of throwing an exc… #162

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec84519
Return Object.class from Instances#getType instead of throwing an exc…
jlsalmon Jun 6, 2016
c173daf
Add a debug message when comparing instances of different types
jlsalmon Jun 9, 2016
8c99190
Fixed intentation
jlsalmon Jun 9, 2016
fa656e9
Improved debug message
jlsalmon Jun 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/integration-test/java/de/danielbechler/diff/ObjectDifferIT.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,8 @@ public class ObjectDifferIT extends Specification {
}

def 'compare with different types'() {
when:
ObjectDifferBuilder.buildDefault().compare('foo', 1337)
then:
thrown IllegalArgumentException
expect:
ObjectDifferBuilder.buildDefault().compare('foo', 1337).changed
}

def 'compare with ignored property'() {
Expand Down
9 changes: 7 additions & 2 deletions src/main/java/de/danielbechler/diff/access/Instances.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import de.danielbechler.util.Assert;
import de.danielbechler.util.Classes;
import de.danielbechler.util.Collections;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Collection;
import java.util.Map;
Expand All @@ -28,6 +30,7 @@

public class Instances
{
private static final Logger logger = LoggerFactory.getLogger(Instances.class);
private final Accessor sourceAccessor;
private final Object working;
private final Object base;
Expand Down Expand Up @@ -225,8 +228,10 @@ else if (sourceAccessorType != null)
// special handling for beans and arrays should go here
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty big change from the original implementation. The exception basically guaranteed, that people don't accidentally compare objects of different types. Now it just silently changes the comparison strategy to "equals only", which is pretty easy to miss.

I think it would be nice to have a little debug message, to make it slightly more obvious and potentially save some poor souls from countless hours of frustrating debugging. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I added an info message - what do you think about the wording?

}
throw new IllegalArgumentException("Detected instances of different types " + types + ". " +
"Instances must either be null or have the exact same type.");

logger.info("Detected instances of different types " + types + ". " +
"Instances should normally either be null or have the exact same type.");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far so good. Maybe replace "Instances should normally either be null or have the exact same type" with something along the lines of "These objects will only be compared via equals method".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

return Object.class;
}

private Class<?> tryToGetTypeFromSourceAccessor()
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/de/danielbechler/diff/comparison/ComparisonService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ public ComparisonStrategy resolveComparisonStrategy(final DiffNode node)
}
}

if (valueType == Object.class)
{
return EQUALS_ONLY_COMPARISON_STRATEGY;
}

return null;
}

Expand Down
9 changes: 3 additions & 6 deletions src/test/java/de/danielbechler/diff/access/InstancesTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,12 @@ import spock.lang.Unroll

class InstancesTest extends Specification {

def "getType: throws IllegalArgumentException if base and working have incompatible types"() {
def "getType: returns Object.class if base and working have different types"() {
setup:
def instances = new Instances(RootAccessor.instance, working, base, null)

when:
instances.getType()

then:
thrown(IllegalArgumentException)
expect:
instances.getType() == Object.class

where:
base | working
Expand Down