Deprecate `Parameterizable`, introduce default `CodecProvider.get(Class<T>, List<Type>, CodecRegistry)` instead #1115

stIncMale · 2023-05-12T08:00:58Z

Turns out, we don't even need to introduce ParameterizedCodecProvider to replace Parameterizable. All we need is a new default method on CodecProvider. And the cost of still supporting Parameterizable is really small: a single if block in ProvidersCodecRegistry.getFromCodecProvider.

I also refactored RecordCodec, CollectionCodec, MapCodecV2, org.bson.codecs.kotlin.DataClassCodec such that they no longer implement Parameterizable.

How is this related to JAVA-4954?

JAVA-4954 is about trying to improve PojoCodecImpl to be more like RecordCodec. Given that we discovered a better alternative to Parameterizable, which can be implemented without breaking public API, it seems worth making the improvement before trying to refactor PojoCodecImpl. Otherwise, we will likely have to refactor PojoCodecImpl again in the future.

JAVA-4967

…rguments, CodecRegistry registry)` Refactor `RecordCodec` and `CollectionCodec`. `MapCodecV2` and `RawDataClassCodec` are still `Parameterizable` and are working. JAVA-4954

JAVA-4954

stIncMale · 2023-05-12T08:04:35Z

bson/src/main/org/bson/codecs/configuration/CodecProvider.java

    /**
     * Get a {@code Codec} using the given context, which includes, most importantly, the Class for which a {@code Codec} is required.
     *
+     * <p>This method is called only if {@link #get(Class, List, CodecRegistry)} is not properly overridden.</p>
+     *
     * @param clazz the Class for which to get a Codec
     * @param registry the registry to use for resolving dependent Codec instances
     * @param <T> the type of the class for which a Codec is required
     * @return the Codec instance, which may be null, if this source is unable to provide one for the requested Class
     */
    <T> Codec<T> get(Class<T> clazz, CodecRegistry registry);


The driver no longer calls the "old" CodecProvider.get, i.e., we may even deprecate this method in the future and remove in 5.0, thus going back to having only one method on CodecProvider.

stIncMale · 2023-05-12T08:15:13Z

bson/src/main/org/bson/codecs/configuration/CodecProvider.java

+    default <T> Codec<T> get(Class<T> clazz, List<Type> typeArguments, CodecRegistry registry) {
+        return get(clazz, registry);
+    }


We don't have Beta in bson, but maybe we should. This will allow us to mark the new method as @Beta(Beta.Reason.CLIENT) and give ourselves more time to realize whether the signature of the method should be different. @jyemin What do you think?

I'm happy enough with this signature that I'm willing to go with it as is.

We were mistakenly happy previously more than once about things in this domain. Let's hope, this is the last time)

stIncMale · 2023-05-12T08:45:32Z

bson/src/main/org/bson/codecs/CollectionCodec.java

@@ -47,7 +44,7 @@
 */
 @SuppressWarnings("rawtypes")
 final class CollectionCodec<C extends Collection<Object>> extends AbstractCollectionCodec<Object, C>
-        implements OverridableUuidRepresentationCodec<C>, Parameterizable {
+        implements OverridableUuidRepresentationCodec<C> {


I noticed that while CollectionCodec implements OverridableUuidRepresentationCodec, ParameterizedCollectionCodec does not. The same goes about MapCodecV2 and ParameterizedMapCodec. Is this intentional / OK?

It's intentional

JAVA-4967

stIncMale · 2023-05-16T22:23:53Z

bson-kotlin/src/main/kotlin/org/bson/codecs/kotlin/DataClassCodecProvider.kt

 public class DataClassCodecProvider : CodecProvider {
    override fun <T : Any> get(clazz: Class<T>, registry: CodecRegistry): Codec<T>? =
-        DataClassCodec.create(clazz.kotlin, registry)
+        get(clazz, emptyList(), registry)
+
+    override fun <T : Any> get(clazz: Class<T>, typeArguments: List<Type>, registry: CodecRegistry): Codec<T>? =
+        DataClassCodec.create(clazz.kotlin, registry, typeArguments)


If this provider is rolled back to the previous implementation, some tests in DataClassCodecTest (for example, testDataClassWithParameterizedDataClass), which this PR did not modify, start failing with CodecConfigurationException: Could not find codec for number with type N. I.e., they do test parameterization and it appear to work when the provider properly overrides the new get method.

JAVA-4967

stIncMale · 2023-05-17T02:50:27Z

bson/src/main/org/bson/internal/ProvidersCodecRegistry.java

@@ -51,40 +53,55 @@ public <T> Codec<T> get(final Class<T> clazz) {
    @Override
    public <T> Codec<T> get(final Class<T> clazz, final List<Type> typeArguments) {
        notNull("typeArguments", typeArguments);
-        isTrueArgument("typeArguments is not empty", !typeArguments.isEmpty());


This assertion and the similar assertion in ChildCodecRegistry.get contradicted the documentation of ChildCodecRegistry.get, which explicitly allows empty typeArguments (this PR has not changed that documentation). While all our tests happened to pass despite the assertions existing, users can pass empty typeArguments and get an unjustified error.

jyemin

Very nice improvement.

jyemin · 2023-05-18T15:33:16Z

bson-record-codec/src/main/org/bson/codecs/record/RecordCodec.java

-    RecordCodec(final Class<T> clazz, final CodecRegistry codecRegistry, final List<Type> types) {
-        if (types.size() != clazz.getTypeParameters().length || types.isEmpty()) {
+    RecordCodec(final Class<T> clazz, final List<Type> types, final CodecRegistry codecRegistry) {
+        if (types.size() != clazz.getTypeParameters().length) {


This is so much nicer

jyemin · 2023-05-18T15:35:08Z

bson/src/main/org/bson/codecs/CollectionCodec.java

@@ -47,7 +44,7 @@
 */
 @SuppressWarnings("rawtypes")
 final class CollectionCodec<C extends Collection<Object>> extends AbstractCollectionCodec<Object, C>
-        implements OverridableUuidRepresentationCodec<C>, Parameterizable {
+        implements OverridableUuidRepresentationCodec<C> {


It's intentional

bson/src/main/org/bson/codecs/configuration/CodecProvider.java

jyemin · 2023-05-18T16:13:01Z

bson/src/main/org/bson/codecs/configuration/CodecProvider.java

+    default <T> Codec<T> get(Class<T> clazz, List<Type> typeArguments, CodecRegistry registry) {
+        return get(clazz, registry);
+    }


I'm happy enough with this signature that I'm willing to go with it as is.

JAVA-4967

stIncMale added 2 commits May 12, 2023 01:44

Introduce default `CodecProvider.get(Class<T> clazz, List<Type> typeA…

2df14bd

…rguments, CodecRegistry registry)` Refactor `RecordCodec` and `CollectionCodec`. `MapCodecV2` and `RawDataClassCodec` are still `Parameterizable` and are working. JAVA-4954

Refactor MapCodecV2

30670c8

JAVA-4954

stIncMale self-assigned this May 12, 2023

stIncMale commented May 12, 2023

View reviewed changes

stIncMale requested a review from jyemin May 12, 2023 08:12

stIncMale commented May 12, 2023

View reviewed changes

Refactor DataClassCodec

1723be4

JAVA-4967

stIncMale commented May 16, 2023

View reviewed changes

Remove assertions that contradict the CodecRegistry.get documentation

4902743

JAVA-4967

stIncMale commented May 17, 2023

View reviewed changes

jyemin requested changes May 19, 2023

View reviewed changes

stIncMale added 2 commits May 31, 2023 09:08

Improve CodecProvider's docs

fe1b60c

JAVA-4967

Improve CodecProvider's docs

8ccf30c

JAVA-4967

stIncMale requested a review from jyemin May 31, 2023 15:15

jyemin approved these changes Jun 1, 2023

View reviewed changes

stIncMale merged commit 77b549d into mongodb:master Jun 2, 2023

stIncMale deleted the JAVA-4954 branch June 2, 2023 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `Parameterizable`, introduce default `CodecProvider.get(Class<T>, List<Type>, CodecRegistry)` instead #1115

Deprecate `Parameterizable`, introduce default `CodecProvider.get(Class<T>, List<Type>, CodecRegistry)` instead #1115

stIncMale commented May 12, 2023 •

edited

Loading

stIncMale May 12, 2023 •

edited

Loading

stIncMale May 12, 2023

jyemin May 18, 2023

stIncMale May 31, 2023

stIncMale May 12, 2023

jyemin May 18, 2023

stIncMale May 16, 2023

stIncMale May 17, 2023

jyemin left a comment

jyemin May 18, 2023

jyemin May 18, 2023

jyemin May 18, 2023

Deprecate Parameterizable, introduce default CodecProvider.get(Class<T>, List<Type>, CodecRegistry) instead #1115

Deprecate Parameterizable, introduce default CodecProvider.get(Class<T>, List<Type>, CodecRegistry) instead #1115

Conversation

stIncMale commented May 12, 2023 • edited Loading

How is this related to JAVA-4954?

stIncMale May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyemin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deprecate `Parameterizable`, introduce default `CodecProvider.get(Class<T>, List<Type>, CodecRegistry)` instead #1115

Deprecate `Parameterizable`, introduce default `CodecProvider.get(Class<T>, List<Type>, CodecRegistry)` instead #1115

stIncMale commented May 12, 2023 •

edited

Loading

stIncMale May 12, 2023 •

edited

Loading