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

DeadLetterQueue uses wrong Serializer to (de)serialize Tokens #2485

Closed
nils-christian opened this issue Nov 15, 2022 · 6 comments · Fixed by #2486
Closed

DeadLetterQueue uses wrong Serializer to (de)serialize Tokens #2485

nils-christian opened this issue Nov 15, 2022 · 6 comments · Fixed by #2486
Assignees
Labels
Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: Resolved Use to signal that work on this issue is done. Type: Bug Use to signal issues that describe a bug within the system.
Milestone

Comments

@nils-christian
Copy link
Contributor

Hi,

I am not entirely sure whether this can be classified as bug, invalid documentation or a missing feature.

Basic information

Steps to reproduce

Start the provided example. Please note that multiple conditions have to come together for this issue to appear:

  • We have to use a DLQ (obviously).
  • We have differently configured serializers for events and tokens (in our case we reconfigured the object mapper for the events and set the token-serializer manually by creating a custom token event store).
  • We have to use a MultiStreamableMessageSource.

In our application all those conditions come together and make the DLQ unusable.

Now, the documentation of the serializer in the builder for the DLQ says: "Sets the Serializer to deserialize the events, metadata and diagnostics of the DeadLetter when storing it to a database.". This is not true, as it is also used for serialization and it is also used for (de)serialization of tokens as can be seen in the EventMessageDeadLetterJpaConverter. I think it should be possible to either configure the (de)serializer for all four classes independent from each other or maybe the DLQ should use the serializer from the token store. The first approach would probably be the easiest one. The DLQ could still use the current serializer as default for all four classes, making this change compatible with 4.6.2.

Expected behaviour

The application works.

Actual behaviour

The application crashes with a deserialization exception.

@nils-christian nils-christian added the Type: Bug Use to signal issues that describe a bug within the system. label Nov 15, 2022
@CodeDrivenMitch
Copy link
Member

Hello @nils-christian,

The reason for using the same serializer is that we are both in control of serialization and deserialization in the same component. So as long as we use the same for serialization as for deserialization, we should be fine.
What is the actual cause of the application's crash? Do you use a special token for the MultiStreamableMessageSource?

We could introduce the same serializer levels as Axon contains: events, messages, and generic as an improvement. But I'd like to understand the situation properly first

@CodeDrivenMitch CodeDrivenMitch self-assigned this Nov 15, 2022
@nils-christian
Copy link
Contributor Author

Hello @MORLACK,

While you are in control of the (de)serialization, you are not in control of the classes themselves, especially the event payloads. Our events use the builder pattern and contain various annotations. This is the reason why the default object mapper cannot handle them properly and why we have to reconfigure it. However, the modified object mapper cannot handle the tokens of the MultiStreamableMessageSource in a correct manner. And this is the reason why we need to configure different serializers (more precisely: different object mappers) for the events and the tokens. This is usually not an issue, but in the DLQ both elements are (de)serialized with the same object mapper so one of them is always (de)serialized in an incorrect way.

To provide a more extreme example: Assume that we (de)serialize the events with XStream and the tokens with Jackson (for whatever reason). This would be possible - just not with the DLQ (unless of course we would implement a custom serializer which is able to recognize the elements to be (de)serialized).

@CodeDrivenMitch
Copy link
Member

Alright, thanks for the explanation. I'll create a PR that allows different serializers to be configured.

@CodeDrivenMitch CodeDrivenMitch linked a pull request Nov 15, 2022 that will close this issue
@smcvb smcvb added Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: In Progress Use to signal this issue is actively worked on. labels Nov 15, 2022
@smcvb smcvb added this to the Release 4.6.3 milestone Nov 15, 2022
@CodeDrivenMitch
Copy link
Member

Hello @nils-christian, I just merged the fix that will be able to resolve this issue for you. It will thus be included in 4.6.3

@CodeDrivenMitch CodeDrivenMitch added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Nov 15, 2022
@nils-christian
Copy link
Contributor Author

Thank you for the quick fix @MORLACK. I will try and test the fix before the release within our specific environment.

@nils-christian
Copy link
Contributor Author

Sorry for the delay, @MORLACK. The fix works for our environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: Resolved Use to signal that work on this issue is done. Type: Bug Use to signal issues that describe a bug within the system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants