-
Notifications
You must be signed in to change notification settings - Fork 9
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
[#368] AxonSerializer
documentation and enforce ReplayToken.context
to String
#370
base: master
Are you sure you want to change the base?
Conversation
Add context field for the ReplayTokenSerializer. Sadly, the context object is of type Any. Hence, we need to find a way to retrieve a primitive type serializer and potentially configured serializers. #bug/support-reset-context
Add todos for the pointer where to work on #bug/support-reset-context
AxonSerializer
documentation and enforce ReplayToken.context
to StringAxonSerializer
documentation and enforce ReplayToken.context
to String
AxonSerializer
documentation and enforce ReplayToken.context
to StringAxonSerializer
documentation and enforce ReplayToken.context
to String
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 found a typo! ;)
Also, I have concerns about serializing the context as String. Could you maybe check if we can preserve the Any
object behavior? If not possible, it is what it is, but only a String feels very restrictive.
kotlin/src/main/kotlin/org/axonframework/extensions/kotlin/serialization/AxonSerializers.kt
Outdated
Show resolved
Hide resolved
* | ||
* @see ReplayToken.context | ||
*/ | ||
val replyTokenContextSerializer = String.serializer().nullable |
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.
Isn't there a way in kotlin serialization, as we have full control over (de)serialization, to encode the type of the payload as the first field, and then serialize the object properties next?
Something like outlined here https://stackoverflow.com/questions/66148137/how-to-serialize-any-type-in-kotlinx-serialization
Giving up the flexibility of the type in here feels wrong. But if there's no other way, so be it.
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 read the explanation from Steven?
#368
I'm wondering why he assumed that we need to introduce breaking change if we'd like to support.
Sadly enough, there's no straightforward solution to this; not without adjusting the context field of the ReplayToken in Axon Framework itself.
That is, however, a breaking change, and as such, cannot be done.
With your proposition, it's just an additional field. I may give it a try... whereas as far as I remember some deserializes breaks if you have additional field (for example: you read the context serialized in Kotlin, in the Java app) - so it might be the case - do not add more fields. The solution with first field, it's also not simple - there is no guarantee that the type field will be first and you still (look at StackOverflow) need to define all possibilites upfront:
@Suppress("UNCHECKED_CAST")
private val dataTypeSerializers: Map<String, KSerializer<Any>> =
mapOf(
"String" to serializer<String>(),
"Int" to serializer<Int>(),
//list them all
).mapValues { (_, v) -> v as KSerializer<Any> }
I just realized it may work in different way (what do you think about that):
I'm not sure if it will work. We may support String by default, but... configure the replayTokenContextSerializer
as polymorphic and let user register all possible types of the context while creating KotlinSerializer.
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.
In my conversations with @lion7 it seemed impractical to do with the current set up.
We discussed that it would've been best if the ReplayToken
didn't contain a serializable Object
at all.
But for example instead the SerializedObject
from AF.
Adjusting the type is the breaking change I was referring too. We can't do that right now at all.
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.
Approving this PR, as it's in line with what the issue describes.
But, let's see how the chat concerning the String
-based ReplayToken#contex
field proceeds between the three of us before merging.
/** | ||
* Serializer for Axon's [TrackingToken] class. | ||
* Provides serialization and deserialization support for nullable instances of TrackingToken. | ||
* This serializer uses [replayTokenContextSerializer] to serialize the context field and now only [String] type or null value is supported! |
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 don't think line 55 belongs to this description, right?
Quality Gate passedIssues Measures |
closes #368