refactor(run.Store): use params.List in place of offset & limit

this opens the door to filtering & aggregating later on, but also provides
a nice pickup in the form of rewriting the list validation code into a
Validate method on params.List

add tests to params package now that it does things slightly above trivial

automation/run/store.go

@@ -7,6 +7,7 @@ import (
 	"sync"
 
 	"github.com/qri-io/qri/automation/workflow"
+	"github.com/qri-io/qri/base/params"
 	"github.com/qri-io/qri/event"
 )
 
@@ -35,15 +36,15 @@ type Store interface {
 	Count(wid workflow.ID) (int, error)
 	// List lists all the runs associated with the workflow.ID in reverse
 	// chronological order
-	List(wid workflow.ID, limit, offset int) ([]*State, error)
+	List(wid workflow.ID, lp params.List) ([]*State, error)
 	// GetLatest returns the most recent run associated with the workflow id
 	GetLatest(wid workflow.ID) (*State, error)
 	// GetStatus returns the status of the latest run based on the
 	// workflow.ID
 	GetStatus(wid workflow.ID) (Status, error)
 	// ListByStatus returns a list of run.State entries with a given status
 	// looking only at the most recent run of each Workflow
-	ListByStatus(s Status, limit, offset int) ([]*State, error)
+	ListByStatus(s Status, lp params.List) ([]*State, error)
 }
 
 // EventAdder is an extension interface that optimizes writing an event to a run
@@ -79,7 +80,10 @@ func newWorkflowMeta() *workflowMeta {
 	}
 }
 
-var _ Store = (*MemStore)(nil)
+var (
+	_ Store      = (*MemStore)(nil)
+	_ EventAdder = (*MemStore)(nil)
+)
 
 // NewMemStore returns a MemStore
 func NewMemStore() *MemStore {
@@ -146,18 +150,11 @@ func (s *MemStore) Count(wid workflow.ID) (int, error) {
 
 // List lists all the runs associated with the workflow.ID in reverse
 // chronological order
-func (s *MemStore) List(wid workflow.ID, limit, offset int) ([]*State, error) {
-	fetchAll := false
-	switch {
-	case limit == -1 && offset == 0:
-		fetchAll = true
-	case limit < 0:
-		return nil, fmt.Errorf("limit of %d is out of bounds", limit)
-	case offset < 0:
-		return nil, fmt.Errorf("offset of %d is out of bounds", offset)
-	case limit == 0:
-		return []*State{}, nil
+func (s *MemStore) List(wid workflow.ID, lp params.List) ([]*State, error) {
+	if err := lp.Validate(); err != nil {
+		return nil, err
 	}
+
 	s.mu.Lock()
 	defer s.mu.Unlock()
 	wfm, ok := s.workflows[wid]
@@ -175,13 +172,13 @@ func (s *MemStore) List(wid workflow.ID, limit, offset int) ([]*State, error) {
 		runs = append(runs, run)
 	}
 
-	if offset >= len(runs) {
+	if lp.Offset >= len(runs) {
 		return []*State{}, nil
 	}
 
-	start := offset
-	end := offset + limit
-	if end > len(runs) || fetchAll {
+	start := lp.Offset
+	end := lp.Offset + lp.Limit
+	if end > len(runs) || lp.All() {
 		end = len(runs)
 	}
 	return runs[start:end], nil
@@ -217,17 +214,9 @@ func (s *MemStore) GetStatus(wid workflow.ID) (Status, error) {
 
 // ListByStatus returns a list of run.State entries with a given status
 // looking only at the most recent run of each Workflow
-func (s *MemStore) ListByStatus(status Status, limit, offset int) ([]*State, error) {
-	fetchAll := false
-	switch {
-	case limit == -1 && offset == 0:
-		fetchAll = true
-	case limit < 0:
-		return nil, fmt.Errorf("limit of %d is out of bounds", limit)
-	case offset < 0:
-		return nil, fmt.Errorf("offset of %d is out of bounds", offset)
-	case limit == 0:
-		return []*State{}, nil
+func (s *MemStore) ListByStatus(status Status, lp params.List) ([]*State, error) {
+	if err := lp.Validate(); err != nil {
+		return nil, err
 	}
 
 	set := NewSet()
@@ -245,13 +234,13 @@ func (s *MemStore) ListByStatus(status Status, limit, offset int) ([]*State, err
 		}
 	}
 
-	if offset >= set.Len() {
+	if lp.Offset >= set.Len() {
 		return []*State{}, nil
 	}
 
-	start := offset
-	end := offset + limit
-	if end > set.Len() || fetchAll {
+	start := lp.Offset
+	end := lp.Offset + lp.Limit
+	if end > set.Len() || lp.All() {
 		end = set.Len()
 	}
 

automation/spec/run.go

@@ -8,6 +8,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/qri-io/qri/automation/run"
 	"github.com/qri-io/qri/automation/workflow"
+	"github.com/qri-io/qri/base/params"
 )
 
 // AssertRunStore confirms the expected behavior of a run.Store interface
@@ -60,10 +61,10 @@ func AssertRunStore(t *testing.T, store run.Store) {
 		t.Errorf("store.Count count mismatch, expected 3, got %d", gotCount)
 	}
 
-	if _, err := store.List(badWID, -1, 0); !errors.Is(err, run.ErrUnknownWorkflowID) {
+	if _, err := store.List(badWID, params.ListAll); !errors.Is(err, run.ErrUnknownWorkflowID) {
 		t.Fatalf("store.List should emit a run.ErrUnknownWorkflowID error when given a workflow ID not associated with any runs in the Store")
 	}
-	gotRuns, err := store.List(wid, -1, 0)
+	gotRuns, err := store.List(wid, params.ListAll)
 	if err != nil {
 		t.Fatalf("store.List unexpected error: %s", err)
 	}
@@ -93,7 +94,7 @@ func AssertRunStore(t *testing.T, store run.Store) {
 		t.Errorf("store.GetStatus mismatch: expected %q, got %q", run.RSWaiting, gotStatus)
 	}
 
-	gotRuns, err = store.ListByStatus(run.RSWaiting, -1, 0)
+	gotRuns, err = store.ListByStatus(run.RSWaiting, params.ListAll)
 	if err != nil {
 		t.Fatalf("store.ListByStatus unexpected error: %s", err)
 	}

base/params/list.go

@@ -1,6 +1,8 @@
 // Package params defines generic input parameter structures
 package params
 
+import "fmt"
+
 // ListAll uses a limit of -1 & offset of 0 as a sentinel value for listing
 // all items
 var ListAll = List{Limit: -1}
@@ -13,6 +15,22 @@ type List struct {
 	Offset  int
 }
 
+// All returns true if List is attempting to list all available items
+func (lp List) All() bool {
+	return lp.Limit == -1 && lp.Offset == 0
+}
+
+// Validate returns an error if any fields or field combinations are invalid
+func (lp List) Validate() error {
+	if lp.Limit < 0 && lp.Limit != -1 {
+		return fmt.Errorf("limit of %d is out of bounds", lp.Limit)
+	}
+	if lp.Offset < 0 {
+		return fmt.Errorf("offset of %d is out of bounds", lp.Offset)
+	}
+	return nil
+}
+
 // WithOffsetLimit returns a new List struct that replaces the offset & limit
 // fields
 func (lp List) WithOffsetLimit(offset, limit int) List {

base/params/list_test.go

@@ -0,0 +1,72 @@
+package params
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestParamsWith(t *testing.T) {
+	expect := List{
+		OrderBy: []string{"1", "2", "3"},
+		Filter:  []string{"a", "b", "c"},
+		Offset:  200,
+		Limit:   100,
+	}
+
+	got := ListAll.WithFilters("a", "b", "c").WithOrderBy("1", "2", "3").WithOffsetLimit(200, 100)
+
+	if diff := cmp.Diff(expect, got); diff != "" {
+		t.Errorf("result mismatch (-want +got):\n%s", diff)
+	}
+}
+
+func TestParamsValidate(t *testing.T) {
+	good := []List{
+		{Limit: -1, Offset: 0},
+	}
+
+	for i, c := range good {
+		t.Run(fmt.Sprintf("good_case_%d", i), func(t *testing.T) {
+			if err := c.Validate(); err != nil {
+				t.Errorf("unexpected error: %s", err)
+			}
+		})
+	}
+
+	bad := []List{
+		{Offset: -1},
+		{Limit: -2},
+	}
+
+	for i, c := range bad {
+		t.Run(fmt.Sprintf("bad_case_%d", i), func(t *testing.T) {
+			if err := c.Validate(); err == nil {
+				t.Errorf("expected error, got none")
+			}
+		})
+	}
+}
+
+func TestListParamsAll(t *testing.T) {
+	cases := []struct {
+		p      List
+		expect bool
+	}{
+		{List{Limit: -1, Offset: 0}, true},
+		{List{OrderBy: []string{"date"}, Limit: -1, Offset: 0}, true},
+		{List{Filter: []string{"user:noodles"}, Limit: -1, Offset: 0}, true},
+
+		{List{Limit: 0, Offset: 0}, false},
+		{List{Limit: -1, Offset: 100000}, false},
+		{List{OrderBy: []string{"time"}, Limit: 0, Offset: 0}, false},
+	}
+
+	for i, c := range cases {
+		got := c.p.All()
+		if c.expect != got {
+			t.Errorf("case %d result mismatch. want: %t got: %t", i, c.expect, got)
+		}
+	}
+}