From a2021b34c8b8237c657977faf20ca08cc0232805 Mon Sep 17 00:00:00 2001 From: pajlada Date: Sat, 10 Jun 2023 22:14:42 +0200 Subject: [PATCH] Handle broken YouTube URLs (#488) Links like `https://www.youtube.com/watch?v=cD7YFUYLpDc?feature=share` can be opened in a browser and it will correctly load the video - with this change we load the correct video too --- CHANGELOG.md | 1 + internal/resolvers/youtube/helpers.go | 13 +++- internal/resolvers/youtube/video_resolver.go | 23 ++++++- .../resolvers/youtube/video_resolver_test.go | 62 ++++++++++++------- 4 files changed, 75 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a544cec2..4cf7e4b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased - Breaking: Remove the `/twitchemotes/` endpoints. See [issue 332](https://github.com/Chatterino/api/issues/332) for more information. (#465) +- Fix: We do some more YouTube video ID parsing to ensure broken links (such as `youtube.com/watch?v=foobar?feature=share`) still return the actual video ID, since this is how the browser operates. (#488) ## 2.0.2 diff --git a/internal/resolvers/youtube/helpers.go b/internal/resolvers/youtube/helpers.go index 65bdd6af..ad432f32 100644 --- a/internal/resolvers/youtube/helpers.go +++ b/internal/resolvers/youtube/helpers.go @@ -29,7 +29,18 @@ func getYoutubeVideoIDFromURL(url *url.URL) string { return rest } - return url.Query().Get("v") + v := url.Query().Get("v") + + if v == "" { + // Early out so we can make assumptions about the response from Fields + return "" + } + + fields := strings.FieldsFunc(v, func(r rune) bool { + return r == '?' + }) + + return fields[0] } func getYoutubeVideoIDFromURL2(url *url.URL) string { diff --git a/internal/resolvers/youtube/video_resolver.go b/internal/resolvers/youtube/video_resolver.go index 23362224..d1241e29 100644 --- a/internal/resolvers/youtube/video_resolver.go +++ b/internal/resolvers/youtube/video_resolver.go @@ -14,12 +14,33 @@ type YouTubeVideoResolver struct { videoCache cache.Cache } +type contextKey string + +var ( + contextVideoID = contextKey("videoID") + + errMissingVideoIDValue = errors.New("missing video ID in context") +) + +func videoIDFromContext(ctx context.Context) (string, error) { + videoID, ok := ctx.Value(contextVideoID).(string) + if !ok { + return "", errMissingVideoIDValue + } + + return videoID, nil +} + func (r *YouTubeVideoResolver) Check(ctx context.Context, url *url.URL) (context.Context, bool) { if !utils.IsSubdomainOf(url, "youtube.com") { return ctx, false } - return ctx, getYoutubeVideoIDFromURL(url) != "" + videoID := getYoutubeVideoIDFromURL(url) + + ctx = context.WithValue(ctx, contextVideoID, videoID) + + return ctx, videoID != "" } var ( diff --git a/internal/resolvers/youtube/video_resolver_test.go b/internal/resolvers/youtube/video_resolver_test.go index 8859978f..b3d5d831 100644 --- a/internal/resolvers/youtube/video_resolver_test.go +++ b/internal/resolvers/youtube/video_resolver_test.go @@ -68,48 +68,66 @@ func TestVideoResolver(t *testing.T) { c.Run("Check", func(c *qt.C) { type checkTest struct { - label string - input *url.URL - expected bool + label string + input *url.URL + expected bool + expectedVideoID string } tests := []checkTest{ { - label: "Correct domain, correct path", - input: utils.MustParseURL("https://youtube.com/watch?v=foobar"), - expected: true, + label: "Correct domain, correct path", + input: utils.MustParseURL("https://youtube.com/watch?v=foobar"), + expected: true, + expectedVideoID: "foobar", }, { - label: "Correct domain, embed path", - input: utils.MustParseURL("https://youtube.com/embed/foobar"), - expected: true, + label: "Correct domain, embed path", + input: utils.MustParseURL("https://youtube.com/embed/foobar"), + expected: true, + expectedVideoID: "foobar", }, { - label: "Correct domain, shorts path", - input: utils.MustParseURL("https://youtube.com/shorts/foobar"), - expected: true, + label: "Correct domain, shorts path", + input: utils.MustParseURL("https://youtube.com/shorts/foobar"), + expected: true, + expectedVideoID: "foobar", }, { - label: "Correct (sub)domain, correct path", - input: utils.MustParseURL("https://www.youtube.com/watch?v=foobar"), - expected: true, + label: "Correct (sub)domain, correct path", + input: utils.MustParseURL("https://www.youtube.com/watch?v=foobar"), + expected: true, + expectedVideoID: "foobar", }, { - label: "Correct domain, no path", - input: utils.MustParseURL("https://youtube.com"), - expected: false, + label: "Correct (sub)domain, correct path, broken link that we fix", + input: utils.MustParseURL("https://www.youtube.com/watch?v=foobar?feature=share"), + expected: true, + expectedVideoID: "foobar", }, { - label: "Incorrect domain", - input: utils.MustParseURL("https://example.com/watch?v=foobar"), - expected: false, + label: "Correct domain, no path", + input: utils.MustParseURL("https://youtube.com"), + expected: false, + expectedVideoID: "", + }, + { + label: "Incorrect domain", + input: utils.MustParseURL("https://example.com/watch?v=foobar"), + expected: false, + expectedVideoID: "", }, } for _, test := range tests { c.Run(test.label, func(c *qt.C) { - _, output := resolver.Check(ctx, test.input) + ctx, output := resolver.Check(ctx, test.input) c.Assert(output, qt.Equals, test.expected) + if test.expected { + videoID, err := videoIDFromContext(ctx) + c.Assert(err, qt.IsNil) + c.Assert(videoID, qt.Equals, test.expectedVideoID) + } }) } })