-
Notifications
You must be signed in to change notification settings - Fork 176
fix(parser): Removed the nullable type from the md5OfMessageAttributes in SqsRecordSchema #4165
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
Conversation
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
|
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.
Great work, thank you for fixing the schema and congrats for your first PR merged in this project 🎉
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
Hi. After this "fix", I am getting this error.
Logging the event shows that I do get this attribute as From the logs (strigified payload)
I am not quite sure in what circumstances this occurs, but we should probably revert this change. Thanks |
Hi @bboure, thanks for reporting.
I think this would be helpful to understand if the type and schemas are indeed off on not. For now, as discussed in the linked issue we assumed that the type was correct and we weren't but we also weren't able to reproduce the field being If you could share steps to reproduce the field being |
Thanks @dreamorosi So, here is a snippet of my code that reproduces this issue. From another Lambda function: export const sendMessageBatch = async (
queueUrl: string,
messages: Array<{
id: string;
body: string;
delayInSeconds?: number;
}>,
) => {
const entries: SendMessageBatchRequestEntry[] = messages.map((message) => ({
Id: message.id,
MessageBody: message.body,
DelaySeconds: message.delayInSeconds,
}));
const command = new SendMessageBatchCommand({
QueueUrl: queueUrl,
Entries: entries,
});
await sqsClient.send(command);
}; Usage const events = [
{ id: '123', body: JSON.stirngify({ foo: 'bar' }) }
];
await sendMessageBatch(NOTIFICATION_QUEUE_URL, events); After some research, I figured that adding Id: message.id,
MessageBody: message.body,
// Adding this works 👇
MessageAttributes: {
ContentType: { DataType: 'String', StringValue: 'application/json' },
},
DelaySeconds: message.delayInSeconds, |
And just in case it's relevant: NodeJS version: 22 |
Ok, thanks - we'll look into it on Monday. |
I've looked into it and I'm unable to reproduce the "bug". I have created a minimal CDK stack with 2 functions and 1 queue that you can find here. You're welcome to clone the repo, deploy the stack, and verify the statements below in your own account. The producer function uses the code you shared above to publish messages to the queue, while the consumer one is triggered by the queue and uses various combinations of built-in SQS schemas and envelopes to parse the event. After deploying the stack and triggering the producer (instructions are in the README), you can see from the logs of the consumer function that all parse operations succeed despite the message attributes being empty and the After re-reading the PR and remembering what it was about, this makes sense to me, since the PR marks the field as As you can see from the git history, since it was introduced, the Either way based on the above and the information you provided, I'm not inclined to revert this PR. |
Thanks @dreamorosi After doing some more tests, comparing with our solution, I finally found how to reproduce. It happens when enabling
|
Interesting! Before we fix this, let me ask some questions internally. I find it weird that enabling a seemingly unrelated setting in the Event Source Mapping (ESM) causes the field to be omitted or set to In the meantime I suggest to manually override the field in the schema, or patch the producer like you suggested earlier. |
@dreamorosi I can confirm that |
Summary
This PR removes the
nullable
type from themd5OfMessageAttributes
in theSqsRecordSchema
aligning with the @types/aws-lambda packageChanges
nullable
type from themd5OfMessageAttributes
property in theSqsRecordSchema
firehose-sqs.json
test event to remove thenull
value ofmd5OfMessageAttributes
Issue number: #4109
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.