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 5 commits
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
133 changes: 133 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,133 @@
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 node = differ.compare(obj1,obj2)

def categoriesMapVisitor = new DiffNode.Visitor() {

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

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

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

def categoriesAdderVisitor = new DiffNode.Visitor() {

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

node.addCategories(Arrays.asList("addedWhileVisiting"))
}
}

def "should return all categories"(){
given:
node.visitChildren(categoriesMapVisitor)
expect :
categoriesMapVisitor.mapCategories.get("firstString") == ["cat1"] as Set
categoriesMapVisitor.mapCategories.get("secondString") == ["cat1"] as Set
categoriesMapVisitor.mapCategories.get("thirdString") == ["cat1", "catAnnotation"] as Set
}
Copy link
Owner

Choose a reason for hiding this comment

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

How about doing this instead of using the mapVisitor?

node.getChild("firstString").getCategories() == ["cat1"] as Set
...

Or was there a specific reason for the visitor approach?


def "should return categories added when visiting"(){
given:
node.visitChildren(categoriesAdderVisitor)
node.visitChildren(categoriesMapVisitor)
expect :
categoriesMapVisitor.mapCategories.get("firstString") == ["cat1", "addedWhileVisiting"] as Set
categoriesMapVisitor.mapCategories.get("secondString") == ["cat1", "addedWhileVisiting"] as Set
categoriesMapVisitor.mapCategories.get("thirdString") == ["cat1", "catAnnotation", "addedWhileVisiting"] as Set
}

def "categories should not be modifiable by a client directly"(){
Copy link
Owner

Choose a reason for hiding this comment

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

I think this one would be better off as a simple unit test in de.danielbechler.diff.node.DiffNodeTest.


when:
node.visitChildren(new DiffNode.Visitor() {

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

def cats = node.getCategories()
cats.removeAll()
}
})

then :
thrown UnsupportedOperationException
}

def "should throw exception when added a null collection"(){
Copy link
Owner

Choose a reason for hiding this comment

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

Same here: better off as a unit test.


when:
node.visitChildren(new DiffNode.Visitor() {
@Override
void node(DiffNode node, Visit visit) {

node.addCategories(null)
}
})

then :
def ex = thrown(IllegalArgumentException)
ex.message == "'additionalCategories' must not be null"
}

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,
categoryService);
differProvider.push(new BeanDiffer(
differDispatcher,
introspectionService,
Expand Down
12 changes: 11 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 Down Expand Up @@ -46,6 +47,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 +58,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 +107,10 @@ public DiffNode dispatch(final DiffNode parentNode,
{
parentNode.addChild(node);
}
if(node != null)
{
node.addCategories(categoryResolver.resolveCategories(node));
}
return node;
}

Expand Down
2 changes: 2 additions & 0 deletions 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,6 +24,7 @@

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;
Expand Down Expand Up @@ -279,4 +280,5 @@ public ObjectDifferBuilder and()
{
return rootConfiguration;
}

}
23 changes: 15 additions & 8 deletions 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 @@ -32,12 +32,7 @@
import de.danielbechler.util.Assert;

import java.lang.annotation.Annotation;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
import java.util.*;
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer explicit imports here.

import java.util.concurrent.atomic.AtomicBoolean;

import static java.util.Collections.unmodifiableSet;
Expand Down Expand Up @@ -67,6 +62,7 @@ public class DiffNode
private Class<?> valueType;
private TypeInfo valueTypeInfo;
private IdentityStrategy childIdentityStrategy;
private final Set<String> additionalCategories = new TreeSet<String>();

public void setChildIdentityStrategy(final IdentityStrategy identityStrategy)
{
Expand Down Expand Up @@ -573,7 +569,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,7 +588,9 @@ public final Set<String> getCategories()
categories.addAll(categoriesFromAccessor);
}
}
return categories;
categories.addAll(additionalCategories);

return Collections.unmodifiableSet(categories);
}

/**
Expand Down Expand Up @@ -732,6 +733,12 @@ else if (childCount() > 1)
return sb.toString();
}

public void addCategories(final Collection<String> additionalCategories)
{
Assert.notNull(additionalCategories, "additionalCategories");
this.additionalCategories.addAll(additionalCategories);
}
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