Skip to content

Conversation

@mattgd
Copy link
Contributor

@mattgd mattgd commented Nov 26, 2024

Description

Propagate ConflictExceptions properly and don't mask them as BadRequestExceptions.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

@mattgd mattgd self-assigned this Nov 26, 2024
@mattgd mattgd marked this pull request as ready for review November 27, 2024 14:34
@mattgd mattgd requested a review from a team as a code owner November 27, 2024 14:34
@mattgd mattgd requested review from PaulAsjes and cadef and removed request for cadef November 27, 2024 14:34
except ConflictException as ex:
assert str(ex) == "(message=No message, request_id=request-123)"
except Exception as ex:
assert ex.__class__ == ConflictException
Copy link
Contributor

@gcarvelli gcarvelli Nov 27, 2024

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.

@mattgd mattgd merged commit 78eb99e into main Nov 27, 2024
5 checks passed
@mattgd mattgd deleted the md5/fix-oauth-error-propagation branch November 27, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants