Refactoring: make snapshot sorting non-intrusive to collection contents. #168

api/snapshot.go

@@ -2,10 +2,10 @@ package api
 
 import (
 	"fmt"
-	"sort"
 	"github.com/gin-gonic/gin"
 	"github.com/smira/aptly/deb"
 	"github.com/smira/aptly/query"
+	"sort"
 )
 
 // GET /api/snapshots
@@ -16,12 +16,12 @@ func apiSnapshotsList(c *gin.Context) {
 	collection.RLock()
 	defer collection.RUnlock()
 
-	if SortMethodString != "" {
-		collection.Sort(SortMethodString)
+	if SortMethodString == "" {
+		SortMethodString = "name"
 	}
 
 	result := []*deb.Snapshot{}
-	collection.ForEach(func(snapshot *deb.Snapshot) error {
+	collection.ForEachSorted(SortMethodString, func(snapshot *deb.Snapshot) error {
 		result = append(result, snapshot)
 		return nil
 	})
@@ -32,14 +32,14 @@ func apiSnapshotsList(c *gin.Context) {
 // POST /api/mirrors/:name/snapshots/
 func apiSnapshotsCreateFromMirror(c *gin.Context) {
 	var (
-		err       error
+		err      error
 		repo     *deb.RemoteRepo
 		snapshot *deb.Snapshot
 	)
 
 	var b struct {
-		Name                string `binding:"required"`
-		Description         string
+		Name        string `binding:"required"`
+		Description string
 	}
 
 	if !c.Bind(&b) {
@@ -94,15 +94,15 @@ func apiSnapshotsCreateFromMirror(c *gin.Context) {
 // POST /api/snapshots
 func apiSnapshotsCreate(c *gin.Context) {
 	var (
-		err       error
+		err      error
 		snapshot *deb.Snapshot
 	)
 
 	var b struct {
-		Name                string `binding:"required"`
-		Description         string
-		SourceIDs           []string
-		PackageRefs         []string
+		Name        string `binding:"required"`
+		Description string
+		SourceIDs   []string
+		PackageRefs []string
 	}
 
 	if !c.Bind(&b) {
@@ -110,7 +110,7 @@ func apiSnapshotsCreate(c *gin.Context) {
 	}
 
 	if b.Description == "" {
-		if len(b.SourceIDs) + len(b.PackageRefs) == 0 {
+		if len(b.SourceIDs)+len(b.PackageRefs) == 0 {
 			b.Description = "Created as empty"
 		}
 	}
@@ -119,23 +119,23 @@ func apiSnapshotsCreate(c *gin.Context) {
 	snapshotCollection.Lock()
 	defer snapshotCollection.Unlock()
 
-    sources := make([]*deb.Snapshot, len(b.SourceIDs))
+	sources := make([]*deb.Snapshot, len(b.SourceIDs))
 
-    for i := 0; i < len(b.SourceIDs); i++ {
-        sources[i], err = snapshotCollection.ByUUID(b.SourceIDs[i])
-        if err != nil {
+	for i := 0; i < len(b.SourceIDs); i++ {
+		sources[i], err = snapshotCollection.ByUUID(b.SourceIDs[i])
+		if err != nil {
 			c.Fail(404, err)
 			return
-        }
+		}
 
-        err = snapshotCollection.LoadComplete(sources[i])
-        if err != nil {
+		err = snapshotCollection.LoadComplete(sources[i])
+		if err != nil {
 			c.Fail(500, err)
 			return
-        }
-    }
+		}
+	}
 
-    packageRefs := make([][]byte, len(b.PackageRefs))
+	packageRefs := make([][]byte, len(b.PackageRefs))
 	for i, ref := range b.PackageRefs {
 		packageRefs[i] = []byte(ref)
 	}
@@ -155,14 +155,14 @@ func apiSnapshotsCreate(c *gin.Context) {
 // POST /api/repos/:name/snapshots/:snapname
 func apiSnapshotsCreateFromRepository(c *gin.Context) {
 	var (
-		err       error
+		err      error
 		repo     *deb.LocalRepo
 		snapshot *deb.Snapshot
 	)
 
 	var b struct {
-		Name                string `binding:"required"`
-		Description         string
+		Name        string `binding:"required"`
+		Description string
 	}
 
 	if !c.Bind(&b) {

cmd/snapshot_list.go

@@ -17,10 +17,9 @@ func aptlySnapshotList(cmd *commander.Command, args []string) error {
 	sortMethodString := cmd.Flag.Lookup("sort").Value.Get().(string)
 
 	collection := context.CollectionFactory().SnapshotCollection()
-	collection.Sort(sortMethodString)
 
 	if raw {
-		collection.ForEach(func(snapshot *deb.Snapshot) error {
+		collection.ForEachSorted(sortMethodString, func(snapshot *deb.Snapshot) error {
 			fmt.Printf("%s\n", snapshot.Name)
 			return nil
 		})
@@ -28,11 +27,15 @@ func aptlySnapshotList(cmd *commander.Command, args []string) error {
 		if collection.Len() > 0 {
 			fmt.Printf("List of snapshots:\n")
 
-			collection.ForEach(func(snapshot *deb.Snapshot) error {
+			err = collection.ForEachSorted(sortMethodString, func(snapshot *deb.Snapshot) error {
 				fmt.Printf(" * %s\n", snapshot.String())
 				return nil
 			})
 
+			if err != nil {
+				return err
+			}
+
 			fmt.Printf("\nTo get more information about snapshot, run `aptly snapshot show <name>`.\n")
 		} else {
 			fmt.Printf("\nNo snapshots found, create one with `aptly snapshot create...`.\n")

deb/snapshot.go

@@ -297,6 +297,22 @@ func (collection *SnapshotCollection) ForEach(handler func(*Snapshot) error) err
 	return err
 }
 
+func (collection *SnapshotCollection) ForEachSorted(sortMethod string, handler func(*Snapshot) error) error {
+	sorter, err := newSnapshotSorter(sortMethod, collection)
+	if err != nil {
+		return err
+	}
+
+	for _, i := range sorter.list {
+		err = handler(collection.list[i])
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // Len returns number of snapshots in collection
 // ForEach runs method for each snapshot
 func (collection *SnapshotCollection) Len() int {
@@ -335,51 +351,48 @@ const (
 	SortTime
 )
 
-type snapshotListToSort struct {
-	list       []*Snapshot
+type snapshotSorter struct {
+	list       []int
+	collection *SnapshotCollection
 	sortMethod int
 }
 
-func parseSortMethod(sortMethod string) (int, error) {
+func newSnapshotSorter(sortMethod string, collection *SnapshotCollection) (*snapshotSorter, error) {
+	s := &snapshotSorter{collection: collection}
+
 	switch sortMethod {
 	case "time", "Time":
-		return SortTime, nil
+		s.sortMethod = SortTime
 	case "name", "Name":
-		return SortName, nil
+		s.sortMethod = SortName
+	default:
+		return nil, fmt.Errorf("sorting method \"%s\" unknown", sortMethod)
 	}
 
-	return -1, fmt.Errorf("sorting method \"%s\" unknown", sortMethod)
+	s.list = make([]int, len(collection.list))
+	for i := range s.list {
+		s.list[i] = i
+	}
+
+	sort.Sort(s)
+
+	return s, nil
 }
 
-func (s snapshotListToSort) Swap(i, j int) {
+func (s *snapshotSorter) Swap(i, j int) {
 	s.list[i], s.list[j] = s.list[j], s.list[i]
 }
 
-func (s snapshotListToSort) Less(i, j int) bool {
+func (s *snapshotSorter) Less(i, j int) bool {
 	switch s.sortMethod {
 	case SortName:
-		return s.list[i].Name < s.list[j].Name
+		return s.collection.list[s.list[i]].Name < s.collection.list[s.list[j]].Name
 	case SortTime:
-		return s.list[i].CreatedAt.Before(s.list[j].CreatedAt)
+		return s.collection.list[s.list[i]].CreatedAt.Before(s.collection.list[s.list[j]].CreatedAt)
 	}
 	panic("unknown sort method")
 }
 
-func (s snapshotListToSort) Len() int {
+func (s *snapshotSorter) Len() int {
 	return len(s.list)
 }
-
-func (collection *SnapshotCollection) Sort(sortMethodString string) error {
-	var err error
-	snapshotsToSort := &snapshotListToSort{}
-	snapshotsToSort.list = collection.list
-	snapshotsToSort.sortMethod, err = parseSortMethod(sortMethodString)
-	if err != nil {
-		return err
-	}
-
-	sort.Sort(snapshotsToSort)
-	collection.list = snapshotsToSort.list
-
-    return err
-}

system/t05_snapshot/ListSnapshot7Test_gold

@@ -1 +1,2 @@
+List of snapshots:
 ERROR: sorting method "planet" unknown

system/t05_snapshot/list.py

@@ -87,5 +87,8 @@ class ListSnapshot7Test(BaseTest):
     """
     list snapshots: wrong parameter sort
     """
+    fixtureCmds = [
+        "aptly snapshot create empty empty"
+    ]
     runCmd = "aptly -sort=planet snapshot list"
     expectedCode = 1