Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate ConflictExceptions properly #389

Merged
merged 3 commits into from
Nov 27, 2024
Merged

Propagate ConflictExceptions properly #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
722c39c
Support specific ConflictException.
mattgd Nov 26, 2024
59c4936
Update exception tests.
mattgd Nov 27, 2024
65ab507
Merge branch 'main' into md5/fix-oauth-error-propagation
mattgd Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions tests/test_async_http_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
from unittest.mock import AsyncMock

from tests.test_sync_http_client import STATUS_CODE_TO_EXCEPTION_MAPPING
from workos.exceptions import BadRequestException, BaseRequestException, ServerException
from workos.exceptions import (
BadRequestException,
BaseRequestException,
ConflictException,
ServerException,
)
from workos.utils.http_client import AsyncHTTPClient


Expand Down Expand Up @@ -186,8 +191,6 @@ async def test_request_exceptions_include_expected_request_data(
except expected_exception as ex: # type: ignore
assert ex.message == response_message
assert ex.request_id == request_id
except Exception as ex:
# This'll fail for sure here but... just using the nice error that'd come up
assert ex.__class__ == expected_exception

async def test_bad_request_exceptions_include_expected_request_data(self):
Expand All @@ -210,7 +213,6 @@ async def test_bad_request_exceptions_include_expected_request_data(self):
str(ex)
== "(message=No message, request_id=request-123, error=example_error, error_description=Example error description)"
)
except Exception as ex:
assert ex.__class__ == BadRequestException

async def test_bad_request_exceptions_exclude_expected_request_data(self):
Expand All @@ -228,7 +230,6 @@ async def test_bad_request_exceptions_exclude_expected_request_data(self):
await self.http_client.request("bad_place")
except BadRequestException as ex:
assert str(ex) == "(message=No message, request_id=request-123, foo=bar)"
except Exception as ex:
assert ex.__class__ == BadRequestException

async def test_request_bad_body_raises_expected_exception_with_request_data(self):
Expand All @@ -247,10 +248,24 @@ async def test_request_bad_body_raises_expected_exception_with_request_data(self
except ServerException as ex:
assert ex.message == None
assert ex.request_id == request_id
except Exception as ex:
# This'll fail for sure here but... just using the nice error that'd come up
assert ex.__class__ == ServerException

async def test_conflict_exception(self):
request_id = "request-123"

self.http_client._client.request = AsyncMock(
return_value=httpx.Response(
status_code=409,
headers={"X-Request-ID": request_id},
),
)

try:
await self.http_client.request("bad_place")
except ConflictException as ex:
assert str(ex) == "(message=No message, request_id=request-123)"
assert ex.__class__ == ConflictException

async def test_request_includes_base_headers(
self, capture_and_mock_http_client_request
):
Expand Down
22 changes: 17 additions & 5 deletions tests/test_sync_http_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
AuthorizationException,
BadRequestException,
BaseRequestException,
ConflictException,
ServerException,
)
from workos.utils.http_client import SyncHTTPClient
Expand Down Expand Up @@ -200,8 +201,6 @@ def test_request_exceptions_include_expected_request_data(
except expected_exception as ex: # type: ignore
assert ex.message == response_message
assert ex.request_id == request_id
except Exception as ex:
# This'll fail for sure here but... just using the nice error that'd come up
assert ex.__class__ == expected_exception

def test_bad_request_exceptions_include_request_data(self):
Expand All @@ -228,7 +227,6 @@ def test_bad_request_exceptions_include_request_data(self):
str(ex)
== "(message=No message, request_id=request-123, error=example_error, error_description=Example error description, foo=bar)"
)
except Exception as ex:
assert ex.__class__ == BadRequestException

def test_request_bad_body_raises_expected_exception_with_request_data(self):
Expand All @@ -247,10 +245,24 @@ def test_request_bad_body_raises_expected_exception_with_request_data(self):
except ServerException as ex:
assert ex.message == None
assert ex.request_id == request_id
except Exception as ex:
# This'll fail for sure here but... just using the nice error that'd come up
assert ex.__class__ == ServerException

def test_conflict_exception(self):
request_id = "request-123"

self.http_client._client.request = MagicMock(
return_value=httpx.Response(
status_code=409,
headers={"X-Request-ID": request_id},
),
)

try:
self.http_client.request("bad_place")
except ConflictException as ex:
assert str(ex) == "(message=No message, request_id=request-123)"
assert ex.__class__ == ConflictException
Copy link
Contributor

@gcarvelli gcarvelli Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why we do these multiple-except blocks in this test file?

IIRC Python will only call the first matching except block so this except Exception block will never be entered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great question. After changing the test it appears there's no reason really. Let me update these.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also confirming one of these tests failing doesn't look any different.


def test_request_includes_base_headers(self, capture_and_mock_http_client_request):
request_kwargs = capture_and_mock_http_client_request(self.http_client, {}, 200)

Expand Down
4 changes: 4 additions & 0 deletions workos/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ class BadRequestException(BaseRequestException):
pass


class ConflictException(BaseRequestException):
pass


class NotFoundException(BaseRequestException):
pass

Expand Down
3 changes: 3 additions & 0 deletions workos/utils/_base_http_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from httpx._types import QueryParamTypes

from workos.exceptions import (
ConflictException,
ServerException,
AuthenticationException,
AuthorizationException,
Expand Down Expand Up @@ -101,6 +102,8 @@ def _maybe_raise_error_by_status_code(
raise AuthorizationException(response, response_json)
elif status_code == 404:
raise NotFoundException(response, response_json)
elif status_code == 409:
raise ConflictException(response, response_json)

raise BadRequestException(response, response_json)
elif status_code >= 500 and status_code < 600:
Expand Down