From 47e60d8e5e288eb10c6fb956fb70a87820e5793f Mon Sep 17 00:00:00 2001
From: Dan Callaghan <dcallagh@redhat.com>
Date: Jul 14 2017 05:37:33 +0000
Subject: replace Answer types with exceptions


---

diff --git a/greenwave/api_v1.py b/greenwave/api_v1.py
index f04d342..3ffc90f 100644
--- a/greenwave/api_v1.py
+++ b/greenwave/api_v1.py
@@ -3,7 +3,7 @@
 import requests
 from flask import Blueprint, request, current_app, jsonify
 from werkzeug.exceptions import BadRequest, NotFound, UnsupportedMediaType
-from greenwave.policies import summarize_answers
+from greenwave.policies import summarize
 
 api = (Blueprint('api_v1', __name__))
 
@@ -45,7 +45,7 @@ def make_decision():
     if not applicable_policies:
         raise NotFound('Cannot find any applicable policies for %s' % product_version)
     subjects = [item.strip() for item in request.get_json()['subject'] if item]
-    answers = []
+    unsatisfied_requirements = []
     timeout = current_app.config['REQUESTS_TIMEOUT']
     for item in subjects:
         # XXX make this more efficient than just fetching everything
@@ -65,12 +65,12 @@ def make_decision():
         else:
             waivers = []
         for policy in applicable_policies:
-            answers.extend(policy.check(item, results, waivers))
+            unsatisfied_requirements.extend(policy.check(item, results, waivers))
     res = {
-        'policies_satisified': all(answer.is_satisfied for answer in answers),
-        'summary': summarize_answers(answers),
+        'policies_satisified': len(unsatisfied_requirements) == 0,
+        'summary': summarize(subjects, applicable_policies, unsatisfied_requirements),
         'applicable_policies': [policy.id for policy in applicable_policies],
-        'unsatisfied_requirements': [answer.to_json() for answer in answers
-                                     if not answer.is_satisfied],
+        'unsatisfied_requirements': [unsatisfied_requirement.to_json()
+                                     for unsatisfied_requirement in unsatisfied_requirements],
     }
     return jsonify(res), 200
diff --git a/greenwave/policies.py b/greenwave/policies.py
index 129ee12..e96b870 100644
--- a/greenwave/policies.py
+++ b/greenwave/policies.py
@@ -3,37 +3,14 @@
 import yaml
 
 
-class Answer(object):
-    """
-    Represents the result of evaluating a policy rule against a particular
-    item. But we call it an "answer" because the word "result" is a bit
-    overloaded in here. :-)
-
-    This base class is not used directly -- each answer is an instance of
-    a subclass, depending on what the answer was.
-    """
-
-    pass
-
-
-class RuleSatisfied(Answer):
-    """
-    The rule's requirements are satisfied for this item.
-    """
-
-    is_satisfied = True
-
-
-class RuleNotSatisfied(Answer):
+class RuleNotSatisfied(Exception):
     """
     The rule's requirements are not satisfied for this item.
 
-    Not used directly -- the answer is an instance of a subclass, specifying
-    exactly what was not satisfied.
+    Not raised directly -- one of the more specific subclasses will be raised
+    instead, specifying exactly what was not satisfied.
     """
 
-    is_satisfied = False
-
     def to_json(self):
         """
         Returns a machine-readable description of the problem for API responses.
@@ -79,36 +56,40 @@ class TestResultFailed(RuleNotSatisfied):
         }
 
 
-def summarize_answers(answers):
+def summarize(subjects, applicable_policies, unsatisfied_requirements):
     """
     Produces a one-sentence human-readable summary of the result of evaluating a policy.
 
     Args:
-        answers (list): List of :py:class:`Answers <Answer>` from evaluating a policy.
+        subjects (list): List of items we are evaluating the policies for.
+        applicable_policies (list): List of policies which were evaluated.
+        unsatisfied_requirements (list): List of :py:class:`RuleNotSatisfied` exception
+                                         instances from evaluating a policy.
 
     Returns:
         str: Human-readable summary.
     """
-    if len(answers) == 0:
+    rules_count = len(subjects) * sum(len(policy.rules) for policy in applicable_policies)
+    if rules_count == 0:
         return 'no tests are required'
-    if all(answer.is_satisfied for answer in answers):
+    if not unsatisfied_requirements:
         return 'all required tests passed'
-    failure_count = len([answer for answer in answers if isinstance(answer, TestResultFailed)])
+    failure_count = len([e for e in unsatisfied_requirements if isinstance(e, TestResultFailed)])
     if failure_count:
-        return ('{} of {} required tests failed'.format(failure_count, len(answers)))
-    missing_count = len([answer for answer in answers if isinstance(answer, TestResultMissing)])
-    if missing_count == len(answers):
+        return ('{} of {} required tests failed'.format(failure_count, rules_count))
+    missing_count = len([e for e in unsatisfied_requirements if isinstance(e, TestResultMissing)])
+    if missing_count == rules_count:
         return 'no test results found'
     elif missing_count:
-        return '{} of {} required tests not found'.format(missing_count, len(answers))
+        return '{} of {} required tests not found'.format(missing_count, rules_count)
     return 'inexplicable result'
 
 
 class Rule(yaml.YAMLObject):
     """
     An individual rule within a policy. A policy consists of multiple rules.
-    When the policy is evaluated, each rule returns an answer
-    (instance of :py:class:`Answer`).
+    When the policy is evaluated, each rule raises an instance of
+    :py:class:`RuleNotSatisfied` if it is not satisfied.
 
     This base class is not used directly.
     """
@@ -121,9 +102,6 @@ class Rule(yaml.YAMLObject):
                         for example a build NVR).
             results (list): List of result objects looked up in ResultsDB for this item.
             waivers (list): List of waiver objects looked up in WaiverDB for the results.
-
-        Returns:
-            Answer: An instance of a subclass of :py:class:`Answer` describing the result.
         """
         raise NotImplementedError()
 
@@ -141,15 +119,15 @@ class PassingTestCaseRule(Rule):
     def check(self, item, results, waivers):
         matching_results = [r for r in results if r['testcase']['name'] == self.test_case_name]
         if not matching_results:
-            return TestResultMissing(item, self.test_case_name)
+            raise TestResultMissing(item, self.test_case_name)
         # XXX need to handle multiple results (take the latest)
         matching_result = matching_results[0]
         if matching_result['outcome'] in ['PASSED', 'INFO']:
-            return RuleSatisfied()
+            return
         # XXX limit who is allowed to waive
         if any(w['result_id'] == matching_result['id'] and w['waived'] for w in waivers):
-            return RuleSatisfied()
-        return TestResultFailed(item, self.test_case_name, matching_result['id'])
+            return
+        raise TestResultFailed(item, self.test_case_name, matching_result['id'])
 
     def __repr__(self):
         return "%s(test_case_name=%r)" % (self.__class__.__name__, self.test_case_name)
@@ -169,7 +147,26 @@ class Policy(yaml.YAMLObject):
                 product_version in self.product_versions)
 
     def check(self, item, results, waivers):
-        return [rule.check(item, results, waivers) for rule in self.rules]
+        """
+        Evaluate this policy for the given item.
+
+        Args:
+            item (str): The item we are evaluating ('item' key in ResultsDB,
+                        for example a build NVR).
+            results (list): List of result objects looked up in ResultsDB for this item.
+            waivers (list): List of waiver objects looked up in WaiverDB for the results.
+
+        Returns:
+            list: list of :py:class:`RuleNotSatisfied` instances raised while evaluating the
+                  rules in this policy.
+        """
+        result = []
+        for rule in self.rules:
+            try:
+                rule.check(item, results, waivers)
+            except RuleNotSatisfied as e:
+                result.append(e)
+        return result
 
     def __repr__(self):
         return "%s(id=%r, product_versions=%r, decision_context=%r, rules=%r)" % (
diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py
index afafbbd..a1a130f 100644
--- a/greenwave/tests/test_policies.py
+++ b/greenwave/tests/test_policies.py
@@ -2,20 +2,26 @@
 # SPDX-License-Identifier: GPL-2.0+
 
 from greenwave.app_factory import create_app
-from greenwave.policies import summarize_answers, RuleSatisfied, TestResultMissing, TestResultFailed
+from greenwave.policies import summarize, Policy, PassingTestCaseRule, \
+        TestResultMissing, TestResultFailed
 
 
-def test_summarize_answers():
-    assert summarize_answers([RuleSatisfied()]) == \
+def test_summarize():
+    subjects = ['item']
+    policy = Policy('', [], '', [PassingTestCaseRule('test'), PassingTestCaseRule('test2')])
+    assert summarize(subjects, [policy], []) == \
         'all required tests passed'
-    assert summarize_answers([TestResultFailed('item', 'test', 'id'), RuleSatisfied()]) == \
+    assert summarize(subjects, [policy], [TestResultFailed('item', 'test', 'id')]) == \
         '1 of 2 required tests failed'
-    assert summarize_answers([TestResultMissing('item', 'test')]) == \
+    assert summarize(subjects, [policy],
+                     [TestResultMissing('item', 'test'),
+                      TestResultMissing('item', 'test2')]) == \
         'no test results found'
-    assert summarize_answers([TestResultMissing('item', 'test'),
-                              TestResultFailed('item', 'test', 'id')]) == \
+    assert summarize(subjects, [policy],
+                             [TestResultMissing('item', 'test'),
+                              TestResultFailed('item', 'test2', 'id')]) == \
         '1 of 2 required tests failed'
-    assert summarize_answers([TestResultMissing('item', 'test'), RuleSatisfied()]) == \
+    assert summarize(subjects, [policy], [TestResultMissing('item', 'test')]) == \
         '1 of 2 required tests not found'
 
 

