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

Add kotlin.serialization implementation of Serializer #338

Merged
merged 5 commits into from
Jun 15, 2024

Conversation

lion7
Copy link
Contributor

@lion7 lion7 commented May 28, 2024

This PR adds support for serializing by using kotlinx.serialization and is basically a continuation of #124.

Note that I had to upgrade the Kotlin version (and fix a few generics in QueryGatewayExtensions.kt) to make it work.

@gklijs gklijs self-assigned this May 28, 2024
@gklijs
Copy link

gklijs commented May 28, 2024

Looks great from quickly going through it, I'll try to review it this week.

@CLAassistant
Copy link

CLAassistant commented May 28, 2024

CLA assistant check
All committers have signed the CLA.

@jangalinski
Copy link
Collaborator

Great approach. We are working on a kotlinx.serialization based avro serializer as well and might (re-)use generic features provided by extension-kotlin.
But: kotlinx.serialization is a generic tool for serialization, json is just one aspect. So I would like to have a clearer separation between json-specific and kotlinx-generic features ...

@lion7
Copy link
Contributor Author

lion7 commented May 29, 2024

This implementation is already independent of the serialization format. I changed the main dependency to use the core library instead and added a few more tests for binary formats as well.

It should work with any SerialFormat that either implements StringFormat or BinaryFormat.

Copy link

@gklijs gklijs left a comment

Choose a reason for hiding this comment

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

I have some questions about Axon Specific classes that needs to be serialized, which are not included in the SerializersModule. All new files should have the copyright notice. It would also be nice if we don't need to opt in for experimental api's. As this is a new feature, it's likely better to set 4.10 as the target.

@zambrovski
Copy link
Collaborator

We were playing with KSerializer registration in our Apache Avro approach and came up with a small SPI. By doing so it is pretty easy to write own serializers and let the SPI loader automatically register those.

But before building this, it would be a good idea to document how new KSerializers needs to be registered. In general, do you have any docs describing the implemented features?

@jangalinski
Copy link
Collaborator

We were playing with KSerializer registration in our Apache Avro approach and came up with a small SPI. By doing so it is pretty easy to write own serializers and let the SPI loader automatically register those.

But before building this, it would be a good idea to document how new KSerializers needs to be registered. In general, do you have any docs describing the implemented features?

Actually, this part is already public/released ... peek here if you like: https://github.com/toolisticon/avro-kotlin/tree/develop/avro-kotlin-serialization/src/main/kotlin/spi

@gklijs gklijs added this to the Release 4.10.0 milestone Jun 15, 2024
@gklijs gklijs merged commit 764e339 into AxonFramework:master Jun 15, 2024
1 check passed
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

Looks fine, although I have a couple of questions and remarks.

smcvb added a commit that referenced this pull request Jul 23, 2024
Set since tag

#338
smcvb added a commit that referenced this pull request Jul 23, 2024
Move ArrayResponseType to message/responsetypes. In doing so, we reflect
 the folder structure for the other ResponseType implementation in Axon
 Framework

#338
smcvb added a commit that referenced this pull request Jul 23, 2024
Expand KDoc with construction example

#338
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants