Fix for snapshot creation APIs: locking, package existence checks, consistency checks. #168

More system tests.

api/snapshot.go

@@ -3,6 +3,7 @@ package api
 import (
 	"fmt"
 	"github.com/gin-gonic/gin"
+	"github.com/smira/aptly/database"
 	"github.com/smira/aptly/deb"
 )
 
@@ -45,8 +46,8 @@ func apiSnapshotsCreateFromMirror(c *gin.Context) {
 	}
 
 	collection := context.CollectionFactory().RemoteRepoCollection()
-	collection.Lock()
+	collection.RLock()
-	defer collection.Unlock()
+	defer collection.RUnlock()
 
 	snapshotCollection := context.CollectionFactory().SnapshotCollection()
 	snapshotCollection.Lock()
@@ -82,7 +83,7 @@ func apiSnapshotsCreateFromMirror(c *gin.Context) {
 
 	err = snapshotCollection.Add(snapshot)
 	if err != nil {
-		c.Fail(500, err)
+		c.Fail(400, err)
 		return
 	}
 
@@ -97,10 +98,10 @@ func apiSnapshotsCreate(c *gin.Context) {
 	)
 
 	var b struct {
-		Name        string `binding:"required"`
+		Name            string `binding:"required"`
-		Description string
+		Description     string
-		SourceIDs   []string
+		SourceSnapshots []string
-		PackageRefs []string
+		PackageRefs     []string
 	}
 
 	if !c.Bind(&b) {
@@ -108,7 +109,7 @@ func apiSnapshotsCreate(c *gin.Context) {
 	}
 
 	if b.Description == "" {
-		if len(b.SourceIDs)+len(b.PackageRefs) == 0 {
+		if len(b.SourceSnapshots)+len(b.PackageRefs) == 0 {
 			b.Description = "Created as empty"
 		}
 	}
@@ -117,10 +118,10 @@ func apiSnapshotsCreate(c *gin.Context) {
 	snapshotCollection.Lock()
 	defer snapshotCollection.Unlock()
 
-	sources := make([]*deb.Snapshot, len(b.SourceIDs))
+	sources := make([]*deb.Snapshot, len(b.SourceSnapshots))
 
-	for i := 0; i < len(b.SourceIDs); i++ {
+	for i := range b.SourceSnapshots {
-		sources[i], err = snapshotCollection.ByUUID(b.SourceIDs[i])
+		sources[i], err = snapshotCollection.ByName(b.SourceSnapshots[i])
 		if err != nil {
 			c.Fail(404, err)
 			return
@@ -133,17 +134,33 @@ func apiSnapshotsCreate(c *gin.Context) {
 		}
 	}
 
-	packageRefs := make([][]byte, len(b.PackageRefs))
+	list := deb.NewPackageList()
-	for i, ref := range b.PackageRefs {
+
-		packageRefs[i] = []byte(ref)
+	// verify package refs and build package list
+	for _, ref := range b.PackageRefs {
+		var p *deb.Package
+
+		p, err = context.CollectionFactory().PackageCollection().ByKey([]byte(ref))
+		if err != nil {
+			if err == database.ErrNotFound {
+				c.Fail(404, fmt.Errorf("package %s: %s", ref, err))
+			} else {
+				c.Fail(500, err)
+			}
+			return
+		}
+		err = list.Add(p)
+		if err != nil {
+			c.Fail(400, err)
+			return
+		}
 	}
 
-	packageRefList := &deb.PackageRefList{packageRefs}
+	snapshot = deb.NewSnapshotFromRefList(b.Name, sources, deb.NewPackageRefListFromPackageList(list), b.Description)
-	snapshot = deb.NewSnapshotFromRefList(b.Name, sources, packageRefList, b.Description)
 
 	err = snapshotCollection.Add(snapshot)
 	if err != nil {
-		c.Fail(500, err)
+		c.Fail(400, err)
 		return
 	}
 

system/t12_api/snapshots.py

@@ -1,7 +1,7 @@
 from api_lib import APITest
 
 
-class SnapshotsAPITestCreateShow(APITest):
+class SnapshotsAPITestCreateShowEmpty(APITest):
     """
     GET /api/snapshots/:name, POST /api/snapshots, GET /api/snapshots/:name/packages
     """
@@ -10,6 +10,7 @@ class SnapshotsAPITestCreateShow(APITest):
         snapshot_desc = {u'Description': u'fun snapshot',
                          u'Name': snapshot_name}
 
+        # create empty snapshot
         resp = self.post("/api/snapshots", json=snapshot_desc)
         self.check_subset(snapshot_desc, resp.json())
         self.check_equal(resp.status_code, 201)
@@ -17,8 +18,16 @@ class SnapshotsAPITestCreateShow(APITest):
         self.check_subset(snapshot_desc, self.get("/api/snapshots/" + snapshot_name).json())
         self.check_equal(self.get("/api/snapshots/" + snapshot_name).status_code, 200)
 
+        resp = self.get("/api/snapshots/" + snapshot_name + "/packages")
+        self.check_equal(resp.status_code, 200)
+        self.check_equal(resp.json(), [])
+
         self.check_equal(self.get("/api/snapshots/" + self.random_name()).status_code, 404)
 
+        # create snapshot with duplicate name
+        resp = self.post("/api/snapshots", json=snapshot_desc)
+        self.check_equal(resp.status_code, 400)
+
 
 class SnapshotsAPITestCreateFromRefs(APITest):
     """
@@ -28,25 +37,46 @@ class SnapshotsAPITestCreateFromRefs(APITest):
         snapshot_name = self.random_name()
         snapshot_desc = {u'Description': u'fun snapshot',
                          u'Name': snapshot_name,
-                         u'SourceIDs': ['123']}
+                         u'SourceSnapshots': [self.random_name()]}
 
+        # creating snapshot from missing source snapshot
         resp = self.post("/api/snapshots", json=snapshot_desc)
         self.check_equal(resp.status_code, 404)
 
-        resp = self.post("/api/snapshots", json={"Name": self.random_name()})
+        # create empty snapshot
+        empty_snapshot_name = self.random_name()
+        resp = self.post("/api/snapshots", json={"Name": empty_snapshot_name})
         self.check_equal(resp.status_code, 201)
-        snapshot_desc['SourceIDs'] = [resp.json()["UUID"]]
+        self.check_equal(resp.json()['Description'], 'Created as empty')
 
+        # create and upload package to repo to register package in DB
+        repo_name = self.random_name()
+        self.check_equal(self.post("/api/repos", json={"Name": repo_name}).status_code, 201)
+        d = self.random_name()
+        self.check_equal(self.upload("/api/files/" + d,
+                         "libboost-program-options-dev_1.49.0.1_i386.deb").status_code, 200)
+        self.check_equal(self.post("/api/repos/" + repo_name + "/file/" + d).status_code, 200)
+
+        # create snapshot with empty snapshot as source and package
         snapshot = snapshot_desc.copy()
         snapshot['PackageRefs'] = ["Pi386 libboost-program-options-dev 1.49.0.1 918d2f433384e378"]
+        snapshot['SourceSnapshots'] = [empty_snapshot_name]
         resp = self.post("/api/snapshots", json=snapshot)
         self.check_equal(resp.status_code, 201)
-        self.check_subset(snapshot_desc, resp.json())
+        snapshot.pop('SourceSnapshots')
+        snapshot.pop('PackageRefs')
+        self.check_subset(snapshot, resp.json())
 
-        self.check_subset(snapshot_desc, self.get("/api/snapshots/" + snapshot_name).json())
+        self.check_subset(snapshot, self.get("/api/snapshots/" + snapshot_name).json())
-        self.check_equal(self.get("/api/snapshots/" + snapshot_name).status_code, 200)
+        resp = self.get("/api/snapshots/" + snapshot_name + "/packages")
+        self.check_equal(resp.status_code, 200)
+        self.check_equal(resp.json(), ["Pi386 libboost-program-options-dev 1.49.0.1 918d2f433384e378"])
 
-        self.check_equal(self.get("/api/snapshots/" + self.random_name()).status_code, 404)
+        # create snapshot with unreferenced package
+        resp = self.post("/api/snapshots", json={
+            "Name": self.random_name(),
+            "PackageRefs": ["Pi386 libboost-program-options-dev 1.49.0.1 918d2f433384e378", "Pamd64 no-such-package 1.2 91"]})
+        self.check_equal(resp.status_code, 404)
 
 
 class SnapshotsAPITestCreateFromRepo(APITest):