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

Add SnsMessageConverter and NotificationMessageArgumentResolver to allow reading SNS messages on SQS #898

Merged

Conversation

msosa
Copy link
Contributor

@msosa msosa commented Sep 26, 2023

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

#887

💡 Motivation and Context

It allows for reading an SNS Message on an SQS Queue by using @NotficationMessage

💚 How did you test it?

Manually(will try to add tests when I have time)

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

🔮 Next steps

@tomazfernandes This is more of a draft for now, this works locally for me but was unsure where these files truly belong. I have put then in the sns project for now, since they use @NotificationMessage which lives there, and didn't want SQS project to have a dependency on the SNS one.

Let me know if you agree, or we should create a new annotation to be used.

Also in the previous implementation of NotificationRequestConverter it added MessageAttributes to the message. For my use-case I was able to remove that completely and things worked fine. Let me know if that is actually needed, but I did see SqsHeaderMapper has a lot of the same logic for what was being done for MessageAttributes

@github-actions github-actions bot added the component: sns SNS integration related issue label Sep 26, 2023
@msosa msosa marked this pull request as draft September 26, 2023 21:04
@tomazfernandes
Copy link
Contributor

Hi @msosa, thanks for the PR.

I'm leaning towards having a separate annotation, since the modules are now separated, and I believe it would make sense for a project to consume an SNS message from SQS but not need anything from the SNS module itself.

We'd probably want to have a different name for the annotation so that it's not confusing for the users.

@MatejNedic, do you have any thoughts on this? Anything I might be missing regarding the integration between the modules?

Thanks

@msosa msosa force-pushed the sqs-integration-with-sns-notification-message branch from 3bfb74d to 60b1e3d Compare October 8, 2023 22:23
@github-actions github-actions bot added the component: sqs SQS integration related issue label Oct 8, 2023
@msosa msosa force-pushed the sqs-integration-with-sns-notification-message branch from 60b1e3d to 3c81c9e Compare October 8, 2023 22:24
@msosa
Copy link
Contributor Author

msosa commented Oct 8, 2023

Makes sense, I have moved all the code into SQS and created a new annotation called SnsNotificationMessage(open to alternative names).

@msosa msosa force-pushed the sqs-integration-with-sns-notification-message branch 2 times, most recently from 00ce0a0 to d61525e Compare October 9, 2023 01:52
Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

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

Hey @msosa, thanks, it's looking good.

Added a few comments, let me know if they make sense to you.

It would also be great if you can include an integration test so we can see it in action.

Thanks

Copy link
Contributor Author

@msosa msosa left a comment

Choose a reason for hiding this comment

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

I am now in the process of making the integration test. I have the test hitting the new code, however its failing due to the message I am providing.

I will fix that and complete the test in another commit later on

@jerchende
Copy link

Hi,
I just saw your merge request. I am currently working around this missing feature, since we want to update to spring boot 3. And I wonder if the MessageAttributes from the SNS notification don't need to be mapped into MessageHeader as well?
Best regards,
Johannes

@gustavomonarin
Copy link
Contributor

gustavomonarin commented Feb 20, 2024

Hello @msosa ,

Do you have plans to further work on this MR?

I have implemented something internally very similarly and would love to drop this code in favor of the opensource implementation.

The main difference was related to the MessageConverter. In our case we do use CloudEvents and the current MessageConverter is not able to handle well subtypes likes CloudEvent<MyEvent>. It is a super tiny change but that enables a lot. I would be more than happy to create a mr to your branch if you wish.

@msosa msosa marked this pull request as ready for review February 26, 2024 05:19
@msosa
Copy link
Contributor Author

msosa commented Feb 26, 2024

HI @gustavomonarin,

Yes I do plan on working on this MR, was just waiting for review(I just realized I may not have requested said review)

But yeah am open to checking out and change you mentioned and bringing into this MR

Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

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

Looking good, left a few comments.

/**
* Notification request wrapper.
*/
public static class NotificationRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use a record instead?

@Override
public Object resolveArgument(MethodParameter par, Message<?> msg) throws Exception {
Object object = this.converter.fromMessage(msg, par.getParameterType());
NotificationRequestConverter.NotificationRequest nr = (NotificationRequestConverter.NotificationRequest) object;
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets use Assert.isInstance() here to throw a pretty message in case the instance doesn't match the expected class?

@tomazfernandes
Copy link
Contributor

Hey @msosa, really sorry for the delay - I thought I was the one waiting for your reply. Left a few comments on the PR, please let me know if they make sense to you.

@gustavomonarin thanks for showing up. Can you share a bit more about what's your proposed change?

Thanks.

@msosa msosa force-pushed the sqs-integration-with-sns-notification-message branch from 401d7d0 to 1f4c139 Compare February 27, 2024 02:41
Copy link
Contributor Author

@msosa msosa left a comment

Choose a reason for hiding this comment

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

No problem @tomazfernandes, I had it in draft so it was a bit confusing.

Also in regards to @jerchende's comment, I have implemented it locally similar to how it was done previously. But I had to make this method public static. Would it make sense to move NumberParser to its own class, so we can use the getNumberValue?

@tomazfernandes
Copy link
Contributor

tomazfernandes commented Mar 9, 2024

current MessageConverter is not able to handle well subtypes likes CloudEvent<MyEvent>

@gustavomonarin, can you elaborate on this please?

In order to easily support generic types, this PR makes the SnsMessageConverter a standard spring SmartMessageConverters.

Smart Message Converter is deeply used within spring in a variety of different ways for compatiblity. The current main converter, the CompositeConverter is already a SmartConverter.

This change branch contains the minimal and less intrusive changes, especially no touching open public abstract classes.
@tomazfernandes
Copy link
Contributor

Hi @msosa and @tomazfernandes i mad a small change in the test to cover the case i mentioned.

Also provided two implementation solutions, a small one in the spring way of compatibility and a second one with a bit more ofbreaking change on the bean post processor.

The PR has not much deatils in the description but the two commit messages goes in the details, so i would suggest to review the commits individually.

@gustavomonarin I don't see your PR with these commits, where can I find it?

@tomazfernandes
Copy link
Contributor

@msosa we're pretty much there!

The only thing missing is the documentation, let me know if you need any help with that.

Thanks!

@gustavomonarin
Copy link
Contributor

gustavomonarin commented Mar 12, 2024

Hi @msosa and @tomazfernandes i mad a small change in the test to cover the case i mentioned.

Also provided two implementation solutions, a small one in the spring way of compatibility and a second one with a bit more ofbreaking change on the bean post processor.

The PR has not much deatils in the description but the two commit messages goes in the details, so i would suggest to review the commits individually.

@gustavomonarin I don't see your PR with these commits, where can I find it?

It is on the @msosa original repo, as is based on his changes.

… `ObjectMapper` in `createAdditionalArgumentResolvers`
@github-actions github-actions bot added the type: documentation Documentation or Samples related issue label Mar 13, 2024
@msosa
Copy link
Contributor Author

msosa commented Mar 13, 2024

@tomazfernandes Awesome, appreciate all the feedback!

I gave the documentation a try, let me know what you think.

@tomazfernandes
Copy link
Contributor

@gustavomonarin, I looked at your suggestion to switch from implementing the MessageConverter interface to implementing the SmartMessageConverter interface which allows type hints and it makes sense.

I prefer the solution in this PR as it allows more flexibility for the user and is not a breaking change.

Now, not sure how to coordinate this - we do have an eminent version release coming up and it'd be great to have this feature in it.

@msosa, how would you feel about incorporating @gustavomonarin's changes into your PR?

@msosa
Copy link
Contributor Author

msosa commented Mar 13, 2024

@tomazfernandes of course, I have merged your preferred solution of @gustavomonarin into this PR.

Also I have updated the the documentation section to talk about SNS Messages

@msosa msosa changed the title Add NotificationRequestConverter and NotificationMessageArgumentResolver to allow reading SNS messages on SQS Add SnsMessageConverter and NotificationMessageArgumentResolver to allow reading SNS messages on SQS Mar 13, 2024
Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

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

Almost there @msosa, just one comment on the documentation

docs/src/main/asciidoc/sqs.adoc Outdated Show resolved Hide resolved
@msosa
Copy link
Contributor Author

msosa commented Mar 13, 2024

Removed the Note @tomazfernandes, let me know if there's anything else

Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

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

Looking great @msosa!

Just a couple of finishing touches.

  1. Please add @since 3.1.1 to the javadocs
  2. Please add @gustavomonarin's name under yours in the relevant files
  3. Please add "Since 3.1.1, when receiving SNS messages..."

That, and we're good to go!

Thanks!

@msosa
Copy link
Contributor Author

msosa commented Mar 14, 2024

@tomazfernandes Done! I just used @gustavomonarin username as I wasn't sure if exact first/last name.

@tomazfernandes
Copy link
Contributor

@tomazfernandes Done! I just used @gustavomonarin username as I wasn't sure if exact first/last name.

Thanks @msosa!

@gustavomonarin, please let us know if you want us to name you in any other way.

Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

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

Great work @msosa! And thanks @gustavomonarin for the improvement!

Looking forward for more!

@tomazfernandes tomazfernandes merged commit 6ef09c0 into awspring:main Mar 15, 2024
4 checks passed
@coco1979ka
Copy link

Looks great for Single Message processing. What is your recommendation to receive a List of SNSNotificationMessages for batch processing?

@EstelaGarcia
Copy link

We are also looking for a way to receive a List of SnsNotificationMessages and struggling with this. Any suggestions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: sns SNS integration related issue component: sqs SQS integration related issue type: documentation Documentation or Samples related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants