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

Ensure that Amazon Lambda uses the ObjectMapper bean when available #7508

Merged
merged 1 commit into from
Mar 3, 2020

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Mar 2, 2020

This was the result of me looking into: #6041 (comment)

@geoand geoand force-pushed the amazon-lambda-jackson branch from 8eb25b2 to 0995bb4 Compare March 2, 2020 10:25
@geoand geoand marked this pull request as ready for review March 2, 2020 11:45
@oztimpower
Copy link
Contributor

Validated as working on my build, post rolling in the changes. Nice one!

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I'm a bit torn about that one. I wonder if it's such a good idea to reuse the object mapper if it's one used for a very specific context and that needs a very specific configuration.

Can we discuss it? (Marking it as Request changes so that we have a discussion before merging)

@geoand
Copy link
Contributor Author

geoand commented Mar 2, 2020

Sure, let's talk tomorrow

@stuartwdouglas
Copy link
Member

Maybe this should have a specific Qualifier so you have a bean that is just the object mapper for Lambda.

@geoand
Copy link
Contributor Author

geoand commented Mar 2, 2020

The idea of using the bean from Arc is that it's been properly configured with various Jackson modules automatically by Quarkus.
So in the case that @oztimpower had, the Kotlin Jackson module is configured by Quarkus making Lambda work OOTB with Kotlin.

@gsmet @stuartwdouglas The alternative would be to create a second ObjectMapper bean for Lamdba under the covers, that applies the same modules but also applies the few extra settings that Lambda needs. Like Stuart said this would have a qualifier and we would use that qualifier when pulling the bean out of Arc in the Lambda Recorder code.
How does that sound?

@stuartwdouglas
Copy link
Member

Actually I think the current implementation is probably fine. The would be the main use case for using the mapper with AWS lambda anyway, so any customisers the users provide would be expected to be used for this. I don't think we need to complicate it any further.

@geoand
Copy link
Contributor Author

geoand commented Mar 3, 2020

@gsmet are you OK with this?

@gsmet gsmet merged commit 78c2346 into quarkusio:master Mar 3, 2020
@gsmet
Copy link
Member

gsmet commented Mar 3, 2020

Yes, let's move on then.

@gsmet gsmet added this to the 1.3.0 milestone Mar 3, 2020
@geoand geoand deleted the amazon-lambda-jackson branch March 3, 2020 09:24
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.

4 participants