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

SNS MessageAttributes are optional #208

Merged
merged 3 commits into from
Jun 11, 2021

Conversation

DwayneCoussement
Copy link
Contributor

@DwayneCoussement DwayneCoussement commented Jun 4, 2021

Make MessageAttributes optional on SNS.Message

Motivation:

When testing the following situation:
SNS Topic => SQS Queue => Lambda

Notice that you'll see that SQS will have an SNS Message in the body without MessageAttributes

Note that after further research I also noticed that this is not sufficient:

  • UnsubscribeUrl is transformed to UnsubscribeURL
  • SigningCertUrl is transformed to SigningCertURL

Modifications:

  • Updated the unit tests
  • Made MessageAttributes optional

Result:

MessageAttributes will be optional, and you'll be able to parse an SNS Message in the situation described in the motivation

@swift-server-bot
Copy link

Can one of the admins verify this patch?

5 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@tomerd
Copy link
Contributor

tomerd commented Jun 8, 2021

@swift-server-bot test this please

@tomerd tomerd requested a review from fabianfett June 8, 2021 17:34
@tomerd
Copy link
Contributor

tomerd commented Jun 8, 2021

seems reasonable to me.

@DwayneCoussement can you elaborate on

Note that after further research I also noticed that this is not sufficient:

  • UnsubscribeUrl is transformed to UnsubscribeURL
  • SigningCertUrl is transformed to SigningCertURL

@DwayneCoussement
Copy link
Contributor Author

Sure thing; so imagine having following simplified setup, this is just as a reference, so sorry if there are some mistakes in there:

const topic = new sns.Topic(this, 'MyTopic', {
    displayName: 'Topic'
});
const queue = new sqs.Queue(this, "MyQueue", {
    queueName: 'MyQueue'
});
const myIntegration = new AwsIntegration({
    service: 'sns',
    path: `${myAccount}/${topic.topicArn}`,
    integrationHttpMethod: 'POST',
    options: {
      credentialsRole: myCredentials,
      passthroughBehavior: PassthroughBehavior.NEVER,
      requestParameters: {
        'integration.request.header.Content-Type': "'application/x-www-form-urlencoded'"
      },
      requestTemplates: {
        'application/json':
          "Action=Publish" +
          `&TopicArn=$util.urlEncode('${topic.topicArn}')` +
          "&Message=$util.urlEncode($input.body)"
      },
      integrationResponses: [
        {
          statusCode: "200",
          responseTemplates: {
            "application/json": `{"statusCode": 200}`,
          },
        },
      ],
    }
  });
  const api = new RestApi(this, "myGateway", {
    apiKeySourceType: ApiKeySourceType.HEADER,
  });
  const testMethod = api.root.addResource('test');
  testMethod.addMethod('POST', myIntegration, { apiKeyRequired: true, methodResponses: [{ statusCode: "200" }] })

  topic.addSubscription(new subs.SqsSubscription(queue));
  myLambda.addEventSource(new SqsEventSource(queue));

Notice that if you check the contents of the SQS records, some kind of transform happened on the original SNS message.

MessageAttributes in the above example, these will not be set, so this is the initial fix.
Notice as well that somehow SigningCertUrl is now SigningCertURL and UnsubscribeUrl is now UnsubscribeURL. I haven't looked yet in how and if we should fix this. Any suggestions from your side?

seems reasonable to me.

@DwayneCoussement can you elaborate on

Note that after further research I also noticed that this is not sufficient:

  • UnsubscribeUrl is transformed to UnsubscribeURL
  • SigningCertUrl is transformed to SigningCertURL

@tomerd
Copy link
Contributor

tomerd commented Jun 11, 2021

MessageAttributes in the above example, these will not be set, so this is the initial fix.
Notice as well that somehow SigningCertUrl is now SigningCertURL and UnsubscribeUrl is now UnsubscribeURL. I haven't looked yet in how and if we should fix this. Any suggestions from your side?

I assume AWS may be doing some transformation. cc @bmoffatt

@tomerd tomerd added the 🔨 semver/patch No public API change. label Jun 11, 2021
@tomerd
Copy link
Contributor

tomerd commented Jun 11, 2021

@swift-server-bot test this please

@tomerd tomerd merged commit 9a7d9d0 into swift-server:main Jun 11, 2021
@tomerd tomerd added this to the 0.5.0 milestone Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants