From 674c16d5601296a8c04f7e49fead24c68ea8fa3c Mon Sep 17 00:00:00 2001 From: Henry Walshaw Date: Thu, 29 Aug 2024 13:30:37 +1000 Subject: [PATCH 1/7] Add run message and report to webhook message This does increase the message size, though I imagine that's marginal, and is backwards compatible in that old keys remain in place and aren't modified. The goal is to make the actual GHC app more set and forget, and we can handle the errors with a little more discretion on the webhook end. The report is being sent as URLEncoded form data, which if we send a hierarchy just sends a single key. We want the entire report so encode the JSON as a string and send that. The keys are sorted to make testing easier. --- GeoHealthCheck/notifications.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/GeoHealthCheck/notifications.py b/GeoHealthCheck/notifications.py index f912fa48..d64c2c32 100644 --- a/GeoHealthCheck/notifications.py +++ b/GeoHealthCheck/notifications.py @@ -215,6 +215,8 @@ def do_webhook(config, resource, run, status_changed, result): params['ghc.resource.title'] = resource.title params['ghc.resource.type'] = resource.resource_type params['ghc.resource.view'] = resource_view + params['ghc.run.message'] = run.message + params['ghc.run.report'] = json.dumps(run.report, sort_keys=True) try: r = requests.post(url, params) From 0561e1b815f4cbbc81e6945f432737a7169261f4 Mon Sep 17 00:00:00 2001 From: Henry Walshaw Date: Tue, 10 Sep 2024 11:05:36 +1000 Subject: [PATCH 2/7] Add responses test requirement --- requirements-dev.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements-dev.txt b/requirements-dev.txt index d026ec6d..95976bf5 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,3 +1,4 @@ flake8==5.0.4 Paver==1.3.4 pylint==2.13.9 +responses From 9d2506636ef05017ec131fc6c3965bac70bc15aa Mon Sep 17 00:00:00 2001 From: Henry Walshaw Date: Tue, 10 Sep 2024 12:26:33 +1000 Subject: [PATCH 3/7] Fix the probe error check Python3 division is always float. Much easier to check if the code is 400 or more. --- GeoHealthCheck/probe.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GeoHealthCheck/probe.py b/GeoHealthCheck/probe.py index 44a3ae66..be1da96f 100644 --- a/GeoHealthCheck/probe.py +++ b/GeoHealthCheck/probe.py @@ -298,7 +298,7 @@ def perform_request(self): if self.response: self.log('response: status=%d' % self.response.status_code) - if self.response.status_code / 100 in [4, 5]: + if self.response.status_code >= 400: self.log('Error response: %s' % (str(self.response.text))) def perform_get_request(self, url): From a1b859eaf81ed0ae1b9a7a70628ec95b83aed42f Mon Sep 17 00:00:00 2001 From: Henry Walshaw Date: Tue, 10 Sep 2024 15:59:49 +1000 Subject: [PATCH 4/7] Add recipient to the general fixtures Also update the external links to be https (as otherwise we have to wait for a bunch of redirects in the tests as well). --- GeoHealthCheck/models.py | 12 ++++++++++++ tests/data/fixtures.json | 19 ++++++++++++++----- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/GeoHealthCheck/models.py b/GeoHealthCheck/models.py index 2c09b511..37cd4d76 100644 --- a/GeoHealthCheck/models.py +++ b/GeoHealthCheck/models.py @@ -911,6 +911,18 @@ def load_data(file_path): checks[check_name] = check DB.session.add(check) + # add Recipient, keeping track of DB objects + recipients = {} + for recipient_name in objects.get('recipients', {}): + recipient = objects['recipients'][recipient_name] + + recipient = Recipient(recipient['channel'], recipient['location']) + for resource_name in objects['recipients'][recipient_name]['resources']: + recipient.resources.append(resources[resource_name]) + + recipients[recipient_name] = recipient + DB.session.add(recipient) + db_commit() diff --git a/tests/data/fixtures.json b/tests/data/fixtures.json index 299af403..b64481ad 100644 --- a/tests/data/fixtures.json +++ b/tests/data/fixtures.json @@ -31,7 +31,7 @@ "resource_type": "OGC:WFS", "active": true, "title": "WOUDC Web Feature Service", - "url": "http://geo.woudc.org/ows", + "url": "https://geo.woudc.org/ows", "tags": [ "ows" ] @@ -52,7 +52,7 @@ "resource_type": "OGC:CSW", "active": true, "title": "WOUDC Catalogue Service", - "url": "http://geo.woudc.org/csw", + "url": "https://geo.woudc.org/csw", "tags": [ "ows" ] @@ -85,7 +85,7 @@ "resource_type": "WWW:LINK", "active": true, "title": "WOUDC Definitions Service", - "url": "http://geo.woudc.org/def", + "url": "https://geo.woudc.org/def", "tags": [] }, "OPENGEOGROEP TMS": { @@ -176,7 +176,7 @@ "parameters": { "type_name": "bag:verblijfsobject", "type_ns_prefix": "bag", - "type_ns_uri": "http://bag.geonovum.nl", + "type_ns_uri": "https://bag.geonovum.nl", "srs": "EPSG:28992", "bbox": ["180635", "455870", "180961", "456050"] } @@ -187,7 +187,7 @@ "parameters": { "type_name": "all 5 featuretypes", "type_ns_prefix": "bag", - "type_ns_uri": "http://bag.geonovum.nl", + "type_ns_uri": "https://bag.geonovum.nl", "srs": "EPSG:28992", "bbox": ["180635", "455870", "180961", "456050"] } @@ -427,5 +427,14 @@ "check_class": "GeoHealthCheck.plugins.check.checks.HttpHasImageContentType", "parameters": {} } + }, + "recipients": { + "PDOK BAG WMS": { + "channel": "webhook", + "location": "https://localhost/webhook", + "resources": [ + "PDOK BAG WMS" + ] + } } } From cfeba33cdc002371a466558f47bf6da6f8449344 Mon Sep 17 00:00:00 2001 From: Henry Walshaw Date: Tue, 10 Sep 2024 16:04:29 +1000 Subject: [PATCH 5/7] Add a test to make sure the webhook payload is correct This uses the responses library to intercept the request so we can confirm that the body content is exactly as expected. --- tests/test_resources.py | 67 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/tests/test_resources.py b/tests/test_resources.py index 1b3a8a16..28751eff 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -28,14 +28,19 @@ # # ================================================================= +import json import unittest import os +import responses +from responses.matchers import urlencoded_params_matcher + from init import App from models import (DB, Resource, Run, load_data, Recipient) from healthcheck import run_test_resource -from notifications import _parse_webhook_location +from notifications import _parse_webhook_location, do_webhook from resourceauth import ResourceAuth +import result TEST_DIR = os.path.dirname(os.path.abspath(__file__)) @@ -133,6 +138,66 @@ def testWebhookNotifications(self): except Exception as err: self.assertFalse(success, str(err)) + @responses.activate + def testWebhookPayload(self): + + # create a report from scratch as as local fixture for + + recipient = Recipient.query.filter(Recipient.channel==Recipient.TYPE_WEBHOOK).first() + + resource = recipient.resources[0] + + resource_result = result.ResourceResult(resource) + resource_result.start() + + probe = resource.probe_vars[0] + probe_result = result.ProbeResult(None, probe) + probe_result.start() + + check = probe.check_vars[0] + check_result = result.CheckResult( + check=None, + check_vars=check, + success=False, + message='Failed' + ) + check_result.start() + check_result.stop() + + probe_result.add_result(check_result) + probe_result.stop() + + resource_result.add_result(probe_result) + resource_result.stop() + + run = Run(resource=resource, result=resource_result) + + # Build up the expected payload being sent ot the webhook + + expected_payload = { + 'ghc.result': 'Broken', + 'ghc.resource.url': resource.url, + 'ghc.resource.title': resource.title, + 'ghc.resource.type': resource.resource_type, + 'ghc.resource.view': f'http://host/resource/{resource.identifier}', + 'ghc.run.message': 'Failed', + 'ghc.run.report': json.dumps(resource_result.get_report(), sort_keys=True), + } + + responses.add( + method=responses.POST, + url='https://localhost/webhook', + match=[urlencoded_params_matcher(expected_payload)] + ) + + do_webhook( + config=App.get_config(), + resource=resource, + run=run, + status_changed=None, # unused in do_webhook + result='Broken' + ) + def testSetGetResoureAuth(self): # Test set/get auth for any Resource, tests en/decrypt resource = Resource.query.first() From 48ff5f74c07767c5190bdb22d18bc96a5a990825 Mon Sep 17 00:00:00 2001 From: Henry Walshaw Date: Tue, 10 Sep 2024 16:12:10 +1000 Subject: [PATCH 6/7] Fix up the Recipient query Makes sure we're not accidentally doubling up somewhere --- tests/test_resources.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/test_resources.py b/tests/test_resources.py index 28751eff..837310cf 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -42,6 +42,7 @@ from resourceauth import ResourceAuth import result + TEST_DIR = os.path.dirname(os.path.abspath(__file__)) @@ -95,18 +96,22 @@ def testRunResoures(self): (resource.url, str(resource.runs[0]))) def testNotificationsApi(self): - Rcp = Recipient test_emails = ['test@test.com', 'other@test.com', 'unused@test.com'] invalid_emails = ['invalid', None, object()] # invalid values should raise exception for email in invalid_emails: with self.assertRaises(ValueError): - Rcp.get_or_create('email', email) + Recipient.get_or_create('email', email) for email in test_emails: - Rcp.get_or_create('email', email) - from_db = set(r[0] for r in DB.session.query(Rcp.location).all()) + Recipient.get_or_create('email', email) + from_db = { + r[0] + for r in DB.session.query(Recipient.location).filter( + Recipient.channel == 'email' + ) + } self.assertEqual(from_db, set(test_emails)) r = Resource.query.first() @@ -114,7 +119,7 @@ def testNotificationsApi(self): # unused email should be removed self.assertEqual(set(r.get_recipients('email')), set(test_emails[:2])) - q = Rcp.query.filter(Rcp.location == test_emails[-1]) + q = Recipient.query.filter(Recipient.location == test_emails[-1]) self.assertEqual(q.count(), 0) def testWebhookNotifications(self): From 6a152a8a4c067e07b20fc666aac470e2c3abbef4 Mon Sep 17 00:00:00 2001 From: Henry Walshaw Date: Tue, 10 Sep 2024 16:28:41 +1000 Subject: [PATCH 7/7] flake8 fixes --- GeoHealthCheck/models.py | 3 ++- tests/test_resources.py | 8 ++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/GeoHealthCheck/models.py b/GeoHealthCheck/models.py index 37cd4d76..702742ce 100644 --- a/GeoHealthCheck/models.py +++ b/GeoHealthCheck/models.py @@ -917,7 +917,8 @@ def load_data(file_path): recipient = objects['recipients'][recipient_name] recipient = Recipient(recipient['channel'], recipient['location']) - for resource_name in objects['recipients'][recipient_name]['resources']: + attached_resources = objects['recipients'][recipient_name]['resources'] + for resource_name in attached_resources: recipient.resources.append(resources[resource_name]) recipients[recipient_name] = recipient diff --git a/tests/test_resources.py b/tests/test_resources.py index 837310cf..879be104 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -148,7 +148,9 @@ def testWebhookPayload(self): # create a report from scratch as as local fixture for - recipient = Recipient.query.filter(Recipient.channel==Recipient.TYPE_WEBHOOK).first() + recipient = Recipient.query.filter( + Recipient.channel == Recipient.TYPE_WEBHOOK + ).first() resource = recipient.resources[0] @@ -186,7 +188,9 @@ def testWebhookPayload(self): 'ghc.resource.type': resource.resource_type, 'ghc.resource.view': f'http://host/resource/{resource.identifier}', 'ghc.run.message': 'Failed', - 'ghc.run.report': json.dumps(resource_result.get_report(), sort_keys=True), + 'ghc.run.report': json.dumps( + resource_result.get_report(), sort_keys=True + ), } responses.add(