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.

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

Can return HTTPStatus member when default response is declared #1918

Merged
merged 3 commits into from
May 30, 2024

Conversation

chbndrhnns
Copy link
Contributor

@chbndrhnns chbndrhnns commented Apr 30, 2024

Fixes #1917 .

Changes proposed in this pull request:

  • Look at enum value for return status code if given

@RobbeSneyders
Copy link
Member

Hi @chbndrhnns,

Thanks for the PR.
The response validation is not always hit though. Eg. if it is deactivated, or if there is no validator for the returned content type. We should probably implement this in the response decorator.

@chbndrhnns
Copy link
Contributor Author

Please take a look at the linked issue. My fix only applies when validate_responses is True. Let me know if you still want to move the logic.

@Ruwann
Copy link
Member

Ruwann commented May 28, 2024

The root cause is indeed in the response decorator, as HTTPStatus members are of type IntEnum so they pass the isinstance(..., int) check. We can check explicitly for this first, or we should also be able to resolve it by simply wrapping the status code in another int() call here, so:

status_code = status_code_or_headers

becomes

status_code = int(status_code_or_headers)

Edit: seems like the second if-branch should also catch it, so swapping the order could also work? Not sure in which cases that branch is entered :)

@chbndrhnns
Copy link
Contributor Author

I moved the change to the proposed location.

Copy link
Member

@Ruwann Ruwann left a comment

Choose a reason for hiding this comment

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

Thanks for updates!
Really appreciate the clear issue with reproducible example and PR to fix it!

connexion/decorators/response.py Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

coverage: 94.147%. remained the same
when pulling 16e2b48 on chbndrhnns:httpstatus-with-default
into 823fa6c on spec-first:main.

@Ruwann Ruwann merged commit a930303 into spec-first:main May 30, 2024
8 checks passed
@chbndrhnns chbndrhnns deleted the httpstatus-with-default branch May 30, 2024 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fails with 500 error when returning HTTPStatus and default response is configured
4 participants