spring-cloud-aws icon indicating copy to clipboard operation
spring-cloud-aws copied to clipboard

Published 20 hours ago • verified publisher icon awspring

Add `NotificationRequestConverter` and `NotificationMessageArgumentResolver` to allow reading SNS messages on SQS

Open msosa opened this issue 2 years ago • 6 comments

:loudspeaker: Type of change

  • [ ] Bugfix
  • [ ] New feature
  • [x] Enhancement
  • [ ] Refactoring

:scroll: Description

#887

:bulb: Motivation and Context

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

:green_heart: How did you test it?

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

:pencil: Checklist

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

:crystal_ball: 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

msosa avatar Sep 26 '23 21:09 msosa

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

tomazfernandes avatar Oct 04 '23 02:10 tomazfernandes

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

msosa avatar Oct 08 '23 22:10 msosa

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

jerchende avatar Oct 17 '23 06:10 jerchende

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.

gustavomonarin avatar Feb 20 '24 10:02 gustavomonarin

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

msosa avatar Feb 26 '24 05:02 msosa

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.

tomazfernandes avatar Feb 27 '24 00:02 tomazfernandes

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

@gustavomonarin, can you elaborate on this please?

tomazfernandes avatar Mar 09 '24 18:03 tomazfernandes

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 avatar Mar 11 '24 10:03 gustavomonarin

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 avatar Mar 12 '24 02:03 tomazfernandes

@msosa we're pretty much there!

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

Thanks!

tomazfernandes avatar Mar 12 '24 02:03 tomazfernandes

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.

gustavomonarin avatar Mar 12 '24 09:03 gustavomonarin

@tomazfernandes Awesome, appreciate all the feedback!

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

msosa avatar Mar 13 '24 00:03 msosa

@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?

tomazfernandes avatar Mar 13 '24 00:03 tomazfernandes

@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 avatar Mar 13 '24 01:03 msosa

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

msosa avatar Mar 13 '24 02:03 msosa

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

msosa avatar Mar 14 '24 02:03 msosa

@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.

tomazfernandes avatar Mar 14 '24 02:03 tomazfernandes

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

coco1979ka avatar Mar 27 '24 13:03 coco1979ka

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

EstelaGarcia avatar Apr 03 '24 11:04 EstelaGarcia