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

Added possibility to customize ObjectMapper's KotlinModule #382

Merged
merged 1 commit into from Nov 9, 2022
Merged

Added possibility to customize ObjectMapper's KotlinModule #382

merged 1 commit into from Nov 9, 2022

Conversation

slaha
Copy link
Contributor

@slaha slaha commented Nov 3, 2022

This pull request adds possibility to customize the KotlinModule used in KMongo's ObjectMapper.

The purpose for this feature is to be able to enable KotlinModule's features that are disabled by default (eg. KotlinFeature.SingletonSupport).

@zigzago
Copy link
Member

zigzago commented Nov 8, 2022

Thank you for the PR, it looks great @slaha!

But why duplicating the KotlinFeature list in KotlinModuleConfiguration instead of using a KotlinModule.Builder?

object KotlinModuleConfiguration {
 val moduleBuilder : KotlinModule.Builder = KotlinModule.Builder()
}

If jackson adds a new KotlinFeature, then with the KotlinModule.Builder version there is no need to modify KMongo to support this new feature ;)

What do you think?

@slaha
Copy link
Contributor Author

slaha commented Nov 8, 2022

You are right and it was actually my first idea how to do it. But problem with this approach is that module kmongo-shared does not depend on jackson-module-kotlin so there is no KotlinModule available. I was not sure if it is ok to add the dependency to kmongo-shared because there is not much dependencies in the module.

@zigzago
Copy link
Member

zigzago commented Nov 8, 2022

I agree kmongo-shared may not depend on jackson-module-kotlin. I would move KotlinModuleConfiguration in KMongoConfiguraton file in kmongo-jackson-mapping module.

@slaha
Copy link
Contributor Author

slaha commented Nov 8, 2022

Updated the PR with proposed changes.

@zigzago
Copy link
Member

zigzago commented Nov 9, 2022

Thank you!

@zigzago zigzago merged commit dad79bb into Litote:master Nov 9, 2022
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.

None yet

2 participants