SNS SMS support #457

MatejNedic · 2022-07-15T17:35:18Z

📢 Type of change

Bugfix
New feature
Enhancement
Refactoring

📜 Description

This is currently just a draft how I vision Spring Cloud AWS support for sending SMS messages.
Before I go further with tests @maciejwalkowiak please check design out.

💡 Motivation and Context

💚 How did you test it?

📝 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

maciejwalkowiak · 2022-07-16T06:30:08Z

spring-cloud-aws-sns/src/main/java/io/awspring/cloud/sns/sms/attributes/ADM.java

+ */
+package io.awspring.cloud.sns.sms.attributes;
+
+public class ADM {


Have you considered making these attribute classes immutable?

maciejwalkowiak · 2022-07-16T06:30:59Z

spring-cloud-aws-sns/src/main/java/io/awspring/cloud/sns/sms/attributes/ADM.java

+		private Builder() {
+		}
+
+		public Builder withTtl(Long ttl) {


For the sake of consistency with other builder classes in the project and builder classes in AWS SDK, instead of "withers" method names should be just named with an attribute"

Builder ttl(Long ttl) { ... }

maciejwalkowiak · 2022-07-16T06:32:33Z

...d-aws-sns/src/main/java/io/awspring/cloud/sns/sms/core/DefaultMessageAttributeConverter.java

+ */
+public class DefaultMessageAttributeConverter implements MessageAttributeConverter {
+
+	public Map<String, MessageAttributeValue> convert(SmsMessageAttributes attr) {


Have you considered moving this conversion to SmsMessageAttributes class? Does not seem like there can be any other reasonable implementation of MessageAttributeConverter interface

spring-cloud-aws-sns/src/main/java/io/awspring/cloud/sns/sms/core/SnsSmsOperations.java

MatejNedic · 2022-07-21T15:28:08Z

Will commit sample in following days

spring-cloud-aws-sns/src/main/java/io/awspring/cloud/sns/sms/attributes/AttributeCodes.java

…ttributes/AttributeCodes.java

maciejwalkowiak

Please add @Nullable to attribute classes (I imagine all these parameters are nullable..?).

Also perhaps we can merge packages attributes and core into single sms - this way we could reduce the number of public classes

maciejwalkowiak · 2022-08-03T06:06:15Z

...g-cloud-aws-sns/src/main/java/io/awspring/cloud/sns/sms/attributes/SmsMessageAttributes.java

+	private String entityId;
+	private String templateId;
+	private String messageGroupId;
+	private String deduplicationId;


Deduplication id makes sense only for sending to topics, so it should not be a property on the top level attributes used also for sending to a number. https://docs.aws.amazon.com/sns/latest/dg/fifo-message-dedup.html

maciejwalkowiak · 2022-08-03T06:08:04Z

...g-cloud-aws-sns/src/main/java/io/awspring/cloud/sns/sms/attributes/SmsMessageAttributes.java

+	private String templateId;
+	private String messageGroupId;
+	private String deduplicationId;
+	private String messageStructure;


As far as I can see, the only valid value for message structure is "json" and it is used for sending to topic when more than single protocol endpoints are subscribed to this topic. I think this is "advanced" usage that we don't need to cover with SnsSmsTemaplte - we can support sending only to topics with sms endpoints or if we decide to support mixed topics, this has to be redesigned.

maciejwalkowiak · 2022-08-03T06:11:37Z

spring-cloud-aws-sns/src/main/java/io/awspring/cloud/sns/sms/core/SnsSmsOperations.java

+
+	void sendToTopicArn(String topicArn, String message, SmsMessageAttributes attributes);
+
+	void sendToTargetArn(String targetArn, String message);


What is actually the use case for sending to targetArn? As far as I could find out there is no use case for SMS to send to target arn. Only to topic or directly to a phone number.

maciejwalkowiak · 2022-08-03T06:13:48Z

...loud-aws-sns/src/test/java/io/awspring/cloud/sns/sms/core/SnsSmsTemplateIntegrationTest.java

+	@Test
+	void sendValidMessage_ToPhoneNumber() {
+		Assertions
+				.assertDoesNotThrow(() -> snsSmsTemplate.send("+385 00 000 0000", "Spring Cloud AWS got you covered!"));


Can we assert log message from Localstack?

localstack/localstack#2822

At first glance, I am not seeing it in container logs but will check again later.

If I understand correctly, we're only checking the message is sent and doesn't throw any errors. Maybe we can fetch the message from local stack after sending it, and assert that?

If there are any concurrency concerns, I usually use a CountDownLatch to wait on the test thread until the message is consumed.

Hey @tomazfernandes I tried with getting localstack logs but I am not seeing a message. We can on other hand make operations and template return messageId and check that. Localstack uses moto to mock sms sending.

Hey @MatejNedic, nice work here! Sure, if the SNS API returns an id I guess we should return that to users.

This being an SNS integration, can't we maybe subscribe to the topic and receive the sent message ourselves too? Just guessing, have no clue how this actually works 😄

@MatejNedic Did you set the DEBUG env var to "1"? in LocalStackContainer should be .withEnv("DEBUG", "1") and change the logger for testcontainers to DEBUG too <logger name="org.testcontainers" level="DEBUG"/> just to make sure something useful is in the container's log. Then you can fetch the logs doing String logs = localstack.getLogs(OutputFrame.OutputType.STDOUT, OutputFrame.OutputType.STDERR); and assert against it.

@eddumelendez I tried that as well but I am not seeing the correct message I want to see.
Delivering SMS message to. I am seeing Publishing message to TopicArn: None | Message: Spring Cloud AWS got you covered which makes sense but is still not what I want. I have added tests with at least some kind of message. I tried this against real AWS and I am getting SMS on my phone number without an issue.

@tomazfernandes we have something similar for SNS notifications where we send it to an SQS and we poll it from SQS. Sadly with SMS we can't do that.

MatejNedic · 2022-08-31T18:03:34Z

Tried it against real AWS integration works as expected.

maciejwalkowiak · 2022-08-31T18:37:23Z

Great job! Please add nullability annotations (including package-info.java) and javadocs on public classes & interfaces. Also, conflict has to be resolved.

sonarcloud · 2022-09-01T14:14:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

MatejNedic added 3 commits July 10, 2022 06:24

prepare models

2d4df2e

Introduce SMS support

065c19b

run spotless::apply

2fb91ad

maciejwalkowiak requested changes Jul 16, 2022

View reviewed changes

MatejNedic added 4 commits July 20, 2022 17:56

Refactor and take care of PR

b5411d6

Apply spotless

d55c9ab

remove colon

7c7df0c

add simple docs

5f94fef

github-actions bot added the type: documentation Documentation or Samples related issue label Jul 21, 2022

MatejNedic marked this pull request as ready for review July 21, 2022 15:27

MatejNedic requested a review from eddumelendez as a code owner July 21, 2022 15:27

maciejwalkowiak reviewed Jul 22, 2022

View reviewed changes

spring-cloud-aws-sns/src/main/java/io/awspring/cloud/sns/sms/attributes/AttributeCodes.java Outdated Show resolved Hide resolved

Update spring-cloud-aws-sns/src/main/java/io/awspring/cloud/sns/sms/a…

729fe2a

…ttributes/AttributeCodes.java

maciejwalkowiak requested changes Aug 3, 2022

View reviewed changes

Polish.

f727e77

maciejwalkowiak added component: sns SNS integration related issue type: feature Integration with a new AWS service or bigger change in existing integration and removed type: documentation Documentation or Samples related issue labels Aug 3, 2022

maciejwalkowiak added this to the 3.0.0 M2 milestone Aug 3, 2022

Polish sns sms integration

9a8c89f

github-actions bot added the type: documentation Documentation or Samples related issue label Aug 29, 2022

MatejNedic marked this pull request as draft August 29, 2022 17:57

MatejNedic added 2 commits August 31, 2022 19:38

clean messageStructure

bdc5a8f

Apply formatting

f2113c0

MatejNedic marked this pull request as ready for review August 31, 2022 17:58

MatejNedic requested a review from tomazfernandes as a code owner August 31, 2022 17:58

MatejNedic requested review from maciejwalkowiak and removed request for eddumelendez August 31, 2022 17:58

MatejNedic added 3 commits September 1, 2022 16:04

add Javadocs

4512df2

format

cfc6c51

fix \n

e563607

maciejwalkowiak modified the milestones: 3.0.0 M2, 3.0.0 M3 Sep 2, 2022

MatejNedic and others added 8 commits September 2, 2022 17:06

Take care of comments

1626d22

Merge remote-tracking branch 'origin/main' into sms-support

4348853

polish

fc5f486

Move to single package.

665aa8b

Refactor

25fd43f

Refactor

7908441

Fix providing custom SnsOperations and SnsSmsOperations beans.

3b96232

Polish docs.

4852f13

maciejwalkowiak merged commit 204829f into awspring:main Sep 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNS SMS support #457

SNS SMS support #457

MatejNedic commented Jul 15, 2022

maciejwalkowiak Jul 16, 2022

maciejwalkowiak Jul 16, 2022

maciejwalkowiak Jul 16, 2022

MatejNedic commented Jul 21, 2022

maciejwalkowiak left a comment

maciejwalkowiak Aug 3, 2022

maciejwalkowiak Aug 3, 2022

maciejwalkowiak Aug 3, 2022

maciejwalkowiak Aug 3, 2022

MatejNedic Aug 29, 2022

tomazfernandes Sep 1, 2022

MatejNedic Sep 1, 2022

tomazfernandes Sep 1, 2022

eddumelendez Sep 1, 2022

MatejNedic Sep 2, 2022

MatejNedic commented Aug 31, 2022

maciejwalkowiak commented Aug 31, 2022 •

edited

Loading

sonarcloud bot commented Sep 1, 2022


		void sendToTopicArn(String topicArn, String message, SmsMessageAttributes attributes);

		void sendToTargetArn(String targetArn, String message);

SNS SMS support #457

SNS SMS support #457

Conversation

MatejNedic commented Jul 15, 2022

📢 Type of change

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatejNedic commented Jul 21, 2022

maciejwalkowiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatejNedic commented Aug 31, 2022

maciejwalkowiak commented Aug 31, 2022 • edited Loading

sonarcloud bot commented Sep 1, 2022

maciejwalkowiak commented Aug 31, 2022 •

edited

Loading