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

Attach failed message ids to error object #100

Merged
merged 3 commits into from
Mar 11, 2024
Merged

Conversation

knicola
Copy link
Contributor

@knicola knicola commented Nov 18, 2023

Description:
Throw a custom error, when messages fail to send, with an additional property to expose the failed message ids for programmatic access.

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Why is this change required?:
In cases where we have to take additional steps for the failed messages, we have to parse the error message in order to get their IDs.

Code changes:

  • Add a custom FailedMessagesError error class, with a failedMessages property of type string[] to hold the IDs of the messages that failed to send.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

this is to allow programmatic access to the ids,
rather than having to parse the error message.
@knicola knicola requested review from a team as code owners November 18, 2023 20:33
@nicholasgriffintn
Copy link
Member

Hey, sorry, super busy this week, I'm gonna slot some time to look into this on Friday hopefully, if not before.

@knicola
Copy link
Contributor Author

knicola commented Nov 24, 2023

I have read the CLA Document and I hereby sign the CLA

@knicola
Copy link
Contributor Author

knicola commented Nov 24, 2023

¯\_(ツ)_/¯

@vizeke
Copy link

vizeke commented Dec 8, 2023

I suggest saving the entire BatchResultErrorEntry from the sqs.send response in the FailedMessagesError. This way will be easier to track what's going wrong.

Copy link
Member

@nicholasgriffintn nicholasgriffintn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I just realised that I am super late here, sorry, holiday and crazy work period.

This looks fine to me, although I do need to investigate if this is potentially breaking, probably not.

@nicholasgriffintn nicholasgriffintn added this to the v5.0.0 milestone Mar 11, 2024
@nicholasgriffintn
Copy link
Member

Merging the live, will mark this as a major change just to be safe.

@nicholasgriffintn nicholasgriffintn merged commit 61246de into bbc:main Mar 11, 2024
1 of 2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants