Skip to content

Commit 275513a

Browse files
jflorenciorozza
authored andcommitted
PojoCodec improvents for sub-classes
Pojo Codec fixes with sub-classes and bson creator This commit tries to fix a few issues: 1. It was not possible to overload a getter in a sub-class and make the type more specific (covariant overload). For example, attempting to serialize Bar would fail: ``` // Assume there are setters here as well... class Foo { public Number getNumber() { /*...*/ } } class Bar extends Foo { @OverRide public Long getNumber() { /*...*/ } } ``` This was due to bridge methods not getting picked up, so even though Method#getDeclaredMethods is documented to return methods only from the super class, the Java compiler inserts a bridge method in the sub-class (Bar in this case) for these covariant overrides for legacy interop. 2.a. It was not possible to use a more specific type in a getter, but a more general type in the setter/constructor/static factory method. For example: ``` class Foo { public ImmutableList<String> getNames() { /*...*/ } // Mutable class public void setNames(List<String> names) { /*...*/ } // or immutable class (both didn't work) @BsonCreator public Foo(@BsonProperty("names") List<String> names) { /*...*/ } } ``` The first issue was that strict type equality was enforced. This is a pretty common pattern that should be allowed though - it's typically a bad idea to accept such a specific class in your constructor/setter. In my case, I'm creating my domain models with Immutables, so the generated immutable class will override my getter that returns `List<T>` to return `ImmutableList<T>`. The second issue was that various collections were boxed to specific implementations like ArrayList/HashMap/HashSet. Doing this in TypeData caused quite a few problems. The main being it was no longer possible to understand the type hierarchy (ImmutableList does not extend ArrayList, but it safely implements List). The ArrayList/HashMap/etc selection happens in collection codec as an implementation detail. I think this is a bit cleaner. 3. It was not possible to use immutable collections like Guava's ImmutableList. By fixing the above issues, it works now. Note that we still can't use ImmutableList as an argument that's accepted, but that's a case where it's reasonable to provide a custom codec implementation since there's no way we can do that automatically. 4. @BsonCreator annotations were only searched for in the target class, but not any super classes. It's relatively common to have an abstract base class host all of the static factory methods for the concrete subtypes, so this allows for that pattern. I do the above, but in addition, some of my domain models are generated through Immutables where I don't have the ability to inject annotated static factory methods. ----- I moved code around a bit for better locality and some re-usability. In particular with PropertyReflectionUtils, and TypeData having a few methods of extracting the types out of methods/fields/etc. I tried to add test cases for all of the above things - they all failed prior to these modifications. All other existing bson tests pass. JAVA-2612
1 parent f525edf commit 275513a

19 files changed

+886
-155
lines changed

bson/src/main/org/bson/codecs/pojo/ConventionAnnotationImpl.java

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import java.lang.annotation.Annotation;
2828
import java.lang.reflect.Constructor;
2929
import java.lang.reflect.Method;
30+
import java.lang.reflect.Type;
3031
import java.util.ArrayList;
3132
import java.util.List;
3233

@@ -111,33 +112,43 @@ private <T> void processCreatorAnnotation(final ClassModelBuilder<T> classModelB
111112
}
112113
}
113114

114-
for (Method method : clazz.getDeclaredMethods()) {
115-
if (isStatic(method.getModifiers())) {
116-
for (Annotation annotation : method.getDeclaredAnnotations()) {
117-
if (annotation.annotationType().equals(BsonCreator.class)) {
118-
if (creatorExecutable != null) {
119-
throw new CodecConfigurationException("Found multiple constructors / methods annotated with @BsonCreator");
120-
} else if (!clazz.isAssignableFrom(method.getReturnType())) {
121-
throw new CodecConfigurationException(
122-
format("Invalid method annotated with @BsonCreator. Returns '%s', expected %s", method.getReturnType(),
123-
clazz));
115+
Class<?> bsonCreatorClass = clazz;
116+
boolean foundStaticBsonCreatorMethod = false;
117+
while (bsonCreatorClass != null && !foundStaticBsonCreatorMethod) {
118+
for (Method method : bsonCreatorClass.getDeclaredMethods()) {
119+
if (isStatic(method.getModifiers())) {
120+
for (Annotation annotation : method.getDeclaredAnnotations()) {
121+
if (annotation.annotationType().equals(BsonCreator.class)) {
122+
if (creatorExecutable != null) {
123+
throw new CodecConfigurationException("Found multiple constructors / methods annotated with @BsonCreator");
124+
} else if (!bsonCreatorClass.isAssignableFrom(method.getReturnType())) {
125+
throw new CodecConfigurationException(
126+
format("Invalid method annotated with @BsonCreator. Returns '%s', expected %s",
127+
method.getReturnType(), bsonCreatorClass));
128+
}
129+
creatorExecutable = new CreatorExecutable<T>(clazz, method);
130+
foundStaticBsonCreatorMethod = true;
131+
break;
124132
}
125-
creatorExecutable = new CreatorExecutable<T>(clazz, method);
126-
break;
127133
}
128134
}
129135
}
136+
137+
bsonCreatorClass = bsonCreatorClass.getSuperclass();
130138
}
131139

132140
if (creatorExecutable != null) {
133141
List<BsonProperty> properties = creatorExecutable.getProperties();
134142
List<Class<?>> parameterTypes = creatorExecutable.getParameterTypes();
143+
List<Type> parameterGenericTypes = creatorExecutable.getParameterGenericTypes();
144+
135145
if (properties.size() != parameterTypes.size()) {
136146
throw creatorExecutable.getError(clazz, "All parameters must be annotated with a @BsonProperty in %s");
137147
}
138148
for (int i = 0; i < properties.size(); i++) {
139149
boolean isIdProperty = creatorExecutable.getIdPropertyIndex() != null && creatorExecutable.getIdPropertyIndex().equals(i);
140150
Class<?> parameterType = parameterTypes.get(i);
151+
Type genericType = parameterGenericTypes.get(i);
141152
PropertyModelBuilder<?> propertyModelBuilder = null;
142153

143154
if (isIdProperty) {
@@ -168,6 +179,7 @@ private <T> void processCreatorAnnotation(final ClassModelBuilder<T> classModelB
168179
} else {
169180
// An existing property is found, set its write name
170181
propertyModelBuilder.writeName(bsonProperty.value());
182+
tryToExpandToGenericType(parameterType, propertyModelBuilder, genericType);
171183
}
172184
}
173185

@@ -180,6 +192,17 @@ private <T> void processCreatorAnnotation(final ClassModelBuilder<T> classModelB
180192
}
181193
}
182194

195+
@SuppressWarnings("unchecked")
196+
private static <T> void tryToExpandToGenericType(final Class<?> parameterType, final PropertyModelBuilder<T> propertyModelBuilder,
197+
final Type genericType) {
198+
if (parameterType.isAssignableFrom(propertyModelBuilder.getTypeData().getType())) {
199+
// The existing getter for this field returns a more specific type than what the constructor accepts
200+
// This is typical when the getter returns a specific subtype, but the constructor accepts a more
201+
// general one (e.g.: getter returns ImmutableList<T>, while constructor just accepts List<T>)
202+
propertyModelBuilder.typeData(TypeData.newInstance(genericType, (Class<T>) parameterType));
203+
}
204+
}
205+
183206
private <T, S> PropertyModelBuilder<S> addCreatorPropertyToClassModelBuilder(final ClassModelBuilder<T> classModelBuilder,
184207
final String name,
185208
final Class<S> clazz) {

bson/src/main/org/bson/codecs/pojo/CreatorExecutable.java

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.lang.annotation.Annotation;
2424
import java.lang.reflect.Constructor;
2525
import java.lang.reflect.Method;
26+
import java.lang.reflect.Type;
2627
import java.util.ArrayList;
2728
import java.util.List;
2829

@@ -36,6 +37,7 @@ final class CreatorExecutable<T> {
3637
private final List<BsonProperty> properties = new ArrayList<BsonProperty>();
3738
private final Integer idPropertyIndex;
3839
private final List<Class<?>> parameterTypes = new ArrayList<Class<?>>();
40+
private final List<Type> parameterGenericTypes = new ArrayList<Type>();
3941

4042
CreatorExecutable(final Class<T> clazz, final Constructor<T> constructor) {
4143
this(clazz, constructor, null);
@@ -52,7 +54,10 @@ private CreatorExecutable(final Class<T> clazz, final Constructor<T> constructor
5254
Integer idPropertyIndex = null;
5355

5456
if (constructor != null || method != null) {
55-
parameterTypes.addAll(asList(constructor != null ? constructor.getParameterTypes() : method.getParameterTypes()));
57+
Class<?>[] paramTypes = constructor != null ? constructor.getParameterTypes() : method.getParameterTypes();
58+
Type[] genericParamTypes = constructor != null ? constructor.getGenericParameterTypes() : method.getGenericParameterTypes();
59+
parameterTypes.addAll(asList(paramTypes));
60+
parameterGenericTypes.addAll(asList(genericParamTypes));
5661
Annotation[][] parameterAnnotations = constructor != null ? constructor.getParameterAnnotations()
5762
: method.getParameterAnnotations();
5863

@@ -93,6 +98,10 @@ List<Class<?>> getParameterTypes() {
9398
return parameterTypes;
9499
}
95100

101+
List<Type> getParameterGenericTypes() {
102+
return parameterGenericTypes;
103+
}
104+
96105
@SuppressWarnings("unchecked")
97106
T getInstance() {
98107
checkHasAnExecutable();

0 commit comments

Comments
 (0)