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

Add attr_reader for response data to MailchimpMarketing::ApiError #5

Closed
wants to merge 4 commits into from

Conversation

dhughes
Copy link

@dhughes dhughes commented Jan 23, 2021

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.

@dhughes
Copy link
Author

dhughes commented Feb 13, 2021

I just wanted to give this PR a bump. @farzam-azhar7777 approved this a couple weeks back. Is this something I should expect to be merged to main or released any time soon?

@mc-hannah-morrison
Copy link

Hi @dhughes , apologies for not responding sooner. This SDK is autogenerated from https://github.com/mailchimp/mailchimp-client-lib-codegen
If you can move issues and PRs to that repo we will take a look. Thanks so much!

@acrogenesis
Copy link

This is still an issue

@dhughes
Copy link
Author

dhughes commented Mar 25, 2023

It only took me a couple years, but I opened a PR on the mailchimp-client-lib-codegen repo: mailchimp/mailchimp-client-lib-codegen#326

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.

4 participants