Skip to content
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.

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

Allow abstract methods to be used in JaversGetter #549

Merged
merged 9 commits into from
Jun 29, 2017
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ protected JaversGetter(Method getterMethod, Type resolvedReturnType) {
setterMethod = findSetterForGetter(getterMethod);
}

public JaversGetter(final Method getterMethod, final Type resolvedReturnType, final boolean looksLikeId) {
super(getterMethod, resolvedReturnType, looksLikeId);
setterMethod = findSetterForGetter(getterMethod);
}

@Override
protected Type getRawGenericType() {
return getRawMember().getGenericReturnType();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;

/**
* @author bartosz walacik
Expand All @@ -20,17 +21,23 @@ class JaversGetterFactory {
/**
* List all class getters, including inherited and private.
*/
List<JaversGetter> getAllGetters(){
List<JaversGetter> getAllGetters() {
List<JaversGetter> getters = new ArrayList<>();
TypeResolvingContext context = new TypeResolvingContext();

Class clazz = getterSource;
while (clazz != Object.class) {
while (clazz != null && clazz != Object.class) {
context.addTypeSubstitutions(clazz);
Arrays.stream(clazz.getDeclaredMethods())
.filter(method -> isGetter(method) && !method.isBridge())
.map(getterMethod -> createJaversGetter(getterMethod, context))
.forEach(getters::add);
.forEach(getterMethod -> {
final List<JaversGetter> overridden = getters.stream()
.filter((existing) -> isOverridden(getterMethod, existing.getRawMember()))
.collect(Collectors.toList());
final boolean looksLikeId = overridden.stream().anyMatch(JaversMember::looksLikeId);
getters.removeAll(overridden);
getters.add(createJaversGetter(getterMethod, context, looksLikeId));
});
clazz = clazz.getSuperclass();
}

Expand All @@ -42,7 +49,6 @@ private static boolean isGetter(Method rawMethod) {
hasNoParamters(rawMethod) &&
returnsSomething(rawMethod) &&
isNotStatic(rawMethod) &&
isNotAbstract(rawMethod) &&
isNotNative(rawMethod);
}

Expand All @@ -63,16 +69,35 @@ private static boolean isNotStatic(Method rawMethod) {
return !Modifier.isStatic(rawMethod.getModifiers());
}

private static boolean isNotAbstract(Method rawMethod) {
return !Modifier.isAbstract(rawMethod.getModifiers());
}

private static boolean isNotNative(Method rawMethod) {
return !Modifier.isNative(rawMethod.getModifiers());
}

private JaversGetter createJaversGetter(Method getterMethod, TypeResolvingContext context){
private static boolean isOverridden(final Method parent, final Method toCheck) {
return isSubClass(parent, toCheck) &&
sameMethodName(parent, toCheck) &&
returnTypeCovariant(parent, toCheck) &&
sameArguments(parent, toCheck);
}

private static boolean isSubClass(final Method parent, final Method toCheck) {
return parent.getDeclaringClass().isAssignableFrom(toCheck.getDeclaringClass());
}

private static boolean sameMethodName(final Method parent, final Method toCheck) {
return parent.getName().equals(toCheck.getName());
}

private static boolean returnTypeCovariant(final Method parent, final Method toCheck) {
Copy link
Member

Choose a reason for hiding this comment

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

+1 for taking care about return type covariance

return parent.getReturnType().isAssignableFrom(toCheck.getReturnType());
}

private static boolean sameArguments(final Method parent, final Method toCheck) {
return Arrays.equals(parent.getParameterTypes(), toCheck.getParameterTypes());
}

private JaversGetter createJaversGetter(Method getterMethod, TypeResolvingContext context, boolean looksLikeId) {
Type actualReturnType = context.getSubstitution(getterMethod.getGenericReturnType());
return new JaversGetter(getterMethod, actualReturnType);
return new JaversGetter(getterMethod, actualReturnType, looksLikeId);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import org.javers.common.collections.Sets;
import org.javers.common.validation.Validate;
import org.javers.core.metamodel.property.Property;

import java.lang.annotation.Annotation;
import java.lang.reflect.AccessibleObject;
Expand All @@ -27,14 +28,20 @@
public abstract class JaversMember<T extends Member> {
private final T rawMember; //delegate
private final Optional<Type> resolvedReturnType;
private final boolean looksLikeId;

/**
* @param resolvedReturnType nullable
*/
public JaversMember(T rawMember, Type resolvedReturnType) {
this(rawMember, resolvedReturnType, false);
}

public JaversMember(final T rawMember, final Type resolvedReturnType, final boolean looksLikeId) {
Validate.argumentIsNotNull(rawMember);
this.rawMember = rawMember;
this.resolvedReturnType = Optional.ofNullable(resolvedReturnType);
this.looksLikeId = looksLikeId || hasAnnotation(Sets.asSet(Property.ID_ANN, Property.EMBEDDED_ID_ANN));
setAccessibleIfNecessary(rawMember);
}

Expand Down Expand Up @@ -80,6 +87,10 @@ public boolean hasAnnotation(Set<String> aliases) {
return getAnnotationTypes().stream().anyMatch(annType -> aliases.contains(annType.getSimpleName()));
}

public boolean looksLikeId() {
return looksLikeId;
}

public abstract Object getEvenIfPrivate(Object target);

public abstract void setEvenIfPrivate(Object target, Object value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public Class<?> getRawType() {
* true if property looks like identifier of an Entity, for example has @Id annotation
*/
public boolean looksLikeId() {
return member.hasAnnotation(Sets.asSet(ID_ANN, EMBEDDED_ID_ANN));
return member.looksLikeId();
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package org.javers.core.cases

import groovy.transform.PackageScope
import org.javers.core.JaversBuilder
import org.javers.core.MappingStyle
import org.javers.core.metamodel.annotation.Id
import org.javers.core.metamodel.type.EntityType
import spock.lang.Specification
import spock.lang.Unroll

class AutoValueCase extends Specification {

abstract class AbstractEntity {
@Id
abstract int getId()

abstract int getValue()
}

@PackageScope
final class ConcreteEntity extends AbstractEntity {
private final int id
private final int value

ConcreteEntity(final int id, final int value) {
this.id = id
this.value = value
}

@Override
int getId() {
id
}

@Override
int getValue() {
value
}
}

@Unroll
def "#label should support abstract idGetter"() {
given:
def a = new ConcreteEntity(1, 1)
def b = new ConcreteEntity(1, 2)
def diff = javers.compare(a, b)
def first = javers.commit('Alice', a)
def second = javers.commit('Bob', b)

expect:
diff.changes.size() == 1
diff.changes[0].propertyName == 'value'
diff.changes[0].left == 1
diff.changes[0].right == 2

first.author == 'Alice'
second.author == 'Bob'
second.changes.size() == 1
second.changes[0].propertyName == 'value'
second.changes[0].left == 1
second.changes[0].right == 2
second.snapshots[0].managedType.managedClass.looksLikeId.size() == looksLikeId
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sure there is a better way to test this?


where:
label << ['basic javers', 'javers with bean mapping', 'javers with bean mapping and entity registered']
javers << [
JaversBuilder.javers().build(),
JaversBuilder.javers().withMappingStyle(MappingStyle.BEAN).build(),
JaversBuilder.javers().withMappingStyle(MappingStyle.BEAN).registerEntity(AbstractEntity.class).build()
]
looksLikeId << [0, 1, 1]
Copy link
Contributor Author

@jonfreedman jonfreedman Jun 20, 2017

Choose a reason for hiding this comment

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

The basic javers case could be removed, it's just here for illustration of why this whole change is necessary. If you annotate line 21 with @Id then this test will fail as looksLikeId will be 1.

}

@Unroll
def "should map #entity.simpleName with abstract @IdGetter as EntityType"(){
given:
def javers = JaversBuilder.javers().withMappingStyle(MappingStyle.BEAN).build()

expect:
javers.getTypeMapping(entity) instanceof EntityType

where:
entity << [AbstractEntity, ConcreteEntity]
}
}