From 883f1792ffdfbd1e0e086c0e8835f2e914cf2a30 Mon Sep 17 00:00:00 2001
From: Lukas Holecek <hluk@email.cz>
Date: Mar 21 2019 15:34:23 +0000
Subject: Omit comparing result_id values for decision change


When Greenwave receives new result message from ResultsDB it tries to
compare old decision (ignoring the new result) with new one (for all its
policies) so it can publish decision update message only when the
decision changed.

The new decision was seen as "changed" when any of its data differ from
the old decision. The problem is that decision data include result IDs
so it's always seen as "changed" if the new result is part of the new
decision.

Example:

    # Decision before new result is available:
    {
        "applicable_policies": [
            "taskotron_release_critical_tasks_for_stable"
        ],
        "policies_satisfied": true,
        "satisfied_requirements": [
            {
                "result_id": 27968725,
                "testcase": "dist.abicheck",
                "type": "test-result-passed"
            },
            {
                "result_id": 28015726,
                "testcase": "dist.rpmdeplint",
                "type": "test-result-passed"
            }
        ],
        "summary": "All required tests passed",
        "unsatisfied_requirements": []
    }

    # Decision after new result with ID=28015899 is available:
    {
        "applicable_policies": [
            "taskotron_release_critical_tasks_for_stable"
        ],
        "policies_satisfied": true,
        "satisfied_requirements": [
            {
                "result_id": 27968725,
                "testcase": "dist.abicheck",
                "type": "test-result-passed"
            },
            {
                "result_id": 28015899,
                "testcase": "dist.rpmdeplint",
                "type": "test-result-passed"
            }
        ],
        "summary": "All required tests passed",
        "unsatisfied_requirements": []
    }

With this change, the new decision is seen as "changed" when any of its
data changes *except* result IDs.

Fixes #395

Signed-off-by: Lukas Holecek <hluk@email.cz>

---

diff --git a/functional-tests/consumers/test_resultsdb.py b/functional-tests/consumers/test_resultsdb.py
index 4cbf5fe..676c869 100644
--- a/functional-tests/consumers/test_resultsdb.py
+++ b/functional-tests/consumers/test_resultsdb.py
@@ -156,6 +156,47 @@ def test_consume_new_result(
 
 @mock.patch('greenwave.consumers.resultsdb.fedmsg.config.load_config')
 @mock.patch('greenwave.consumers.resultsdb.fedmsg.publish')
+def test_consume_unchanged_result(
+        mock_fedmsg, load_config, requests_session, greenwave_server,
+        testdatabuilder):
+    load_config.return_value = {'greenwave_api_url': greenwave_server + 'api/v1.0'}
+    nvr = testdatabuilder.unique_nvr(product_version='fc26')
+
+    testdatabuilder.create_result(
+        item=nvr, testcase_name='dist.rpmdeplint', outcome='PASSED')
+    new_result = testdatabuilder.create_result(
+        item=nvr, testcase_name='dist.rpmdeplint', outcome='PASSED')
+
+    message = {
+        'body': {
+            'topic': 'resultsdb.result.new',
+            'msg': {
+                'id': new_result['id'],
+                'outcome': 'PASSED',
+                'testcase': {
+                    'name': 'dist.rpmdeplint',
+                },
+                'data': {
+                    'item': [nvr],
+                    'type': ['koji_build'],
+                }
+            }
+        }
+    }
+    hub = mock.MagicMock()
+    hub.config = {
+        'environment': 'environment',
+        'topic_prefix': 'topic_prefix',
+    }
+    handler = resultsdb.ResultsDBHandler(hub)
+    assert handler.topic == ['topic_prefix.environment.taskotron.result.new']
+    handler.consume(message)
+
+    assert len(mock_fedmsg.mock_calls) == 0
+
+
+@mock.patch('greenwave.consumers.resultsdb.fedmsg.config.load_config')
+@mock.patch('greenwave.consumers.resultsdb.fedmsg.publish')
 def test_invalidate_new_result_with_mocked_cache(
         mock_fedmsg, load_config, requests_session, greenwave_server,
         testdatabuilder):
diff --git a/greenwave/consumers/resultsdb.py b/greenwave/consumers/resultsdb.py
index 7a58834..e43795d 100644
--- a/greenwave/consumers/resultsdb.py
+++ b/greenwave/consumers/resultsdb.py
@@ -106,6 +106,38 @@ def _invalidate_results_cache(
             cache, subject_type, subject_identifier, testcase=None)
 
 
+def _equals_except_keys(lhs, rhs, except_keys):
+    keys = lhs.keys() - except_keys
+    return lhs.keys() == rhs.keys() \
+        and all(lhs[key] == rhs[key] for key in keys)
+
+
+def _is_decision_unchanged(old_decision, decision):
+    """
+    Returns true only if new decision is same as old one
+    (ignores result_id values).
+    """
+    if old_decision is None or decision is None:
+        return old_decision == decision
+
+    requirements_keys = ('satisfied_requirements', 'unsatisfied_requirements')
+    if not _equals_except_keys(old_decision, decision, requirements_keys):
+        return False
+
+    ignore_keys = ('result_id',)
+    for key in requirements_keys:
+        old_requirements = old_decision[key]
+        requirements = decision[key]
+        if len(old_requirements) != len(requirements):
+            return False
+
+        for old_requirement, requirement in zip(old_requirements, requirements):
+            if not _equals_except_keys(old_requirement, requirement, ignore_keys):
+                return False
+
+    return True
+
+
 class ResultsDBHandler(fedmsg.consumers.FedmsgConsumer):
     """
     Handle a new result.
@@ -254,7 +286,7 @@ class ResultsDBHandler(fedmsg.consumers.FedmsgConsumer):
                 log.exception('Failed to retrieve decision for data=%s, error: %s', data, e)
                 continue
 
-            if decision == old_decision:
+            if _is_decision_unchanged(old_decision, decision):
                 log.debug('Skipped emitting fedmsg, decision did not change: %s', decision)
             else:
                 decision.update({