Skip to content

Conversation

@lukebakken
Copy link
Collaborator

Fixes #1831

This PR adds the PublishReturnException class that includes the originating exchange and routing key for a basic.return message. It should be backwards-compatible in the API.

Fixes #1831

This PR adds the `PublishReturnException` class that includes the
originating exchange and routing key for a `basic.return` message. It
should be backwards-compatible in the API.
@lukebakken lukebakken self-assigned this May 8, 2025
@lukebakken lukebakken added this to the 7.2.0 milestone May 8, 2025
@bording
Copy link
Collaborator

bording commented May 9, 2025

While it's good to add these properties to the exception for inspection, I'd also really like to see exception messages being added that display these property values in the message text. Those messages tend to get logged along with stack traces and provide useful information in logs. Custom properties on an exception are much less likely to make it into a log file.

@lukebakken lukebakken requested a review from Gsantomaggio May 9, 2025 16:12
@lukebakken
Copy link
Collaborator Author

@SzymonPobiega and @Gsantomaggio all set for a re-review, thanks.

@lukebakken
Copy link
Collaborator Author

While it's good to add these properties to the exception for inspection, I'd also really like to see exception messages being added that display these property values in the message text

Feel free to suggest a patch! Or, modify my PR directly @bording ... I think you have write access here 😸

@bording bording force-pushed the rabbitmq-dotnet-client-1831 branch from 4a64a83 to 39397a6 Compare May 9, 2025 18:25
@bording bording force-pushed the rabbitmq-dotnet-client-1831 branch from 39397a6 to 5b62dff Compare May 9, 2025 18:30
@bording
Copy link
Collaborator

bording commented May 9, 2025

Feel free to suggest a patch! Or, modify my PR directly @bording ... I think you have write access here 😸

I pushed up a change to add exception messages. The interesting one is for the return messages because there really isn't any useful information to include for a nack.

Add more conditions to the test

Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Copy link
Collaborator Author

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

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

Thanks everyone!

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.

Exception details lost when using async API

5 participants