-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Change PersistenceMessageSerializer base class to SerializerWithStringManifest #5002
base: dev
Are you sure you want to change the base?
Change PersistenceMessageSerializer base class to SerializerWithStringManifest #5002
Conversation
// use reflection to create the generic type of PersistentFSM.PersistentFSMSnapshot | ||
Type[] types = { typeof(string), type.GenericTypeArguments[0], typeof(TimeSpan?) }; | ||
object[] arguments = { message.StateIdentifier, GetPayload(message.Data), timeout }; | ||
Type[] types = { typeof(string), payload.GetType(), typeof(TimeSpan?) }; |
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.
Get the type of the payload from the payload type instead of relying on a generic passed into the method
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.
Minor backwards compat changes requested
&& type.GetGenericTypeDefinition() == typeof(PersistentFSM.PersistentFSMSnapshot<>)) return GetPersistentFSMSnapshot(type, bytes); | ||
switch (manifest) | ||
{ | ||
case null: |
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.
good catch on the null
{ | ||
return shortened; | ||
} | ||
|
||
shortened = cleanAssemblyVersionRegex.Replace(type.AssemblyQualifiedName, string.Empty); | ||
// Defensive coding. `type.AssemblyQualifiedName` can return null if type is of generic type parameter type |
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.
LGTM
throw new SerializationException($"Unimplemented deserialization of message with manifest [{manifest}] in [{GetType()}]"); | ||
} | ||
|
||
public override string Manifest(object obj) |
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.
For the time being, we should return the legacy types - I believe there are some persistence plugins (i.e. SQL ones) that are still relying on manifest data using reflection and while I would like to eventually clean that up, probably best to change as little as possible here first.
case PersistentFSM.StateChangeEvent _: | ||
return StateChangeEventManifest; | ||
return obj.GetType().TypeQualifiedName(); |
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 something like this be OK?
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.
Just use the hard-coded constants
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 have problem with Akka.Persistence.Fsm.PersistentFSM.PersistentFSMSnapshot
because it is a generic class, hard coding would not work
Closes #5001