Skip to content

Remove specialization-related conditional logic from PojoCodecImpl by introducing LazyPropertyModelCodec.NeedsSpecializationCodec #1136

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

Merged
merged 1 commit into from
Jun 9, 2023
Merged

Remove specialization-related conditional logic from PojoCodecImpl by introducing LazyPropertyModelCodec.NeedsSpecializationCodec #1136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 45 additions & 4 deletions bson/src/main/org/bson/codecs/pojo/LazyPropertyModelCodec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,21 @@
import java.util.ArrayList;
import java.util.List;

import static java.lang.String.format;
import static org.bson.codecs.pojo.PojoSpecializationHelper.specializeTypeData;

class LazyPropertyModelCodec<T> implements Codec<T> {
private final PropertyModel<T> propertyModel;
private final CodecRegistry registry;
private final PropertyCodecRegistry propertyCodecRegistry;
private final DiscriminatorLookup discriminatorLookup;

private volatile Codec<T> codec;

LazyPropertyModelCodec(final PropertyModel<T> propertyModel, final CodecRegistry registry,
final PropertyCodecRegistry propertyCodecRegistry, final DiscriminatorLookup discriminatorLookup) {
final PropertyCodecRegistry propertyCodecRegistry) {
this.propertyModel = propertyModel;
this.registry = registry;
this.propertyCodecRegistry = propertyCodecRegistry;
this.discriminatorLookup = discriminatorLookup;
}

@Override
Expand Down Expand Up @@ -77,7 +76,7 @@ private Codec<T> createCodec() {
if (localCodec instanceof PojoCodec) {
PojoCodec<T> pojoCodec = (PojoCodec<T>) localCodec;
ClassModel<T> specialized = getSpecializedClassModel(pojoCodec.getClassModel(), propertyModel);
localCodec = new PojoCodecImpl<>(specialized, registry, propertyCodecRegistry, pojoCodec.getDiscriminatorLookup(), true);
localCodec = new PojoCodecImpl<>(specialized, registry, propertyCodecRegistry, pojoCodec.getDiscriminatorLookup());
}
return localCodec;
}
Expand Down Expand Up @@ -150,4 +149,46 @@ private <V> PropertyModel<V> getSpecializedPropertyModel(final PropertyModel<V>
propertyModel.getPropertyAccessor(), propertyModel.getError(), propertyModel.getBsonRepresentation());
}

/**
* Instances of this codec are supposed to be replaced with usable implementations by {@link LazyPropertyModelCodec#createCodec()}.
*/
static final class NeedSpecializationCodec<T> extends PojoCodec<T> {
private final ClassModel<T> classModel;
private final DiscriminatorLookup discriminatorLookup;

NeedSpecializationCodec(final ClassModel<T> classModel, final DiscriminatorLookup discriminatorLookup) {
this.classModel = classModel;
this.discriminatorLookup = discriminatorLookup;
}

@Override
public T decode(final BsonReader reader, final DecoderContext decoderContext) {
throw exception();
}

@Override
public void encode(final BsonWriter writer, final T value, final EncoderContext encoderContext) {
throw exception();
}

@Override
public Class<T> getEncoderClass() {
return classModel.getType();
}

private CodecConfigurationException exception() {
return new CodecConfigurationException(format("%s contains generic types that have not been specialised.%n"
+ "Top level classes with generic types are not supported by the PojoCodec.", classModel.getName()));
}

@Override
ClassModel<T> getClassModel() {
return classModel;
}

@Override
DiscriminatorLookup getDiscriminatorLookup() {
return discriminatorLookup;
}
}
}
39 changes: 5 additions & 34 deletions bson/src/main/org/bson/codecs/pojo/PojoCodecImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,36 +50,28 @@ final class PojoCodecImpl<T> extends PojoCodec<T> {
private final CodecRegistry registry;
private final PropertyCodecRegistry propertyCodecRegistry;
private final DiscriminatorLookup discriminatorLookup;
private final boolean specialized;

PojoCodecImpl(final ClassModel<T> classModel, final CodecRegistry codecRegistry,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the constructors can now be chained - ie. this one calls the below.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wish ... : one of them (PojoCodecImpl(final ClassModel<T> classModel, final CodecRegistry codecRegistry, final List<PropertyCodecProvider> propertyCodecProviders, final DiscriminatorLookup discriminatorLookup)) has to give this to a new PropertyCodecRegistry, which means that this constructor can neither be called from the other one, nor the other one can call this one.

As far as I can see, only refactoring of the code around PojoCodecImpl may open up opportunities to either calling one of the constructors from the other, or getting rid of one of them. But I don't even know if such refactoring is possible, and if yes, what it will look like.

final List<PropertyCodecProvider> propertyCodecProviders, final DiscriminatorLookup discriminatorLookup) {
final List<PropertyCodecProvider> propertyCodecProviders, final DiscriminatorLookup discriminatorLookup) {
this.classModel = classModel;
this.registry = codecRegistry;
this.discriminatorLookup = discriminatorLookup;
this.propertyCodecRegistry = new PropertyCodecRegistryImpl(this, registry, propertyCodecProviders);
this.specialized = shouldSpecialize(classModel);
specialize();
}

PojoCodecImpl(final ClassModel<T> classModel, final CodecRegistry codecRegistry, final PropertyCodecRegistry propertyCodecRegistry,
final DiscriminatorLookup discriminatorLookup, final boolean specialized) {
PojoCodecImpl(final ClassModel<T> classModel, final CodecRegistry codecRegistry,
final PropertyCodecRegistry propertyCodecRegistry, final DiscriminatorLookup discriminatorLookup) {
this.classModel = classModel;
this.registry = codecRegistry;
this.discriminatorLookup = discriminatorLookup;
this.propertyCodecRegistry = propertyCodecRegistry;
this.specialized = specialized;
specialize();
Copy link
Member

Choose a reason for hiding this comment

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

Can this specialize() logic now be moved into the constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless we want to duplicate a bunch of code, this can only be done if one of the constructors reuses the other one, which does not seem possible.

}

@SuppressWarnings("unchecked")
@Override
public void encode(final BsonWriter writer, final T value, final EncoderContext encoderContext) {
if (!specialized) {
throw new CodecConfigurationException(format("%s contains generic types that have not been specialised.%n"
+ "Top level classes with generic types are not supported by the PojoCodec.", classModel.getName()));
}

if (areEquivalentTypes(value.getClass(), classModel.getType())) {
writer.writeStartDocument();

Expand All @@ -104,10 +96,6 @@ public void encode(final BsonWriter writer, final T value, final EncoderContext
@Override
public T decode(final BsonReader reader, final DecoderContext decoderContext) {
if (decoderContext.hasCheckedDiscriminator()) {
if (!specialized) {
throw new CodecConfigurationException(format("%s contains generic types that have not been specialised.%n"
+ "Top level classes with generic types are not supported by the PojoCodec.", classModel.getName()));
}
InstanceCreator<T> instanceCreator = classModel.getInstanceCreator();
decodeProperties(reader, decoderContext, instanceCreator);
return instanceCreator.getInstance();
Expand Down Expand Up @@ -264,15 +252,13 @@ private <S> void setPropertyValueBsonExtraElements(final InstanceCreator<T> inst
}

private void specialize() {
if (specialized) {
classModel.getPropertyModels().forEach(this::cachePropertyModelCodec);
}
classModel.getPropertyModels().forEach(this::cachePropertyModelCodec);
}

private <S> void cachePropertyModelCodec(final PropertyModel<S> propertyModel) {
if (propertyModel.getCachedCodec() == null) {
Codec<S> codec = propertyModel.getCodec() != null ? propertyModel.getCodec()
: new LazyPropertyModelCodec<>(propertyModel, registry, propertyCodecRegistry, discriminatorLookup);
: new LazyPropertyModelCodec<>(propertyModel, registry, propertyCodecRegistry);
propertyModel.cachedCodec(codec);
}
}
Expand Down Expand Up @@ -328,21 +314,6 @@ private PropertyModel<?> getPropertyModelByWriteName(final ClassModel<T> classMo
return null;
}

private static <T> boolean shouldSpecialize(final ClassModel<T> classModel) {
if (!classModel.hasTypeParameters()) {
return true;
}

for (Map.Entry<String, TypeParameterMap> entry : classModel.getPropertyNameToTypeParameterMap().entrySet()) {
TypeParameterMap typeParameterMap = entry.getValue();
PropertyModel<?> propertyModel = classModel.getPropertyModel(entry.getKey());
if (typeParameterMap.hasTypeParameters() && (propertyModel == null || propertyModel.getCodec() == null)) {
return false;
}
}
return true;
}

@Override
DiscriminatorLookup getDiscriminatorLookup() {
return discriminatorLookup;
Expand Down
30 changes: 26 additions & 4 deletions bson/src/main/org/bson/codecs/pojo/PojoCodecProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,20 +69,20 @@ public static Builder builder() {

@Override
public <T> Codec<T> get(final Class<T> clazz, final CodecRegistry registry) {
return getPojoCodec(clazz, registry);
return createCodec(clazz, registry);
}

@SuppressWarnings("unchecked")
private <T> PojoCodec<T> getPojoCodec(final Class<T> clazz, final CodecRegistry registry) {
private <T> PojoCodec<T> createCodec(final Class<T> clazz, final CodecRegistry registry) {
ClassModel<T> classModel = (ClassModel<T>) classModels.get(clazz);
if (classModel != null) {
return new PojoCodecImpl<>(classModel, registry, propertyCodecProviders, discriminatorLookup);
return createCodec(classModel, registry, propertyCodecProviders, discriminatorLookup);
} else if (automatic || (clazz.getPackage() != null && packages.contains(clazz.getPackage().getName()))) {
try {
classModel = createClassModel(clazz, conventions);
if (clazz.isInterface() || !classModel.getPropertyModels().isEmpty()) {
discriminatorLookup.addClassModel(classModel);
return new AutomaticPojoCodec<>(new PojoCodecImpl<>(classModel, registry, propertyCodecProviders,
return new AutomaticPojoCodec<>(createCodec(classModel, registry, propertyCodecProviders,
discriminatorLookup));
}
} catch (Exception e) {
Expand All @@ -93,6 +93,13 @@ private <T> PojoCodec<T> getPojoCodec(final Class<T> clazz, final CodecRegistry
return null;
}

private static <T> PojoCodec<T> createCodec(final ClassModel<T> classModel, final CodecRegistry codecRegistry,
final List<PropertyCodecProvider> propertyCodecProviders, final DiscriminatorLookup discriminatorLookup) {
return shouldSpecialize(classModel)
? new PojoCodecImpl<>(classModel, codecRegistry, propertyCodecProviders, discriminatorLookup)
: new LazyPropertyModelCodec.NeedSpecializationCodec<>(classModel, discriminatorLookup);
}

/**
* A Builder for the PojoCodecProvider
*/
Expand Down Expand Up @@ -218,4 +225,19 @@ private static <T> ClassModel<T> createClassModel(final Class<T> clazz, final Li
}
return builder.build();
}

private static boolean shouldSpecialize(final ClassModel<?> classModel) {
if (!classModel.hasTypeParameters()) {
return true;
}

for (Map.Entry<String, TypeParameterMap> entry : classModel.getPropertyNameToTypeParameterMap().entrySet()) {
TypeParameterMap typeParameterMap = entry.getValue();
PropertyModel<?> propertyModel = classModel.getPropertyModel(entry.getKey());
if (typeParameterMap.hasTypeParameters() && (propertyModel == null || propertyModel.getCodec() == null)) {
return false;
}
}
return true;
}
}
6 changes: 3 additions & 3 deletions bson/src/test/unit/org/bson/codecs/pojo/PojoTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,11 @@ static PojoCodecProvider.Builder getPojoCodecProviderBuilder(final Class<?>... c
return builder;
}

<T> PojoCodecImpl<T> getCodec(final PojoCodecProvider.Builder builder, final Class<T> clazz) {
return (PojoCodecImpl<T>) getCodecRegistry(builder).get(clazz);
<T> PojoCodec<T> getCodec(final PojoCodecProvider.Builder builder, final Class<T> clazz) {
return (PojoCodec<T>) getCodecRegistry(builder).get(clazz);
}

<T> PojoCodecImpl<T> getCodec(final Class<T> clazz) {
<T> PojoCodec<T> getCodec(final Class<T> clazz) {
return getCodec(getPojoCodecProviderBuilder(clazz), clazz);
}

Expand Down