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 attempt to validate Response Streamed #467

Closed
wants to merge 1 commit into from

Conversation

EmlynC
Copy link

@EmlynC EmlynC commented Jun 13, 2017

If the response is of Response Streamed then get_data() throws a RuntimeError. Response streams are likely to be binary data so there is little need to validate these as it's out of the scope of Swagger validation.

Fixes #401.

If the response is of `Response Streamed` then `get_data()` throws a RuntimeError. Response streams are likely to binary data so there is little need to validate these as it's out of the scope of Swagger.
@coveralls
Copy link

coveralls commented Jun 13, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling ef41835 on globalstressindex:master into 4403b66 on zalando:master.

@hjacobs
Copy link
Contributor

hjacobs commented Jun 13, 2017

Thanks for the PR! Please check the flake8 formatting, see https://travis-ci.org/zalando/connexion/jobs/242354204

@hjacobs
Copy link
Contributor

hjacobs commented Jun 13, 2017

Unit test would be appreciated too 😏

@EmlynC
Copy link
Author

EmlynC commented Jun 14, 2017

@hjacobs sure, I'll add unit tests, sorry for being lazy! We're TDD at Felix so there isn't really any excuses. I'll fix the PEP8 problems too and I'll add the commits to this current PR.

@hjacobs
Copy link
Contributor

hjacobs commented Jul 29, 2017

@EmlynC any update?

@EmlynC
Copy link
Author

EmlynC commented Jul 31, 2017

@hjacobs thanks for the prompt, I'll try and find some time today.

@gelineau
Copy link

Hi,
We are using connexion in our project. Thank you for making it open source.

The bug that this PR corrects is quite annoying for us. We would appreciate that the PR be merged.
Can I add the unit tests and PEP8 formatting that is missing ?
Should I modify this PR, or post a new PR ?

Thank you for your answer.

@EmlynC
Copy link
Author

EmlynC commented Feb 21, 2018

Wow, I dropped the ball here sorry @gelineau — I had said that we'd provide this ... about 8 months ago 😞

Please note that you can also bypass this check, by making sure the consumes and produces property of your endpoint in swagger is set to something other than application/json — in our case we set it to application/x-sqlite3. We ended up doing that and forgetting this PR, ultimately, coming back to this code it feels like it should throw a warning that asks "Are you trying to decode a JSON object or is this binary data?"e

I'm happy to add the unit tests and PEP fixes, let me find some time to get this fixed up today.

@kkthxbye
Copy link

kkthxbye commented Jul 13, 2018

Please note that you can also bypass this check, by making sure the consumes and produces property of your endpoint in swagger is set to something other than application/json

But response is built https://github.com/zalando/connexion/blob/master/connexion/decorators/response.py#L86
and response.get_data() crashes
https://github.com/zalando/connexion/blob/master/connexion/apis/flask_api.py#L203
before compatibility-based response validation skip
https://github.com/zalando/connexion/blob/master/connexion/decorators/response.py#L39
(called at https://github.com/zalando/connexion/blob/master/connexion/decorators/response.py#L87)

I guess that's because status code is needed in order to pick a schema from responses to validate response against.

Which means such a workaround doesn't work.

@hjacobs
Copy link
Contributor

hjacobs commented Apr 25, 2020

I'm closing this PR for now (as it's pretty old and there are no recent updates).

@hjacobs hjacobs closed this Apr 25, 2020
@gitttt
Copy link

gitttt commented Apr 29, 2020

There is no more consumes and produces in openapi 3.x. How do we solve this problem with openapi 3.x if we cannot use the mentioned workaround?

cshorler added a commit to cshorler/connexion that referenced this pull request Jun 25, 2020
RobbeSneyders pushed a commit to cshorler/connexion that referenced this pull request Feb 9, 2022
RobbeSneyders pushed a commit to cshorler/connexion that referenced this pull request Feb 19, 2022
RobbeSneyders added a commit that referenced this pull request Feb 19, 2022
* rework PR #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>
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>
phdru pushed a commit to phdru/connexion that referenced this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Returning files via connexion.send_file or connexion.send_from_directory return an error
6 participants