From 2db2a3d713deac857980706a306e7599be7fa3b6 Mon Sep 17 00:00:00 2001 From: David Nakabaale Date: Mon, 23 Dec 2024 10:09:25 -0500 Subject: [PATCH 1/6] handle not existing customer exception --- koku/koku/middleware.py | 6 +++++- koku/masu/management/commands/migrate_trino_tables.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/koku/koku/middleware.py b/koku/koku/middleware.py index a53d962d2f..2a43116e92 100644 --- a/koku/koku/middleware.py +++ b/koku/koku/middleware.py @@ -261,7 +261,11 @@ def create_customer(account, org_id, request_method): UNIQUE_ACCOUNT_COUNTER.inc() LOG.info("Created new customer from account_id %s and org_id %s.", account, org_id) except IntegrityError: - customer = Customer.objects.filter(org_id=org_id).get() + try: + customer = Customer.objects.filter(org_id=org_id).get() + except Customer.DoesNotExist: + LOG.warning(f"Customer with org_id {org_id} does not exist. Returning None.") + return None return customer diff --git a/koku/masu/management/commands/migrate_trino_tables.py b/koku/masu/management/commands/migrate_trino_tables.py index 3fd59bfe5e..9905d3588c 100644 --- a/koku/masu/management/commands/migrate_trino_tables.py +++ b/koku/masu/management/commands/migrate_trino_tables.py @@ -343,7 +343,7 @@ def run_trino_sql(sql, schema=None) -> list[t.Optional[list[int]]]: return cur.fetchall() except TrinoExternalError as err: if err.error_name == "HIVE_METASTORE_ERROR" and n < (retries): - LOG.warn( + LOG.warning( f"{err.message}. Attempt number {attempt} of {retries} failed. " f"Trying {remaining_retries} more time{'s' if remaining_retries > 1 else ''} " f"after waiting {wait:.2f}s." From 126328aee84afb8c88cbe96694d51b7c1d69a23c Mon Sep 17 00:00:00 2001 From: David Nakabaale Date: Fri, 3 Jan 2025 14:58:37 -0500 Subject: [PATCH 2/6] - update docstring - update logging - add unit tests --- koku/koku/middleware.py | 40 +++++++++++++++++++++++----- koku/koku/test_middleware.py | 51 ++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/koku/koku/middleware.py b/koku/koku/middleware.py index 2a43116e92..75478b9291 100644 --- a/koku/koku/middleware.py +++ b/koku/koku/middleware.py @@ -29,6 +29,7 @@ from django_tenants.middleware import TenantMainMiddleware from prometheus_client import Counter +from api.common import log_json from api.common import RH_IDENTITY_HEADER from api.common.pagination import EmptyResultsSetPagination from api.iam.models import Customer @@ -247,10 +248,11 @@ class IdentityHeaderMiddleware(MiddlewareMixin): def create_customer(account, org_id, request_method): """Create a customer. Args: - account (str): The account identifier - org_id (str): The org_id identifier + account (str): The account identifier. + org_id (str): The org_id identifier. + request_method (str): The HTTP request method. Returns: - (Customer) The created customer + Customer | None: The created or retrieved customer, or None if not saved or found. """ try: with transaction.atomic(): @@ -260,15 +262,39 @@ def create_customer(account, org_id, request_method): customer.save() UNIQUE_ACCOUNT_COUNTER.inc() LOG.info("Created new customer from account_id %s and org_id %s.", account, org_id) + return customer + else: + LOG.warning( + log_json( + msg="Customer object not saved", + request_method=request_method, + account_id=account, + org_id=org_id, + ) + ) + return None + except IntegrityError: + LOG.warning( + log_json( + msg="IntegrityError when creating customer. Attempting to fetch existing record.", + account_id=account, + org_id=org_id, + ) + ) try: customer = Customer.objects.filter(org_id=org_id).get() + return customer except Customer.DoesNotExist: - LOG.warning(f"Customer with org_id {org_id} does not exist. Returning None.") + LOG.warning( + log_json( + msg="Customer does not exist. Returning None.", + account_id=account, + org_id=org_id, + ) + ) return None - return customer - def _get_access(self, user): """Obtain access for given user from RBAC service.""" if settings.ENHANCED_ORG_ADMIN and user.admin: @@ -430,7 +456,7 @@ def process_response(self, request, response): class RequestTimingMiddleware(MiddlewareMixin): """A class to add total time taken to a request/response.""" - def process_request(self, request): # noqa: C901 + def process_request(self, request): """Process request to add start time. Args: request (object): The request object diff --git a/koku/koku/test_middleware.py b/koku/koku/test_middleware.py index f62512f780..f1127c389d 100644 --- a/koku/koku/test_middleware.py +++ b/koku/koku/test_middleware.py @@ -15,6 +15,7 @@ from cachetools import TTLCache from django.core.cache import caches from django.core.exceptions import PermissionDenied +from django.db.utils import IntegrityError from django.db.utils import OperationalError from django.http import JsonResponse from django.test.utils import modify_settings @@ -482,6 +483,56 @@ def test_process_service_account_identity(self): middleware = IdentityHeaderMiddleware(self.mock_get_response) middleware.process_request(mock_request) + @patch("api.iam.models.Customer.save") + def test_create_customer_valid_request_method(self, mock_save): + """Test creating a customer with a valid request method.""" + + mock_save.return_value = None + customer = IdentityHeaderMiddleware.create_customer("test_account", "test_org", "POST") + + self.assertIsNotNone(customer) + self.assertEqual(customer.account_id, "test_account") + mock_save.assert_called_once() + + def test_create_customer_invalid_request_method(self): + """Test that no customer is created for a GET request method.""" + + customer = IdentityHeaderMiddleware.create_customer("test_account", "test_org", "GET") + + self.assertIsNone(customer) + + @patch("api.iam.models.Customer.objects.filter") + @patch("api.iam.models.Customer.save", side_effect=IntegrityError) + def test_create_customer_integrity_error_existing_customer(self, mock_save, mock_filter): + """Test fetching an existing customer when an IntegrityError occurs.""" + + mock_query_set = MagicMock() + mock_filter.return_value = mock_query_set + mock_query_set.get.return_value = MagicMock(account_id="test_account", org_id="test_org") + + customer = IdentityHeaderMiddleware.create_customer("test_account", "test_org", "POST") + + self.assertIsNotNone(customer) + mock_save.assert_called_once() + self.assertEqual(customer.org_id, "test_org") + mock_filter.assert_called_once_with(org_id="test_org") + + @patch("api.iam.models.Customer.objects.filter") + @patch("api.iam.models.Customer.save", side_effect=IntegrityError) + def test_create_customer_integrity_error_customer_does_not_exist(self, mock_save, mock_filter): + """Test that None is returned if an IntegrityError occurs and the customer does not exist.""" + + mock_query_set = MagicMock() + mock_filter.return_value = mock_query_set + mock_query_set.get.side_effect = Customer.DoesNotExist + + customer = IdentityHeaderMiddleware.create_customer("test_account", "test_org", "POST") + + self.assertIsNone(customer) + mock_save.assert_called_once() + mock_filter.assert_called_once_with(org_id="test_org") + mock_query_set.get.assert_called_once() + class RequestTimingMiddlewareTest(IamTestCase): """Tests against the koku tenant middleware.""" From 0b23289c6d3f4b7b0fe8a7ef4d00e61681b854c6 Mon Sep 17 00:00:00 2001 From: David Nakabaale Date: Mon, 6 Jan 2025 11:16:12 -0500 Subject: [PATCH 3/6] return customer object for request types --- koku/koku/middleware.py | 32 ++++++-------------------------- koku/koku/test_middleware.py | 11 ++--------- 2 files changed, 8 insertions(+), 35 deletions(-) diff --git a/koku/koku/middleware.py b/koku/koku/middleware.py index 75478b9291..c809ebd3d8 100644 --- a/koku/koku/middleware.py +++ b/koku/koku/middleware.py @@ -29,7 +29,6 @@ from django_tenants.middleware import TenantMainMiddleware from prometheus_client import Counter -from api.common import log_json from api.common import RH_IDENTITY_HEADER from api.common.pagination import EmptyResultsSetPagination from api.iam.models import Customer @@ -252,7 +251,7 @@ def create_customer(account, org_id, request_method): org_id (str): The org_id identifier. request_method (str): The HTTP request method. Returns: - Customer | None: The created or retrieved customer, or None if not saved or found. + Customer | None: The created or retrieved customer, or None if not found. """ try: with transaction.atomic(): @@ -262,39 +261,20 @@ def create_customer(account, org_id, request_method): customer.save() UNIQUE_ACCOUNT_COUNTER.inc() LOG.info("Created new customer from account_id %s and org_id %s.", account, org_id) - return customer - else: - LOG.warning( - log_json( - msg="Customer object not saved", - request_method=request_method, - account_id=account, - org_id=org_id, - ) - ) - return None except IntegrityError: LOG.warning( - log_json( - msg="IntegrityError when creating customer. Attempting to fetch existing record.", - account_id=account, - org_id=org_id, - ) + f"IntegrityError when creating customer from account_id {account} and org_id {org_id}. " + "Attempting to fetch existing record." ) try: customer = Customer.objects.filter(org_id=org_id).get() - return customer except Customer.DoesNotExist: - LOG.warning( - log_json( - msg="Customer does not exist. Returning None.", - account_id=account, - org_id=org_id, - ) - ) + LOG.warning(f"Customer for org_id {org_id} does not exist. Returning None.") return None + return customer + def _get_access(self, user): """Obtain access for given user from RBAC service.""" if settings.ENHANCED_ORG_ADMIN and user.admin: diff --git a/koku/koku/test_middleware.py b/koku/koku/test_middleware.py index f1127c389d..43ef281154 100644 --- a/koku/koku/test_middleware.py +++ b/koku/koku/test_middleware.py @@ -484,8 +484,8 @@ def test_process_service_account_identity(self): middleware.process_request(mock_request) @patch("api.iam.models.Customer.save") - def test_create_customer_valid_request_method(self, mock_save): - """Test creating a customer with a valid request method.""" + def test_create_customer(self, mock_save): + """Test creating a customer.""" mock_save.return_value = None customer = IdentityHeaderMiddleware.create_customer("test_account", "test_org", "POST") @@ -494,13 +494,6 @@ def test_create_customer_valid_request_method(self, mock_save): self.assertEqual(customer.account_id, "test_account") mock_save.assert_called_once() - def test_create_customer_invalid_request_method(self): - """Test that no customer is created for a GET request method.""" - - customer = IdentityHeaderMiddleware.create_customer("test_account", "test_org", "GET") - - self.assertIsNone(customer) - @patch("api.iam.models.Customer.objects.filter") @patch("api.iam.models.Customer.save", side_effect=IntegrityError) def test_create_customer_integrity_error_existing_customer(self, mock_save, mock_filter): From 09aab51543737777ad31476402c779d64d38f2d2 Mon Sep 17 00:00:00 2001 From: David Nakabaale Date: Mon, 6 Jan 2025 15:04:57 -0500 Subject: [PATCH 4/6] return empty Customer object instead of None --- koku/koku/middleware.py | 6 +++--- koku/koku/test_middleware.py | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/koku/koku/middleware.py b/koku/koku/middleware.py index c809ebd3d8..0446540c7d 100644 --- a/koku/koku/middleware.py +++ b/koku/koku/middleware.py @@ -251,7 +251,7 @@ def create_customer(account, org_id, request_method): org_id (str): The org_id identifier. request_method (str): The HTTP request method. Returns: - Customer | None: The created or retrieved customer, or None if not found. + Customer : The created or retrieved customer, or empty customer object if not found. """ try: with transaction.atomic(): @@ -270,8 +270,8 @@ def create_customer(account, org_id, request_method): try: customer = Customer.objects.filter(org_id=org_id).get() except Customer.DoesNotExist: - LOG.warning(f"Customer for org_id {org_id} does not exist. Returning None.") - return None + LOG.warning(f"Customer for org_id {org_id} does not exist. Returning empty object.") + customer = Customer(account_id="", org_id="", schema_name="") return customer diff --git a/koku/koku/test_middleware.py b/koku/koku/test_middleware.py index 43ef281154..5bfaac6e87 100644 --- a/koku/koku/test_middleware.py +++ b/koku/koku/test_middleware.py @@ -521,7 +521,8 @@ def test_create_customer_integrity_error_customer_does_not_exist(self, mock_save customer = IdentityHeaderMiddleware.create_customer("test_account", "test_org", "POST") - self.assertIsNone(customer) + self.assertIsNotNone(customer) + self.assertEqual(customer.org_id, "") mock_save.assert_called_once() mock_filter.assert_called_once_with(org_id="test_org") mock_query_set.get.assert_called_once() From 79dddaf713da483c39ab3c9626f1ddf16aff4c7c Mon Sep 17 00:00:00 2001 From: David Nakabaale Date: Wed, 8 Jan 2025 12:49:14 -0500 Subject: [PATCH 5/6] update log level --- koku/koku/middleware.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/koku/koku/middleware.py b/koku/koku/middleware.py index 0446540c7d..4d7450e987 100644 --- a/koku/koku/middleware.py +++ b/koku/koku/middleware.py @@ -270,7 +270,7 @@ def create_customer(account, org_id, request_method): try: customer = Customer.objects.filter(org_id=org_id).get() except Customer.DoesNotExist: - LOG.warning(f"Customer for org_id {org_id} does not exist. Returning empty object.") + LOG.info(f"Customer for org_id {org_id} does not exist. Returning empty object.") customer = Customer(account_id="", org_id="", schema_name="") return customer From 22c633345e73cde1f125ff81c397450a046d4fc2 Mon Sep 17 00:00:00 2001 From: David Nakabaale Date: Tue, 14 Jan 2025 06:29:51 -0500 Subject: [PATCH 6/6] start with loggingg the IntegrityError --- koku/koku/middleware.py | 19 ++++++++++--------- koku/koku/test_middleware.py | 17 ----------------- 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/koku/koku/middleware.py b/koku/koku/middleware.py index 4d7450e987..cc5c9ffeaf 100644 --- a/koku/koku/middleware.py +++ b/koku/koku/middleware.py @@ -29,6 +29,7 @@ from django_tenants.middleware import TenantMainMiddleware from prometheus_client import Counter +from api.common import log_json from api.common import RH_IDENTITY_HEADER from api.common.pagination import EmptyResultsSetPagination from api.iam.models import Customer @@ -251,7 +252,7 @@ def create_customer(account, org_id, request_method): org_id (str): The org_id identifier. request_method (str): The HTTP request method. Returns: - Customer : The created or retrieved customer, or empty customer object if not found. + Customer : The created or retrieved customer. """ try: with transaction.atomic(): @@ -262,16 +263,16 @@ def create_customer(account, org_id, request_method): UNIQUE_ACCOUNT_COUNTER.inc() LOG.info("Created new customer from account_id %s and org_id %s.", account, org_id) - except IntegrityError: + except IntegrityError as err: LOG.warning( - f"IntegrityError when creating customer from account_id {account} and org_id {org_id}. " - "Attempting to fetch existing record." + log_json( + msg="IntegrityError when creating customer. Attempting to fetch existing record", + account=account, + org_id=org_id, + exc_info=err, + ) ) - try: - customer = Customer.objects.filter(org_id=org_id).get() - except Customer.DoesNotExist: - LOG.info(f"Customer for org_id {org_id} does not exist. Returning empty object.") - customer = Customer(account_id="", org_id="", schema_name="") + customer = Customer.objects.filter(org_id=org_id).get() return customer diff --git a/koku/koku/test_middleware.py b/koku/koku/test_middleware.py index 5bfaac6e87..65ee4b6909 100644 --- a/koku/koku/test_middleware.py +++ b/koku/koku/test_middleware.py @@ -510,23 +510,6 @@ def test_create_customer_integrity_error_existing_customer(self, mock_save, mock self.assertEqual(customer.org_id, "test_org") mock_filter.assert_called_once_with(org_id="test_org") - @patch("api.iam.models.Customer.objects.filter") - @patch("api.iam.models.Customer.save", side_effect=IntegrityError) - def test_create_customer_integrity_error_customer_does_not_exist(self, mock_save, mock_filter): - """Test that None is returned if an IntegrityError occurs and the customer does not exist.""" - - mock_query_set = MagicMock() - mock_filter.return_value = mock_query_set - mock_query_set.get.side_effect = Customer.DoesNotExist - - customer = IdentityHeaderMiddleware.create_customer("test_account", "test_org", "POST") - - self.assertIsNotNone(customer) - self.assertEqual(customer.org_id, "") - mock_save.assert_called_once() - mock_filter.assert_called_once_with(org_id="test_org") - mock_query_set.get.assert_called_once() - class RequestTimingMiddlewareTest(IamTestCase): """Tests against the koku tenant middleware."""