plugins.twitch: better access token error handling #5011
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gravyboat
approved these changes
Dec 4, 2022
src/streamlink/plugins/twitch.py
Outdated
| @@ -421,18 +423,30 @@ def access_token(self, is_live, channel_or_vod): | |||
| validate.union_get("signature", "value"), | |||
| ) | |||
|
|
|||
| return self.call(query, schema=validate.Schema( | |||
| {"data": validate.any( | |||
| return self.call(query, acceptable_status=(200, 401), schema=validate.Schema( | |||
There was a problem hiding this comment.
Are these the only status codes that are ever returned from a "successful" request?
There was a problem hiding this comment.
400 and 403 could maybe be added, but apart from that, I don't think anything else will be useful when trying to parse the {"error": str, ...} response schema. The remaining 4xx errors or any 5xx errors will be handled by the HTTPSession where it'll raise a PluginError instead.
There was a problem hiding this comment.
Sounds good to me. Figured it was worth double checking just in case so there's no issue when this goes live that will require a second release. 👍
- Refactor `TwitchAPI` class: - Set `CLIENT_ID` as a class attribute (for test references) - Allow `call()` to pass custom headers (currently unused) - Remove unneeded `json.dumps()` - Handle `PlaybackAccessToken` error response and log error messages - Add `Options.clear()` for resetting plugin options state after tests - Add access token API tests
bastimeyer
force-pushed
the
plugins/twitch/access-token-error-handling
branch
from
d41d557 to
8e21e43
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TwitchAPIclass:CLIENT_IDas a class attribute (for test references)call()to pass custom headers (currently unused)json.dumps()PlaybackAccessTokenerror response and log error messagesOptions.clear()for resetting plugin options state after testsResolves #5009
This adds error logging to the access token processing and also adds tests for the access token responses and how the
--twitch-api-headerand--twitch-access-token-paraminfluence the access token request.Small side-note in regards to the added
Options.clear()method:I've already mentioned somewhere in the
Plugin.bind()removal in 5.0.0 (#4744 / #4768) thatPlugin.optionswill also need to get fixed at some point. Similar to the previously bound class attributes,Plugin.optionsis a class attribute that gets shared accross all plugin classes unless it gets overridden bystreamlink_cli.main.setup_plugin_args(). This means that when streamlink_cli is not involved in plugin construction, e.g. in tests or when using the Streamlink session API directly, setting any kind of plugin option always requires clearing the options, which is bad. I'm pretty sure that there are a couple of tests which are polluting the shared plugin options dict. I'll check once I get the time.The reason why I didn't touch
Plugin.optionsat the time is that removingPlugin.bind()was already a lot of work. In hindsight, this should've been done in one go, because changing it will probably be a breaking API change, because of how the plugins will need to get initialized. Currently, the options are set by the session instance before plugins are initialized, and it expects the options dict to be available on the plugin class. Options should however only be set on plugin instances, so there's no shared options dict, neither between plugin classes, nor between different instances of the same plugin.I will need to have a closer look at this and figure out a better solution before making any further comments. This PR should be fine though.