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

Fix #134 0.76.0: SNS -> publish to subscribed SQS queue, "Records" assumption leads to lost message, empty array. #134 #173

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

huangjimmy
Copy link

@huangjimmy huangjimmy commented Feb 25, 2023

Fix #134

There is no need to check if (sub["Attributes"]["RawMessageDelivery"] === "true") inside publishSqs, this RawMessageDelivery check-process is already handled by publish.

The event object is what should be sent directly to an sqs endpoint.

This PR also works with serverless-offline-sqs plugin.

Tested locally with serverless-offline-sqs

unit test pass.

➜  serverless-offline-sns git:(issue_134_publish_to_sqs) ✗ yarn test
yarn run v1.22.19
$ nyc ts-mocha "test/**/*.ts" --slow 300 -p src/tsconfig.json


  test
    ✔ should start on offline start
    ✔ should start on command start
    ✔ should send event to topic ARN
    ✔ should send event to target ARN
    ✔ should send event with pseudo parameters
    ✔ should send event with MessageAttributes and subject
    ✔ should return a valid response to publish
    ✔ should send a message to a E.164 phone number
    ✔ should error
    ✔ should use the custom host for subscription urls
    ✔ should use the custom subscribe endpoint for subscription urls
    ✔ should unsubscribe
    ✔ should read env variable
    ✔ should read env variable for function
    ✔ should convert pseudo param on load
    ✔ should completely reload the module every time if cache invalidation is enabled (210ms)
    ✔ should send event to handlers with more than one dot in the filename
    ✔ should support async handlers with no callback
    ✔ should not send event when filter policies exist and fail
    ✔ should send event when filter policies exist and pass
    ✔ should not send event when multiple filter policies exist and the message only contains one
    ✔ should not send event when multiple filter policies exist and the message only satisfies one
    ✔ should not wrap the event when the sub's raw message delivery is true
    ✔ should list topics
    ✔ should subscribe
    ✔ should handle empty resource definition
    ✔ should handle messageGroupId


  27 passing (3s)

✨  Done in 4.62s.
➜  serverless-offline-sns git:(issue_134_publish_to_sqs) ✗ 

…ds" assumption leads to lost message, empty array. mj1618#134

There is no need to check `    if (sub["Attributes"]["RawMessageDelivery"] === "true") ` inside `publishSqs`, this `RawMessageDelivery` check-process is already handled by `publish`.
The `event` object is what should be sent directly to an sqs endpoint.

This also works with serverless-offline-sqs plugin.
@jimmy-shaojun jimmy-shaojun force-pushed the issue_134_publish_to_sqs branch from 3935b5d to a11eea8 Compare February 25, 2023 02:35
@ormu5
Copy link

ormu5 commented Mar 25, 2023

I left existing logic and addressed this with:

    // No parsing if raw message delivery or local SQS-SNS subscriptions
    // https://docs.aws.amazon.com/sns/latest/dg/sns-sqs-as-subscriber.html
    if (sub["Attributes"]["RawMessageDelivery"] === "true" || JSON.parse(event).Records === undefined) {

In addition I added functionality to detect and add SQS/SNS subscriptions (that does not seem to occur with the latest version of this lib). My fork can be found: https://www.npmjs.com/package/@ormu5/serverless-offline-sns. Would like to see development pick up again on this plugin; it makes for a potent local development environment when everything's working...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0.76.0: SNS -> publish to subscribed SQS queue, "Records" assumption leads to lost message, empty array.
3 participants