Skip to content

added category resolution on compare #159

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

Closed
wants to merge 9 commits into from
Closed

added category resolution on compare #159

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1d15505
added category resolution on compare
scompo Feb 29, 2016
7b8b206
added suggested changes and test for new behavior
scompo Mar 4, 2016
ac8a359
Returned unmodifiable set and changed test to expect an UnsupportedOp…
scompo Mar 4, 2016
8cf1557
Injected CategoryService directly in ObjectDifferBuilder.
scompo Mar 6, 2016
316602f
Removed resolveCategories method from InclusionService, (no need for …
scompo Mar 6, 2016
4f03e4a
Moved CategoriesTestIT into it's own package
scompo Mar 6, 2016
b459cf7
Removed getters and setters generated by Groovy
scompo Mar 6, 2016
b8c3267
Moved unit tests in DiffNodeTest.
scompo Mar 6, 2016
1944cae
Implemented suggested changes.
scompo Mar 7, 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
79 changes: 79 additions & 0 deletions src/integration-test/java/de/danielbechler/diff/CategoriesTestIT.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package de.danielbechler.diff
Copy link
Owner

Choose a reason for hiding this comment

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

Let's move this to a new package categories, since all the other major configuration sections got their own one as well.


import de.danielbechler.diff.introspection.ObjectDiffProperty
import de.danielbechler.diff.node.DiffNode
import de.danielbechler.diff.node.Visit
import de.danielbechler.diff.path.NodePath
import spock.lang.Specification

class CategoriesTestIT extends Specification{

def obj1 = new MyObject("aaa","aaa", "aaa")
def obj2 = new MyObject("bbb","bbb", "bbb")
def differ = ObjectDifferBuilder.startBuilding()
.categories()
.ofNode(NodePath.with("firstString")).toBe("cat1")
.ofNode(NodePath.with("secondString")).toBe("cat1")
.ofNode(NodePath.with("thirdString")).toBe("cat1")
.and()
.build()

def categoriesVisitor = new DiffNode.Visitor() {

Map<String, Set<String>> mapCategories = new HashMap<>();

@Override
void node(DiffNode node, Visit visit) {

mapCategories.put(node.getPropertyName(), node.getCategories())
}
}

def "should return all categories"(){
given:
differ.compare(obj1,obj2).visitChildren(categoriesVisitor)
expect :
categoriesVisitor.mapCategories.get("firstString") == ["cat1"] as Set
categoriesVisitor.mapCategories.get("secondString") == ["cat1"] as Set
categoriesVisitor.mapCategories.get("thirdString") == ["cat1","catAnnotation"] as Set
}

class MyObject{

def firstString
def secondString
def thirdString

MyObject(firstString,secondString,thirdString) {

this.firstString = firstString
this.secondString = secondString
this.thirdString = thirdString
}

def getFirstString() {
Copy link
Owner

Choose a reason for hiding this comment

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

By the way: these getters and setters are automatically generated by Groovy. The only ones you actually need are those for the thirdString due to the annotation.

return firstString
}

void setFirstString(firstString) {
this.firstString = firstString
}

def getSecondString() {
return secondString
}

void setSecondString(secondString) {
this.secondString = secondString
}

@ObjectDiffProperty(categories = ["catAnnotation"])
def getThirdString() {
return thirdString
}

void setThirdString(thirdString) {
this.thirdString = thirdString
}
}
}
3 changes: 2 additions & 1 deletion src/main/java/de/danielbechler/diff/ObjectDifferBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@ public ObjectDiffer build()
circularReferenceService,
inclusionService,
returnableNodeService,
introspectionService);
introspectionService,
inclusionService);
Copy link
Owner

Choose a reason for hiding this comment

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

You can just inject the categoryService here. That way the InclusionService doesn't need to implement the CategoryResolver interface.

differProvider.push(new BeanDiffer(
differDispatcher,
introspectionService,
Expand Down
14 changes: 13 additions & 1 deletion src/main/java/de/danielbechler/diff/differ/DifferDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import de.danielbechler.diff.access.Accessor;
import de.danielbechler.diff.access.Instances;
import de.danielbechler.diff.access.PropertyAwareAccessor;
import de.danielbechler.diff.category.CategoryResolver;
import de.danielbechler.diff.introspection.PropertyReadException;
import de.danielbechler.diff.circular.CircularReferenceDetector;
import de.danielbechler.diff.circular.CircularReferenceDetectorFactory;
Expand All @@ -34,6 +35,9 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Set;
import java.util.TreeSet;

import static de.danielbechler.diff.circular.CircularReferenceDetector.CircularReferenceException;

/**
Expand All @@ -46,6 +50,7 @@ public class DifferDispatcher
private final CircularReferenceDetectorFactory circularReferenceDetectorFactory;
private final CircularReferenceExceptionHandler circularReferenceExceptionHandler;
private final IsIgnoredResolver isIgnoredResolver;
private final CategoryResolver categoryResolver;
private final IsReturnableResolver isReturnableResolver;
private final PropertyAccessExceptionHandlerResolver propertyAccessExceptionHandlerResolver;
private static final ThreadLocal<CircularReferenceDetector> workingThreadLocal = new ThreadLocal<CircularReferenceDetector>();
Expand All @@ -56,14 +61,18 @@ public DifferDispatcher(final DifferProvider differProvider,
final CircularReferenceExceptionHandler circularReferenceExceptionHandler,
final IsIgnoredResolver ignoredResolver,
final IsReturnableResolver returnableResolver,
final PropertyAccessExceptionHandlerResolver propertyAccessExceptionHandlerResolver)
final PropertyAccessExceptionHandlerResolver propertyAccessExceptionHandlerResolver,
final CategoryResolver categoryResolver)
{
Assert.notNull(differProvider, "differFactory");
this.differProvider = differProvider;

Assert.notNull(ignoredResolver, "ignoredResolver");
this.isIgnoredResolver = ignoredResolver;

Assert.notNull(categoryResolver, "categoryResolver");
this.categoryResolver = categoryResolver;

this.circularReferenceDetectorFactory = circularReferenceDetectorFactory;
this.circularReferenceExceptionHandler = circularReferenceExceptionHandler;
this.isReturnableResolver = returnableResolver;
Expand Down Expand Up @@ -101,6 +110,9 @@ public DiffNode dispatch(final DiffNode parentNode,
{
parentNode.addChild(node);
}
if(node != null) {
node.setCategoriesFromConfig(categoryResolver.resolveCategories(node));
}
return node;
}

Expand Down
8 changes: 7 additions & 1 deletion src/main/java/de/danielbechler/diff/inclusion/InclusionService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,14 @@

import java.util.Collection;
import java.util.LinkedList;
import java.util.Set;
Copy link
Owner

Choose a reason for hiding this comment

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

Since this file doesn't contain any actual changes anymore, could you please bring it back into its original condition, so it doesn't show up in the diff?


import static de.danielbechler.diff.inclusion.Inclusion.DEFAULT;
import static de.danielbechler.diff.inclusion.Inclusion.EXCLUDED;
import static de.danielbechler.diff.inclusion.Inclusion.INCLUDED;

@SuppressWarnings("OverlyComplexAnonymousInnerClass")
public class InclusionService implements InclusionConfigurer, IsIgnoredResolver
public class InclusionService implements InclusionConfigurer, IsIgnoredResolver, CategoryResolver
Copy link
Owner

Choose a reason for hiding this comment

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

There is no need to add the CategoryResolver interface here, because that's the purpose of the CategoryService.

{
private final ObjectDifferBuilder rootConfiguration;
private final CategoryResolver categoryResolver;
Expand Down Expand Up @@ -279,4 +280,9 @@ public ObjectDifferBuilder and()
{
return rootConfiguration;
}

public Set<String> resolveCategories(DiffNode node) {

return categoryResolver.resolveCategories(node);
}
}
20 changes: 19 additions & 1 deletion src/main/java/de/danielbechler/diff/node/DiffNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public class DiffNode
private Class<?> valueType;
private TypeInfo valueTypeInfo;
private IdentityStrategy childIdentityStrategy;
private Set<String> categoriesFromConfig;
Copy link
Owner

Choose a reason for hiding this comment

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

The name is a bit too specific for my taste. I would prefer additionalCategories because it makes the Set more versatile.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, and maybe make it final and just initialize it with an empty Set. That way we never need to worry about NPEs, which are currently possible because the assiciated setter allows to assign null.


public void setChildIdentityStrategy(final IdentityStrategy identityStrategy)
{
Expand All @@ -90,6 +91,7 @@ public DiffNode(final DiffNode parentNode, final Accessor accessor, final Class<
Assert.notNull(accessor, "accessor");
this.accessor = accessor;
this.valueType = valueType;
this.categoriesFromConfig = Collections.emptySet();
setParentNode(parentNode);
}

Expand All @@ -102,6 +104,7 @@ private DiffNode()
{
this.parentNode = ROOT;
this.accessor = RootAccessor.getInstance();
this.categoriesFromConfig = Collections.emptySet();
}

/**
Expand Down Expand Up @@ -573,7 +576,10 @@ public boolean isExcluded()
return false;
}

// TODO These categories should also contain the ones configured via CategoryService
/**
* Returns a {@link java.util.Set} of {@link java.lang.String}
* @return
*/
public final Set<String> getCategories()
{
final Set<String> categories = new TreeSet<String>();
Expand All @@ -589,6 +595,8 @@ public final Set<String> getCategories()
categories.addAll(categoriesFromAccessor);
}
}
categories.addAll(categoriesFromConfig);

return categories;
}

Expand Down Expand Up @@ -732,6 +740,16 @@ else if (childCount() > 1)
return sb.toString();
}

private Set<String> getCategoriesFromConfig() {
Copy link
Owner

Choose a reason for hiding this comment

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

This getter is never used, so there is no need for it to exist.


return categoriesFromConfig;
}

public void setCategoriesFromConfig(Set<String> categoriesFromConfig) {

this.categoriesFromConfig = categoriesFromConfig;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Unfortunately there is no way to avoid making this method part of the public API, so people will eventually start (ab)using it. That's one of the reasons why I'd prefer a different name for the field, as mentioned above. Although I wouldn't even expose the field via getter or setter at all.

If we can't avoid leaking this detail to the outside, let's make it a cool feature: let's say we call the method addCategories and instead of replacing the categories, we just append them to a set. Then people are able to add their own categories later on, when traversing the result. That may allow for some advanced filtering based on criteria that go far beyond the things the ObjectDiffer is (or should be) capable of.

Calling it addCategories suggests the direct relationship with getCategories and by making it append-only people won't be confused when they try to remove a category that comes from the annotation or the parent node.

I imagine something like this:

public final void addCategories(final Collection<String> additionalCategories)
{
    Assert.notNull(additionalCategories, "additionalCategories");
    this.additionalCategories.addAll(additionalCategories);
}

In this example I changed the parameter type from Set to Collection, because the underlying object is a Set anyway so we don't care what collection type they pass.


/**
* @return Returns the path to the first node in the hierarchy that represents the same object instance as
* this one. (Only if {@link #isCircular()} returns <code>true</code>.
Expand Down
5 changes: 4 additions & 1 deletion src/test/java/de/danielbechler/diff/differ/DifferDispatcherShould.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import de.danielbechler.diff.access.Accessor;
import de.danielbechler.diff.access.Instances;
import de.danielbechler.diff.access.RootAccessor;
import de.danielbechler.diff.category.CategoryResolver;
import de.danielbechler.diff.circular.CircularReferenceDetector;
import de.danielbechler.diff.circular.CircularReferenceDetectorFactory;
import de.danielbechler.diff.circular.CircularReferenceExceptionHandler;
Expand Down Expand Up @@ -62,6 +63,8 @@ public class DifferDispatcherShould
@Mock
private IsIgnoredResolver ignoredResolver;
@Mock
private CategoryResolver categoryResolver;
@Mock
private IsReturnableResolver returnableResolver;
@Mock
private Instances instances;
Expand All @@ -79,7 +82,7 @@ public void setUp() throws Exception
when(instances.access(any(Accessor.class))).thenReturn(accessedInstances);
when(accessedInstances.getSourceAccessor()).thenReturn(accessor);

differDispatcher = new DifferDispatcher(differProvider, circularReferenceDetectorFactory, circularReferenceExceptionHandler, ignoredResolver, returnableResolver, propertyAccessExceptionHandlerResolver);
differDispatcher = new DifferDispatcher(differProvider, circularReferenceDetectorFactory, circularReferenceExceptionHandler, ignoredResolver, returnableResolver, propertyAccessExceptionHandlerResolver, categoryResolver);
}

@Test
Expand Down
7 changes: 6 additions & 1 deletion src/test/java/de/danielbechler/diff/differ/DifferDispatcherTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package de.danielbechler.diff.differ

import de.danielbechler.diff.access.*
import de.danielbechler.diff.category.CategoryResolver
import de.danielbechler.diff.circular.CircularReferenceDetector
import de.danielbechler.diff.circular.CircularReferenceDetectorFactory
import de.danielbechler.diff.circular.CircularReferenceExceptionHandler
Expand Down Expand Up @@ -53,14 +54,18 @@ class DifferDispatcherTest extends Specification {
def isReturnableResolver = Stub IsReturnableResolver, {
isReturnable(_ as DiffNode) >> true
}
def categoryResolver = Stub CategoryResolver, {
resolveCategories(_ as DiffNode) >> Collections.emptySet()
}
def propertyAccessExceptionHandlerResolver = Mock PropertyAccessExceptionHandlerResolver
def differDispatcher = new DifferDispatcher(
differProvider,
circularReferenceDetectorFactory,
circularReferenceExceptionHandler,
isIgnoredResolver,
isReturnableResolver,
propertyAccessExceptionHandlerResolver)
propertyAccessExceptionHandlerResolver,
categoryResolver)

@Ignore
def "when a circular reference is detected"() {
Expand Down