refactor: simplify Youtube feeds discovery

internal/reader/subscription/finder.go

@@ -5,7 +5,6 @@ package subscription // import "miniflux.app/v2/internal/reader/subscription"
 
 import (
 	"bytes"
-	"errors"
 	"fmt"
 	"io"
 	"log/slog"
@@ -25,18 +24,9 @@ import (
 	"golang.org/x/net/html/charset"
 )
 
-type youtubeKind string
-
-const (
-	youtubeIDKindChannel  youtubeKind = "channel"
-	youtubeIDKindPlaylist youtubeKind = "playlist"
-)
-
 var (
 	youtubeHostRegex    = regexp.MustCompile(`youtube\.com$`)
 	youtubeChannelRegex = regexp.MustCompile(`channel/(.*)$`)
-
-	errNotYoutubeUrl = fmt.Errorf("this website is not a YouTube page")
 )
 
 type SubscriptionFinder struct {
@@ -80,77 +70,58 @@ func (f *SubscriptionFinder) FindSubscriptions(websiteURL, rssBridgeURL string)
 		LastModified: responseHandler.LastModified(),
 	}
 
-	// Step 1) Check if the website URL is a feed.
+	// Step 1) Check if the website URL is already a feed.
 	if feedFormat, _ := parser.DetectFeedFormat(f.feedResponseInfo.Content); feedFormat != parser.FormatUnknown {
 		f.feedDownloaded = true
 		return Subscriptions{NewSubscription(responseHandler.EffectiveURL(), responseHandler.EffectiveURL(), feedFormat)}, nil
 	}
 
-	var subscriptions Subscriptions
-
-	// Step 2) Parse URL to find feeds from YouTube.
-	kind, _, err := youtubeURLIDExtractor(websiteURL)
-	slog.Debug("YouTube URL ID extraction", slog.String("website_url", websiteURL), slog.Any("kind", kind), slog.Any("err", err))
+	// Step 2) Check if the website URL is a YouTube channel.
+	slog.Debug("Try to detect feeds from YouTube channel page", slog.String("website_url", websiteURL))
+	if subscriptions, localizedError := f.FindSubscriptionsFromYouTubeChannelPage(websiteURL); localizedError != nil {
+		return nil, localizedError
+	} else if len(subscriptions) > 0 {
+		slog.Debug("Subscriptions found from YouTube channel page", slog.String("website_url", websiteURL), slog.Any("subscriptions", subscriptions))
+		return subscriptions, nil
+	}
 
-	// If YouTube url has been detected, return the YouTube feed
-	if err == nil || !errors.Is(err, errNotYoutubeUrl) {
-		switch kind {
-		case youtubeIDKindChannel:
-			slog.Debug("Try to detect feeds from YouTube channel page", slog.String("website_url", websiteURL))
-			subscriptions, localizedError = f.FindSubscriptionsFromYouTubeChannelPage(websiteURL)
-			if localizedError != nil {
-				return nil, localizedError
-			}
-		case youtubeIDKindPlaylist:
-			slog.Debug("Try to detect feeds from YouTube playlist page", slog.String("website_url", websiteURL))
-			subscriptions, localizedError = f.FindSubscriptionsFromYouTubePlaylistPage(websiteURL)
-			if localizedError != nil {
-				return nil, localizedError
-			}
-		}
-		if len(subscriptions) > 0 {
-			slog.Debug("Subscriptions found from YouTube page", slog.String("website_url", websiteURL), slog.Any("subscriptions", subscriptions))
-			return subscriptions, nil
-		}
+	// Step 3) Check if the website URL is a YouTube playlist.
+	slog.Debug("Try to detect feeds from YouTube playlist page", slog.String("website_url", websiteURL))
+	if subscriptions, localizedError := f.FindSubscriptionsFromYouTubePlaylistPage(websiteURL); localizedError != nil {
+		return nil, localizedError
+	} else if len(subscriptions) > 0 {
+		slog.Debug("Subscriptions found from YouTube playlist page", slog.String("website_url", websiteURL), slog.Any("subscriptions", subscriptions))
+		return subscriptions, nil
 	}
 
-	// Step 3) Parse web page to find feeds from HTML meta tags.
+	// Step 4) Parse web page to find feeds from HTML meta tags.
 	slog.Debug("Try to detect feeds from HTML meta tags",
 		slog.String("website_url", websiteURL),
 		slog.String("content_type", responseHandler.ContentType()),
 	)
-	subscriptions, localizedError = f.FindSubscriptionsFromWebPage(websiteURL, responseHandler.ContentType(), bytes.NewReader(responseBody))
-	if localizedError != nil {
+	if subscriptions, localizedError := f.FindSubscriptionsFromWebPage(websiteURL, responseHandler.ContentType(), bytes.NewReader(responseBody)); localizedError != nil {
 		return nil, localizedError
-	}
-
-	if len(subscriptions) > 0 {
+	} else if len(subscriptions) > 0 {
 		slog.Debug("Subscriptions found from web page", slog.String("website_url", websiteURL), slog.Any("subscriptions", subscriptions))
 		return subscriptions, nil
 	}
 
-	// Step 4) Check if the website URL can use RSS-Bridge.
+	// Step 5) Check if the website URL can use RSS-Bridge.
 	if rssBridgeURL != "" {
 		slog.Debug("Try to detect feeds with RSS-Bridge", slog.String("website_url", websiteURL))
-		subscriptions, localizedError := f.FindSubscriptionsFromRSSBridge(websiteURL, rssBridgeURL)
-		if localizedError != nil {
+		if subscriptions, localizedError := f.FindSubscriptionsFromRSSBridge(websiteURL, rssBridgeURL); localizedError != nil {
 			return nil, localizedError
-		}
-
-		if len(subscriptions) > 0 {
+		} else if len(subscriptions) > 0 {
 			slog.Debug("Subscriptions found from RSS-Bridge", slog.String("website_url", websiteURL), slog.Any("subscriptions", subscriptions))
 			return subscriptions, nil
 		}
 	}
 
-	// Step 5) Check if the website has a known feed URL.
+	// Step 6) Check if the website has a known feed URL.
 	slog.Debug("Try to detect feeds from well-known URLs", slog.String("website_url", websiteURL))
-	subscriptions, localizedError = f.FindSubscriptionsFromWellKnownURLs(websiteURL)
-	if localizedError != nil {
+	if subscriptions, localizedError := f.FindSubscriptionsFromWellKnownURLs(websiteURL); localizedError != nil {
 		return nil, localizedError
-	}
-
-	if len(subscriptions) > 0 {
+	} else if len(subscriptions) > 0 {
 		slog.Debug("Subscriptions found with well-known URLs", slog.String("website_url", websiteURL), slog.Any("subscriptions", subscriptions))
 		return subscriptions, nil
 	}
@@ -301,57 +272,40 @@ func (f *SubscriptionFinder) FindSubscriptionsFromRSSBridge(websiteURL, rssBridg
 }
 
 func (f *SubscriptionFinder) FindSubscriptionsFromYouTubeChannelPage(websiteURL string) (Subscriptions, *locale.LocalizedErrorWrapper) {
-	kind, id, _ := youtubeURLIDExtractor(websiteURL)
-
-	if kind == youtubeIDKindChannel {
-		feedURL := fmt.Sprintf(`https://www.youtube.com/feeds/videos.xml?channel_id=%s`, id)
-		return Subscriptions{NewSubscription(websiteURL, feedURL, parser.FormatAtom)}, nil
+	decodedUrl, err := url.Parse(websiteURL)
+	if err != nil {
+		return nil, locale.NewLocalizedErrorWrapper(err, "error.invalid_site_url", err)
 	}
-	slog.Debug("This website is not a YouTube channel page, the regex doesn't match", slog.String("website_url", websiteURL))
-
-	return nil, nil
-}
 
-func (f *SubscriptionFinder) FindSubscriptionsFromYouTubePlaylistPage(websiteURL string) (Subscriptions, *locale.LocalizedErrorWrapper) {
-	kind, id, _ := youtubeURLIDExtractor(websiteURL)
+	if !youtubeHostRegex.MatchString(decodedUrl.Host) {
+		slog.Debug("This website is not a YouTube page, the regex doesn't match", slog.String("website_url", websiteURL))
+		return nil, nil
+	}
 
-	if kind == youtubeIDKindPlaylist {
-		feedURL := fmt.Sprintf(`https://www.youtube.com/feeds/videos.xml?playlist_id=%s`, id)
+	if matches := youtubeChannelRegex.FindStringSubmatch(decodedUrl.Path); len(matches) == 2 {
+		feedURL := fmt.Sprintf(`https://www.youtube.com/feeds/videos.xml?channel_id=%s`, matches[1])
 		return Subscriptions{NewSubscription(websiteURL, feedURL, parser.FormatAtom)}, nil
 	}
 
-	slog.Debug("This website is not a YouTube playlist page, the regex doesn't match", slog.String("website_url", websiteURL))
-
 	return nil, nil
 }
 
-func youtubeURLIDExtractor(websiteURL string) (idKind youtubeKind, id string, err error) {
+func (f *SubscriptionFinder) FindSubscriptionsFromYouTubePlaylistPage(websiteURL string) (Subscriptions, *locale.LocalizedErrorWrapper) {
 	decodedUrl, err := url.Parse(websiteURL)
 	if err != nil {
-		return
+		return nil, locale.NewLocalizedErrorWrapper(err, "error.invalid_site_url", err)
 	}
 
 	if !youtubeHostRegex.MatchString(decodedUrl.Host) {
 		slog.Debug("This website is not a YouTube page, the regex doesn't match", slog.String("website_url", websiteURL))
-		err = errNotYoutubeUrl
-		return
+		return nil, nil
 	}
 
-	switch {
-	case strings.HasPrefix(decodedUrl.Path, "/channel"):
-		idKind = youtubeIDKindChannel
-		matches := youtubeChannelRegex.FindStringSubmatch(decodedUrl.Path)
-		id = matches[1]
-		return
-	case strings.HasPrefix(decodedUrl.Path, "/watch") && decodedUrl.Query().Has("list"):
-		idKind = youtubeIDKindPlaylist
-		id = decodedUrl.Query().Get("list")
-		return
-	case strings.HasPrefix(decodedUrl.Path, "/playlist"):
-		idKind = youtubeIDKindPlaylist
-		id = decodedUrl.Query().Get("list")
-		return
+	if (strings.HasPrefix(decodedUrl.Path, "/watch") && decodedUrl.Query().Has("list")) || strings.HasPrefix(decodedUrl.Path, "/playlist") {
+		playlistID := decodedUrl.Query().Get("list")
+		feedURL := fmt.Sprintf(`https://www.youtube.com/feeds/videos.xml?playlist_id=%s`, playlistID)
+		return Subscriptions{NewSubscription(websiteURL, feedURL, parser.FormatAtom)}, nil
 	}
-	err = fmt.Errorf("finder: unable to extract youtube id from URL: %s", websiteURL)
-	return
+
+	return nil, nil
 }

internal/reader/subscription/finder_test.go

@@ -4,94 +4,184 @@
 package subscription
 
 import (
-	"errors"
 	"strings"
 	"testing"
 )
 
 func TestFindYoutubePlaylistFeed(t *testing.T) {
-	scenarios := map[string]string{
-		"https://www.youtube.com/playlist?list=PLOOwEPgFWm_NHcQd9aCi5JXWASHO_n5uR":            "https://www.youtube.com/feeds/videos.xml?playlist_id=PLOOwEPgFWm_NHcQd9aCi5JXWASHO_n5uR",
-		"https://www.youtube.com/playlist?list=PLOOwEPgFWm_N42HlCLhqyJ0ZBWr5K1QDM":            "https://www.youtube.com/feeds/videos.xml?playlist_id=PLOOwEPgFWm_N42HlCLhqyJ0ZBWr5K1QDM",
-		"https://www.youtube.com/watch?v=dQw4w9WgXcQ&list=PLOOwEPgFWm_N42HlCLhqyJ0ZBWr5K1QDM": "https://www.youtube.com/feeds/videos.xml?playlist_id=PLOOwEPgFWm_N42HlCLhqyJ0ZBWr5K1QDM",
-	}
-
-	for websiteURL, expectedFeedURL := range scenarios {
-		subscriptions, localizedError := NewSubscriptionFinder(nil).FindSubscriptionsFromYouTubePlaylistPage(websiteURL)
-		if localizedError != nil {
-			t.Fatalf(`Parsing a correctly formatted YouTube playlist page should not return any error: %v`, localizedError)
-		}
-
-		if len(subscriptions) != 1 {
-			t.Fatal(`Incorrect number of subscriptions returned`)
-		}
-
-		if subscriptions[0].URL != expectedFeedURL {
-			t.Errorf(`Unexpected Feed, got %s, instead of %s`, subscriptions[0].URL, expectedFeedURL)
-		}
+	type testResult struct {
+		websiteURL     string
+		feedURL        string
+		discoveryError bool
 	}
-}
 
-func TestYoutubeIdExtractor(t *testing.T) {
-	type testResult struct {
-		ID    string
-		Kind  youtubeKind
-		error error
-	}
-	urls := map[string]testResult{
-		"https://www.youtube.com/watch?v=dQw4w9WgXcQ&list=PLOOwEPgFWm_N42HlCLhqyJ0ZBWr5K1QDM": {
-			ID:    "PLOOwEPgFWm_N42HlCLhqyJ0ZBWr5K1QDM",
-			Kind:  youtubeIDKindPlaylist,
-			error: nil,
+	scenarios := []testResult{
+		// Video URL
+		{
+			websiteURL: "https://www.youtube.com/watch?v=dQw4w9WgXcQ",
+			feedURL:    "",
+		},
+		// Video URL with position argument
+		{
+			websiteURL: "https://www.youtube.com/watch?v=dQw4w9WgXcQ&t=1",
+			feedURL:    "",
+		},
+		// Video URL with position argument
+		{
+			websiteURL: "https://www.youtube.com/watch?t=1&v=dQw4w9WgXcQ",
+			feedURL:    "",
+		},
+		// Channel URL
+		{
+			websiteURL: "https://www.youtube.com/channel/UC-Qj80avWItNRjkZ41rzHyw",
+			feedURL:    "",
+		},
+		// Channel URL with name
+		{
+			websiteURL: "https://www.youtube.com/@ABCDEFG",
+			feedURL:    "",
+		},
+		// Playlist URL
+		{
+			websiteURL: "https://www.youtube.com/playlist?list=PLOOwEPgFWm_NHcQd9aCi5JXWASHO_n5uR",
+			feedURL:    "https://www.youtube.com/feeds/videos.xml?playlist_id=PLOOwEPgFWm_NHcQd9aCi5JXWASHO_n5uR",
+		},
+		// Playlist URL with video ID
+		{
+			websiteURL: "https://www.youtube.com/watch?v=dQw4w9WgXcQ&list=PLOOwEPgFWm_N42HlCLhqyJ0ZBWr5K1QDM",
+			feedURL:    "https://www.youtube.com/feeds/videos.xml?playlist_id=PLOOwEPgFWm_N42HlCLhqyJ0ZBWr5K1QDM",
 		},
-		"https://www.youtube.com/playlist?list=PLOOwEPgFWm_NHcQd9aCi5JXWASHO_n5uR": {
-			ID:    "PLOOwEPgFWm_NHcQd9aCi5JXWASHO_n5uR",
-			Kind:  youtubeIDKindPlaylist,
-			error: nil,
+		// Playlist URL with video ID and index argument
+		{
+			websiteURL: "https://www.youtube.com/watch?v=6IutBmRJNLk&list=PLOOwEPgFWm_NHcQd9aCi5JXWASHO_n5uR&index=4",
+			feedURL:    "https://www.youtube.com/feeds/videos.xml?playlist_id=PLOOwEPgFWm_NHcQd9aCi5JXWASHO_n5uR",
 		},
-		"https://www.youtube.com/channel/UC-Qj80avWItNRjkZ41rzHyw": {
-			ID:    "UC-Qj80avWItNRjkZ41rzHyw",
-			Kind:  youtubeIDKindChannel,
-			error: nil,
+		// Non-Youtube URL
+		{
+			websiteURL: "https://www.example.com/channel/UC-Qj80avWItNRjkZ41rzHyw",
+			feedURL:    "",
 		},
-		"https://www.example.com/channel/UC-Qj80avWItNRjkZ41rzHyw": {
-			ID:    "",
-			Kind:  "",
-			error: errNotYoutubeUrl,
+		// Invalid URL
+		{
+			websiteURL:     "https://example|org/",
+			feedURL:        "",
+			discoveryError: true,
 		},
 	}
 
-	for websiteURL, expected := range urls {
-		kind, id, err := youtubeURLIDExtractor(websiteURL)
-		if !errors.Is(err, expected.error) {
-			t.Fatalf(`Unexpected error: %v got %v`, expected.error, err)
+	for _, scenario := range scenarios {
+		subscriptions, localizedError := NewSubscriptionFinder(nil).FindSubscriptionsFromYouTubePlaylistPage(scenario.websiteURL)
+		if scenario.discoveryError {
+			if localizedError == nil {
+				t.Fatalf(`Parsing an invalid URL should return an error`)
+			}
 		}
-		if id != expected.ID {
-			t.Fatalf(`Unexpected ID: %v got %v`, expected.ID, id)
-		}
-		if kind != expected.Kind {
-			t.Fatalf(`Unexpected Kind: %v got %v`, expected.Kind, kind)
+
+		if scenario.feedURL == "" {
+			if len(subscriptions) > 0 {
+				t.Fatalf(`Parsing a non-playlist URL should not return any subscription: %q`, scenario.websiteURL)
+			}
+		} else {
+			if localizedError != nil {
+				t.Fatalf(`Parsing a correctly formatted YouTube playlist page should not return any error: %v`, localizedError)
+			}
+
+			if len(subscriptions) != 1 {
+				t.Fatalf(`Incorrect number of subscriptions returned`)
+			}
+
+			if subscriptions[0].URL != scenario.feedURL {
+				t.Errorf(`Unexpected Feed, got %s, instead of %s`, subscriptions[0].URL, scenario.feedURL)
+			}
 		}
 	}
 }
 
 func TestFindYoutubeChannelFeed(t *testing.T) {
-	scenarios := map[string]string{
-		"https://www.youtube.com/channel/UC-Qj80avWItNRjkZ41rzHyw": "https://www.youtube.com/feeds/videos.xml?channel_id=UC-Qj80avWItNRjkZ41rzHyw",
+	type testResult struct {
+		websiteURL     string
+		feedURL        string
+		discoveryError bool
 	}
 
-	for websiteURL, expectedFeedURL := range scenarios {
-		subscriptions, localizedError := NewSubscriptionFinder(nil).FindSubscriptionsFromYouTubeChannelPage(websiteURL)
-		if localizedError != nil {
-			t.Fatalf(`Parsing a correctly formatted YouTube channel page should not return any error: %v`, localizedError)
-		}
+	scenarios := []testResult{
+		// Video URL
+		{
+			websiteURL: "https://www.youtube.com/watch?v=dQw4w9WgXcQ",
+			feedURL:    "",
+		},
+		// Video URL with position argument
+		{
+			websiteURL: "https://www.youtube.com/watch?v=dQw4w9WgXcQ&t=1",
+			feedURL:    "",
+		},
+		// Video URL with position argument
+		{
+			websiteURL: "https://www.youtube.com/watch?t=1&v=dQw4w9WgXcQ",
+			feedURL:    "",
+		},
+		// Channel URL
+		{
+			websiteURL: "https://www.youtube.com/channel/UC-Qj80avWItNRjkZ41rzHyw",
+			feedURL:    "https://www.youtube.com/feeds/videos.xml?channel_id=UC-Qj80avWItNRjkZ41rzHyw",
+		},
+		// Channel URL with name
+		{
+			websiteURL: "https://www.youtube.com/@ABCDEFG",
+			feedURL:    "",
+		},
+		// Playlist URL
+		{
+			websiteURL: "https://www.youtube.com/playlist?list=PLOOwEPgFWm_NHcQd9aCi5JXWASHO_n5uR",
+			feedURL:    "",
+		},
+		// Playlist URL with video ID
+		{
+			websiteURL: "https://www.youtube.com/watch?v=dQw4w9WgXcQ&list=PLOOwEPgFWm_N42HlCLhqyJ0ZBWr5K1QDM",
+			feedURL:    "",
+		},
+		// Playlist URL with video ID and index argument
+		{
+			websiteURL: "https://www.youtube.com/watch?v=6IutBmRJNLk&list=PLOOwEPgFWm_NHcQd9aCi5JXWASHO_n5uR&index=4",
+			feedURL:    "",
+		},
+		// Non-Youtube URL
+		{
+			websiteURL: "https://www.example.com/channel/UC-Qj80avWItNRjkZ41rzHyw",
+			feedURL:    "",
+		},
+		// Invalid URL
+		{
+			websiteURL:     "https://example|org/",
+			feedURL:        "",
+			discoveryError: true,
+		},
+	}
 
-		if len(subscriptions) != 1 {
-			t.Fatal(`Incorrect number of subscriptions returned`)
+	for _, scenario := range scenarios {
+		subscriptions, localizedError := NewSubscriptionFinder(nil).FindSubscriptionsFromYouTubeChannelPage(scenario.websiteURL)
+		if scenario.discoveryError {
+			if localizedError == nil {
+				t.Fatalf(`Parsing an invalid URL should return an error`)
+			}
 		}
 
-		if subscriptions[0].URL != expectedFeedURL {
-			t.Errorf(`Unexpected Feed, got %s, instead of %s`, subscriptions[0].URL, expectedFeedURL)
+		if scenario.feedURL == "" {
+			if len(subscriptions) > 0 {
+				t.Fatalf(`Parsing a non-channel URL should not return any subscription: %q`, scenario.websiteURL)
+			}
+		} else {
+			if localizedError != nil {
+				t.Fatalf(`Parsing a correctly formatted YouTube channel page should not return any error: %v`, localizedError)
+			}
+
+			if len(subscriptions) != 1 {
+				t.Fatalf(`Incorrect number of subscriptions returned`)
+			}
+
+			if subscriptions[0].URL != scenario.feedURL {
+				t.Errorf(`Unexpected Feed, got %s, instead of %s`, subscriptions[0].URL, scenario.feedURL)
+			}
 		}
 	}
 }