From d7637d310f3cf3693901bfb48f9039f1c8f200d6 Mon Sep 17 00:00:00 2001 From: Joseph Florencio Date: Thu, 13 Aug 2020 18:28:30 -0700 Subject: [PATCH 1/2] Pojo Codec: Detect property models on extended interfaces If you have an interface like: ``` public interface SampleInterface { int getFirst(); String getSecond(); } ``` which is implemented as: ``` public abstract class SampleImplementor implements SampleInterface { public abstract boolean isThird(); } ``` With a concrete implementation of SampleImplementor. When `PojoBuilderHelper` goes to create the property models, it only checks for methods on the current class, and super classes - not interfaces. In the above example, this means the property model will only have "third", and not "first" or "second". Today, you can manually get around that by creating a @BsonCreator by hand: ``` public abstract class SampleImplementor implements SampleInterface { @BsonCreator public static SampleImplementor newInstance( @BsonProperty("first") int first, @BsonProperty("second") String second, @BsonProperty("third") boolean third) { return new SampleImplementorImpl(first, second, third); } public abstract boolean isThird(); } ``` The presence of the `@BsonProperty` on the `@BsonCreator` method will create the property models. Conversely though, if you want to leverage a Convention implementation to dynamically create a `InstanceCreator` that knows how to find `SampleImplementorImpl` above, you won't be able to. `InstanceCreator` only is provided properties for which `PropertyModel` exists, so above, since `PojoBuilderHelper` didn't discover the interface fields, and there is no exposed API to add property models, your `InstanceCreator` will never be provided the `first` and `second` fields present on the interface. Simply put, if you provide the pojo codec a class that is not concrete, extends an interface for methods, and does not have a @BsonCreator annotation, there is no way to implement a InstanceCreator implementation that works. We've worked around this problem for years and created 700+ hand written @BsonCreator annotations, but, enough is enough :-) This fix is relatively straight forward: Update `PojoBuilderHelper` to scan implementing classes and interfaces, which provides a fully populated property model. --- .../bson/codecs/pojo/PojoBuilderHelper.java | 39 ++++++++-- .../org/bson/codecs/pojo/PojoCustomTest.java | 17 ++++- .../pojo/entities/ComposeInterfaceModel.java | 74 +++++++++++++++++++ .../pojo/entities/InterfaceModelImpl.java | 10 ++- ...erfaceModelBInstanceCreatorConvention.java | 54 ++++++++++++++ 5 files changed, 184 insertions(+), 10 deletions(-) create mode 100644 bson/src/test/unit/org/bson/codecs/pojo/entities/ComposeInterfaceModel.java create mode 100644 bson/src/test/unit/org/bson/codecs/pojo/entities/conventions/InterfaceModelBInstanceCreatorConvention.java diff --git a/bson/src/main/org/bson/codecs/pojo/PojoBuilderHelper.java b/bson/src/main/org/bson/codecs/pojo/PojoBuilderHelper.java index a1b55290ac0..f0261ad9a9d 100644 --- a/bson/src/main/org/bson/codecs/pojo/PojoBuilderHelper.java +++ b/bson/src/main/org/bson/codecs/pojo/PojoBuilderHelper.java @@ -25,6 +25,7 @@ import java.lang.reflect.TypeVariable; import java.util.ArrayList; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -50,12 +51,12 @@ static void configureClassModelBuilder(final ClassModelBuilder classModel ArrayList annotations = new ArrayList(); Set propertyNames = new TreeSet(); Map propertyTypeParameterMap = new HashMap(); - Class currentClass = clazz; String declaringClassName = clazz.getSimpleName(); - TypeData parentClassTypeData = null; Map> propertyNameMap = new HashMap>(); - while (!currentClass.isEnum() && currentClass.getSuperclass() != null) { + for (ClassWithParentTypeData currentClassWithParentTypeData : getClassHierarchy(clazz, null)) { + Class currentClass = currentClassWithParentTypeData.clazz; + TypeData parentClassTypeData = currentClassWithParentTypeData.parentClassTypeData; annotations.addAll(asList(currentClass.getDeclaredAnnotations())); List genericTypeNames = new ArrayList(); for (TypeVariable> classTypeVariable : currentClass.getTypeParameters()) { @@ -118,11 +119,6 @@ static void configureClassModelBuilder(final ClassModelBuilder classModel } parentClassTypeData = TypeData.newInstance(currentClass.getGenericSuperclass(), currentClass); - currentClass = currentClass.getSuperclass(); - } - - if (currentClass.isInterface()) { - annotations.addAll(asList(currentClass.getDeclaredAnnotations())); } for (String propertyName : propertyNames) { @@ -261,6 +257,33 @@ static V stateNotNull(final String property, final V value) { return value; } + @SuppressWarnings("unchecked") + private static Set> getClassHierarchy(final Class clazz, + final TypeData classTypeData) { + Set> classesToScan = new LinkedHashSet<>(); + Class currentClass = clazz; + TypeData parentClassTypeData = classTypeData; + while (currentClass != null && !currentClass.isEnum() && !currentClass.equals(Object.class)) { + classesToScan.add(new ClassWithParentTypeData<>(currentClass, parentClassTypeData)); + parentClassTypeData = TypeData.newInstance(currentClass.getGenericSuperclass(), currentClass); + for (Class interfaceClass : currentClass.getInterfaces()) { + classesToScan.addAll(getClassHierarchy((Class) interfaceClass, parentClassTypeData)); + } + currentClass = currentClass.getSuperclass(); + } + return classesToScan; + } + + private static final class ClassWithParentTypeData { + private final Class clazz; + private final TypeData parentClassTypeData; + + private ClassWithParentTypeData(final Class clazz, final TypeData parentClassTypeData) { + this.clazz = clazz; + this.parentClassTypeData = parentClassTypeData; + } + } + private PojoBuilderHelper() { } } diff --git a/bson/src/test/unit/org/bson/codecs/pojo/PojoCustomTest.java b/bson/src/test/unit/org/bson/codecs/pojo/PojoCustomTest.java index 1d9213a0c81..5b38b7e7529 100644 --- a/bson/src/test/unit/org/bson/codecs/pojo/PojoCustomTest.java +++ b/bson/src/test/unit/org/bson/codecs/pojo/PojoCustomTest.java @@ -30,6 +30,7 @@ import org.bson.codecs.pojo.entities.AsymmetricalCreatorModel; import org.bson.codecs.pojo.entities.AsymmetricalIgnoreModel; import org.bson.codecs.pojo.entities.AsymmetricalModel; +import org.bson.codecs.pojo.entities.ComposeInterfaceModel; import org.bson.codecs.pojo.entities.ConcreteAndNestedAbstractInterfaceModel; import org.bson.codecs.pojo.entities.ConcreteCollectionsModel; import org.bson.codecs.pojo.entities.ConcreteStandAloneAbstractInterfaceModel; @@ -39,6 +40,8 @@ import org.bson.codecs.pojo.entities.CustomPropertyCodecOptionalModel; import org.bson.codecs.pojo.entities.GenericTreeModel; import org.bson.codecs.pojo.entities.InterfaceBasedModel; +import org.bson.codecs.pojo.entities.InterfaceModelB; +import org.bson.codecs.pojo.entities.InterfaceModelImpl; import org.bson.codecs.pojo.entities.InvalidCollectionModel; import org.bson.codecs.pojo.entities.InvalidGetterAndSetterModel; import org.bson.codecs.pojo.entities.InvalidMapModel; @@ -65,6 +68,7 @@ import org.bson.codecs.pojo.entities.conventions.CreatorConstructorPrimitivesModel; import org.bson.codecs.pojo.entities.conventions.CreatorConstructorThrowsExceptionModel; import org.bson.codecs.pojo.entities.conventions.CreatorMethodThrowsExceptionModel; +import org.bson.codecs.pojo.entities.conventions.InterfaceModelBInstanceCreatorConvention; import org.bson.codecs.pojo.entities.conventions.MapGetterImmutableModel; import org.bson.codecs.pojo.entities.conventions.MapGetterMutableModel; import org.bson.codecs.pojo.entities.conventions.MapGetterNonEmptyModel; @@ -499,7 +503,7 @@ public void testDecodingInvalidMapModel() { public void testEncodingInvalidCollectionModel() { try { encodesTo(getPojoCodecProviderBuilder(InvalidCollectionModel.class), new InvalidCollectionModel(asList(1, 2, 3)), - "{collectionField: [1, 2, 3]}"); + "{collectionField: [1, 2, 3]}"); } catch (CodecConfigurationException e) { assertTrue(e.getMessage().startsWith("Failed to encode 'InvalidCollectionModel'. Encoding 'collectionField' errored with:")); throw e; @@ -512,6 +516,17 @@ public void testInvalidMapModelWithCustomPropertyCodecProvider() { "{'invalidMap': {'1': 1, '2': 2}}"); } + @Test + public void testInterfaceModelCreatorMadeInConvention() { + roundTrip( + getPojoCodecProviderBuilder(ComposeInterfaceModel.class, InterfaceModelB.class, InterfaceModelImpl.class) + .conventions(Collections.singletonList(new InterfaceModelBInstanceCreatorConvention())), + new ComposeInterfaceModel("someTitle", + new InterfaceModelImpl("a", "b")), + "{'title': 'someTitle', 'nestedModel': {'propertyA': 'a', 'propertyB': 'b'}}" + ); + } + @Test(expected = CodecConfigurationException.class) public void testConstructorNotPublicModel() { decodingShouldFail(getCodec(ConstructorNotPublicModel.class), "{'integerField': 99}"); diff --git a/bson/src/test/unit/org/bson/codecs/pojo/entities/ComposeInterfaceModel.java b/bson/src/test/unit/org/bson/codecs/pojo/entities/ComposeInterfaceModel.java new file mode 100644 index 00000000000..b2d7beb765d --- /dev/null +++ b/bson/src/test/unit/org/bson/codecs/pojo/entities/ComposeInterfaceModel.java @@ -0,0 +1,74 @@ +/* + * Copyright 2008-present MongoDB, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.bson.codecs.pojo.entities; + +import java.util.Objects; + +public class ComposeInterfaceModel { + private String title; + private InterfaceModelB nestedModel; + + public ComposeInterfaceModel() { + } + + public ComposeInterfaceModel(final String title, final InterfaceModelB nestedModel) { + this.title = title; + this.nestedModel = nestedModel; + } + + public String getTitle() { + return title; + } + + public void setTitle(final String title) { + this.title = title; + } + + public InterfaceModelB getNestedModel() { + return nestedModel; + } + + public void setNestedModel(final InterfaceModelB nestedModel) { + this.nestedModel = nestedModel; + } + + @Override + public boolean equals(final Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + ComposeInterfaceModel that = (ComposeInterfaceModel) o; + return Objects.equals(title, that.title) + && Objects.equals(nestedModel, that.nestedModel); + } + + @Override + public int hashCode() { + return Objects.hash(title, nestedModel); + } + + @Override + public String toString() { + return "ComposeInterfaceModel{" + + "title='" + title + '\'' + + ", nestedModel=" + nestedModel + + '}'; + } +} diff --git a/bson/src/test/unit/org/bson/codecs/pojo/entities/InterfaceModelImpl.java b/bson/src/test/unit/org/bson/codecs/pojo/entities/InterfaceModelImpl.java index 90828dc8e6e..9db110c6115 100644 --- a/bson/src/test/unit/org/bson/codecs/pojo/entities/InterfaceModelImpl.java +++ b/bson/src/test/unit/org/bson/codecs/pojo/entities/InterfaceModelImpl.java @@ -63,7 +63,15 @@ public boolean equals(final Object o) { @Override public int hashCode() { int result = getPropertyA() != null ? getPropertyA().hashCode() : 0; - result = 31 * result + getPropertyB() != null ? getPropertyB().hashCode() : 0; + result = 31 * result + (getPropertyB() != null ? getPropertyB().hashCode() : 0); return result; } + + @Override + public String toString() { + return "InterfaceModelImpl{" + + "propertyA='" + getPropertyA() + "', " + + "propertyB='" + getPropertyB() + '\'' + + '}'; + } } diff --git a/bson/src/test/unit/org/bson/codecs/pojo/entities/conventions/InterfaceModelBInstanceCreatorConvention.java b/bson/src/test/unit/org/bson/codecs/pojo/entities/conventions/InterfaceModelBInstanceCreatorConvention.java new file mode 100644 index 00000000000..88781c40513 --- /dev/null +++ b/bson/src/test/unit/org/bson/codecs/pojo/entities/conventions/InterfaceModelBInstanceCreatorConvention.java @@ -0,0 +1,54 @@ +/* + * Copyright 2008-present MongoDB, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.bson.codecs.pojo.entities.conventions; + +import org.bson.codecs.pojo.ClassModelBuilder; +import org.bson.codecs.pojo.Convention; +import org.bson.codecs.pojo.InstanceCreator; +import org.bson.codecs.pojo.PropertyModel; +import org.bson.codecs.pojo.entities.InterfaceModelB; +import org.bson.codecs.pojo.entities.InterfaceModelImpl; + +public class InterfaceModelBInstanceCreatorConvention implements Convention { + @Override + @SuppressWarnings("unchecked") + public void apply(final ClassModelBuilder classModelBuilder) { + if (classModelBuilder.getType().equals(InterfaceModelB.class)) { + // Simulate a custom implementation of InstanceCreator factory + // (This one can be generated automatically, but, a real use case can have an advanced reflection based + // solution that the POJO Codec doesn't support out of the box) + ((ClassModelBuilder) classModelBuilder).instanceCreatorFactory(() -> { + InterfaceModelB interfaceModelB = new InterfaceModelImpl(); + return new InstanceCreator() { + @Override + public void set(final S value, final PropertyModel propertyModel) { + if (propertyModel.getName().equals("propertyA")) { + interfaceModelB.setPropertyA((String) value); + } else if (propertyModel.getName().equals("propertyB")) { + interfaceModelB.setPropertyB((String) value); + } + } + + @Override + public InterfaceModelB getInstance() { + return interfaceModelB; + } + }; + }); + } + } +} From 84010e7b005046bc596bd46e1158d05bc25415d2 Mon Sep 17 00:00:00 2001 From: Joseph Florencio Date: Tue, 18 Aug 2020 15:50:16 -0700 Subject: [PATCH 2/2] Remove redundant parentClassTypeData line --- bson/src/main/org/bson/codecs/pojo/PojoBuilderHelper.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/bson/src/main/org/bson/codecs/pojo/PojoBuilderHelper.java b/bson/src/main/org/bson/codecs/pojo/PojoBuilderHelper.java index f0261ad9a9d..9bcce174380 100644 --- a/bson/src/main/org/bson/codecs/pojo/PojoBuilderHelper.java +++ b/bson/src/main/org/bson/codecs/pojo/PojoBuilderHelper.java @@ -117,8 +117,6 @@ static void configureClassModelBuilder(final ClassModelBuilder classModel } } } - - parentClassTypeData = TypeData.newInstance(currentClass.getGenericSuperclass(), currentClass); } for (String propertyName : propertyNames) {