-
Notifications
You must be signed in to change notification settings - Fork 15
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
fixed SNS MessageAttributes parsing #94
base: master
Are you sure you want to change the base?
Conversation
Code Climate has analyzed commit d53339d and detected 0 issues on this pull request. View more on Code Climate. |
@AlexanderKudryavsky I am not sure what goes wrong here... If the queue messages are generated by SNS, then they will never by gzipped or stored in S3, you should probably not send messages yourself in the same queue an SNS can send messages in. If you do think this is a valid case, I don't see in AWS docs where the property MessageAttributes exists on the SNS message body. Can you please refer me to it? Same goes for the new way you suggest to parse attributes. |
if (opts.unwrapSns) { | ||
const unwrapped: SNSBody = JSON.parse(this.body || EMPTY_BODY); | ||
this.body = unwrapped.Message; | ||
this.subject = unwrapped.Subject; | ||
this.topicArn = unwrapped.TopicArn; | ||
this.attributes = parseMessageAttributes(unwrapped.MessageAttributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseMessageAttributes expects a map and not a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also be aware that this will cause an overide of the real message attributes.... so if you send a regular message (not via SNS) this will remove all the attributes. Might be better to merge the attributes (_.extend() for example)
@@ -21,6 +21,7 @@ export interface IMessageOpts { | |||
} | |||
|
|||
interface SNSBody { | |||
MessageAttributes: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a proper typing for that and then use that interface in the parseMessageAttributes and related functions (please use type guards to parse properly depending on the type we receive)
@@ -27,8 +27,8 @@ export const parseMessageAttributes = (messageAttributes: SQS.MessageBodyAttribu | |||
}; | |||
|
|||
const parseAttributeValue = (unparsedAttribute: SQS.MessageAttributeValue): IMessageAttribute => { | |||
const type = unparsedAttribute.DataType; | |||
const stringValue = unparsedAttribute.StringValue; | |||
const type = unparsedAttribute.DataType || unparsedAttribute.Type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please parse according to the actual type using type guards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@regevbr
If you include raw in sns, then the structure of the message changes, the attributes are no longer in the body, but it still does not work correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you give exampels please. I'm guessing that if you are using the raw option, you don't need the unwrapSns flag turned on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@regevbr
unwrapSns: false
works. In this case, unwrapSns
works somehow differently than I expected, the messages structure I posted to the Issue cannot be handled correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, so lets try to handle it in this PR
@AlexanderKudryavsky please also add tests and make sure the code compiles ( |
Kudos, SonarCloud Quality Gate passed! |
Summary
This PR fixes/implements the following bugs/features
Explain the motivation for making this change. What existing problem does the pull request solve?
Test plan (required)