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

Query responses are serialized with the Default Serializer instead of the Message Serializer #2229

Closed
Arnaud-J opened this issue May 17, 2022 · 7 comments
Assignees
Labels
Priority 4: Would Lowest priority. Would-be-nice to include issues when time allows it. 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.

Comments

@Arnaud-J
Copy link
Contributor

Basic information

  • Axon Framework version: 4.5.8
  • JDK version: 11
  • Complete executable reproducer if available (e.g. GitHub Repo):

Steps to reproduce

This scenario describes what I was doing at that time, but it can be reproduced in many other ways.

My Event Serializer and Message Serializer (as defined in the doc) are configured to use Jackson. I do not configure the default Serializer, meaning it will use the XStream implementation by default.

I implemented a PageResponseType as explained in this issue comment for my query responses.

When sending a query expecting a PageResponseType response, I ended up with an XStream security exception.

Expected behaviour

I expected XStream not to be involved in the serialization of the response, but rather Jackson because I configured a Message Serializer for Axon Framework.

Actual behaviour

XStream was used as a serializer implementation, meaning Axon's default Serializer was used, resulting in an error in my case (because I did not explicitly configure XStream to allow my response class, but this is another matter).


To summarize, reading the docs made me believe that the Message Serializer would be used for Query messages AND Query responses, but it seems to be used only for the Query message, leaving the Query response to the default serializer.
Is this intentional? If yes, then this issue should not be a bug report but I would really like to see this explained in the documentation.

As a side note, this behavior may be the same for Command responses, but I did not test it.

Cheers!

@Arnaud-J Arnaud-J added the Type: Bug Use to signal issues that describe a bug within the system. label May 17, 2022
@smcvb
Copy link
Member

smcvb commented May 19, 2022

Thanks for drafting this concern with us, @Arnaud-J.
I've validated the code, and the statement is correct when using the AxonServerQueryBus.
Hence, it's not necessarily done for all query responses, as the approach is dictated by the Query Bus implementation.

Nonetheless, you are correct in this, and honestly, I need to figure out why this is the case.
Differently put, what the intent behind this was (somewhere in 2019).
As soon as I have an answer, I'll update you again.

My apologies for any inconvenience this might have caused you!

@smcvb smcvb added Priority 4: Would Lowest priority. Would-be-nice to include issues when time allows it. Status: Under Discussion Use to signal that the issue in question is being discussed. labels May 19, 2022
@smcvb smcvb self-assigned this May 19, 2022
@smcvb
Copy link
Member

smcvb commented May 20, 2022

Hi @Arnaud-J, I've checked with the rest of the team, confirming the guess I had.
When constructing the serializer separation, we made the breakdown between event payloads, message payloads, and the rest.

Theoretically, the ResponseType implementations are not the payload but part of Axon's message logic.
And part of the Axon Server API that requires a serialized format of the response type.

Due to this the ResponseType, and your PageResponseType too, are serialized by the generic serializer instead of the message serializer.

Is this intentional? If yes, then this issue should not be a bug report but I would really like to see this explained in the documentation.

So you are right, this is intentional.
And definitely not clear in the Reference Guide.
My apologies for that.

Just out of curiosity but would you be up for drafting an issue for this, so that we do not forget?
At the very least, I will close this issue as resolved.

@smcvb smcvb closed this as completed May 20, 2022
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: Under Discussion Use to signal that the issue in question is being discussed. labels May 20, 2022
@Arnaud-J
Copy link
Contributor Author

Hi @smcvb! Thanks for confirming. I created the issue for the reference guide as requested.

Also, I would like to question the choice of using the default serializer for ResponseType implementations. These classes are wrappers for the actual responses used in the business logic of an application. I believe they are more tied to the business logic than the framework or server implementation, the same messages payload classes.

Thus, to my understanding, we would tend to share theses response classes in the code (probably in a coreapi package, and maybe through a library if we need microservices) next to our message classes (at least our query classes because they are tightly coupled). So I think the following sentence in the default serializer documentation is not valid for the query responses:

These objects are generally not shared between different applications, and...

What do you think?

@smcvb
Copy link
Member

smcvb commented May 24, 2022

Also, I would like to question the choice of using the default serializer for ResponseType implementations.
These classes are wrappers for the actual responses used in the business logic of an application.

Thanks for raising your concerns here, @Arnaud-J! Communication is key for us to improve the product suite.
However, I think there's a difference in expectation of the ResponseType implementation here.

The ResponseType implementation should only dictate the format of the response.
Not the response itself.
For example, the InstanceResponseType only contains the Class<?> expectedResponseType.
In turn, the MultipleInstancesResponseType only contains an InstanceResponseType, ensuring it's adjusted to a List.

I would thus expect the PageResponseType to similarly contain just a Class (or another way to deduce what the expected response type is).
As such, it does not contain the query response "payload", but only defines what the type and (optional) carry format (e.g. List, Page, Flux...) of that payload is.

Does this clarify the intent of the ResponseType for you, @Arnaud-J?

@Arnaud-J
Copy link
Contributor Author

Ah I think I get it! Thanks for taking the time to explain.
Let me rephrase so I can be sure I get it right.

I indeed have a PageResponseType describing the type of response I expect, which is a custom Page implementation.
So I have this scenario in mind:

  1. Send a FindAllSomethingQuery query through the query gateway specifying that a PageResponseType(Something.class) is expected:
    • the message serializer is used to serialize the payload and metadata of the query
    • the default serializer is used to serialize the expected response type (and any other Axon-specific stuff that I do not know about)
  2. Axon Server routes the query to the correct application
    • the message serializer is used to deserialize the payload and metadata of the query into a FindAllSomethingQuery instance
    • the default serializer is used to deserialize the expected response type (and any other Axon-specific stuff that I do not know about)
  3. A @QueryHandler method returns a Page<Something> as the response data
    • the message serializer is used to serialize the actual response data, meaning a Page<Something>
  4. Axon Server routes the response data to the application that sent the query
    • the message serializer is used to deserialize the response data into a Page<Something> instance

@smcvb Is this scenario correct?

@smcvb
Copy link
Member

smcvb commented May 25, 2022

In the core, this is correct, @Arnaud-J, although the ordering is slightly different.
Given the fact you're trying to reflect the implementation, I'll give a short description of the flow:

  1. The QueryGateway invokes the primary QueryBus, which happens to be the AxonServerQueryBus in an Axon Server environment.
    • no serialization happens here yet.
  2. The AxonServerQueryBus provides the query to Axon Server for routing, which includes:
    • the message serializer is used to deserialize the payload and metadata of the query into a FindAllSomethingQuery instance
    • the default serializer is used to deserialize the expected response type (and any other Axon-specific stuff that I do not know about)
  3. Axon Server delegates the Query Message to the right application instance, entering the "Axon Framework"-using application through the AxonSeverQueryBus, which will:
    • the message serializer is used to deserialize the payload and metadata of the query into a FindAllSomethingQuery instance
    • the default serializer is used to deserialize the expected response type (and any other Axon-specific stuff that I do not know about)
  4. The AxonServerQueryBus uses the deserialized QueryMessage and delegates this to the local QueryBus.
    • the local QueryBus is the SimpleQueryBus
  5. The SimpleQueryBus provides the query message to the matching query handler, using the ResponseType in question to find the match (through ResponeType#matches(Type).
  6. The @QueryHandler annotated method provides the response to the SimpleQueryBus.
  7. The SimpleQueryBus passes the response on to the AxonServerQueryBus, which will serializer the query response using the message serializer.
  8. The AxonServerQueryBus passes the query response to Axon Server, that returns the result to the query dispatching application.
  9. The message serializer of the query dispatching application deserializes the query response into a (gRPC) wrapped response payload.
  10. The wrapped response payload is pulled through ResponseType#convert(Object), to finally adjust the response into your Page<Something> result.

@Arnaud-J
Copy link
Contributor Author

Everything makes sense in my head now! Thank you again for taking the time to give so much details!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 4: Would Lowest priority. Would-be-nice to include issues when time allows it. 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

No branches or pull requests

2 participants