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

Handle JSON::ParserError when http response is HTML and raise ShopifyAPI::Errors::HttpResponseError #1113

Merged
merged 7 commits into from
Apr 3, 2023

Conversation

kaarelss
Copy link
Contributor

@kaarelss kaarelss commented Feb 13, 2023

Handle JSON::ParserError when response is HTML and pass response body to ShopifyAPI::Clients::HttpResponse because it accepts body also as string.

Description

Fixes #1091
Fixed #1107

Please, include a summary of what the PR is for:

  • It is not possible to determine HTTP response code when response body is HTML and JSON::ParserError is thrown. So rescuing JSON::ParserError would raise ShopifyAPI::Errors::HttpResponseError and add response code.

How has this been tested?

All tests passed as well as new test is included for this new functionality:
test_json_parser_error in http_client_test.rb

Checklist:

  • My commit message follow the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the project documentation.
  • I have added a changelog line.

@ClaytonPassmore
Copy link
Contributor

@kaarelss
Copy link
Contributor Author

Nice! I believe this would fix the following:

Actually I added the nil? guard from your PR, but I guess it should be merged first, because you have a test created there. Thank you for mentioning #1107. Also added #1091

@vishals-vebuin
Copy link

@kaarelss is there any idea? how much time it will take to fix it?

@ClaytonPassmore
Copy link
Contributor

@vishals-vebuin we're waiting on someone from Shopify to approve.

As far as I can tell, the last community PR they interacted with was from October 20th, 2022.

@ClaytonPassmore
Copy link
Contributor

@kaarelss heads up that #1098 got merged, so you may need to update your branch 👍

@kaarelss
Copy link
Contributor Author

@kaarelss heads up that #1098 got merged, so you may need to update your branch 👍

Thanks @ClaytonPassmore Will do.

Co-authored-by: Clayton Passmore <ClaytonPassmore@users.noreply.github.com>
@nelsonwittwer
Copy link
Contributor

@kaarelss - Do you mind fixing that linting issue on your end, I tried doing it with GitHub's editor here but it added a trailing whitespace 🤕 Would love to get this out for our release here in the next day or two, thank you for your contributions! 🎉

@kaarelss
Copy link
Contributor Author

kaarelss commented Apr 3, 2023

@kaarelss - Do you mind fixing that linting issue on your end, I tried doing it with GitHub's editor here but it added a trailing whitespace 🤕 Would love to get this out for our release here in the next day or two, thank you for your contributions! 🎉

Done

@nelsonwittwer nelsonwittwer merged commit f665705 into Shopify:main Apr 3, 2023
nickmealey pushed a commit to heliumdevelopment/shopify-api-ruby that referenced this pull request Jun 6, 2023
…API::Errors::HttpResponseError (Shopify#1113)

* Handle JSON::ParserError when http response is html

* Added line in changelog

* Update lib/shopify_api/clients/http_client.rb

Co-authored-by: Clayton Passmore <ClaytonPassmore@users.noreply.github.com>

* Update lib/shopify_api/clients/http_client.rb

rubocop

* Removes trailing whitespace

---------

Co-authored-by: Nelson <nelsonwittwer@users.noreply.github.com>
Co-authored-by: Clayton Passmore <ClaytonPassmore@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants