Add support for serializing java record types

[eranl]

Please add support for serializing java record types the same way POJOs are supported.

[milaGGL]

Hi @eranl, happy holidays.

Thank you for your suggestion. I will bring this up for team discussion and keep you updated.

[eranl]

Hi @milaGGL,

Since record type support can't be added directly in this jdk8-based library, I have implemented some changes that allow for plugging in external custom mappers, such as for record types. Should I submit a PR with those changes?

I've also implemented the main code for a record mapper plugin, which could be released in a separate, jdk17-based library.

[eranl]

Actually, there is a way to add record type support in this library, using reflection. I'm happy to implement such an approach as well.

[milaGGL]

Hi @milaGGL,

Since record type support can't be added directly in this jdk8-based library, I have implemented some changes that allow for plugging in external custom mappers, such as for record types. Should I submit a PR with those changes?

I've also implemented the main code for a record mapper plugin, which could be released in a separate, jdk17-based library.

Yes, please create a pull request. An informative PR will be very helpful when I bring this up to team discussion after the holiday break.

If I understand correctly, the record type support needs to be implemented differently for jdk8-based library and jdk11-based library. Which library is using the reflection? Could you please create separate PRs for both implementations?

[eranl]

To clarify, the two options are:

1. add a record type mapper directly in this jdk8-based library, using reflection. Reflection is needed because records are not supported in jdk8.
2. add mapper plugin support to this library, and create a separate jdk17-based library containing a record type mapper, using that plugin support. Reflection would not be needed in this case, since records are fully supported in jdk17. This is the option I already have an implementation for.

Each approach has its advantages. option 2 would allow for plugging in any custom mapper, which can be seen as both an advantage and a disadvantage.

Let me submit a PR for option 2, to kick off the discussion.

Happy New Year!

[milaGGL]

Hi @eranl, just want to follow up on the PR you have mentioned.

[eranl]

Hi @milaGGL,

PR posted.

[milaGGL]

Thank you @eranl. I will keep you updated on the discussion progress.

[milaGGL]

Hi @ernal. After a team discussion on the PR, we are concerned about making the classes public. It is going to be a public API change in the SDK and there are very strict requirements for it.

However, we are interested in the first option you mentioned. It would be much appreciated if you could put up a PR for that.

Meanwhile, we are going to take note of this serialization feature request and the PR you have send in for future reference. Thank you so much for your suggestions.

[eranl]

Hi @milaGGL,

Makes sense. I'll try to generate a PR for option 1. Will keep you posted.

[eranl]

Hi @milaGGL,

PR for option 1 created.

[milaGGL]

Hi @eranl, thank your for putting up a PR. I will bring this to team discussion and keep the thread updated.

[eranl]

Hi @milaGGL, any update?

[milaGGL]

Hi @eranl, we are considering the suggestion you have provided and one of my teammate is taking closer look at the PR. Unfortunately, it will take some time to synchronize it with other ongoing developments, and I am unable to provide an ETA.

[eranl]

Hi @milaGGL, you said you were interested in this change and requested that I submit a PR for it, which I did months ago. Why is it taking so long to even respond to it?

[milaGGL]

Hi @eranl, my apologies for the late response. This feature request and PR is added into our backlog, along with other issues and requests. I am unable to provide a a specific timeline, but there is a discussion scheduled with my teammate, and I will keep you updated.

[eranl]

Thanks

[milaGGL]

Hi @eranl, I wanted to drop you a quick note and say sorry for not getting back to your issue ticket sooner. I had a discussion with my teammate, and we both think that supporting Record type is a feature we want in the long run. The PR you submitted is a solid kickoff – thank you so much for the contribution!

However, it is a quite complicated feature. We'll need to dive into the nitty-gritty details and get the whole team on board. I am drafting up a document to support the implementation of this feature, and will keep this thread updated.

By the way, in the PR, you have created annotations for 3 types, DocumentId, PropertyName, and ServerTimestamp. May I ask what is the reasoning behind this selection?

[eranl]

Hi @milaGGL, thanks for the update. Looking forward to your doc.

To your question:
As I wrote in PR comment # 3, the existing ThrowOnExtraProperties & IgnoreExtraProperties annotations are supported, so there was no need for new ones (no need for @target(ElementType.RECORD_COMPONENT), since these are class annotations).
As I wrote in PR comment # 1, exclusion of fields is not supported, so there was no need for an Exclude annotation.
That left me with the three you mentioned.

[eranl]

Hi @milaGGL, an update: I've figured out a way to implement this without requiring new annotation classes, which eliminates the need for any JDK17 code, other than for tests. Should I update the PR?

[milaGGL]

Hi @eranl, that sounds great! I haven't get a chance to address this issue due to the priority, but it is on my radar. Meanwhile, any insights are very much appreciated.

[eranl]

Hi @milaGGL, an update: I've figured out a way to implement this without requiring new annotation classes, which eliminates the need for any JDK17 code, other than for tests. Should I update the PR?

Hi @milaGGL, just pushed an update to the PR with this improvement.

[eranl]

Hi @milaGGL, another update: I've found a way to include the record test code in the main google-cloud-firestore module, so I just pushed a commit eliminating the jdk17 module altogether.

[bfncs]

Hi there, Records have been stable since JDK 16, with many new language features based on it delievered since then. It would be really helpful if we could use this with Firestore SDK in the future. Has there been any progress on the issue?