Skip to content

Conversation

@npc203
Copy link
Contributor

@npc203 npc203 commented Oct 1, 2021

Description of the changes

Report by alec in discord. When the status of request to fetch token from spotify isn't 200, The content-type is not json.
This PR handles it ,makes sure the error is propagated and handled.

@github-actions github-actions bot added the Category: Cogs - Audio This is related to the Audio cog. label Oct 1, 2021
@npc203
Copy link
Contributor Author

npc203 commented Oct 2, 2021

While we are still here, the self.get() suffers from the same problem, it tries to parse json despite the response not necessarily being one (mostly when response status is 5xx). However fixing the get req needs lots of other changes and error handling pretty much all over the api interface, because it is used in many places unlike post (which just occurs once). Thus I didn't cramp it up into a single PR and let better people deal with it. but do let me know if you would like me to update this PR with a similar approach as done in post.

@Jackenmen Jackenmen added hacktoberfest-accepted Type: Bug Unexpected behavior, result, or exception. In case of PRs, it is a fix for the foregoing. labels Oct 2, 2021
@Jackenmen Jackenmen added this to the 3.4.15 milestone Oct 2, 2021
"""Make a POST call to spotify."""
async with self.session.post(url, data=payload, headers=headers) as r:
data = await r.json(loads=json.loads)
if r.status != 200:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still should be checking for status code as some (if not most) non-200 responses contain valid JSON. Yes, with current usage this doesn't technically matter but we should be doing it nonetheless.
If we do this, we could probably even simplify get_access_token() and remove the try-except with KeyError from it and just directly assign instead.

@Jackenmen
Copy link
Member

While we are still here, the self.get() suffers from the same problem, it tries to parse json despite the response not necessarily being one (mostly when response status is 5xx). However fixing the get req needs lots of other changes and error handling pretty much all over the api interface, because it is used in many places unlike post (which just occurs once). Thus I didn't cramp it up into a single PR and let better people deal with it. but do let me know if you would like me to update this PR with a similar approach as done in post.

I suppose this could be added to this PR as well, it's part of the same issue.

@Jackenmen Jackenmen self-assigned this Dec 26, 2021
@Jackenmen Jackenmen modified the milestones: 3.4.15, 3.4.16 Dec 29, 2021
@Kowlin Kowlin modified the milestones: 3.4.17, 3.5.0 Apr 2, 2022
@Jackenmen Jackenmen modified the milestones: 3.5.0, 3.5.1 Nov 11, 2022
@Jackenmen Jackenmen modified the milestones: 3.5.6, 3.5.7 Mar 18, 2024
@Jackenmen Jackenmen modified the milestones: 3.5.9, 3.5.10 Apr 20, 2024
@Jackenmen Jackenmen modified the milestones: 3.5.14, 3.5.x Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Category: Cogs - Audio This is related to the Audio cog. Type: Bug Unexpected behavior, result, or exception. In case of PRs, it is a fix for the foregoing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants