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

Parse all RabbitMQ Management API responses for errors. #152

Merged

Conversation

niclic
Copy link
Contributor

@niclic niclic commented May 12, 2020

Here is the PR for #151.

What does this PR do?

Parses the response body for all RabbitMQ Management API requests, not just GET requests, returning an error if one exists.

This provides a more consistent experience for the end user, as well as providing valuable diagnostic information for RabbitMQ Management API errors when they occur (e.g. invalid requests).

What changes were made?

The important change is to client.go. I extracted the existing parsing logic into a new method called parseResponseErrors, and call this from executeRequest, which every API call ultimately uses.

What does this change?

For GET requests, nothing changes. However, for PUT and DELETE requests, any error returned from the RabbitMQ Management API will now be returned by rabbithole. This includes 404 Not Found, when trying to delete a resource than does not exist.

All this means is that end users should be checking the err return value, which is standard practice in go.

What else changed?

  • I updated tests to check the value of the err return value. For a small number of tests, this highlighted some flaws, which I fixed. For the majority of tests, nothing changed. This also had the benefit of clearing up GolangCI-Lint warnings, since those were related to not checking the err return value.

  • I also added some clean up to a few tests because I noticed dangling resources (users, exchanges) after all tests had finished running.

  • Ran go mod tidyto prune any no-longer-needed dependencies from go.mod.

@michaelklishin
Copy link
Owner

This looks reasonable, thank you. I wonder if we should treat 404 Not Found as a success for DELETE operations? Deleting a queue or exchange via a RabbitMQ [AMQP 0-9-1] client would not result in an error starting with 3.0. CLI tools are not always idempotent, however, but there's a fair amount of community consensus that this is annoying more than helpful.

WDYT?

@niclic
Copy link
Contributor Author

niclic commented May 13, 2020

I agree.

The intent of DELETE seems clear. I shouldn't have to inspect an error to see if something bad really happened or not. The API can be consistent (i.e. idempotent), even if the underlying server may not always be.

The correct way to verify a DELETE is to follow up with a GET and handle the expected 404 response.

Check that last commit for the update.

@michaelklishin michaelklishin merged commit cb015fa into michaelklishin:master May 13, 2020
@michaelklishin
Copy link
Owner

Thank you!

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.

2 participants