return the result from the publisher to the caller.

[robvanpamel]

Description of changes:

When a message fails from eventbridge, the error result was not shown. This could lead to some issues in the future.
I adapted this so the errormessage and if required the errorcode can be retrieved

As the publisher is abstracted across EventBridge, SQS and SNS I had to make some sacrifices for naming; event <-> message.
Please have a look at it and provide some feedback so it can be adapted where you need it.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[normj]

For SNS and SQS a user would need to catch exceptions and for EventBridge they would need to check the response which is likely to be forgotten. For example we forgot to think about it. I believe EventBridge's PutEvents API does the HTTP 200 with errors pattern because it supports batch event publishing. Since we are only sending a single event I wonder if we should have the EventBridge publisher check if response for failure and then we should throw a Messaging exception. Update the other publishers to also throw the same Messaging exception wrapping the SDK's exception. What do you think?

If we decide to support the batch flow in the future we can handle that threw separate APIs.

[robvanpamel]

I agree to create a separate API for batch processing in the future.
So to continue, we keep the existing IPublishResponse to return a messageid / eventid but remove the ErrorMessage from it. I do think the messageid is required as a lot of people use this. At least on my 2 previous projects we had this in place.

For returning the errormessage to the user with eventbridge, we check the response and throw a Message Exception. Let's call it FailedToPublishException. ( Like the others it would inherit from AWSMessagingException)

For the other publishers SQS and SNS, we catch the exceptions and wrap it with the same FailedToPublishException.
Do you mind if the existing exceptions, eg InvalidMessageException would be wrapped in the FailedToPublishException as an innerexception as well (https://github.com/awslabs/aws-dotnet-messaging/blob/main/src/AWS.Messaging/Publishers/SNS/SNSPublisher.cs )? That way it can be easily implemented.

[normj]

I agree we should have the IPublishResponse with MessageId. I'm preferring MessageId over EventId since we are calling this our messaging library.

Yes to your logic for checking the response from event bridge and throw FailedToPublishException exception if there is a failure.

All publishers should throw FailedToPublishException and the XML docs for the publish method should indicate that with the <exception> tag. For SQS and SNS it will wrap the service exception. The FailedToPublishException exception when only be for for if the service call fails in some way. The InvalidMessageException is for bad configuration and I don't think should be wrapped in the FailedToPublishException.

[robvanpamel]

Changes are done. I did had some issues in GIT so the local history looks strange. But I assume you can squash it, so that does not causes issues.
Let me know if you see something else!

[normj]

Thanks @robvanpamel. Don't worry about the commits, we will squash when we merge. The PR looks good to me. Let me get another team member on the team to review it.

[philasmar]

Could you please rebase your PR on top of dev?
Also, could you please follow https://github.com/awslabs/aws-dotnet-messaging/blob/dev/CONTRIBUTING.md in order to add a change file in your PR?

[robvanpamel]

I rebased, added the change file and pointed to dev iso main

[robvanpamel]

Is any action from my side required to integrate this into the next release?

[normj]

@robvanpamel Your side is good. We just need schedule a release to include your change. Thanks for the PR.

[normj]

Version 0.9.1 is out with this PR. Thanks again for the contribution.