From f41d0725736b1aab1d4b571a673f4d7be2607897 Mon Sep 17 00:00:00 2001 From: Alessio Date: Sun, 6 Mar 2022 20:31:04 -0800 Subject: [PATCH] Improve error handling some more --- .golangci.yaml | 13 +++++-------- cmd/twitter/main.go | 2 +- persistence/media_download.go | 4 ++-- persistence/profile.go | 13 ++----------- persistence/profile_test.go | 3 +-- persistence/tweet_queries.go | 5 +++-- persistence/tweet_trove_queries.go | 10 +++++----- persistence/user_queries.go | 19 ++++++++++--------- persistence/user_queries_test.go | 18 +++++++++--------- scraper/api_errors.go | 8 ++++++-- scraper/api_types.go | 4 ++-- scraper/api_types_v2.go | 6 +++--- scraper/link_expander.go | 4 ++-- scraper/tweet.go | 8 ++++---- scraper/tweet_trove.go | 8 ++++++-- scraper/user.go | 2 +- 16 files changed, 62 insertions(+), 65 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 54ea499..078ee34 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -26,7 +26,7 @@ linters: # - wrapcheck - lll - godox - # - errorlint + - errorlint # # all available settings of specific linters @@ -63,13 +63,10 @@ linters-settings: # - io.Copy(*bytes.Buffer) # - io.Copy(os.Stdout) -# errorlint: -# # Check whether fmt.Errorf uses the %w verb for formatting errors. See the readme for caveats -# errorf: true -# # Check for plain type assertions and type switches -# asserts: true -# # Check for plain error comparisons -# comparison: true + errorlint: + errorf: true # Ensure Errorf only uses %w (not %v or %s etc) for errors + asserts: true # Require errors.As instead of type-asserting + comparison: true # Require errors.Is instead of equality-checking # exhaustive: # # check switch statements in generated files also diff --git a/cmd/twitter/main.go b/cmd/twitter/main.go index 3c2565d..865b138 100644 --- a/cmd/twitter/main.go +++ b/cmd/twitter/main.go @@ -230,7 +230,7 @@ func download_tweet_content(tweet_identifier string) { tweet, err := profile.GetTweetById(tweet_id) if err != nil { - panic(fmt.Sprintf("Couldn't get tweet (ID %d) from database: %s", tweet_id, err.Error())) + panic(fmt.Errorf("Couldn't get tweet (ID %d) from database:\n %w", tweet_id, err)) } err = profile.DownloadTweetContentFor(&tweet) if err != nil { diff --git a/persistence/media_download.go b/persistence/media_download.go index 8fdc595..bc0c7b0 100644 --- a/persistence/media_download.go +++ b/persistence/media_download.go @@ -36,12 +36,12 @@ func (d DefaultDownloader) Curl(url string, outpath string) error { data, err := ioutil.ReadAll(resp.Body) if err != nil { - return fmt.Errorf("Error downloading image %s: %s", url, err.Error()) + return fmt.Errorf("Error downloading image %s:\n %w", url, err) } err = os.WriteFile(outpath, data, 0644) if err != nil { - return fmt.Errorf("Error writing to path: %s, url: %s: %s", outpath, url, err.Error()) + return fmt.Errorf("Error writing to path %s, url %s:\n %w", outpath, url, err) } return nil } diff --git a/persistence/profile.go b/persistence/profile.go index 9b84f71..2b4418e 100644 --- a/persistence/profile.go +++ b/persistence/profile.go @@ -22,16 +22,7 @@ type Profile struct { DB *sql.DB } -/** - * Custom error - */ -type ErrTargetAlreadyExists struct { - target string -} - -func (err ErrTargetAlreadyExists) Error() string { - return fmt.Sprintf("Target already exists: %s", err.target) -} +var ErrTargetAlreadyExists = fmt.Errorf("Target already exists") /** * Create a new profile in the given location. @@ -45,7 +36,7 @@ func (err ErrTargetAlreadyExists) Error() string { */ func NewProfile(target_dir string) (Profile, error) { if file_exists(target_dir) { - return Profile{}, ErrTargetAlreadyExists{target_dir} + return Profile{}, fmt.Errorf("Could not create target %q:\n %w", target_dir, ErrTargetAlreadyExists) } settings_file := path.Join(target_dir, "settings.yaml") diff --git a/persistence/profile_test.go b/persistence/profile_test.go index 3952e68..9b70ea7 100644 --- a/persistence/profile_test.go +++ b/persistence/profile_test.go @@ -40,8 +40,7 @@ func TestNewProfileInvalidPath(t *testing.T) { _, err = persistence.NewProfile(gibberish_path) require.Error(err, "Should have failed to create a profile in an already existing directory!") - _, is_right_type := err.(persistence.ErrTargetAlreadyExists) - assert.True(t, is_right_type, "Expected 'ErrTargetAlreadyExists' error, got %T instead", err) + assert.ErrorIs(t, err, persistence.ErrTargetAlreadyExists) } /** diff --git a/persistence/tweet_queries.go b/persistence/tweet_queries.go index 89c5496..2d641be 100644 --- a/persistence/tweet_queries.go +++ b/persistence/tweet_queries.go @@ -3,6 +3,7 @@ package persistence import ( "database/sql" "strings" + "errors" "offline_twitter/scraper" ) @@ -82,7 +83,7 @@ func (p Profile) IsTweetInDatabase(id scraper.TweetID) bool { var dummy string err := db.QueryRow("select 1 from tweets where id = ?", id).Scan(&dummy) if err != nil { - if err != sql.ErrNoRows { + if !errors.Is(err, sql.ErrNoRows) { // A real error panic(err) } @@ -189,7 +190,7 @@ func (p Profile) CheckTweetContentDownloadNeeded(tweet scraper.Tweet) bool { var is_content_downloaded bool err := row.Scan(&is_content_downloaded) if err != nil { - if err == sql.ErrNoRows { + if errors.Is(err, sql.ErrNoRows) { return true } else { panic(err) diff --git a/persistence/tweet_trove_queries.go b/persistence/tweet_trove_queries.go index 5f52085..e5adeea 100644 --- a/persistence/tweet_trove_queries.go +++ b/persistence/tweet_trove_queries.go @@ -15,12 +15,12 @@ func (p Profile) SaveTweetTrove(trove TweetTrove) { // Download download their tiny profile image err := p.DownloadUserProfileImageTiny(&u) if err != nil { - panic(fmt.Sprintf("Error downloading user content for user with ID %d and handle %s: %s", u.ID, u.Handle, err.Error())) + panic(fmt.Errorf("Error downloading user content for user with ID %d and handle %s:\n %w", u.ID, u.Handle, err)) } err = p.SaveUser(&u) if err != nil { - panic(fmt.Sprintf("Error saving user with ID %d and handle %s: %s", u.ID, u.Handle, err.Error())) + panic(fmt.Errorf("Error saving user with ID %d and handle %s:\n %w", u.ID, u.Handle, err)) } fmt.Println(u.Handle, u.ID) // If the User's ID was updated in saving (i.e., Unknown User), update it in the Trove too @@ -33,19 +33,19 @@ func (p Profile) SaveTweetTrove(trove TweetTrove) { for _, t := range trove.Tweets { err := p.SaveTweet(t) if err != nil { - panic(fmt.Sprintf("Error saving tweet ID %d: %s", t.ID, err.Error())) + panic(fmt.Errorf("Error saving tweet ID %d:\n %w", t.ID, err)) } err = p.DownloadTweetContentFor(&t) if err != nil { - panic(fmt.Sprintf("Error downloading tweet content for tweet ID %d: %s", t.ID, err.Error())) + panic(fmt.Errorf("Error downloading tweet content for tweet ID %d:\n %w", t.ID, err)) } } for _, r := range trove.Retweets { err := p.SaveRetweet(r) if err != nil { - panic(fmt.Sprintf("Error saving retweet with ID %d from user ID %d: %s", r.RetweetID, r.RetweetedByID, err.Error())) + panic(fmt.Errorf("Error saving retweet with ID %d from user ID %d:\n %w", r.RetweetID, r.RetweetedByID, err)) } } } diff --git a/persistence/user_queries.go b/persistence/user_queries.go index 500a031..9af9747 100644 --- a/persistence/user_queries.go +++ b/persistence/user_queries.go @@ -2,6 +2,7 @@ package persistence import ( "fmt" + "errors" "database/sql" "offline_twitter/scraper" ) @@ -16,7 +17,7 @@ import ( func (p Profile) SaveUser(u *scraper.User) error { if u.IsNeedingFakeID { err := p.DB.QueryRow("select id from users where lower(handle) = lower(?)", u.Handle).Scan(&u.ID) - if err == sql.ErrNoRows { + if errors.Is(err, sql.ErrNoRows) { // We need to continue-- create a new fake user u.ID = p.NextFakeUserID() } else if err == nil { @@ -24,7 +25,7 @@ func (p Profile) SaveUser(u *scraper.User) error { return nil } else { // A real error occurred - panic(fmt.Sprintf("Error checking for existence of fake user with handle %q: %s", u.Handle, err.Error())) + panic(fmt.Errorf("Error checking for existence of fake user with handle %q:\n %w", u.Handle, err)) } } @@ -79,7 +80,7 @@ func (p Profile) UserExists(handle scraper.UserHandle) bool { var dummy string err := db.QueryRow("select 1 from users where lower(handle) = lower(?)", handle).Scan(&dummy) if err != nil { - if err != sql.ErrNoRows { + if !errors.Is(err, sql.ErrNoRows) { // A real error panic(err) } @@ -109,7 +110,7 @@ func (p Profile) GetUserByHandle(handle scraper.UserHandle) (scraper.User, error where lower(handle) = lower(?) `, handle) - if err == sql.ErrNoRows { + if errors.Is(err, sql.ErrNoRows) { return ret, ErrNotInDatabase{"User", handle} } return ret, nil @@ -136,7 +137,7 @@ func (p Profile) GetUserByID(id scraper.UserID) (scraper.User, error) { from users where id = ? `, id) - if err == sql.ErrNoRows { + if errors.Is(err, sql.ErrNoRows) { return ret, ErrNotInDatabase{"User", id} } return ret, err @@ -164,7 +165,7 @@ func (p Profile) CheckUserContentDownloadNeeded(user scraper.User) bool { var banner_image_url string err := row.Scan(&is_content_downloaded, &profile_image_url, &banner_image_url) if err != nil { - if err == sql.ErrNoRows { + if errors.Is(err, sql.ErrNoRows) { return true } else { panic(err) @@ -189,14 +190,14 @@ func (p Profile) CheckUserContentDownloadNeeded(user scraper.User) bool { func (p Profile) SetUserFollowed(user *scraper.User, is_followed bool) { result, err := p.DB.Exec("update users set is_followed = ? where id = ?", is_followed, user.ID) if err != nil { - panic(fmt.Sprintf("Error inserting user with handle %q: %s", user.Handle, err.Error())) + panic(fmt.Errorf("Error inserting user with handle %q:\n %w", user.Handle, err)) } count, err := result.RowsAffected() if err != nil { - panic("Unknown error: " + err.Error()) + panic(fmt.Errorf("Unknown error retrieving row count:\n %w", err)) } if count != 1 { - panic(fmt.Sprintf("User with handle %q not found", user.Handle)) + panic(fmt.Errorf("User with handle %q not found", user.Handle)) } user.IsFollowed = is_followed } diff --git a/persistence/user_queries_test.go b/persistence/user_queries_test.go index ad5903a..1f8fef3 100644 --- a/persistence/user_queries_test.go +++ b/persistence/user_queries_test.go @@ -194,14 +194,14 @@ func TestFollowUnfollowUser(t *testing.T) { // Ensure the change was persisted user_reloaded, err := profile.GetUserByHandle(user.Handle) require.NoError(t, err) - assert.Equal(user.ID, user_reloaded.ID) // Verify it's the same user + assert.Equal(user.ID, user_reloaded.ID) // Verify it's the same user assert.True(user_reloaded.IsFollowed) - err = profile.SaveUser(&user) // should NOT un-set is_followed + err = profile.SaveUser(&user) // should NOT un-set is_followed assert.NoError(err) user_reloaded, err = profile.GetUserByHandle(user.Handle) require.NoError(t, err) - assert.Equal(user.ID, user_reloaded.ID) // Verify it's the same user + assert.Equal(user.ID, user_reloaded.ID) // Verify it's the same user assert.True(user_reloaded.IsFollowed) profile.SetUserFollowed(&user, false) @@ -210,7 +210,7 @@ func TestFollowUnfollowUser(t *testing.T) { // Ensure the change was persisted user_reloaded, err = profile.GetUserByHandle(user.Handle) require.NoError(t, err) - assert.Equal(user.ID, user_reloaded.ID) // Verify it's the same user + assert.Equal(user.ID, user_reloaded.ID) // Verify it's the same user assert.False(user_reloaded.IsFollowed) } @@ -233,16 +233,16 @@ func TestCreateUnknownUserWithHandle(t *testing.T) { err := profile.SaveUser(&user) assert.NoError(err) - assert.Equal(scraper.UserID(next_id + 1), user.ID) + assert.Equal(scraper.UserID(next_id+1), user.ID) // Ensure the change was persisted user_reloaded, err := profile.GetUserByHandle(user.Handle) require.NoError(t, err) - assert.Equal(handle, user_reloaded.Handle) // Verify it's the same user - assert.Equal(scraper.UserID(next_id + 1), user_reloaded.ID) + assert.Equal(handle, user_reloaded.Handle) // Verify it's the same user + assert.Equal(scraper.UserID(next_id+1), user_reloaded.ID) // Why not tack this test on here: make sure NextFakeUserID works as expected - assert.Equal(next_id + 2, profile.NextFakeUserID()) + assert.Equal(next_id+2, profile.NextFakeUserID()) } /** @@ -266,7 +266,7 @@ func TestCreateUnknownUserWithHandleThatAlreadyExists(t *testing.T) { // The real user should not have been overwritten at all user_reloaded, err := profile.GetUserByID(user.ID) assert.NoError(err) - assert.False(user_reloaded.IsIdFake) // This one particularly + assert.False(user_reloaded.IsIdFake) // This one particularly assert.Equal(user.Handle, user_reloaded.Handle) assert.Equal(user.Bio, user_reloaded.Bio) assert.Equal(user.DisplayName, user_reloaded.DisplayName) diff --git a/scraper/api_errors.go b/scraper/api_errors.go index c257c71..cd69a67 100644 --- a/scraper/api_errors.go +++ b/scraper/api_errors.go @@ -4,5 +4,9 @@ import ( "fmt" ) -var END_OF_FEED = fmt.Errorf("End of feed") -var DOESNT_EXIST = fmt.Errorf("Doesn't exist") +var ( + END_OF_FEED = fmt.Errorf("End of feed") + DOESNT_EXIST = fmt.Errorf("Doesn't exist") + EXTERNAL_API_ERROR = fmt.Errorf("Unexpected result from external API") + API_PARSE_ERROR = fmt.Errorf("Couldn't parse the result returned from the API") +) diff --git a/scraper/api_types.go b/scraper/api_types.go index 414659b..ccd780f 100644 --- a/scraper/api_types.go +++ b/scraper/api_types.go @@ -271,7 +271,7 @@ func (u UserResponse) ConvertToAPIUser() APIUser { } else if api_error.Name == "NotFoundError" { ret.DoesntExist = true } else { - panic(fmt.Sprintf("Unknown api error: %q", api_error.Message)) + panic(fmt.Errorf("Unknown api error %q:\n %w", api_error.Message, EXTERNAL_API_ERROR)) } } @@ -401,7 +401,7 @@ func (t *TweetResponse) HandleTombstones() []UserHandle { short_text, ok := tombstone_types[entry.GetTombstoneText()] if !ok { - panic(fmt.Sprintf("Unknown tombstone text: %s", entry.GetTombstoneText())) + panic(fmt.Errorf("Unknown tombstone text %q:\n %w", entry.GetTombstoneText(), EXTERNAL_API_ERROR)) } tombstoned_tweet.TombstoneText = short_text diff --git a/scraper/api_types_v2.go b/scraper/api_types_v2.go index 873b543..f4513a3 100644 --- a/scraper/api_types_v2.go +++ b/scraper/api_types_v2.go @@ -186,7 +186,7 @@ func (api_result APIV2Result) ToTweetTrove(ignore_null_entries bool) TweetTrove var ok bool tombstoned_tweet.TombstoneText, ok = tombstone_types[quoted_api_result.Result.Tombstone.Text.Text] if !ok { - panic(fmt.Sprintf("Unknown tombstone text: %s", quoted_api_result.Result.Tombstone.Text.Text)) + panic(fmt.Errorf("Unknown tombstone text %q:\n %w", quoted_api_result.Result.Tombstone.Text.Text, EXTERNAL_API_ERROR)) } tombstoned_tweet.ID = int64(int_or_panic(api_result.Result.Legacy.APITweet.QuotedStatusIDStr)) handle, err := ParseHandleFromTweetUrl(api_result.Result.Legacy.APITweet.QuotedStatusPermalink.ExpandedURL) @@ -209,7 +209,7 @@ func (api_result APIV2Result) ToTweetTrove(ignore_null_entries bool) TweetTrove // and the retweeted TweetResults; it should only be parsed for the real Tweet, not the Retweet main_tweet, ok := ret.Tweets[TweetID(api_result.Result.Legacy.ID)] if !ok { - panic(fmt.Sprintf("Tweet trove didn't contain its own tweet: %d", api_result.Result.Legacy.ID)) + panic(fmt.Errorf("Tweet trove didn't contain its own tweet with ID %d:\n %w", api_result.Result.Legacy.ID, EXTERNAL_API_ERROR)) } if api_result.Result.Card.Legacy.Name == "summary_large_image" || api_result.Result.Card.Legacy.Name == "player" { url := api_result.Result.Card.ParseAsUrl() @@ -225,7 +225,7 @@ func (api_result APIV2Result) ToTweetTrove(ignore_null_entries bool) TweetTrove main_tweet.Urls[i] = url } if !found { - panic(fmt.Sprintf("Couldn't find the url in tweet ID: %d", api_result.Result.Legacy.ID)) + panic(fmt.Errorf("Couldn't find the url in tweet ID %d:\n %w", api_result.Result.Legacy.ID, EXTERNAL_API_ERROR)) } } else if strings.Index(api_result.Result.Card.Legacy.Name, "poll") == 0 { // Process polls diff --git a/scraper/link_expander.go b/scraper/link_expander.go index f892288..97122a2 100644 --- a/scraper/link_expander.go +++ b/scraper/link_expander.go @@ -24,12 +24,12 @@ func ExpandShortUrl(short_url string) string { panic(err) // TODO: handle timeouts } if resp.StatusCode != 301 { - panic(fmt.Sprintf("Unknown status code returned when expanding short url %q: %s", short_url, resp.Status)) + panic(fmt.Errorf("Unknown status code returned when expanding short url %q: %s\n %w", short_url, resp.Status, EXTERNAL_API_ERROR)) } long_url := resp.Header.Get("Location") if long_url == "" { - panic(fmt.Sprintf("Header didn't have a Location field for short url %q", short_url)) + panic(fmt.Errorf("Header didn't have a Location field for short url %q:\n %w", short_url, EXTERNAL_API_ERROR)) } return long_url } diff --git a/scraper/tweet.go b/scraper/tweet.go index 965e80e..3b8c5be 100644 --- a/scraper/tweet.go +++ b/scraper/tweet.go @@ -126,7 +126,7 @@ func ParseSingleTweet(apiTweet APITweet) (ret Tweet, err error) { // Process images for _, media := range apiTweet.Entities.Media { if media.Type != "photo" { // TODO: remove this eventually - panic(fmt.Sprintf("Unknown media type: %q", media.Type)) + panic(fmt.Errorf("Unknown media type %q:\n %w", media.Type, EXTERNAL_API_ERROR)) } new_image := ParseAPIMedia(media) new_image.TweetID = ret.ID @@ -145,7 +145,7 @@ func ParseSingleTweet(apiTweet APITweet) (ret Tweet, err error) { for _, mention := range strings.Split(apiTweet.Entities.ReplyMentions, " ") { if mention != "" { if mention[0] != '@' { - panic(fmt.Sprintf("Unknown ReplyMention value: %s", apiTweet.Entities.ReplyMentions)) + panic(fmt.Errorf("Unknown ReplyMention value %q:\n %w", apiTweet.Entities.ReplyMentions, EXTERNAL_API_ERROR)) } ret.ReplyMentions = append(ret.ReplyMentions, UserHandle(mention[1:])) } @@ -158,7 +158,7 @@ func ParseSingleTweet(apiTweet APITweet) (ret Tweet, err error) { continue } if len(apiTweet.ExtendedEntities.Media) != 1 { - panic(fmt.Sprintf("Surprising ExtendedEntities: %v", apiTweet.ExtendedEntities.Media)) + panic(fmt.Errorf("Surprising ExtendedEntities: %v\n %w", apiTweet.ExtendedEntities.Media, EXTERNAL_API_ERROR)) } new_video := ParseAPIVideo(apiTweet.ExtendedEntities.Media[0], ret.ID) ret.Videos = []Video{new_video} @@ -194,7 +194,7 @@ func GetTweet(id TweetID) (Tweet, error) { api := API{} tweet_response, err := api.GetTweet(id, "") if err != nil { - return Tweet{}, fmt.Errorf("Error in API call: %s", err) + return Tweet{}, fmt.Errorf("Error in API call:\n %w", err) } single_tweet, ok := tweet_response.GlobalObjects.Tweets[fmt.Sprint(id)] diff --git a/scraper/tweet_trove.go b/scraper/tweet_trove.go index 1dab95a..749ccc1 100644 --- a/scraper/tweet_trove.go +++ b/scraper/tweet_trove.go @@ -83,7 +83,7 @@ func (trove *TweetTrove) FetchTombstoneUsers() { log.Debug("Getting tombstone user: " + handle) user, err := GetUser(handle) if err != nil { - panic(fmt.Sprintf("Error getting tombstoned user: %s\n %s", handle, err.Error())) + panic(fmt.Errorf("Error getting tombstoned user with handle %q: \n %w", handle, err)) } if user.ID == 0 { @@ -124,7 +124,11 @@ func (trove *TweetTrove) FillMissingUserIDs() { if !is_found { // The user probably deleted deleted their account, and thus `scraper.GetUser` failed. So // they're not in this trove's Users. - panic(fmt.Sprintf("Couldn't fill out this Tweet's UserID: %d, %s", tweet.ID, tweet.UserHandle)) + panic(fmt.Errorf( + "Couldn't find user ID for user %q, while filling missing UserID in tweet with ID %d", + tweet.UserHandle, + tweet.ID, + )) } tweet.UserID = user.ID trove.Tweets[i] = tweet diff --git a/scraper/user.go b/scraper/user.go index b23beac..8f941cf 100644 --- a/scraper/user.go +++ b/scraper/user.go @@ -207,7 +207,7 @@ func (u User) GetTinyProfileImageUrl() string { // Check that the format is as expected r := regexp.MustCompile(`(\.\w{2,4})$`) if !r.MatchString(u.ProfileImageUrl) { - panic(fmt.Sprintf("Weird profile image url: %s", u.ProfileImageUrl)) + panic(fmt.Errorf("Weird profile image url (here is the file extension?): %s", u.ProfileImageUrl)) } return r.ReplaceAllString(u.ProfileImageUrl, "_normal$1") }