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 ruby error response body and add attributes #326

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dhughes
Copy link

@dhughes dhughes commented Mar 25, 2023

Description

This PR is based on a PR I originally opened on the generated mailchimp-marketing-ruby gem in 2021. At the time I didn't know that the gem was generated and the PR was rejected several months later. I never got around to submitting the patch to this project and have been using my own fork. However, I would like to see if my change could make it into this repo.

This PR should generally be backwards compatible, with the exception that the error message won't show a stringified version of a ruby hash.

Here's the summary from the original PR on the wrong repo:

The MailchimpMarketing::ApiError is used to wrap up error responses from the Mailchimp API. This class is used to raise errors encountered by the ApiClient class. In the case that we have a response body, the body's json is parsed into a hash and this hash is passed in as a named argument to the ApiError constructor. In the case that a hash is provided to the ApiErrors constructor (and the hash doesn't have a :message key), it is simply passed to StandardError constructor.

Because a hash is being passed to the StandardError constructor, the message value of the resulting error is the result of calling to_s on the hash. EG:

pry(#<ListsController>)> error.message
=> "{:status=>401, :response_body=>\"{\\\"type\\\":\\\"http://developer.mailchimp.com/documentation/mailchimp/guides/error-glossary/\\\",\\\"title\\\":\\\"API Key Invalid\\\",\\\"status\\\":401,\\\"detail\\\":\\\"Your API key may be invalid, or you've attempted to access the wrong datacenter.\\\",\\\"instance\\\":\\\"ff205bd4-c256-4570-aaf0-6764d0032a8a\\\"}\"}"

Note that the value of message is definitely not JSON. 😄

The ApiError class' constructor does take all of the named values provided to the constructor and store them as instance variables.

It would be nice to have access to the response body, if available. I do not feel comfortable calling eval on the hash stored in message, especially since this could potentially be a string. I do not want to use instance_variable_get to read the instance variable, since this would break encapsulation.

As such, this PR simply introduces a new attr_reader for :response_headers and :response_body. Now this data is safely accessible from outside of the error class.

As a note, there are no uses of :response_header in this gem. However, the comments in the ApiError comments show this as an example option, so I added the reader. This doesn't cover any cases where other non-standard attributes are provided to the constructor.

Known Issues

This commit updates the ApiError class in the generated Ruby gem. The
class has been updated to not pass hash arguments directly to the StandardError
constructor. Only a possible title or message hash attribute is passed
to StandardError. This prevents Ruby from stringifying a hash and using
that as the error message, which is both ugly and unusable.
@cla-bot
Copy link

cla-bot bot commented Mar 25, 2023

Contributor License Agreement Instructions
Thanks for your pull request. Before we can review your work, you’ll need to sign a Contributor License Agreement (CLA).

Please download the appropriate CLA below. Once downloaded, please read, sign, and send back to us at developer-cla@mailchimp.com. Please note, this account is not monitored so please visit https://mailchimp.com/contact/ if you need support.

Individual CLA: Mailchimp Individual CLA
Corporate CLA: Mailchimp Corporate CLA

Once you’ve emailed us the signed CLA, please reply here (e.g. CLA signed and sent!) and we’ll verify it.

What to do if you already signed the CLA
Individual signers
• If you’ve previously sent us a signed CLA, please reply here letting us know and we’ll verify. If we are unable to verify, It’s possible we don’t have your GitHub username or you’re using a different email address on your Git commit. Check that the CLA you previously submitted was sent to us using the email address associated with your GitHub username and verify that your email is set on your Git commits.
Corporate signers
• Your company has a Point of Contact (POC) who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you’ve previously sent us an updated CLA, please reply here letting us know and we’ll verify.
• The email used to register you as an authorized contributor must be the email used for the Git commit.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

@dhughes
Copy link
Author

dhughes commented Mar 25, 2023

CLA signed and sent!

@dhughes
Copy link
Author

dhughes commented Mar 25, 2023

I submitted another PR earlier today and had signed and submitted the CLA at that time.

@luxflux
Copy link

luxflux commented Feb 13, 2024

Any update on this? Would love to see this merged.

@jimryanzulueta
Copy link

Same here, update on this?

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.

3 participants