lorax-composer: Return UnknownBlueprint errors when using deleted blueprints

Reading a blueprint wasn't checking to see if it had been deleted so it
was returning the most recent commit before it had been deleted. This
allowed things like starting a compose with a blueprint that technically
doesn't exist.

One exception to this is the /changes/ route, it must be available so
that you can use the commit hash to undo a delete.

This also adds tests for the various operations.

(cherry picked from commit d32f477e0b)

Resolves: rhbz#1683442

src/pylorax/api/recipes.py

@@ -408,6 +408,9 @@ def read_recipe_commit(repo, branch, recipe_name, commit=None):
     If no commit is passed the master:filename is returned, otherwise it will be
     commit:filename
     """
+    if not repo_file_exists(repo, branch, recipe_filename(recipe_name)):
+        raise RecipeFileError("Unknown blueprint")
+
     (_, recipe_toml) = read_commit(repo, branch, recipe_filename(recipe_name), commit)
     return recipe_from_toml(recipe_toml)
 
@@ -631,6 +634,9 @@ def tag_recipe_commit(repo, branch, recipe_name):
 
     Uses tag_file_commit()
     """
+    if not repo_file_exists(repo, branch, recipe_filename(recipe_name)):
+        raise RecipeFileError("Unknown blueprint")
+
     return tag_file_commit(repo, branch, recipe_filename(recipe_name))
 
 def tag_file_commit(repo, branch, filename):
@@ -920,3 +926,21 @@ def recipe_diff(old_recipe, new_recipe):
     diffs.extend(diff_items("Group", old_recipe["groups"], new_recipe["groups"]))
 
     return diffs
+
+def repo_file_exists(repo, branch, filename):
+    """Return True if the filename exists on the branch
+
+    :param repo: Open repository
+    :type repo: Git.Repository
+    :param branch: Branch name
+    :type branch: str
+    :param filename: Filename to check
+    :type filename: str
+    :returns: True if the filename exists on the HEAD of the branch, False otherwise.
+    :rtype: bool
+    """
+    commit = head_commit(repo, branch).get_id().to_string()
+    commit_id = Git.OId.new_from_string(commit)
+    commit_obj = repo.lookup(commit_id, Git.Commit)
+    tree = commit_obj.get_tree()
+    return tree.get_by_name(filename) is not None

src/pylorax/api/v0.py

@@ -1001,7 +1001,7 @@ from pylorax.api.queue import queue_status, build_status, uuid_delete, uuid_stat
 from pylorax.api.queue import uuid_tar, uuid_image, uuid_cancel, uuid_log
 from pylorax.api.recipes import RecipeError, list_branch_files, read_recipe_commit, recipe_filename, list_commits
 from pylorax.api.recipes import recipe_from_dict, recipe_from_toml, commit_recipe, delete_recipe, revert_recipe
-from pylorax.api.recipes import tag_recipe_commit, recipe_diff
+from pylorax.api.recipes import tag_recipe_commit, recipe_diff, RecipeFileError
 from pylorax.api.regexes import VALID_API_STRING
 from pylorax.api.workspace import workspace_read, workspace_write, workspace_delete
 from pylorax.api.yumbase import update_metadata
@@ -1023,12 +1023,21 @@ def take_limits(iterable, offset, limit):
     return iterable[offset:][:limit]
 
 def blueprint_exists(api, branch, blueprint_name):
+    """Return True if the blueprint exists
+
+    :param api: flask object
+    :type api: Flask
+    :param branch: Branch name
+    :type branch: str
+    :param recipe_name: Recipe name to read
+    :type recipe_name: str
+    """
     try:
         with api.config["GITLOCK"].lock:
             read_recipe_commit(api.config["GITLOCK"].repo, branch, blueprint_name)
 
         return True
-    except RecipeError:
+    except (RecipeError, RecipeFileError):
         return False
 
 def v0_api(api):
@@ -1087,6 +1096,10 @@ def v0_api(api):
             try:
                 with api.config["GITLOCK"].lock:
                     git_blueprint = read_recipe_commit(api.config["GITLOCK"].repo, branch, blueprint_name)
+            except RecipeFileError as e:
+                # Adding an exception would be redundant, skip it
+                git_blueprint = None
+                log.error("(v0_blueprints_info) %s", str(e))
             except Exception as e:
                 git_blueprint = None
                 exceptions.append(str(e))
@@ -1145,20 +1158,19 @@ def v0_api(api):
         errors = []
         for blueprint_name in [n.strip() for n in blueprint_names.split(",")]:
             filename = recipe_filename(blueprint_name)
-
-            if not blueprint_exists(api, branch, blueprint_name):
-                errors.append({"id": UNKNOWN_BLUEPRINT, "msg": "Unknown blueprint name: %s" % blueprint_name})
-                continue
-
             try:
                 with api.config["GITLOCK"].lock:
                     commits = list_commits(api.config["GITLOCK"].repo, branch, filename)
-                    limited_commits = take_limits(list_commits(api.config["GITLOCK"].repo, branch, filename), offset, limit)
             except Exception as e:
                 errors.append({"id": BLUEPRINTS_ERROR, "msg": "%s: %s" % (blueprint_name, str(e))})
                 log.error("(v0_blueprints_changes) %s", str(e))
             else:
+                if commits:
+                    limited_commits = take_limits(commits, offset, limit)
                     blueprints.append({"name":blueprint_name, "changes":limited_commits, "total":len(commits)})
+                else:
+                    # no commits means there is no blueprint in the branch
+                    errors.append({"id": UNKNOWN_BLUEPRINT, "msg": "%s" % blueprint_name})
 
         blueprints = sorted(blueprints, key=lambda r: r["name"].lower())
 
@@ -1307,6 +1319,9 @@ def v0_api(api):
         try:
             with api.config["GITLOCK"].lock:
                 tag_recipe_commit(api.config["GITLOCK"].repo, branch, blueprint_name)
+        except RecipeFileError as e:
+            log.error("(v0_blueprints_tag) %s", str(e))
+            return jsonify(status=False, errors=[{"id": UNKNOWN_BLUEPRINT, "msg": str(e)}]), 400
         except Exception as e:
             log.error("(v0_blueprints_tag) %s", str(e))
             return jsonify(status=False, errors=[{"id": BLUEPRINTS_ERROR, "msg": str(e)}]), 400
@@ -1331,6 +1346,9 @@ def v0_api(api):
         if VALID_API_STRING.match(branch) is None:
             return jsonify(status=False, errors=[{"id": INVALID_CHARS, "msg": "Invalid characters in branch argument"}]), 400
 
+        if not blueprint_exists(api, branch, blueprint_name):
+            return jsonify(status=False, errors=[{"id": UNKNOWN_BLUEPRINT, "msg": "Unknown blueprint name: %s" % blueprint_name}])
+
         try:
             if from_commit == "NEWEST":
                 with api.config["GITLOCK"].lock:
@@ -1397,6 +1415,9 @@ def v0_api(api):
                 try:
                     with api.config["GITLOCK"].lock:
                         blueprint = read_recipe_commit(api.config["GITLOCK"].repo, branch, blueprint_name)
+                except RecipeFileError as e:
+                    # adding an error here would be redundant, skip it
+                    log.error("(v0_blueprints_freeze) %s", str(e))
                 except Exception as e:
                     errors.append({"id": BLUEPRINTS_ERROR, "msg": "%s: %s" % (blueprint_name, str(e))})
                     log.error("(v0_blueprints_freeze) %s", str(e))
@@ -1457,6 +1478,9 @@ def v0_api(api):
                 try:
                     with api.config["GITLOCK"].lock:
                         blueprint = read_recipe_commit(api.config["GITLOCK"].repo, branch, blueprint_name)
+                except RecipeFileError as e:
+                    # adding an error here would be redundant, skip it
+                    log.error("(v0_blueprints_depsolve) %s", str(e))
                 except Exception as e:
                     errors.append({"id": BLUEPRINTS_ERROR, "msg": "%s: %s" % (blueprint_name, str(e))})
                     log.error("(v0_blueprints_depsolve) %s", str(e))
@@ -1795,6 +1819,9 @@ def v0_api(api):
         if VALID_API_STRING.match(blueprint_name) is None:
             errors.append({"id": INVALID_CHARS, "msg": "Invalid characters in API path"})
 
+        if not blueprint_exists(api, branch, blueprint_name):
+            errors.append({"id": UNKNOWN_BLUEPRINT, "msg": "Unknown blueprint name: %s" % blueprint_name})
+
         if errors:
             return jsonify(status=False, errors=errors), 400
 

tests/pylorax/test_server.py

@@ -199,13 +199,13 @@ class ServerTestCase(unittest.TestCase):
         data = json.loads(resp.data)
         self.assertEqual(data, info_dict_2)
 
-        info_dict_3 = {"changes":[],
+    def test_03_blueprints_info_none(self):
-                       "errors":[{"id": UNKNOWN_BLUEPRINT, "msg": "missing-blueprint: No commits for missing-blueprint.toml on the master branch."}],
+        """Test the /api/v0/blueprints/info route with an unknown blueprint"""
-                       "blueprints":[]
-                      }
         resp = self.server.get("/api/v0/blueprints/info/missing-blueprint")
         data = json.loads(resp.data)
-        self.assertEqual(data, info_dict_3)
+        self.assertNotEqual(data, None)
+        self.assertTrue(len(data["errors"]) > 0)
+        self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint")
 
     def test_04_blueprints_changes(self):
         """Test the /api/v0/blueprints/changes route"""
@@ -396,6 +396,21 @@ class ServerTestCase(unittest.TestCase):
         # Make sure the workspace file is gone
         self.assertEqual(os.path.exists(joinpaths(self.repo_dir, "git/workspace/master/example-glusterfs.toml")), False)
 
+    # This has to run after the above test
+    def test_10_blueprints_delete_2(self):
+        """Test running a compose with the deleted blueprint"""
+        # Trying to start a compose with a deleted blueprint should fail
+        test_compose = {"blueprint_name": "example-glusterfs",
+                        "compose_type": "tar",
+                        "branch": "master"}
+
+        resp = self.server.post("/api/v0/compose?test=2",
+                                data=json.dumps(test_compose),
+                                content_type="application/json")
+        data = json.loads(resp.data)
+        self.assertNotEqual(data, None)
+        self.assertEqual(data["status"], False, "Compose of deleted blueprint did not fail: %s" % data)
+
     def test_11_blueprints_undo(self):
         """Test POST /api/v0/blueprints/undo/<blueprint_name>/<commit>"""
         resp = self.server.get("/api/v0/blueprints/changes/example-glusterfs")
@@ -1387,6 +1402,71 @@ class ServerTestCase(unittest.TestCase):
         resp = self.server.get("/api/v0/compose/log/" + UTF8_TEST_STRING)
         self.assertInputError(resp)
 
+    # A series of tests for dealing with deleted blueprints
+    def test_deleted_bp_00_setup(self):
+        """Setup a deleted blueprint for use in the tests"""
+        # Start by creating a new blueprint for this series of tests and then
+        # deleting it.
+        test_blueprint = {"description": "A blueprint that has been deleted",
+                       "name":"deleted-blueprint",
+                       "version": "0.0.1",
+                       "modules":[{"name":"glusterfs", "version":"5.*"},
+                                  {"name":"glusterfs-cli", "version":"5.*"}],
+                       "packages":[{"name":"samba", "version":"4.*.*"},
+                                   {"name":"tmux", "version":"2.8"}],
+                       "groups": []}
+
+        resp = self.server.post("/api/v0/blueprints/new",
+                                data=json.dumps(test_blueprint),
+                                content_type="application/json")
+        data = json.loads(resp.data)
+        self.assertEqual(data, {"status":True})
+
+        resp = self.server.delete("/api/v0/blueprints/delete/deleted-blueprint")
+        data = json.loads(resp.data)
+        self.assertEqual(data, {"status":True})
+
+    def test_deleted_bp_01_show(self):
+        """Test blueprint show with deleted blueprint"""
+        resp = self.server.get("/api/v0/blueprints/info/deleted-blueprint")
+        data = json.loads(resp.data)
+        self.assertNotEqual(data, None)
+        self.assertTrue(len(data["errors"]) > 0)
+        self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint")
+
+    def test_deleted_bp_02_depsolve(self):
+        """Test blueprint depsolve with deleted blueprint"""
+        resp = self.server.get("/api/v0/blueprints/depsolve/deleted-blueprint")
+        data = json.loads(resp.data)
+        self.assertNotEqual(data, None)
+        self.assertTrue(len(data["errors"]) > 0)
+        self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint")
+
+    def test_deleted_bp_03_diff(self):
+        """Test blueprint diff with deleted blueprint"""
+        resp = self.server.get("/api/v0/blueprints/diff/deleted-blueprint/NEWEST/WORKSPACE")
+        data = json.loads(resp.data)
+        self.assertNotEqual(data, None)
+        self.assertTrue(len(data["errors"]) > 0)
+        self.assertEqual(data["status"], False)
+        self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint")
+
+    def test_deleted_bp_04_freeze(self):
+        """Test blueprint freeze with deleted blueprint"""
+        resp = self.server.get("/api/v0/blueprints/freeze/deleted-blueprint")
+        data = json.loads(resp.data)
+        self.assertNotEqual(data, None)
+        self.assertTrue(len(data["errors"]) > 0)
+        self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint")
+
+    def test_deleted_bp_05_tag(self):
+        """Test blueprint tag with deleted blueprint"""
+        resp = self.server.post("/api/v0/blueprints/tag/deleted-blueprint")
+        data = json.loads(resp.data)
+        self.assertNotEqual(data, None)
+        self.assertTrue(len(data["errors"]) > 0)
+        self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint")
+
 @contextmanager
 def in_tempdir(prefix='tmp'):
     """Execute a block of code with chdir in a temporary location"""