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.

Sign up for GitHub

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

Jump to bottom

Improve so that KotlinSerialization can be applied before Jackson2 #29009

Closed
wants to merge 3 commits into from
Closed

Improve so that KotlinSerialization can be applied before Jackson2 #29009

wants to merge 3 commits into from

Conversation

meloning
Copy link
Contributor

@meloning meloning commented Aug 24, 2022 •
edited
Loading

Reference Issue: Issue-29008

I expect to need test code, but I can't find the previously written test code. Can you give me a guide on writing test code?

@pivotal-cla
Copy link

@meloning Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@meloning Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 24, 2022
@meloning meloning changed the title Improved so that KotlinSerialization can be applied before Jackson2 Improve so that KotlinSerialization can be applied before Jackson2 Aug 24, 2022
@sdeleuze sdeleuze self-assigned this Aug 24, 2022
@sdeleuze
Copy link
Contributor

sdeleuze commented Aug 26, 2022 •
edited
Loading

The proposed change on RestTemplate would be consistent with what we do in WebMvcConfigurationSupport here and WebClient which do that because Kotlin serialization require an explicit signal (the class should be annotated with @Serializable) otherwise serialization is delegated to Jackson.

@rstoyanchev Are you ok with doing that on 5.3.x even if we are late in the patch release cycle? It will be a slight change of behavior but should impact very few people and I think it could be considered as a bug and a lack of consistency.

@meloning
Copy link
Contributor Author

Also, parts AbstractMessageBrokerConfiguration and AllEncompassingFormHttpMessageConverter are in the same situation, can I fix it in the PR and upload it?

@meloning
Improved so that KotlinSerialization can be applied before Jackson2
527b857 
target class:
- RestTemplate class
- AbstractMessageBrokerConfiguration class
- AllEncompassingFormHttpMessageConverter class
@meloning
Copy link
Contributor Author

meloning commented Aug 27, 2022 •
edited
Loading

Hello @sdeleuze @rstoyanchev
I added kotlinSerialization test case in RestTemplateTests and fixed the case where MessageBrokerConfigurationTests that test AbstractMessageBrokerConfiguration fails.

Please check and review.
Thank you

@meloning
Copy link
Contributor Author

meloning commented Sep 1, 2022

Hi @rstoyanchev @sdeleuze
There is no answer yet, so I'm leaving a comment again.
Please let me know if there are any errors or if there are any additional considerations.

Please check.
Thank you

@sdeleuze
Copy link
Contributor

sdeleuze commented Sep 2, 2022

Merged a refined version via bb3ada4, thanks!

@sdeleuze sdeleuze closed this Sep 2, 2022
@sdeleuze sdeleuze added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug theme: kotlin An issue related to Kotlin support and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sdeleuze sdeleuze

Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) theme: kotlin An issue related to Kotlin support type: bug A general bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@meloning @pivotal-cla @sdeleuze @spring-projects-issues