Fix how we cache check properties

Previously we did not save the cache of logically grouped
items because the semantics around doing that are a little
fuzzy but that meant that scenarios could not reference the
cache of a check that had grouped requirement types. This
patch enables the caching of grouped requirements on the basis
that a cache will be saved if (A) the result is True and
(B) it is the first to be saved in a group or list (C) it
the same has type as the previous one to be saved i.e.
'A AND (B or C)'.

hotsos/core/ycheck/engine/properties/common.py

@@ -590,17 +590,23 @@ def eval_ungrouped_item(self, item):
         """
         final_results = []
         for entry in item:
+            # For the purposes of caching we treat a list with > 1 item as
+            # "grouped".
             result = self.get_item_result_callback(entry,
-                                                   is_default_group=True)
+                                                   grouped=len(item) > 1)
             final_results.append(result)
 
         return final_results
 
     @abc.abstractmethod
-    def get_item_result_callback(self, item, is_default_group=False):
+    def get_item_result_callback(self, item, grouped=False):
         """
         Must be implemented to evaluate a single item and return its
         result.
+
+        @param item: property we want to evaluate
+        @param grouped: whether or not the item is part of a logical group or
+                        list.
         """
 
     def _is_op_group(self, prop):
@@ -635,7 +641,9 @@ def eval_op_group_items(self, logical_op_group):
 
                 log.debug("op group member has %s item(s)", len(member))
                 if len(member) == 1:
-                    group_results.append(self.get_item_result_callback(member))
+                    result = self.get_item_result_callback(member,
+                                                           grouped=True)
+                    group_results.append(result)
                     num_single += 1
                     continue
 
@@ -649,7 +657,7 @@ def eval_op_group_items(self, logical_op_group):
                                   result, logical_op)
                         break
 
-                    result = self.get_item_result_callback(entry)
+                    result = self.get_item_result_callback(entry, grouped=True)
                     group_results.append(result)
                     prev = result
                     num_list += 1

hotsos/core/ycheck/engine/properties/conclusions.py

@@ -121,7 +121,7 @@ def _override_mapped_member_types(cls):
     def add_checks_instances(self, checks):
         self.checks_instances = checks
 
-    def get_item_result_callback(self, item, is_default_group=False):
+    def get_item_result_callback(self, item, grouped=False):
         name = str(item)
         if name not in self.checks_instances:
             raise Exception("no check found with name {}".format(name))

hotsos/core/ycheck/engine/properties/requires/requires.py

@@ -18,6 +18,7 @@
 
 class YPropertyRequiresBase(YPropertyMappedOverrideBase,
                             LogicalCollectionHandler):
+    CACHE_CHECK_KEY = '__PREVIOUSLY_CACHED_PROPERTY_TYPE'
 
     @classmethod
     def _override_mapped_member_types(cls):
@@ -30,13 +31,42 @@ def _override_mapped_member_types(cls):
                 path.YRequirementTypePath,
                 varops.YPropertyVarOps]
 
-    def get_item_result_callback(self, item, is_default_group=False):
+    def get_item_result_callback(self, item, grouped=False):
+        """
+        Evaluate item and return its result.
+
+        Also copy the cache of the item into the local cache. If part of a
+        logical group whereby more than one item is being evaluated to get
+        a final results, we only copy the cache data if the result is True
+        and  either (a) it is the first to be saved or (b) it has the same type
+        as the previous one to be saved.
+
+        For example with the following group:
+
+        or:
+          - apt: foo
+          - apt: bar
+
+        If package foo exists and bar does not, the final contents of the group
+        will that from 'apt: foo'. If both exists then it will be the result of
+        'apt: bar'.
+
+        @param item: YRequirementType object
+        @param grouped: If True, item is part of a (logical) group of one or
+                        more items or a list with more than one item to which
+                        to default logical operator (AND) will be applied.
+        @return: boolean result i.e. True or False
+        """
         result = item()
-        # if its a single group we can cache the results but otherwise it wont
-        # make sense since all will conflict.
-        if is_default_group:
+        previously_cached = getattr(self.cache, self.CACHE_CHECK_KEY)
+        req_prop_type = item.__class__.__name__
+        if not grouped or (result and
+                           previously_cached in (None, req_prop_type)):
+            if previously_cached is None:
+                self.cache.set(self.CACHE_CHECK_KEY, req_prop_type)
+
             log.debug("merging item (type=%s) cache id=%s into cache id=%s",
-                      item.__class__.__name__, item.cache.id, self.cache.id)
+                      req_prop_type, item.cache.id, self.cache.id)
             self.cache.merge(item.cache)
 
         return result

hotsos/core/ycheck/engine/properties/requires/types/config.py

@@ -61,7 +61,7 @@ def and_group_stop_on_first_false(self):
         """
         return False
 
-    def get_item_result_callback(self, item, is_default_group=False):
+    def get_item_result_callback(self, item, grouped=False):
         handlers = self.context.assertions_ctxt['cfg_handlers']
         if not handlers:
             return

tests/unit/test_ycheck_properties.py

@@ -1,9 +1,14 @@
 import datetime
+import os
+import shutil
+import tempfile
 from unittest import mock
 
 import yaml
 from hotsos.core.config import HotSOSConfig
 from hotsos.core.host_helpers.config import SectionalConfigBase
+from hotsos.core.issues.utils import IssuesStore
+from hotsos.core.ycheck import scenarios
 from hotsos.core.ycheck.engine import (
     YDefsSection,
     YDefsLoader,
@@ -196,6 +201,209 @@ def __init__(self, name, state, has_instances, start_time):
 """  # noqa
 
 
+class TempScenarioDefs(object):
+
+    def __init__(self):
+        self.root = None
+        self.path = None
+
+    def __enter__(self):
+        self.root = tempfile.mkdtemp()
+        HotSOSConfig.plugin_yaml_defs = self.root
+        self.path = os.path.join(self.root, 'scenarios',
+                                 HotSOSConfig.plugin_name, 'test.yaml')
+        os.makedirs(os.path.dirname(self.path))
+        return self
+
+    def __exit__(self, *args):
+        shutil.rmtree(self.root)
+        return False
+
+
+class TestYamlRequiresTypeCache(utils.BaseTestCase):
+
+    @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': 'ii foo 123 amd64'})
+    def test_single_item(self):
+        mydef = YDefsSection('mydef', yaml.safe_load("chk1:\n  apt: foo"))
+        for entry in mydef.leaf_sections:
+            self.assertTrue(entry.requires.passes)
+            expected = {'__PREVIOUSLY_CACHED_PROPERTY_TYPE':
+                        'YRequirementTypeAPT',
+                        'package': 'foo',
+                        'passes': True,
+                        'version': '123'}
+            self.assertEqual(entry.requires.cache.data, expected)
+
+    @utils.create_data_root({'sos_commands/dpkg/dpkg_-l':
+                             'ii foo 123 amd64'})
+    def test_grouped_items_first_true(self):
+        """ If the first item evaluates to True we get that one. """
+        scenario = """
+        checks:
+          c1:
+            or:
+              - apt: foo
+              - apt: bar
+        conclusions:
+          c1:
+            decision: c1
+            raises:
+              type: SystemWarning
+              message: '{pkg}'
+              format-dict:
+                pkg: '@checks.c1.requires.package'
+        """  # noqa
+        with TempScenarioDefs() as tmpscenarios:
+            with open(tmpscenarios.path, 'w') as fd:
+                fd.write(scenario)
+
+            scenarios.YScenarioChecker().load_and_run()
+            issues = list(IssuesStore().load().values())[0]
+            self.assertEqual(issues[0]['message'], 'foo')
+
+    @utils.create_data_root({'sos_commands/dpkg/dpkg_-l':
+                             'ii bar 123 amd64'})
+    def test_grouped_items_last_true(self):
+        """ If the last item evaluates to True we get that one. """
+        scenario = """
+        checks:
+          c1:
+            or:
+              - apt: foo
+              - apt: bar
+        conclusions:
+          c1:
+            decision: c1
+            raises:
+              type: SystemWarning
+              message: '{pkg}'
+              format-dict:
+                pkg: '@checks.c1.requires.package'
+        """  # noqa
+        with TempScenarioDefs() as tmpscenarios:
+            with open(tmpscenarios.path, 'w') as fd:
+                fd.write(scenario)
+
+            scenarios.YScenarioChecker().load_and_run()
+            issues = list(IssuesStore().load().values())[0]
+            self.assertEqual(issues[0]['message'], 'bar')
+
+    @utils.create_data_root({'sos_commands/dpkg/dpkg_-l':
+                             'ii foo 123 amd64\nii bar 123 amd64'})
+    def test_grouped_items_all_true(self):
+        """ If all items evaluate to True we get the cache of the last one. """
+        scenario = """
+        checks:
+          c1:
+            or:
+              - apt: foo
+              - apt: bar
+        conclusions:
+          c1:
+            decision: c1
+            raises:
+              type: SystemWarning
+              message: '{pkg}'
+              format-dict:
+                pkg: '@checks.c1.requires.package'
+        """  # noqa
+        with TempScenarioDefs() as tmpscenarios:
+            with open(tmpscenarios.path, 'w') as fd:
+                fd.write(scenario)
+
+            scenarios.YScenarioChecker().load_and_run()
+            issues = list(IssuesStore().load().values())[0]
+            self.assertEqual(issues[0]['message'], 'bar')
+
+    @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': ''})
+    def test_grouped_items_all_false(self):
+        """ If the all items evaluates to False nothing is copied. """
+        scenario = """
+        checks:
+          c1:
+            or:
+              - apt: foo
+              - apt: bar
+        conclusions:
+          c1:
+            decision: c1
+            raises:
+              type: SystemWarning
+              message: '{pkg}'
+              format-dict:
+                pkg: '@checks.c1.requires.package'
+        """
+        with TempScenarioDefs() as tmpscenarios:
+            with open(tmpscenarios.path, 'w') as fd:
+                fd.write(scenario)
+
+            scenarios.YScenarioChecker().load_and_run()
+            self.assertEqual(len(IssuesStore().load()), 0)
+
+    @utils.create_data_root({'sos_commands/dpkg/dpkg_-l':
+                             'ii foo 123 amd64\nii bar 123 amd64',
+                             'sos_commands/snap/snap_list_--all':
+                             'snapd 2.54.2 14549 latest/stable xxx'})
+    def test_grouped_items_all_true_mixed_types_apt_first(self):
+        """ If the all items evaluates to False nothing is copied. """
+        scenario = """
+        checks:
+          c1:
+            or:
+              - apt: foo
+              - apt: bar
+              - snap: snapd
+        conclusions:
+          c1:
+            decision: c1
+            raises:
+              type: SystemWarning
+              message: '{pkg}'
+              format-dict:
+                pkg: '@checks.c1.requires.package'
+        """
+        with TempScenarioDefs() as tmpscenarios:
+            with open(tmpscenarios.path, 'w') as fd:
+                fd.write(scenario)
+
+            scenarios.YScenarioChecker().load_and_run()
+            issues = list(IssuesStore().load().values())[0]
+            self.assertEqual(issues[0]['message'], 'bar')
+
+    @utils.create_data_root({'sos_commands/dpkg/dpkg_-l':
+                             'ii foo 123 amd64\nii bar 123 amd64',
+                             'sos_commands/snap/snap_list_--all':
+                             'snapd 2.54.2 14549 latest/stable xxx'})
+    def test_grouped_items_all_true_mixed_types_snap_first(self):
+        """ If the all items evaluates to False nothing is copied. """
+        scenario = """
+        checks:
+          c1:
+            or:
+              - snap: snapd
+              - apt: foo
+              - apt: bar
+        conclusions:
+          c1:
+            decision: c1
+            raises:
+              type: SystemWarning
+              message: '{pkg}'
+              format-dict:
+                pkg: '@checks.c1.requires.package'
+        """
+        with TempScenarioDefs() as tmpscenarios:
+            with open(tmpscenarios.path, 'w') as fd:
+                fd.write(scenario)
+
+            scenarios.YScenarioChecker().load_and_run()
+            issues = list(IssuesStore().load().values())[0]
+            # NOTE: dicts and lists are currently being evaluated in
+            #       alphabetical order. Not clear why but that explains why
+            #       grouped items give this (unexpected) result.
+            self.assertEqual(issues[0]['message'], 'bar')
+
+
 class TestYamlRequiresTypeAPT(utils.BaseTestCase):
 
     @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': DPKG_L})