search

trs-80 / WWW-Spotify

Perl wrapper for Spotify Web API
Other
5 stars 3 forks source link

Request errors are not handled #11

Closed grr closed 1 year ago

grr commented 3 years ago

There doesn't appear to be any handling for errors encountered by WWW::Mechanize. The UA is created with autocheck disabled, so there is no automatic error handling, But there is also no internal checks for failures. So my script will occasionally die with, when auto_json_decode is enabled.

malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "Server closed connec...") at /src/perlbrew/perls/perl-5.32/lib/site_perl/5.32.1/WWW/Spotify.pm line 452.

The Content-Type header is also ignored when auto-decoding is performed. It doesn't make sense to attempt to decode as JSON or XML if the response doesn't provide that format.

oalders commented 3 years ago

For the first issue, you can always supply your own UA with autocheck enabled: See https://metacpan.org/pod/WWW::Spotify#ua

It should really be checking Content-Type before treating the response body as JSON, though.

I haven't used this module in a while. Not sure about @trs-80. Having said that, if you want to propose a pull request to improve this, I'm happy to look at it and see about getting a new release out.

grr commented 2 years ago

For the first issue, you can always supply your own UA with autocheck enabled: See https://metacpan.org/pod/WWW::Spotify#ua

The code would still die so now I'd have to wrap every call in an eval and check the error. FYI: the pod has an error. The last line in that example calls the constructor for WWW::Mechanize instead of WWW::Spotify.

I was hoping to provide an LWP subclass as the UA so I could use LWP::UserAgent::Determined, which retries failed requests, but don't think that's possible. A WWW::Mechanize subclass is required, even though WWW::Spotify looks like it only uses the base LWP::UserAgent methods internally. I haven't found a Mechanize subclass that does something similar.

oalders commented 2 years ago

https://metacpan.org/release/OALDERS/WWW-Spotify-0.011 is now on CPAN. You should be able to use LWP::UserAgent::Determined now. That doesn't address the core issue here, but that's what I had the bandwidth for today. I'm not actually using this module, so if someone wanted to provide a PR to address the content-type checking and error handling, that would be most welcome.

trs-80 commented 2 years ago

@oalders I have committed some changes to address error handling in the error_handling branch would appreciate your review when you get a chance.

oalders commented 2 years ago

@trs-80 do you want to create a pull request from your branch? That would make for easier review. You can set it to "draft" if it's still in the early stages.

trs-80 commented 2 years ago

@grr error handling changes have been pushed to the master branch - these have not been released on CPAN yet as there is a separate change (removal of XML) that I would like to complete before releasing to CPAN. If you are able to test the changes provided it would greatly appreciated.

The methods that may be of interest to you are:

die_on_response_error custom_request_handler (and _result) response_status response_content_type last_error

See README or pod for details on the above.

Hopefully a combination of those can address you concerns with error handling.

trs-80 commented 2 years ago

@grr error handling changes have been pushed to the master branch - these have not been released on CPAN yet as there is a separate change (removal of XML) that I would like to complete before releasing to CPAN. If you are able to test the changes provided it would greatly appreciated.

The methods that may be of interest to you are:

die_on_response_error custom_request_handler (and _result) response_status response_content_type last_error

See README or pod for details on the above.

Hopefully a combination of those can address you concerns with error handling.

The changes have now been pushed to CPAN as part of the 0.012 release