Skip to content
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.

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

Support encoding/decoding of parameterized records #1005

Merged
merged 3 commits into from
Sep 30, 2022
Merged

Conversation

jyemin
Copy link
Contributor

@jyemin jyemin commented Sep 28, 2022

@jyemin jyemin self-assigned this Sep 28, 2022
@jyemin
Copy link
Contributor Author

jyemin commented Sep 28, 2022

Initially I didn't consider the case where the type parameter was used directly as the type of one of the record components. I wonder if I'm missing anything else. Please try to think of other test cases.

@jyemin
Copy link
Contributor Author

jyemin commented Sep 29, 2022

Unfortunately, this doesn't handle the case of a parameterized record with cyclical type references. For example, if you attempt to create codecs for this record"

record TestSelfReferentialHolderRecord(
                                        @BsonId String id,
                                        TestSelfReferentialRecord<String> selfReferentialRecord) {}

where TestSelfReferentialRecord is defined as:

public record TestSelfReferentialRecord<T>(
                                        T name,
                                        @Nullable TestSelfReferentialRecord<T> left,
                                        @Nullable TestSelfReferentialRecord<T> right) {}

you'll get a recursion resulting in a StackOverflowException:

	at org.bson.codecs.record.RecordCodec.<init>
	at org.bson.codecs.record.RecordCodec.parameterize
	at org.bson.codecs.record.RecordCodec$ComponentModel.computeCodec
	at org.bson.codecs.record.RecordCodec$ComponentModel.<init>
	at org.bson.codecs.record.RecordCodec.getComponentModels
	at org.bson.codecs.record.RecordCodec.<init>

and so on.

To fix this we need to break the cycle somehow. But I think that can be the subject of another ticket and PR. This one adds value even if it doesn't handle cycles.

Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

Add a regression test case for the resolving of types which move.

Add a ticket for the self referential example JAVA-4745

@jyemin jyemin requested a review from rozza September 30, 2022 13:17
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

One nit - location of the records.

Other than that LGTM

@jyemin jyemin requested a review from rozza September 30, 2022 13:21
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@katcharov katcharov left a comment

Choose a reason for hiding this comment

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

LGTM

@jyemin jyemin merged commit ba39a2c into mongodb:master Sep 30, 2022
@jyemin jyemin deleted the j4740 branch September 30, 2022 15:06
ispringer pushed a commit to evergage/mongo-java-driver that referenced this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants