Merge pull request #304 from rusq/list-channels-fix

Fix list channels logic

cmd/slackdump/internal/list/channels.go

@@ -37,9 +37,12 @@ and -chan-cache-retention flags to control the cache behavior.
 	RequireAuth: true,
 }
 
+var noresolve bool
+
 func init() {
 	CmdListChannels.Flag.BoolVar(&chanCacheOpts.Disabled, "no-chan-cache", chanCacheOpts.Disabled, "disable channel cache")
 	CmdListChannels.Flag.DurationVar(&chanCacheOpts.Retention, "chan-cache-retention", chanCacheOpts.Retention, "channel cache retention time.  After this time, the cache is considered stale and will be refreshed.")
+	CmdListChannels.Flag.BoolVar(&noresolve, "no-resolve", noresolve, "do not resolve user IDs to names")
 }
 
 func listChannels(ctx context.Context, cmd *base.Command, args []string) error {
@@ -62,7 +65,7 @@ func listChannels(ctx context.Context, cmd *base.Command, args []string) error {
 		trace.Logf(ctx, "cache miss", "teamID=%s", teamID)
 		cc, err := sess.GetChannels(ctx)
 		if err != nil {
-			return nil, "", err
+			return nil, "", fmt.Errorf("error getting channels: %w", err)
 		}
 		if err := saveCache(cfg.CacheDir(), teamID, cc); err != nil {
 			// warn, but don't fail

cmd/slackdump/internal/list/list.go

@@ -8,8 +8,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"runtime/trace"
-	"time"
 
 	"github.com/charmbracelet/huh"
 	"github.com/rusq/fsadapter"
@@ -107,9 +105,13 @@ func list(ctx context.Context, listFn listFunc) error {
 	}
 
 	teamID := sess.Info().TeamID
-	users, ok := data.(types.Users)
-	if !ok {
-		users, err = getCachedUsers(ctx, sess, m, teamID)
+	users, ok := data.(types.Users) // Hax
+	if !ok && !noresolve {
+		if cfg.NoUserCache {
+			users, err = sess.GetUsers(ctx)
+		} else {
+			users, err = getCachedUsers(ctx, sess, m, teamID)
+		}
 		if err != nil {
 			return err
 		}
@@ -149,46 +151,6 @@ func saveData(ctx context.Context, fs fsadapter.FS, data any, filename string, t
 	return nil
 }
 
-type userGetter interface {
-	GetUsers(ctx context.Context) (types.Users, error)
-}
-
-type userCacher interface {
-	LoadUsers(teamID string, retention time.Duration) ([]slack.User, error)
-	CacheUsers(teamID string, users []slack.User) error
-}
-
-func getCachedUsers(ctx context.Context, ug userGetter, m userCacher, teamID string) ([]slack.User, error) {
-	lg := logger.FromContext(ctx)
-
-	users, err := m.LoadUsers(teamID, cfg.UserCacheRetention)
-	if err == nil {
-		return users, nil
-	}
-
-	// failed to load from cache
-	if !errors.Is(err, cache.ErrExpired) && !errors.Is(err, cache.ErrEmpty) {
-		// some funky error
-		return nil, err
-	}
-
-	lg.Println("user cache expired or empty, caching users")
-
-	// getting users from API
-	users, err = ug.GetUsers(ctx)
-	if err != nil {
-		return nil, err
-	}
-
-	// saving users to cache, will ignore any errors, but notify the user.
-	if err := m.CacheUsers(teamID, users); err != nil {
-		trace.Logf(ctx, "error", "saving user cache to %q, error: %s", userCacheBase, err)
-		lg.Printf("warning: failed saving user cache to %q: %s, but nevermind, let's continue", userCacheBase, err)
-	}
-
-	return users, nil
-}
-
 // fmtPrint prints the given data to the given writer, using the given format.
 // It should be supplied with prepopulated users, as it may need to look up
 // users by ID.
@@ -222,7 +184,7 @@ func makeFilename(prefix string, teamID string, ext string) string {
 	return fmt.Sprintf("%s-%s%s", prefix, teamID, ext)
 }
 
-func wizard(ctx context.Context, listFn listFunc) error {
+func wizard(context.Context, listFunc) error {
 	// pick format
 	var types []string
 	for _, t := range format.All() {

cmd/slackdump/internal/list/list_test.go

@@ -0,0 +1 @@
+package list

cmd/slackdump/internal/list/users.go

@@ -2,11 +2,20 @@ package list
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"os"
+	"runtime/trace"
+	"time"
 
+	"github.com/rusq/slack"
 	"github.com/rusq/slackdump/v3"
 	"github.com/rusq/slackdump/v3/cmd/slackdump/internal/cfg"
 	"github.com/rusq/slackdump/v3/cmd/slackdump/internal/golang/base"
+	"github.com/rusq/slackdump/v3/internal/cache"
+	"github.com/rusq/slackdump/v3/internal/osext"
+	"github.com/rusq/slackdump/v3/logger"
+	"github.com/rusq/slackdump/v3/types"
 )
 
 var CmdListUsers = &base.Command{
@@ -49,3 +58,44 @@ func wizUsers(ctx context.Context, cmd *base.Command, args []string) error {
 		return users, filename, err
 	})
 }
+
+//go:generate mockgen -source=users.go -destination=mocks_test.go -package=list userGetter,userCacher
+
+type userGetter interface {
+	GetUsers(ctx context.Context) (types.Users, error)
+}
+
+type userCacher interface {
+	LoadUsers(teamID string, retention time.Duration) ([]slack.User, error)
+	CacheUsers(teamID string, users []slack.User) error
+}
+
+func getCachedUsers(ctx context.Context, ug userGetter, m userCacher, teamID string) ([]slack.User, error) {
+	lg := logger.FromContext(ctx)
+
+	users, err := m.LoadUsers(teamID, cfg.UserCacheRetention)
+	if err == nil {
+		return users, nil
+	}
+
+	// failed to load from cache
+	if !errors.Is(err, cache.ErrExpired) && !errors.Is(err, cache.ErrEmpty) && !os.IsNotExist(err) && !osext.IsPathError(err) {
+		// some funky error
+		return nil, err
+	}
+	lg.Println("user cache expired or empty, caching users")
+
+	// getting users from API
+	users, err = ug.GetUsers(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	// saving users to cache, will ignore any errors, but notify the user.
+	if err := m.CacheUsers(teamID, users); err != nil {
+		trace.Logf(ctx, "error", "saving user cache to %q, error: %s", userCacheBase, err)
+		lg.Printf("warning: failed saving user cache to %q: %s, but nevermind, let's continue", userCacheBase, err)
+	}
+
+	return users, nil
+}

cmd/slackdump/internal/list/users_test.go

@@ -0,0 +1,104 @@
+package list
+
+import (
+	"context"
+	"errors"
+	"io/fs"
+	"reflect"
+	"testing"
+
+	"github.com/rusq/slack"
+	"go.uber.org/mock/gomock"
+)
+
+func Test_getCachedUsers(t *testing.T) {
+	var (
+		testUsers = []slack.User{
+			{ID: "U1"},
+			{ID: "U2"},
+			{ID: "U3"},
+		}
+	)
+	type args struct {
+		ctx    context.Context
+		teamID string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		expect  func(c *MockuserCacher, g *MockuserGetter)
+		want    []slack.User
+		wantErr bool
+	}{
+		/* oh happy days */
+		{
+			"users loaded from cache",
+			args{context.Background(), "TEAM1"},
+			func(c *MockuserCacher, g *MockuserGetter) {
+				c.EXPECT().LoadUsers("TEAM1", gomock.Any()).Return(testUsers, nil)
+			},
+			testUsers,
+			false,
+		},
+		{
+			"getting users from API ok (recoverable cache error)",
+			args{context.Background(), "TEAM1"},
+			func(c *MockuserCacher, g *MockuserGetter) {
+				c.EXPECT().LoadUsers("TEAM1", gomock.Any()).Return(nil, &fs.PathError{})
+				g.EXPECT().GetUsers(gomock.Any()).Return(testUsers, nil)
+				c.EXPECT().CacheUsers("TEAM1", testUsers).Return(nil)
+			},
+			testUsers,
+			false,
+		},
+		{
+			"saving cache fails, but we continue",
+			args{context.Background(), "TEAM1"},
+			func(c *MockuserCacher, g *MockuserGetter) {
+				c.EXPECT().LoadUsers("TEAM1", gomock.Any()).Return(nil, &fs.PathError{})
+				g.EXPECT().GetUsers(gomock.Any()).Return(testUsers, nil)
+				c.EXPECT().CacheUsers("TEAM1", testUsers).Return(errors.New("disk mulching detected"))
+			},
+			testUsers,
+			false,
+		},
+		/* unhappy days */
+		{
+			"unrecoverable error",
+			args{context.Background(), "TEAM1"},
+			func(c *MockuserCacher, g *MockuserGetter) {
+				c.EXPECT().LoadUsers("TEAM1", gomock.Any()).Return(nil, errors.New("frobnication error"))
+			},
+			nil,
+			true,
+		},
+		{
+			"getting users from API fails",
+			args{context.Background(), "TEAM1"},
+			func(c *MockuserCacher, g *MockuserGetter) {
+				c.EXPECT().LoadUsers("TEAM1", gomock.Any()).Return(nil, &fs.PathError{})
+				g.EXPECT().GetUsers(gomock.Any()).Return(nil, errors.New("blip"))
+			},
+			nil,
+			true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctrl := gomock.NewController(t)
+			muc := NewMockuserCacher(ctrl)
+			mug := NewMockuserGetter(ctrl)
+
+			tt.expect(muc, mug)
+
+			got, err := getCachedUsers(tt.args.ctx, mug, muc, tt.args.teamID)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("getCachedUsers() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("getCachedUsers() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

internal/osext/errors.go

@@ -0,0 +1,12 @@
+package osext
+
+import (
+	"errors"
+	"io/fs"
+)
+
+// IsPathError reports whether the error is an fs.PathError.
+func IsPathError(err error) bool {
+	var pathError *fs.PathError
+	return errors.As(err, &pathError)
+}