JAVA-3567: Add support for records #581

bdeleonardis1 · 2020-09-24T18:20:45Z

Leaving as a draft for the following reasons:

Testing is not finished. I was unable to get Junit running successfully in preview mode so I wrote my own little test runner. The tests I wrote make me confident that my implementation is correct, but we're going to want to add more coverage before merging this. All of the tests I have written so far are integration tests. I did not write a full suite of tests because I didn't want to waste time writing them in one format, when they'll just need to be converted to Junit tests before this gets merged anyway.
Annotations are not supported. I was having a problem getting the ElementType.RecordComponent to work because of preview mode. If we are able to use ElementType.RecordComponent it will be very easy to support annotations, but if this causes a Java 8 compatibility problem we'll have to think harder about how to do that.
There is a bunch of junk in this diff that I needed to do to get preview mode to work that needs to be deleted before it is merged.

I think it makes the most sense to hold off on fixing these problems until records are generally available instead of preview mode because it will save a lot of time and configuration headaches.

bdeleonardis1 · 2020-09-24T18:34:15Z

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

+            try {
+                getter = clazz.getDeclaredMethod(name);
+            } catch (NoSuchMethodException e) {
+                throw new AssertionError("Record must have getter method with same name as field");


I wasn't sure what to put in the catch block. These calls should never fail because the methods are guaranteed to to exist with records.

Generally we seem to use a CodecConfiguration exception for pretty much all exceptions in Codecs. I think an IllegalStateException would be fine here and below. Also pass the NoSuchMethodException exception on as well.

Got it, done.

bdeleonardis1 · 2020-09-24T18:35:55Z

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

+    /**
+     * A Builder for the RecordCodecProvider
+     */
+    public static final class Builder {


This class unfortunately contains a lot of duplicate code with the Builder in PojoCodecProvider. I considered making an abstract builder, but that would require me to either make the instance variables protected (which the style checker complained about) or add getter methods for all of the instance variables. I figured the extra code that was required to make that work wasn't worth it, but let me know if that was a mistake.

We also may want to consider making an abstract UserDefinedCodecProvider, because RecordCodecProvider and PojoCodecProvider are so similar.

I agree with your decision to copy the code

bdeleonardis1 · 2020-09-24T18:37:59Z

build.gradle

@@ -79,8 +79,8 @@ configure(coreProjects) {
 configure(javaProjects) {
    apply plugin: 'java-library'

-    sourceCompatibility = JavaVersion.VERSION_1_8


You can ignore all this, we'll have to delete it before merging anyway.

bdeleonardis1 · 2020-09-24T18:39:17Z

record-support/src/main/com/mongodb/record/RecordTest.java

+import static org.bson.codecs.configuration.CodecRegistries.fromProviders;
+import static org.bson.codecs.configuration.CodecRegistries.fromRegistries;
+
+public class RecordTest {


This file is very ugly because it won't be merged. You can ignore the test logic, it just implements round trip testing and go straight to the tests if you want to look at them (but like I said those tests aren't comprehensive).

Also I had to put this stuff in a new module because I couldn't get the bson module to run with Java 14 in preview mode. However, the bson module can still access the 14 preview features, if we run the code from a different module.

jyemin · 2020-09-24T18:51:20Z

If we are able to use ElementType.RecordComponent it will be very easy to support annotations, but if this causes a Java 8 compatibility problem we'll have to think harder about how to do that.

I think this will be ok. We will have to compile all the annotations with Java 16, but still target Java 8. This isn't so different with what we do now: compile with Java 11 but target Java 8.

rozza

Looks really good, I had a couple of comments

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

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

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

bdeleonardis1 · 2020-09-28T13:04:03Z

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

-                        properties.add((BsonProperty) annotation);
-                        break;
-                    }
+    static Integer initializeDataFromAnnotations(Annotation[][] parameterAnnotations, List<String> properties) {


A couple things strike me as a little sloppy in this method...

Passing an empty list in, just so things can be added to it

Doing two things at once (initializing properties list and returning the id index)

If I didn't have this method, the code would need to be duplicated in both Constructor and MethodCreatorExecutable. If I broke it into two separate functions it would require two passes over the annotations list.

bdeleonardis1 · 2020-09-28T13:12:16Z

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

    static <T> void configureClassModelBuilder(final ClassModelBuilder<T> classModelBuilder, final Class<T> clazz) {
        classModelBuilder.type(notNull("clazz", clazz));

+        if (isRecord(clazz)) {


@jyemin in the design doc you suggested "inverting the dependency" so that whoever constructed the ClassModelBuilder was responsible for configuring it correctly (i.e. RecordCodecProvider calls configureRecordClassModelBuilder). I implemented this initially, but I realized this would introduce a breaking change.

ClassModel::builder is part of the public API and it constructs a ClassModelBuilder which originally called the configureClassModelBuilder method. To invert the dependency we would get rid of this call. Previously users get a fully configured classModelBuilder just by calling ClassModel#builder, now it isn't configured it all. This is bad because we accept ClassModels in our PojoCodecProvider in one of the register functions. I believe this means we can't invert the dependency so I did it this way instead.

rozza

This is looking good - couple of minor nits.

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

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

rozza · 2020-09-28T13:07:37Z

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

+            try {
+                getter = clazz.getDeclaredMethod(name);
+            } catch (NoSuchMethodException e) {
+                throw new AssertionError("Record must have getter method with same name as field");


Generally we seem to use a CodecConfiguration exception for pretty much all exceptions in Codecs. I think an IllegalStateException would be fine here and below. Also pass the NoSuchMethodException exception on as well.

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

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

bdeleonardis1 · 2020-10-05T16:27:18Z

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

@@ -55,6 +55,7 @@
    private String discriminator;
    private String discriminatorKey;
    private String idPropertyName;
+    private boolean isRecord;


I added this to avoid a call to PojoBuilderHelper::isRecord in ConventionAnnotationImpl. It's not added to the ClassModel that is built.

bdeleonardis1 · 2020-10-05T16:30:41Z

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


-        classModelBuilder.instanceCreatorFactory(new InstanceCreatorFactoryImpl<T>(new CreatorExecutable<T>(clazz, noArgsConstructor)));
+            for (Annotation a : recordComponent.getAnnotations()) {


Because records are immutable, they won't have setters, so we can't really have the read/write breakdown we use for annotations in Pojos. Therefore I just add all the annotations to both the read and write annotations.

Also, by implementing it this way, I am requiring users to put the annotation on the RecordComponent (as opposed to a custom getter if they implement one). This seems reasonable to me, as overriding the getter (or writing a new one) should be pretty rare and I can't think of a reason a user would want to annotate the getter instead of the record component.

bdeleonardis1 · 2020-10-05T16:31:45Z

bson/src/test/unit/org/bson/codecs/pojo/PojoCustomTest.java

@@ -633,7 +633,7 @@ public void testRoundTripWithoutBsonAnnotation() {
        return conventions;
    }

-    class ObjectCodec implements Codec<Object> {
+    static class ObjectCodec implements Codec<Object> {


Necessary in order to reuse ObjectCodec in my RecordCustomTest.

bdeleonardis1 · 2020-10-05T16:33:55Z

bson/src/test/unit/org/bson/codecs/pojo/RecordCustomTest.java

+        encodesTo(getRecordCodecRegistry(BsonRepresentationUnsupportedIntRecord.class), new BsonRepresentationRecord("notanobjectid", 1), null);
+    }
+
+    private static <T> RecordCodecProvider getRecordProviderFromClassModel(ClassModelBuilder<T> classModelBuilder) {


These helpers are in this class as opposed to PojoTestCase because I wanted to keep all record related code out of PojoTestCase in case the record test code needs to go in a separate module. The way I organized it is helpers specific to RecordCustomTest go in RecordCustomTest, helpers specific to RecordRoundTripTest go in RecordRoundTripTest, and tests shared by both of them go in RecordRoundTrip and are package private.

jyemin

It's looking great, and the tests are very thorough. Just a few comments.

Also: so that we don't forget, can you also add the RecordCodecProvider to the default registriy in MongoClientSettings?

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

jyemin · 2020-09-30T18:45:13Z

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

+    /**
+     * A Builder for the RecordCodecProvider
+     */
+    public static final class Builder {


I agree with your decision to copy the code

jyemin · 2020-09-30T18:57:52Z

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

    static <T> void configureClassModelBuilder(final ClassModelBuilder<T> classModelBuilder, final Class<T> clazz) {
        classModelBuilder.type(notNull("clazz", clazz));

+        if (isRecord(clazz)) {


bson/src/main/org/bson/codecs/pojo/annotations/BsonId.java

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

bson/src/test/unit/org/bson/codecs/pojo/Student.java

jyemin

While reviewing the last commit I found a few more nits for you to consider.

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

rozza · 2020-10-08T08:58:20Z

I think this looks great - I'm looking forward to it landing in master!

Great work on migrating all the pojo test models over to records - I forgot how many there are! And for testing the record / pojo interop.

jyemin · 2020-10-08T12:28:24Z

Also LGTM! Great work!

bdeleonardis1 · 2020-10-08T13:51:46Z

This branch has been pushed to the main repo and will be merged once Java 16 is released.

cardouken · 2021-05-27T16:28:56Z

This branch has been pushed to the main repo and will be merged once Java 16 is released.

@bdeleonardis1 Any news now that it's been released for a while?

jyemin · 2021-05-27T17:24:34Z

@cardouken thanks for your interest. This is definitely on our radar to merge, but it turns out there is more work to be done on the build side before it's ready to go. IIRC we have to upgrade Gradle in order to build with Java 16, and that upgrade is not straightforward.

cardouken · 2021-05-27T17:35:45Z

@jyemin thanks for the quick update, looking forward to it coming out.

nikammerlaan · 2021-10-18T20:39:46Z

Any news on this now?

jyemin · 2021-11-24T20:15:18Z

This is unblocked. The driver has been upgraded to Gradle 7.3 and builds with Java 17. So we're going to pick this back up soon.

fmendezp · 2022-02-22T07:21:56Z

@jyemin thanks for the several updates. Do you have any further information about the possibility of this moving forward?

jyemin · 2022-02-28T11:52:51Z

We are still planning to move forward, but encountered a few more build complications. Apologies for the further delays.

fmendezp · 2022-02-28T13:47:47Z

Thank you for the update!
Happy to read that it is still planned to move forward. I understand that this topic comes with its share of complications. I hope it will be possible to solve them.

hfoxy · 2022-08-08T23:00:33Z

Any further news on this?

jyemin · 2022-08-08T23:09:08Z

See JAVA-3567. It was released in 4.6.0.

hfoxy · 2022-08-08T23:41:52Z

I see, thanks!

bdeleonardis1 marked this pull request as draft September 24, 2020 18:21

bdeleonardis1 force-pushed the 3567 branch from 766277c to a529069 Compare September 24, 2020 18:32

bdeleonardis1 commented Sep 24, 2020

View reviewed changes

bdeleonardis1 requested a review from jyemin September 24, 2020 18:45

rozza requested changes Sep 25, 2020

View reviewed changes

bson/src/main/org/bson/codecs/pojo/RecordCodecProvider.java Show resolved Hide resolved

bson/src/main/org/bson/codecs/pojo/PojoBuilderHelper.java Show resolved Hide resolved

bson/src/main/org/bson/codecs/pojo/CreatorExecutable.java Outdated Show resolved Hide resolved

bdeleonardis1 force-pushed the 3567 branch 5 times, most recently from d46d57f to 1ca9c6a Compare September 28, 2020 13:07

bdeleonardis1 commented Sep 28, 2020

View reviewed changes

rozza reviewed Sep 28, 2020

View reviewed changes

bdeleonardis1 force-pushed the 3567 branch from 31b312d to 2fd4dbd Compare October 5, 2020 16:24

bdeleonardis1 commented Oct 5, 2020

View reviewed changes

bdeleonardis1 requested a review from rozza October 5, 2020 16:35

jyemin requested changes Oct 6, 2020

View reviewed changes

bdeleonardis1 force-pushed the 3567 branch 3 times, most recently from d6a4f6d to 1fc710c Compare October 6, 2020 19:00

bdeleonardis1 requested a review from jyemin October 6, 2020 19:07

jyemin reviewed Oct 6, 2020

View reviewed changes

bdeleonardis1 requested a review from jyemin October 7, 2020 13:54

jyemin approved these changes Oct 8, 2020

View reviewed changes

JAVA-3567: Add support for records

1e87688

bdeleonardis1 force-pushed the 3567 branch from 0634cc5 to 1e87688 Compare October 8, 2020 13:37

bdeleonardis1 closed this Oct 8, 2020


		classModelBuilder.instanceCreatorFactory(new InstanceCreatorFactoryImpl<T>(new CreatorExecutable<T>(clazz, noArgsConstructor)));
		for (Annotation a : recordComponent.getAnnotations()) {

JAVA-3567: Add support for records #581

JAVA-3567: Add support for records #581

Conversation

bdeleonardis1 commented Sep 24, 2020 • edited

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

Choose a reason for hiding this comment

jyemin commented Sep 24, 2020

rozza 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

rozza 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

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

jyemin left a comment

Choose a reason for hiding this comment

rozza commented Oct 8, 2020

jyemin commented Oct 8, 2020

bdeleonardis1 commented Oct 8, 2020

cardouken commented May 27, 2021 • edited

jyemin commented May 27, 2021

cardouken commented May 27, 2021

nikammerlaan commented Oct 18, 2021

jyemin commented Nov 24, 2021

fmendezp commented Feb 22, 2022

jyemin commented Feb 28, 2022

fmendezp commented Feb 28, 2022

hfoxy commented Aug 8, 2022

jyemin commented Aug 8, 2022

hfoxy commented Aug 8, 2022

bdeleonardis1 commented Sep 24, 2020 •

edited

cardouken commented May 27, 2021 •

edited