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

checking the status code returned by youtube #5052

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

unixfox
Copy link
Member

@unixfox unixfox commented Nov 2, 2024

Should provide a better error message for the 429 rate limit: #5032

@unixfox unixfox requested a review from a team as a code owner November 2, 2024 22:54
@unixfox unixfox requested review from SamantazFox and syeopite and removed request for a team and SamantazFox November 2, 2024 22:54
src/invidious/yt_backend/youtube_api.cr Outdated Show resolved Hide resolved
@SamantazFox
Copy link
Member

Such messages should be translated in the future, but that requires some rework of the translate functions first to support interpolation of more than one field.

@unixfox unixfox merged commit ac6e796 into iv-org:master Nov 7, 2024
8 of 9 checks passed
@iBicha
Copy link
Contributor

iBicha commented Nov 7, 2024

I feel like this change made things slightly worse. Invidious erroring using to show where things were failing (things that are not parsing, etc) and while this is the correct way to do it, now all we have is the status code without further context

@unixfox
Copy link
Member Author

unixfox commented Nov 7, 2024

I think based on the URL alone it's going to be pretty obvious.

Usually even on errors, the yt api just returns a 200 status code.

@iBicha
Copy link
Contributor

iBicha commented Nov 7, 2024

I think based on the URL alone it's going to be pretty obvious.

Usually even on errors, the yt api just returns a 200 status code.

I think it should at least contain the error from the body to make this more helpful than it was

@unixfox
Copy link
Member Author

unixfox commented Nov 7, 2024

We would need an error template which summarize the error AND print the full error stack, but we don't have one.

For the moment, that's the best that we can do for clearly explaining to the user what's going on.

I still insist on the case that when you receive another status code other than 200 then something really wrong is going on. Many people will discover it and create new issues, the URL causing the issue will come up too at some point.

@iBicha
Copy link
Contributor

iBicha commented Nov 7, 2024

I mean the YT response body can be just added to the body of InfoException, that is also fine

@unixfox
Copy link
Member Author

unixfox commented Nov 8, 2024

But then these messages will never be able to be translated.

Hence why I said, we do not have a page template with a summary of the error AND the full stack error.

@unixfox
Copy link
Member Author

unixfox commented Nov 8, 2024

And I forgot to say that we already parse the error given by youtube here:

if initial_data.has_key?("error")

Here #5032 why Invidious is crashing. Is that it expect youtube to give a JSON body but receive an HTML one.

Now that I think again. Maybe we could only throw a simple error status code message when the status code is not 200 AND it's not a JSON body.

I think youtube probably return a JSON body with an error and a status code of 400

@syeopite
Copy link
Member

syeopite commented Nov 8, 2024

I have a PR opened upstream in Kemal to allow for routes pertaining to specific raised errors kemalcr/kemal#688

I haven't had time to work on it lately but once it is finished and merged we should be able to create customized error pages for different exceptions

acheong08 pushed a commit to acheong08/invidious that referenced this pull request Nov 8, 2024
* checking the status code returned by youtube

* add documentation link

* Update src/invidious/yt_backend/youtube_api.cr

Co-authored-by: syeopite <[email protected]>

---------

Co-authored-by: syeopite <[email protected]>
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