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

Don't validate streamed responses #1259

Merged

Conversation

cshorler
Copy link
Contributor

prior to validation get_connexion_response ultimately calls response.get_data()

I haven't found a workaround for this and the old PR was closed due to inactivity.

I changed it to use getattr as the object may not be a response (could be tuple).

Fixes #904 .

there are also a few other issues raised.

Changes proposed in this pull request:

  • bypass for response.is_streamed, otherwise no change in behaviour

@coveralls
Copy link

coveralls commented Jun 25, 2020

Coverage Status

Coverage increased (+0.001%) to 96.294% when pulling e00e609 on cshorler:fix_streamed_response_validation into bed4b95 on zalando:master.

@RobbeSneyders
Copy link
Member

Hi @cshorler, thx for the PR. Hopefully you're still interested in merging this :)

In the perfect case, we might want to try to validate the response in a streaming fashion, but this already looks like an improvement. I see two downside as well though:

  • Response headers are also not validated anymore
  • Users might not expect that validation is deactivated on streaming responses

I think we can address this by logging a warning about this when we skip validation.

Also, we moved to the main branch, so can you please rebase and change the target of your PR?

@RobbeSneyders
Copy link
Member

Also, since is_streamed is Flask / werkzeug specific, we should add this as a property to the ConnexionResponse class instead of accessing it directly.

@RobbeSneyders RobbeSneyders force-pushed the fix_streamed_response_validation branch from e00e609 to c78f523 Compare February 9, 2022 19:53
@RobbeSneyders RobbeSneyders changed the base branch from master to main February 9, 2022 20:39
@RobbeSneyders
Copy link
Member

Fixes #401 as well

@RobbeSneyders RobbeSneyders changed the title rework PR #467 - don't attempt to validate streamed responses Don't validate streamed responses Feb 9, 2022
@coveralls
Copy link

coveralls commented Feb 9, 2022

Pull Request Test Coverage Report for Build 1820352240

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 97.064%

Totals Coverage Status
Change from base Build 1819970533: 0.003%
Covered Lines: 2843
Relevant Lines: 2929

💛 - Coveralls

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!

This is currently only for flask. For aiohttp, we might need to check what the response class is (https://docs.aiohttp.org/en/stable/web_reference.html#response-classes) but I'm not that familiar with aiohttp myself

@RobbeSneyders RobbeSneyders merged commit d375ea8 into spec-first:main Feb 19, 2022
vbxx3 pushed a commit to Savannah-Group/connexion that referenced this pull request Apr 9, 2022
* rework PR spec-first#467 - don't attempt to validate streamed responses

* Add is_streamed property to ConnexionResponse

* Adhere to response.direct_passthrough

* Add test for file response validation

* Add warning about skipping validation for streamed response

Co-authored-by: Robbe Sneyders <robbe.sneyders@ml6.eu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants