From 25fa76d628c3e2974723658710d66ca3189e780b Mon Sep 17 00:00:00 2001 From: rhatgadkar-goog Date: Mon, 2 Dec 2024 22:13:15 +0000 Subject: [PATCH 1/4] feat: improve aiohttp client error messages This change is similar to https://github.com/GoogleCloudPlatform/cloud-sql-python-connector/pull/1201, except for AlloyDB. In short, we want to use the AlloyDB API's response body for error messages, because they are more detailed. --- google/cloud/alloydb/connector/client.py | 34 +++++- requirements-test.txt | 1 + tests/unit/test_client.py | 137 ++++++++++++++++++++++- 3 files changed, 165 insertions(+), 7 deletions(-) diff --git a/google/cloud/alloydb/connector/client.py b/google/cloud/alloydb/connector/client.py index 6edb9ee4..59e923a4 100644 --- a/google/cloud/alloydb/connector/client.py +++ b/google/cloud/alloydb/connector/client.py @@ -124,8 +124,20 @@ async def _get_metadata( url = f"{self._alloydb_api_endpoint}/{API_VERSION}/projects/{project}/locations/{region}/clusters/{cluster}/instances/{name}/connectionInfo" - resp = await self._client.get(url, headers=headers, raise_for_status=True) - resp_dict = await resp.json() + resp = await self._client.get(url, headers=headers) + # try to get response json for better error message + try: + resp_dict = await resp.json() + if resp.status >= 400: + # if detailed error message is in json response, use as error message + message = resp_dict.get("error", {}).get("message") + if message: + resp.reason = message + # skip, raise_for_status will catch all errors in finally block + except Exception: + pass + finally: + resp.raise_for_status() # Remove trailing period from PSC DNS name. psc_dns = resp_dict.get("pscDnsName") @@ -175,10 +187,20 @@ async def _get_client_certificate( "useMetadataExchange": self._use_metadata, } - resp = await self._client.post( - url, headers=headers, json=data, raise_for_status=True - ) - resp_dict = await resp.json() + resp = await self._client.post(url, headers=headers, json=data) + # try to get response json for better error message + try: + resp_dict = await resp.json() + if resp.status >= 400: + # if detailed error message is in json response, use as error message + message = resp_dict.get("error", {}).get("message") + if message: + resp.reason = message + # skip, raise_for_status will catch all errors in finally block + except Exception: + pass + finally: + resp.raise_for_status() return (resp_dict["caCert"], resp_dict["pemCertificateChain"]) diff --git a/requirements-test.txt b/requirements-test.txt index 11bf97eb..86ac93ac 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -6,3 +6,4 @@ pytest-asyncio==0.24.0 pytest-cov==6.0.0 pytest-aiohttp==1.0.5 SQLAlchemy[asyncio]==2.0.36 +aioresponses==0.7.7 diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 3da68079..4fb032b0 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -15,7 +15,8 @@ import json from typing import Any, Optional -from aiohttp import web +from aiohttp import ClientResponseError, web +from aioresponses import aioresponses from mocks import FakeCredentials import pytest @@ -138,6 +139,76 @@ async def test__get_metadata_with_psc( } +async def test__get_metadata_error( + credentials: FakeCredentials, +) -> None: + """ + Test that AlloyDB API error messages are raised for _get_metadata. + """ + # mock AlloyDB API calls with exceptions + client = AlloyDBClient( + alloydb_api_endpoint="https://alloydb.googleapis.com", + quota_project=None, + credentials=credentials, + ) + get_url = "https://alloydb.googleapis.com/v1beta/projects/my-project/locations/my-region/clusters/my-cluster/instances/my-instance/connectionInfo" + resp_body = { + "error": { + "code": 403, + "message": "AlloyDB API has not been used in project 123456789 before or it is disabled", + } + } + with aioresponses() as mocked: + mocked.get( + get_url, + status=403, + payload=resp_body, + repeat=True, + ) + with pytest.raises(ClientResponseError) as exc_info: + await client._get_metadata("my-project", "my-region", "my-cluster", "my-instance") + exc = exc_info.value + assert exc.status == 403 + assert ( + exc.message + == "AlloyDB API has not been used in project 123456789 before or it is disabled" + ) + await client.close() + + +async def test__get_metadata_error_parsing_json( + credentials: FakeCredentials, +) -> None: + """ + Test that AlloyDB API error messages are raised for _get_metadata when + response JSON fails to be parsed. + """ + # mock AlloyDB API calls with exceptions + client = AlloyDBClient( + alloydb_api_endpoint="https://alloydb.googleapis.com", + quota_project=None, + credentials=credentials, + ) + get_url = "https://alloydb.googleapis.com/v1beta/projects/my-project/locations/my-region/clusters/my-cluster/instances/my-instance/connectionInfo" + resp_body = ["error"] # invalid json + with aioresponses() as mocked: + mocked.get( + get_url, + status=403, + payload=resp_body, + repeat=True, + ) + with pytest.raises(ClientResponseError) as exc_info: + await client._get_metadata("my-project", "my-region", "my-cluster", "my-instance") + exc = exc_info.value + assert exc.status == 403 + assert ( + exc.message + != "AlloyDB API has not been used in project 123456789 before or it is disabled" + ) + await client.close() + + @pytest.mark.asyncio async def test__get_client_certificate( client: Any, credentials: FakeCredentials @@ -157,6 +228,70 @@ async def test__get_client_certificate( assert cert_chain[2] == "This is the root cert" +async def test__get_client_certificate_error( + credentials: FakeCredentials, +) -> None: + """ + Test that AlloyDB API error messages are raised for _get_client_certificate. + """ + # mock AlloyDB API calls with exceptions + client = AlloyDBClient( + alloydb_api_endpoint="https://alloydb.googleapis.com", + quota_project=None, + credentials=credentials, + ) + post_url = "https://alloydb.googleapis.com/v1beta/projects/my-project/locations/my-region/clusters/my-cluster:generateClientCertificate" + resp_body = { + "error": { + "code": 404, + "message": "The AlloyDB instance does not exist.", + } + } + with aioresponses() as mocked: + mocked.post( + post_url, + status=404, + payload=resp_body, + repeat=True, + ) + with pytest.raises(ClientResponseError) as exc_info: + await client._get_client_certificate("my-project", "my-region", "my-cluster", "") + exc = exc_info.value + assert exc.status == 404 + assert exc.message == "The AlloyDB instance does not exist." + await client.close() + + +async def test__get_client_certificate_error_parsing_json( + credentials: FakeCredentials, +) -> None: + """ + Test that AlloyDB API error messages are raised for _get_client_certificate + when response JSON fails to be parsed. + """ + # mock AlloyDB API calls with exceptions + client = AlloyDBClient( + alloydb_api_endpoint="https://alloydb.googleapis.com", + quota_project=None, + credentials=credentials, + ) + post_url = "https://alloydb.googleapis.com/v1beta/projects/my-project/locations/my-region/clusters/my-cluster:generateClientCertificate" + resp_body = ["error"] # invalid json + with aioresponses() as mocked: + mocked.post( + post_url, + status=404, + payload=resp_body, + repeat=True, + ) + with pytest.raises(ClientResponseError) as exc_info: + await client._get_client_certificate("my-project", "my-region", "my-cluster", "") + exc = exc_info.value + assert exc.status == 404 + assert exc.message != "The AlloyDB instance does not exist." + await client.close() + + @pytest.mark.asyncio async def test_AlloyDBClient_init_(credentials: FakeCredentials) -> None: """ From 92b00986613c0d65ac78d088cc6659f166c816eb Mon Sep 17 00:00:00 2001 From: rhatgadkar-goog Date: Tue, 3 Dec 2024 02:55:36 +0000 Subject: [PATCH 2/4] Fix lint errors --- tests/unit/test_client.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 4fb032b0..7a00af73 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -15,7 +15,8 @@ import json from typing import Any, Optional -from aiohttp import ClientResponseError, web +from aiohttp import ClientResponseError +from aiohttp import web from aioresponses import aioresponses from mocks import FakeCredentials import pytest @@ -166,7 +167,9 @@ async def test__get_metadata_error( repeat=True, ) with pytest.raises(ClientResponseError) as exc_info: - await client._get_metadata("my-project", "my-region", "my-cluster", "my-instance") + await client._get_metadata( + "my-project", "my-region", "my-cluster", "my-instance" + ) exc = exc_info.value assert exc.status == 403 assert ( @@ -190,7 +193,7 @@ async def test__get_metadata_error_parsing_json( credentials=credentials, ) get_url = "https://alloydb.googleapis.com/v1beta/projects/my-project/locations/my-region/clusters/my-cluster/instances/my-instance/connectionInfo" - resp_body = ["error"] # invalid json + resp_body = ["error"] # invalid json with aioresponses() as mocked: mocked.get( get_url, @@ -199,7 +202,9 @@ async def test__get_metadata_error_parsing_json( repeat=True, ) with pytest.raises(ClientResponseError) as exc_info: - await client._get_metadata("my-project", "my-region", "my-cluster", "my-instance") + await client._get_metadata( + "my-project", "my-region", "my-cluster", "my-instance" + ) exc = exc_info.value assert exc.status == 403 assert ( @@ -255,7 +260,9 @@ async def test__get_client_certificate_error( repeat=True, ) with pytest.raises(ClientResponseError) as exc_info: - await client._get_client_certificate("my-project", "my-region", "my-cluster", "") + await client._get_client_certificate( + "my-project", "my-region", "my-cluster", "" + ) exc = exc_info.value assert exc.status == 404 assert exc.message == "The AlloyDB instance does not exist." @@ -276,7 +283,7 @@ async def test__get_client_certificate_error_parsing_json( credentials=credentials, ) post_url = "https://alloydb.googleapis.com/v1beta/projects/my-project/locations/my-region/clusters/my-cluster:generateClientCertificate" - resp_body = ["error"] # invalid json + resp_body = ["error"] # invalid json with aioresponses() as mocked: mocked.post( post_url, @@ -285,7 +292,9 @@ async def test__get_client_certificate_error_parsing_json( repeat=True, ) with pytest.raises(ClientResponseError) as exc_info: - await client._get_client_certificate("my-project", "my-region", "my-cluster", "") + await client._get_client_certificate( + "my-project", "my-region", "my-cluster", "" + ) exc = exc_info.value assert exc.status == 404 assert exc.message != "The AlloyDB instance does not exist." From 8eca8ca6dd59c4bb670a760548f47fca9f9680b5 Mon Sep 17 00:00:00 2001 From: rhatgadkar-goog Date: Tue, 3 Dec 2024 18:12:49 +0000 Subject: [PATCH 3/4] Change to use exc_info.value --- tests/unit/test_client.py | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 7a00af73..a7bead56 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -170,10 +170,9 @@ async def test__get_metadata_error( await client._get_metadata( "my-project", "my-region", "my-cluster", "my-instance" ) - exc = exc_info.value - assert exc.status == 403 + assert exc_info.value.status == 403 assert ( - exc.message + exc_info.value.message == "AlloyDB API has not been used in project 123456789 before or it is disabled" ) await client.close() @@ -205,10 +204,9 @@ async def test__get_metadata_error_parsing_json( await client._get_metadata( "my-project", "my-region", "my-cluster", "my-instance" ) - exc = exc_info.value - assert exc.status == 403 + assert exc_info.value.status == 403 assert ( - exc.message + exc_info.value.message != "AlloyDB API has not been used in project 123456789 before or it is disabled" ) await client.close() @@ -263,9 +261,8 @@ async def test__get_client_certificate_error( await client._get_client_certificate( "my-project", "my-region", "my-cluster", "" ) - exc = exc_info.value - assert exc.status == 404 - assert exc.message == "The AlloyDB instance does not exist." + assert exc_info.value.status == 404 + assert exc_info.value.message == "The AlloyDB instance does not exist." await client.close() @@ -295,9 +292,8 @@ async def test__get_client_certificate_error_parsing_json( await client._get_client_certificate( "my-project", "my-region", "my-cluster", "" ) - exc = exc_info.value - assert exc.status == 404 - assert exc.message != "The AlloyDB instance does not exist." + assert exc_info.value.status == 404 + assert exc_info.value.message != "The AlloyDB instance does not exist." await client.close() From 641702b1d1171a85929c88597dcb528eb127ac03 Mon Sep 17 00:00:00 2001 From: rhatgadkar-goog Date: Wed, 4 Dec 2024 02:02:42 +0000 Subject: [PATCH 4/4] Address PR comments --- tests/unit/test_client.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index a7bead56..e4b2fdbb 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -182,8 +182,8 @@ async def test__get_metadata_error_parsing_json( credentials: FakeCredentials, ) -> None: """ - Test that AlloyDB API error messages are raised for _get_metadata when - response JSON fails to be parsed. + Test that aiohttp default error messages are raised when _get_metadata gets + a bad JSON response. """ # mock AlloyDB API calls with exceptions client = AlloyDBClient( @@ -205,10 +205,7 @@ async def test__get_metadata_error_parsing_json( "my-project", "my-region", "my-cluster", "my-instance" ) assert exc_info.value.status == 403 - assert ( - exc_info.value.message - != "AlloyDB API has not been used in project 123456789 before or it is disabled" - ) + assert exc_info.value.message == "Forbidden" await client.close() @@ -270,8 +267,8 @@ async def test__get_client_certificate_error_parsing_json( credentials: FakeCredentials, ) -> None: """ - Test that AlloyDB API error messages are raised for _get_client_certificate - when response JSON fails to be parsed. + Test that aiohttp default error messages are raised when + _get_client_certificate gets a bad JSON response. """ # mock AlloyDB API calls with exceptions client = AlloyDBClient( @@ -293,7 +290,7 @@ async def test__get_client_certificate_error_parsing_json( "my-project", "my-region", "my-cluster", "" ) assert exc_info.value.status == 404 - assert exc_info.value.message != "The AlloyDB instance does not exist." + assert exc_info.value.message == "Not Found" await client.close()