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

Updated serializer configuration and made them lazy #280

Merged
merged 5 commits into from Apr 26, 2022

Conversation

lfgcampos
Copy link
Contributor

The goal for this PR is to do the same we did on Framework, making Serializers lazy and only initializing them if the user doesn't provide one.

It has an added benefit of not always initializing XStream.
For making it work, the initialization of the messageConverter was also pushed to the end of the builder call.

I am not sure we have meaningful tests to be added in this case.

@gklijs
Copy link
Collaborator

gklijs commented Apr 22, 2022

This is expected to be a whole or part of the fix for AxonFramework/AxonFramework#2184

@lfgcampos lfgcampos requested a review from gklijs April 25, 2022 12:28
Copy link
Collaborator

@gklijs gklijs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There still some complains from sonar, looks like adding @SuppressWarnings("squid:S1874") is the only valid fix for the parametrized types. Not sure there is a good fix for the duplicated code. Guess we could have some helper function, for example to get a default supplier? https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&sinceLeakPeriod=true&pullRequest=280&id=AxonFramework_extension-kafka

Copy link
Collaborator

@gklijs gklijs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks 👍

Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a little bit of JavaDoc. Otherwise, this is good to go if you ask me.

Keeps it consistent with the framework and gives us a chance to notify the user they are using a default serializer and they shouldn't
@sonarcloud
Copy link

sonarcloud bot commented Apr 26, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

74.0% 74.0% Coverage
15.1% 15.1% Duplication

@lfgcampos lfgcampos requested a review from smcvb April 26, 2022 11:05
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still think this looks fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants