From 3b67f6a80cd70208d866f5b663e567adf2df096c Mon Sep 17 00:00:00 2001
From: Mattias Wadman <mattias.wadman@gmail.com>
Date: Tue, 5 Nov 2019 00:45:14 +0100
Subject: [PATCH] Better errors and error handling

Fix issue with ignore errors for playlists, now ignore
errors if we get info JSON with a non empty ID.
---
 goutubedl.go      | 59 +++++++++++++++++++++++++++++------------------
 goutubedl_test.go | 30 +++++++++++++++++++-----
 2 files changed, 61 insertions(+), 28 deletions(-)

diff --git a/goutubedl.go b/goutubedl.go
index 33091f8..4a50ecf 100644
--- a/goutubedl.go
+++ b/goutubedl.go
@@ -6,6 +6,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -29,13 +30,19 @@ type nopPrinter struct{}
 
 func (nopPrinter) Print(v ...interface{}) {}
 
-// Error youtube-dl specific error
-type Error string
+// YoutubedlError is a error from youtube-dl
+type YoutubedlError string
 
-func (e Error) Error() string {
+func (e YoutubedlError) Error() string {
 	return string(e)
 }
 
+// ErrNotAPlaylist error when single entry when expected a playlist
+var ErrNotAPlaylist = errors.New("single entry when expected a playlist")
+
+// ErrNotASingleEntry error when playlist when expected a single entry
+var ErrNotASingleEntry = errors.New("playlist when expected a single entry")
+
 // Info youtube-dl info
 type Info struct {
 	// Generated from youtube-dl README using:
@@ -241,9 +248,10 @@ func infoFromURL(ctx context.Context, rawURL string, options Options) (info Info
 	cmd := exec.CommandContext(
 		ctx,
 		Path,
+		// see comment below about ignoring errors for playlists
+		"--ignore-errors",
 		"--no-call-home",
 		"--no-cache-dir",
-		"--ignore-errors",
 		"--skip-download",
 		"--restrict-filenames",
 		// provide URL via stdin for security, youtube-dl has some run command args
@@ -302,28 +310,35 @@ func infoFromURL(ctx context.Context, rawURL string, options Options) (info Info
 		}
 	}
 
-	// HACK: --ignore-errors still return error message and exit code != 0
-	// so workaround is to assume things went ok if we get some json on stdout
-	if len(stdoutBuf.Bytes()) == 0 {
+	infoSeemsOk := false
+	if len(stdoutBuf.Bytes()) > 0 {
+		if infoErr := json.Unmarshal(stdoutBuf.Bytes(), &info); infoErr != nil {
+			return Info{}, nil, infoErr
+		}
+
+		isPlaylist := info.Type == "playlist" || info.Type == "multi_video"
+		switch {
+		case options.Type == TypePlaylist && !isPlaylist:
+			return Info{}, nil, ErrNotAPlaylist
+		case options.Type == TypeSingle && isPlaylist:
+			return Info{}, nil, ErrNotASingleEntry
+		default:
+			// any type
+		}
+
+		// HACK: --ignore-errors still return error message and exit code != 0
+		// so workaround is to assume things went ok if we get some ok json on stdout
+		infoSeemsOk = info.ID != ""
+	}
+
+	if !infoSeemsOk {
 		if errMessage != "" {
-			return Info{}, nil, Error(errMessage)
+			return Info{}, nil, YoutubedlError(errMessage)
 		} else if cmdErr != nil {
 			return Info{}, nil, cmdErr
 		}
-	}
 
-	if infoErr := json.Unmarshal(stdoutBuf.Bytes(), &info); infoErr != nil {
-		return Info{}, nil, infoErr
-	}
-
-	isPlaylist := info.Type == "playlist" || info.Type == "multi_video"
-	switch {
-	case options.Type == TypePlaylist && !isPlaylist:
-		return Info{}, nil, fmt.Errorf("is not a playlist")
-	case options.Type == TypeSingle && isPlaylist:
-		return Info{}, nil, fmt.Errorf("is not a single entry")
-	default:
-		// any type
+		return Info{}, nil, fmt.Errorf("unknown error")
 	}
 
 	get := func(url string) (*http.Response, error) {
@@ -371,7 +386,7 @@ func infoFromURL(ctx context.Context, rawURL string, options Options) (info Info
 		}
 	}
 
-	// as we ignore errors some entries might show up as null
+	// as we ignore errors for playlists some entries might show up as null
 	if options.Type == TypePlaylist {
 		var filteredEntrise []Info
 		for _, e := range info.Entries {
diff --git a/goutubedl_test.go b/goutubedl_test.go
index f4bd019..74ed1a1 100644
--- a/goutubedl_test.go
+++ b/goutubedl_test.go
@@ -181,6 +181,20 @@ func TestPlaylist(t *testing.T) {
 	}
 }
 
+func TestTestUnsupportedURL(t *testing.T) {
+	defer leaktest.Check(t)()
+
+	_, ydlResultErr := New(context.Background(), "https://www.google.com", Options{})
+	if ydlResultErr == nil {
+		t.Errorf("expected unsupported url")
+	}
+	expectedErr := "Unsupported URL: https://www.google.com"
+	if ydlResultErr != nil && ydlResultErr.Error() != expectedErr {
+		t.Errorf("expected error %q got %q", expectedErr, ydlResultErr.Error())
+
+	}
+}
+
 func TestPlaylistWithPrivateVideo(t *testing.T) {
 	defer leaktest.Check(t)()
 
@@ -233,22 +247,26 @@ func TestSubtitles(t *testing.T) {
 	}
 }
 
-func TestErrorIsNotAPlaylist(t *testing.T) {
+func TestErrorNotAPlaylist(t *testing.T) {
+	defer leakChecks(t)()
+
 	_, ydlResultErr := New(context.Background(), testVideoRawURL, Options{
 		Type:              TypePlaylist,
 		DownloadThumbnail: false,
 	})
-	if ydlResultErr.Error() != "is not a playlist" {
-		t.Errorf("expected is playlist error")
+	if ydlResultErr != ErrNotAPlaylist {
+		t.Errorf("expected is playlist error, got %s", ydlResultErr)
 	}
 }
 
-func TestErrorIsNotASingleEntry(t *testing.T) {
+func TestErrorNotASingleEntry(t *testing.T) {
+	defer leakChecks(t)()
+
 	_, ydlResultErr := New(context.Background(), playlistRawURL, Options{
 		Type:              TypeSingle,
 		DownloadThumbnail: false,
 	})
-	if ydlResultErr.Error() != "is not a single entry" {
-		t.Errorf("expected is not a playlist error")
+	if ydlResultErr != ErrNotASingleEntry {
+		t.Errorf("expected is single entry error, got %s", ydlResultErr)
 	}
 }