From e2e80eea2f64a1d0e349e10737db31e3b804238a Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Wed, 16 Sep 2020 19:57:01 -0400 Subject: [PATCH 1/3] Add tornado-problem-details support --- VERSION | 2 +- setup.cfg | 1 + sprockets_postgres.py | 25 ++++++++- tests.py | 118 ++++++++++++++++++++++++++++++------------ 4 files changed, 110 insertions(+), 36 deletions(-) diff --git a/VERSION b/VERSION index 347f583..bc80560 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.4.1 +1.5.0 diff --git a/setup.cfg b/setup.cfg index d63605c..20f299b 100644 --- a/setup.cfg +++ b/setup.cfg @@ -54,6 +54,7 @@ testing = flake8-rst-docstrings flake8-tuple pygments + tornado-problem-details [coverage:run] branch = True diff --git a/sprockets_postgres.py b/sprockets_postgres.py index d3da35b..48586f2 100644 --- a/sprockets_postgres.py +++ b/sprockets_postgres.py @@ -16,6 +16,10 @@ from aiodns import error as aiodns_error from aiopg import pool from psycopg2 import errors, extras from tornado import ioloop, web +try: + import problemdetails +except ImportError: # pragma: nocover + problemdetails = None LOGGER = logging.getLogger('sprockets-postgres') @@ -699,7 +703,13 @@ class RequestHandlerMixin: def on_postgres_error(self, metric_name: str, exc: Exception) -> typing.Optional[Exception]: - """Override for different error handling behaviors + """Invoked when an error occurs when executing a query + + If `tornado-problem-details` is available, + :exc:`problemdetails.Problem` will be raised instead of + :exc:`tornado.web.HTTPError`. + + Override for different error handling behaviors. Return an exception if you would like for it to be raised, or swallow it here. @@ -709,12 +719,25 @@ class RequestHandlerMixin: exc.__class__.__name__, self.__class__.__name__, metric_name, str(exc).split('\n')[0]) if isinstance(exc, ConnectionException): + if problemdetails: + raise problemdetails.Problem( + status_code=503, title='Database Connection Error', + detail=str(exc)) raise web.HTTPError(503, reason='Database Connection Error') elif isinstance(exc, asyncio.TimeoutError): + if problemdetails: + raise problemdetails.Problem( + status_code=500, title='Query Timeout', detail=str(exc)) raise web.HTTPError(500, reason='Query Timeout') elif isinstance(exc, errors.UniqueViolation): + if problemdetails: + raise problemdetails.Problem( + status_code=409, title='Unique Violation', detail=str(exc)) raise web.HTTPError(409, reason='Unique Violation') elif isinstance(exc, psycopg2.Error): + if problemdetails: + raise problemdetails.Problem( + status_code=500, title='Database Error', detail=str(exc)) raise web.HTTPError(500, reason='Database Error') return exc diff --git a/tests.py b/tests.py index a28fb29..c4bdb8a 100644 --- a/tests.py +++ b/tests.py @@ -10,6 +10,7 @@ from urllib import parse import aiopg import asynctest +import problemdetails import psycopg2 import pycares from asynctest import mock @@ -99,6 +100,11 @@ class ExecuteRequestHandler(RequestHandler): 'value': result.row['value'] if result.row else None}) +class ProblemDetailsExecuteRequestHandler( + problemdetails.ErrorWriter, ExecuteRequestHandler): + pass + + class InfluxDBRequestHandler(ExecuteRequestHandler): def __init__(self, *args, **kwargs): @@ -305,6 +311,7 @@ class TestCase(testing.SprocketsHttpTestCase): web.url('/multi-row', MultiRowRequestHandler), web.url('/no-error', NoErrorRequestHandler), web.url('/no-row', NoRowRequestHandler), + web.url('/pdexecute', ProblemDetailsExecuteRequestHandler), web.url('/row-count-no-rows', RowCountNoRowsRequestHandler), web.url('/status', sprockets_postgres.StatusRequestHandler), web.url('/timeout-error', TimeoutErrorRequestHandler), @@ -425,40 +432,6 @@ class RequestHandlerMixinTestCase(TestCase): self.assertEqual(body['count'], 0) self.assertListEqual(body['rows'], []) - @mock.patch('aiopg.cursor.Cursor.execute') - def test_postgres_execute_timeout_error(self, execute): - execute.side_effect = asyncio.TimeoutError() - response = self.fetch('/execute?value=1') - self.assertEqual(response.code, 500) - self.assertIn(b'Query Timeout', response.body) - - @mock.patch('aiopg.cursor.Cursor.execute') - def test_postgres_execute_unique_violation(self, execute): - execute.side_effect = errors.UniqueViolation() - response = self.fetch('/execute?value=1') - self.assertEqual(response.code, 409) - self.assertIn(b'Unique Violation', response.body) - - @mock.patch('aiopg.cursor.Cursor.execute') - def test_postgres_execute_error(self, execute): - execute.side_effect = psycopg2.Error() - response = self.fetch('/execute?value=1') - self.assertEqual(response.code, 500) - self.assertIn(b'Database Error', response.body) - - @mock.patch('aiopg.cursor.Cursor.fetchone') - def test_postgres_programming_error(self, fetchone): - fetchone.side_effect = psycopg2.ProgrammingError() - response = self.fetch('/execute?value=1') - self.assertEqual(response.code, 200) - self.assertIsNone(json.loads(response.body)['value']) - - @mock.patch('aiopg.connection.Connection.cursor') - def test_postgres_cursor_raises(self, cursor): - cursor.side_effect = asyncio.TimeoutError() - response = self.fetch('/execute?value=1') - self.assertEqual(response.code, 503) - def test_postgres_cursor_operational_error_reconnects(self): original = aiopg.connection.Connection.cursor @@ -524,6 +497,83 @@ class RequestHandlerMixinTestCase(TestCase): response = self.fetch('/unhandled-exception') self.assertEqual(response.code, 422) + @mock.patch('aiopg.cursor.Cursor.execute') + def test_postgres_execute_timeout_error(self, execute): + execute.side_effect = asyncio.TimeoutError() + response = self.fetch('/pdexecute?value=1') + self.assertEqual(response.code, 500) + problem = json.loads(response.body) + self.assertEqual(problem['title'], 'Query Timeout') + + @mock.patch('aiopg.cursor.Cursor.execute') + def test_postgres_execute_unique_violation(self, execute): + execute.side_effect = errors.UniqueViolation() + response = self.fetch('/pdexecute?value=1') + self.assertEqual(response.code, 409) + problem = json.loads(response.body) + self.assertEqual(problem['title'], 'Unique Violation') + + @mock.patch('aiopg.cursor.Cursor.execute') + def test_postgres_execute_error(self, execute): + execute.side_effect = psycopg2.Error() + response = self.fetch('/pdexecute?value=1') + self.assertEqual(response.code, 500) + problem = json.loads(response.body) + self.assertEqual(problem['title'], 'Database Error') + + @mock.patch('aiopg.cursor.Cursor.fetchone') + def test_postgres_programming_error(self, fetchone): + fetchone.side_effect = psycopg2.ProgrammingError() + response = self.fetch('/pdexecute?value=1') + self.assertEqual(response.code, 200) + self.assertIsNone(json.loads(response.body)['value']) + + +class RequestHandlerMixinHTTPErrorTestCase(TestCase): + + def setUp(self): + super().setUp() + self._problemdetails = sprockets_postgres.problemdetails + sprockets_postgres.problemdetails = None + + def tearDown(self): + sprockets_postgres.problemdetails = self._problemdetails + super().tearDown() + + @mock.patch('aiopg.cursor.Cursor.execute') + def test_postgres_execute_timeout_error(self, execute): + execute.side_effect = asyncio.TimeoutError() + response = self.fetch('/execute?value=1') + self.assertEqual(response.code, 500) + self.assertIn(b'Query Timeout', response.body) + + @mock.patch('aiopg.cursor.Cursor.execute') + def test_postgres_execute_unique_violation(self, execute): + execute.side_effect = errors.UniqueViolation() + response = self.fetch('/execute?value=1') + self.assertEqual(response.code, 409) + self.assertIn(b'Unique Violation', response.body) + + @mock.patch('aiopg.cursor.Cursor.execute') + def test_postgres_execute_error(self, execute): + execute.side_effect = psycopg2.Error() + response = self.fetch('/execute?value=1') + self.assertEqual(response.code, 500) + self.assertIn(b'Database Error', response.body) + + @mock.patch('aiopg.cursor.Cursor.fetchone') + def test_postgres_programming_error(self, fetchone): + fetchone.side_effect = psycopg2.ProgrammingError() + response = self.fetch('/execute?value=1') + self.assertEqual(response.code, 200) + self.assertIsNone(json.loads(response.body)['value']) + + @mock.patch('aiopg.connection.Connection.cursor') + def test_postgres_cursor_raises(self, cursor): + cursor.side_effect = asyncio.TimeoutError() + response = self.fetch('/execute?value=1') + self.assertEqual(response.code, 503) + class TransactionTestCase(TestCase): From 839fb820341032da5e726fc6d3632e1b0429c574 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Thu, 17 Sep 2020 11:40:34 -0400 Subject: [PATCH 2/3] Fix a missing edge case for no error handler, add tests The error handler behavior descended down through the connection management part of the code and that was missed, this covers that. Add test coverage for the new branches --- sprockets_postgres.py | 5 ++++- tests.py | 45 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/sprockets_postgres.py b/sprockets_postgres.py index 48586f2..126f4f8 100644 --- a/sprockets_postgres.py +++ b/sprockets_postgres.py @@ -373,7 +373,10 @@ class ApplicationMixin: _attempt + 1) as connector: yield connector return - exc = on_error('postgres_connector', ConnectionException(str(err))) + if on_error is None: + raise ConnectionException(str(err)) + exc = on_error( + 'postgres_connector', ConnectionException(str(err))) if exc: raise exc else: # postgres_status.on_error does not return an exception diff --git a/tests.py b/tests.py index c4bdb8a..56b6152 100644 --- a/tests.py +++ b/tests.py @@ -168,6 +168,18 @@ class NoErrorRequestHandler(ErrorRequestHandler): return None +class NoMixinRequestHandler(web.RequestHandler): + + async def get(self): + try: + async with self.application.postgres_connector() as connector: + await connector.execute('SELECT 1', {}) + except sprockets_postgres.ConnectionException: + raise web.HTTPError(503) + else: + raise web.HTTPError(418) + + class NoRowRequestHandler(RequestHandler): GET_SQL = """\ @@ -257,7 +269,7 @@ class UnhandledExceptionRequestHandler(RequestHandler): await self.postgres_execute(self.GET_SQL) except psycopg2.DataError: raise web.HTTPError(422) - raise web.HTTPError(500, 'This should have failed') + raise web.HTTPError(418, 'This should have failed') def on_postgres_error(self, metric_name: str, @@ -271,6 +283,14 @@ class UnhandledExceptionRequestHandler(RequestHandler): return exc +class DontRaiseRequestHandler(UnhandledExceptionRequestHandler): + + def on_postgres_error(self, + metric_name: str, + exc: Exception) -> typing.Optional[Exception]: + return None + + class Application(sprockets_postgres.ApplicationMixin, app.Application): @@ -303,6 +323,7 @@ class TestCase(testing.SprocketsHttpTestCase): self.app = Application(handlers=[ web.url('/callproc', CallprocRequestHandler), web.url('/count', CountRequestHandler), + web.url('/dont-raise', DontRaiseRequestHandler), web.url('/error', ErrorRequestHandler), web.url('/error-passthrough', ErrorPassthroughRequestHandler), web.url('/execute', ExecuteRequestHandler), @@ -310,6 +331,7 @@ class TestCase(testing.SprocketsHttpTestCase): web.url('/metrics-mixin', MetricsMixinRequestHandler), web.url('/multi-row', MultiRowRequestHandler), web.url('/no-error', NoErrorRequestHandler), + web.url('/no-mixin', NoMixinRequestHandler), web.url('/no-row', NoRowRequestHandler), web.url('/pdexecute', ProblemDetailsExecuteRequestHandler), web.url('/row-count-no-rows', RowCountNoRowsRequestHandler), @@ -529,7 +551,7 @@ class RequestHandlerMixinTestCase(TestCase): self.assertIsNone(json.loads(response.body)['value']) -class RequestHandlerMixinHTTPErrorTestCase(TestCase): +class HTTPErrorTestCase(TestCase): def setUp(self): super().setUp() @@ -574,6 +596,25 @@ class RequestHandlerMixinHTTPErrorTestCase(TestCase): response = self.fetch('/execute?value=1') self.assertEqual(response.code, 503) + def test_on_error_no_exception_branch(self): + response = self.fetch('/dont-raise') + self.assertEqual(response.code, 418) + + +class NoMixinTestCase(TestCase): + + @mock.patch('aiopg.cursor.Cursor.execute') + def test_postgres_cursor_raises(self, execute): + execute.side_effect = psycopg2.ProgrammingError() + response = self.fetch('/no-mixin') + self.assertEqual(response.code, 500) + + @mock.patch('aiopg.pool.Pool.acquire') + def test_postgres_status_connect_error(self, acquire): + acquire.side_effect = asyncio.TimeoutError() + response = self.fetch('/no-mixin') + self.assertEqual(response.code, 503) + class TransactionTestCase(TestCase): From 499972a7e876bbf8feea5d1799fb227382c9591c Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Thu, 17 Sep 2020 12:05:18 -0400 Subject: [PATCH 3/3] Remove detail due to potential info disclosure issues --- sprockets_postgres.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/sprockets_postgres.py b/sprockets_postgres.py index 126f4f8..0c253ec 100644 --- a/sprockets_postgres.py +++ b/sprockets_postgres.py @@ -724,23 +724,22 @@ class RequestHandlerMixin: if isinstance(exc, ConnectionException): if problemdetails: raise problemdetails.Problem( - status_code=503, title='Database Connection Error', - detail=str(exc)) + status_code=503, title='Database Connection Error') raise web.HTTPError(503, reason='Database Connection Error') elif isinstance(exc, asyncio.TimeoutError): if problemdetails: raise problemdetails.Problem( - status_code=500, title='Query Timeout', detail=str(exc)) + status_code=500, title='Query Timeout') raise web.HTTPError(500, reason='Query Timeout') elif isinstance(exc, errors.UniqueViolation): if problemdetails: raise problemdetails.Problem( - status_code=409, title='Unique Violation', detail=str(exc)) + status_code=409, title='Unique Violation') raise web.HTTPError(409, reason='Unique Violation') elif isinstance(exc, psycopg2.Error): if problemdetails: raise problemdetails.Problem( - status_code=500, title='Database Error', detail=str(exc)) + status_code=500, title='Database Error') raise web.HTTPError(500, reason='Database Error') return exc