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

Change serialization strategy #123

Merged
merged 11 commits into from
Sep 2, 2024
Merged

Change serialization strategy #123

merged 11 commits into from
Sep 2, 2024

Conversation

yuanchen233
Copy link
Collaborator

@yuanchen233 yuanchen233 commented Aug 18, 2024

Using Kotlinx json serializer instead of Jakson wrapper.
This avoid converting incoming http messages into Jackson TreeNode before using core Json serializer.
Error handling in Jackson TreeNode were redundant, since kotlinx.serialization is eventually used.

Main changes:

  • For all ApplicationServiceRequest endpoints, we treat incoming request as String and use carp.core json serializer to decode and build the request object.
  • For all responses with carp.core domain models, we use core serializers to encode them into json string.
  • For all Snapshot data saved in the database through core repository, core serialization is used to create the snapshot, but ObjectMapper.readTree is still used to create Jsonb values.
  • Service layer usage of ObjectMapper is switched to Json

Reasons/Advantages:

  • Carp core uses kotlinx.serialization, it's hard to maintain/keep track of which method to use.
  • Jackson is Java first, it's creating annoying issues on how it handles Kotlin objects.
  • Jackson rely on Reflection and serialize everything even if it does not know the type. While Kotlinx generate serializers at compile time, this ensures type safety, and also faster at runtime.

Other notes and TODOs:

  • A serialization Json Bean is created for spring to autowire in some service classes, since current testing library need it to mock the value. We should consider to use this Bean for all occurrence, or import the object. (we are not changing the state of this serializer object, so I went with import object approach in the first place).
  • Not all webservices domain models are using kotlinx, since they they are inherited from old java project. Serialization for them requires more testing, especially when saving 'Datastream' to the database.
  • Account module is updated, since it also communicate with `Keycloak so more thoughts are needed here.
  • Spring support kotlinx.serialization now, but it requires no Jackson configuration in classpath, consider working towards this.

@yuanchen233 yuanchen233 changed the title Change deserialization strategy for ApplicationServiceRequest Change serialization strategy Aug 23, 2024
* Use kotlinx serializer for snapshots

* Clean up

* Change serialization strategy for deployment and protocol
@yuanchen233 yuanchen233 self-assigned this Aug 23, 2024
@yuanchen233 yuanchen233 marked this pull request as draft August 23, 2024 08:46
@yuanchen233 yuanchen233 linked an issue Aug 28, 2024 that may be closed by this pull request
@yuanchen233 yuanchen233 marked this pull request as ready for review August 28, 2024 12:37
Copy link
Contributor

@davidscavnicky davidscavnicky Aug 29, 2024

Choose a reason for hiding this comment

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

very nice - @RequestBody httpMessage: String, will accept .json content as string
I understand now why you're making thin layer of WS regarding to serializers;

davidscavnicky
davidscavnicky previously approved these changes Aug 30, 2024
@davidscavnicky
Copy link
Contributor

this is to be merged, since I want to follow up on unit tests. unsure if @NGrech wants to review this or the talk we had about this change was sufficient

@yuanchen233 yuanchen233 merged commit 1b157e6 into develop Sep 2, 2024
1 check passed
@yuanchen233 yuanchen233 deleted the generic-serializer branch September 2, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify (de)serializer declarations
2 participants