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
Chatterino · Jun 10, 2023 · a2021b3 · a2021b3
1 parent 1c4e013
commit a2021b3
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 24 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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)
+				}
 			})
 		}
 	})