Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add
kotlin.serialization
implementation ofSerializer
#124New 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 ofSerializer
#124Changes from all commits
87ee5ff
43c5d0e
4bf4ddf
0af12e5
2b7735b
1256d2d
f7d4ee9
798b3fd
edb6903
b8fbd02
c113040
7acce4c
aa6804f
20479a8
e0f411a
8d7b70d
34844c8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that the hard requirement on the
@Serializable
annotation makes it pretty tough to use this serializer for Axon objects, like theTrackingToken
.I've skimmed the Kotlin Serializer documentation somewhat but didn't spot a solution for this just yet.
Do you, perchance, have a solution in mind for this, @hiddewie?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the toughest question :)
As far as I can see, there are two solutions:
@Serializable
. This adds the compile time requirement of having the Kotlin compiler plugin dependency in the build process. That may or may not be OK. Although, there are also Jackson annotations in the source code already. In runtime there is no Kotlin requirement.To illustrate option 2, I added a small example and custom serializer with test.
The third option is abandoning this approach because the Kotlin serialization is not wanted in the core framework, and it is not worth the effort to maintain the custom serializers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(After thinking about option 1 a bit more: I have not tested adding
@Serializable
on a Java class and then running the Kotlin compiler plugin on it. Theoretically it should work but in practice I don't know)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the insights here.
If you'd be at leisure to validate whether it is even possible to add the
@Serializable
annotation to a Java class, that would be amazing (and save me some personal investigation. 😅).In the meantime, I'll start an internal discussion about whether we want to add the
@Serializable
annotation to the core of Axon Framework.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a few things to test how the interoperability of Java classes with Kotlin annotations, and the Kotlin annotation processors work together.
What I did:
serializer
functions, see instructions here https://github.com/Kotlin/kotlinx.serialization#maven@kotlinx.serialization.Serializable
to ConfigTokenWhat I saw:
What I did:
What I saw:
What I did:
What I saw:
So my conclusion is that the Kotlin serialization compiler plugin only works for Kotlin source files, and that the
.serializer()
static method cannot be generated for Java source files.That is very unfortunate, I might open an issue with the Kotlin Serialization team to ask if that is supposed to work (ref Kotlin/kotlinx.serialization#1687).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, we can not assume that every class that comes here will have a serializer attached to it (nor be a Kotlin class)
If there is no serializer defined, or just based on types, we can fall back on a registry that we can keep as a map of Type (Class, Class name, etc) and a Serializer. That map could be configurable so someone might add support for classes without
@Serializable
like Java Dates, and we can pre-populate it with AF serializers configuration. Or keep the AF serializers in a separate config, but that's a finer detail.This and
this is what I mean, as we need to pass the serializer explicitly in the very broad generic setting anyway, we might as well keep it somewhere in a config map, similar as to how this cache is doing but is just caching all serializers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to elaborate a little on the
ExperimentalSerializationApi
you're "opting in" here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
@Serializer
annotation below is marked as experimental. It might change its implementation slightly between versions of Kotlin Serialization. (https://kotlin.github.io/kotlinx.serialization/kotlinx-serialization-core/kotlinx-serialization-core/kotlinx.serialization/-experimental-serialization-api/index.html)We might wait to merge this feature until the used serialization API is marked fully stable, although the Axon
extension-kotlin
library is also marked as experimental itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to clarify, only
@Serializer
which is used to hook custom serializer objects is marked as Experimental, the rest of Json serialization is not experimental anymore and is stable. Is this a correct assumption @hiddewie ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly. The following is stable:
But the following needs experimental opt in:
In particular doing this for Java classes gives compiler errors, and it might simply not be supported (see Kotlin/kotlinx.serialization#1687)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the custom serializer implemented manually for the internal Axon type. These type of implementations are needed if the
@Serializable
annotation is not used in combination with the compiler plugin.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd assume this should be constructed for every concrete implementation of serializable Axon objects in that case, correct?
This would at least enable moving further with this serializer, so that's good.
If we go this route, it will require some KDoc, of course ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw https://github.com/Kotlin/kotlinx.serialization/blob/f673bc2/docs/serializers.md#deriving-external-serializer-for-another-kotlin-class-experimental for the first time. This would allow us to skip the 'manual' implementation of the serializer.
Unfortunately I can't yet get it to work, I run into this bug in the Kotlin complier plugin (Kotlin/kotlinx.serialization#1680).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried Surrogate serializers and Delegating serializers?
They look much simpler and might just get the job done. Hopefully, they might just work and there would be no hidden issues like you encountered previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(#124 (comment)) I changed the fully-custom example
ConfigTokenSerializer
into a serializer using aConfigTokenSurrogate
. Its slightly better maintainable, but still a LOT of work to do (and maintain) for every Axon class.The work means, for every Axon class:
@Serializable
The serializers for surrogates are very simple and similar (only a few lines of code). However the surrogates themselves need to contain exactly the public data fields of the class they are a surrogate for. This will cause bugs if a field is forgotten in the surrogate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am missing test cases that validate if the
KotlinSerializer
can be used for Axon objects, like theMessage
,TrackingToken
, andSagaEntry
.Assuming that's because those objects aren't annotated with the
@Serializable
annotation.However, the
Serializer
in Axon must de-/serialize those objects for it to be usable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. See https://github.com/AxonFramework/extension-kotlin/pull/124/files#r645048334 for the comment about serializing internal classes.
I added one test case for the
ConfigToken
, and every internal class that should be serializable can be added in that way, if there is a serializer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. To make this
Serializer
implementation workable for Axon Framework entirely, these tests will be a requirement too.Making these can wait until we've concluded the conversation on the
Serializer
implementation though.On the subject of using the
@Serializable
annotation on Axon components, and if that even works for Java classes, that is.