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

Fix correct parsing of collections in AmazonLambdaRecorder #40464

Merged
merged 1 commit into from
Jul 4, 2024
Merged

Fix correct parsing of collections in AmazonLambdaRecorder #40464

merged 1 commit into from
Jul 4, 2024

Conversation

hamburml
Copy link
Contributor

@hamburml hamburml commented May 5, 2024 •
edited by geoand
Loading

test to make sure that the objectMapper parses the Input correct.

@patriot1burke thx for the hint.

I also adjusted the objectWriter but I am not sure if this is really necessary.

@geoand geoand requested a review from patriot1burke May 7, 2024 06:54
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
gsmet
gsmet requested changes May 10, 2024
View reviewed changes
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.

Thanks for the PR! I spotted something that looks a bit odd to me.

Also the tests are not passing.

...azon-lambda/runtime/src/main/java/io/quarkus/amazon/lambda/runtime/AmazonLambdaRecorder.java Outdated Show resolved Hide resolved
@hamburml hamburml requested a review from gsmet May 10, 2024 14:28
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@hamburml hamburml force-pushed the feature/AmazonLambdaHandlerCollections branch 4 times, most recently from 46ee579 to ecfbe08 Compare May 10, 2024 17:11
@hamburml hamburml requested a review from gsmet May 10, 2024 17:24
@hamburml hamburml force-pushed the feature/AmazonLambdaHandlerCollections branch from ecfbe08 to 0886509 Compare May 10, 2024 17:37
@hamburml hamburml requested a review from gsmet May 10, 2024 17:40
@gsmet gsmet force-pushed the feature/AmazonLambdaHandlerCollections branch from 0886509 to 06d24d4 Compare May 13, 2024 14:24
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
gsmet
gsmet approved these changes May 13, 2024
View reviewed changes
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.

CI is happy so let's merge this.

Thanks for your contribution!

@gsmet
Copy link
Member

gsmet commented May 13, 2024

@patriot1burke are you happy with it? If so, we can merge.

@patriot1burke
Copy link
Contributor

@gsmet My reviews have not been resolved yet.

@gsmet
Copy link
Member

gsmet commented May 15, 2024

@patriot1burke which reviews? I don't see any in this PR? Could you point me to where your comments are?

@geoand
Copy link
Contributor

geoand commented May 15, 2024

Maybe the review was not posted by accident (the GH UI tends to make this extremely easy to happen)?

@hamburml
Copy link
Contributor Author

Maybe he means this comment in the linked Issue #40349 (comment)
To be honest I never had issues with the output of the lambda, so I did not do the same to the objectWriter.

patriot1burke
patriot1burke reviewed May 15, 2024
View reviewed changes
Copy link
Contributor

@patriot1burke patriot1burke left a comment

Choose a reason for hiding this comment

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

See my reviews now?

...azon-lambda/runtime/src/main/java/io/quarkus/amazon/lambda/runtime/AmazonLambdaRecorder.java Outdated Show resolved Hide resolved
...a/deployment/src/test/java/io/quarkus/amazon/lambda/deployment/testing/PersonListLambda.java Outdated Show resolved Hide resolved
...azon-lambda/runtime/src/main/java/io/quarkus/amazon/lambda/runtime/AmazonLambdaRecorder.java Show resolved Hide resolved
...azon-lambda/runtime/src/main/java/io/quarkus/amazon/lambda/runtime/AmazonLambdaRecorder.java Show resolved Hide resolved
@patriot1burke
Copy link
Contributor

Maybe he means this comment in the linked Issue #40349 (comment) To be honest I never had issues with the output of the lambda, so I did not do the same to the objectWriter.

That is one of the comments. This PR is incomplete without supporting generic return type.

@hamburml
Copy link
Contributor Author

@patriot1burke Now your comments are visible. Maybe tomorrow I will find some time. Not sure. Thanks.

@hamburml
Copy link
Contributor Author

hamburml commented Jun 3, 2024 •
edited
Loading

@patriot1burke I just pushed a new Test. Now I have a RequestHandler<List<Person>, List<Person>> and a RequestHandler<List<String>, List<Person>> lambda handler. They work out of the box, didn't need to change the writer. Any advice is appreciated, I do not know what I should do.

If it is a small change maybe you could do it yourself. I just want to use that feature and remove my workaround in my code :)

@hamburml hamburml force-pushed the feature/AmazonLambdaHandlerCollections branch from 1090537 to b640031 Compare June 12, 2024 18:36
@hamburml
Copy link
Contributor Author

@patriot1burke I do not give up! I just renamed the tests and added new ones.
We now have a Test for an handleRequest which comes from an abstract class.
We now have a Test where handleRequest gets as Input a Collection and as output a Collection.
The tests are green on my end. Please take a look.

@patriot1burke
Copy link
Contributor

There's a conflict now with main. Need to resolve conflicts

@hamburml hamburml force-pushed the feature/AmazonLambdaHandlerCollections branch from ccd7d5e to cd5056e Compare July 3, 2024 13:08
@hamburml
Copy link
Contributor Author

hamburml commented Jul 3, 2024

There's a conflict now with main. Need to resolve conflicts

I just rebased. I cant see any conflicts. Am I missing something here?

@hamburml hamburml force-pushed the feature/AmazonLambdaHandlerCollections branch 2 times, most recently from c4c7275 to 45fec2c Compare July 3, 2024 18:44
@hamburml
Fix correct parsing of collections in AmazonLambdaRecorder with Colle…
df27b36 
…ctionInputReader

add test for abstractHandler
add test for collection input and collection output
rename tests for clarification what they are for
@hamburml hamburml force-pushed the feature/AmazonLambdaHandlerCollections branch from 45fec2c to df27b36 Compare July 3, 2024 18:47
@hamburml
Copy link
Contributor Author

hamburml commented Jul 3, 2024

This is really strange. Under GitHub actions I see the workflow run and it looks like it does not run. But I do not know why.

@hamburml hamburml requested review from patriot1burke and gsmet July 3, 2024 18:52
@patriot1burke
Copy link
Contributor

I don't see the conflict anymore

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 3, 2024
@gsmet gsmet merged commit 0c20277 into quarkusio:main Jul 4, 2024
18 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jul 4, 2024
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 4, 2024
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 4, 2024
@gsmet
Copy link
Member

gsmet commented Jul 4, 2024

And merged! Thanks a lot for your efforts!

@hamburml
Copy link
Contributor Author

@gsmet could we backport this to current LTS?

@geoand
Copy link
Contributor

geoand commented Jul 15, 2024

+1 on backporting, but we'll also need #41896 because the test that was added with this is flaky

@geoand geoand mentioned this pull request Jul 15, 2024
Merged
@gsmet gsmet modified the milestones: 3.13 - main, 3.12.3 Jul 16, 2024
@gsmet gsmet modified the milestones: 3.12.3, 3.8.6 Aug 14, 2024
@hamburml hamburml deleted the feature/AmazonLambdaHandlerCollections branch November 4, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

@patriot1burke patriot1burke patriot1burke approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/amazon-lambda kind/enhancement New feature or request
Projects
None yet
Milestone
3.8.6
Development

Successfully merging this pull request may close these issues.

HandleRequestCollectionHelper for Quarkus Amazon Lambda
4 participants
@hamburml @gsmet @patriot1burke @geoand