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

Support binary payloads and responses, and non-UTF8 string payloads for Python functions #217

Closed
wants to merge 2 commits into from

Conversation

jpauwels
Copy link

closes #205 and replaces #207

Description

In addition to handling binary input as in #207, this PR also handles binary output, properly encodes returned Python dicts as compliant JSON (double quotes instead of single quotes) and can handle all text encodings. All this processing is controlled by the headers according to web standards.

Motivation and Context

  • I have raised an issue to propose this change (required)

Which issue(s) this PR fixes

Fixes #205

How Has This Been Tested?

Deployed on an internal project that takes audio files/JSON as payload and response, running on Ubuntu and macOS.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Version change (see: Impact to existing users)

Impact to existing users

UTF8 text payloads (the only ones supported so far) are passed as before.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: Johan Pauwels <johan.pauwels@gmail.com>
Signed-off-by: Johan Pauwels <johan.pauwels@gmail.com>
@LucasRoesler
Copy link
Member

I think this looks good, @alexellis what do you think?

@alexellis
Copy link
Member

I haven't had a chance to look at it yet.

It may be better just to use the existing support for binary responses in the HTTP/flask templates that I added whilst building out the LF training course on serverless

openfaas/python-flask-template@9fb5527

if ret != None:
print(ret)

# encode response to JSON if necessary
Copy link
Member

Choose a reason for hiding this comment

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

This part seems fine in principle, but be aware that you also need to set the mime type in the function's deployment. It cannot be dynamic with the classic watchdog.

st = get_stdin()
st = sys.stdin.buffer.read()
# decode text to string
content_type, type_options = parse_header(os.getenv('Http_Content_Type', ''))
Copy link
Member

Choose a reason for hiding this comment

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

You don't mention application/x-www-form-urlencoded in the PR description?

@alexellis
Copy link
Member

Quite a few changes and output from testing. It would be good to see a sample GitHub repo where you have deployed samples from each of the scenarios that are supported in the response writing code.

@alexellis
Copy link
Member

@jpauwels we didn't hear back from you after providing feedback, so I'm going to assume you're no longer interested in seeing this through.

For anyone else landing here, do check out the python3-flask/http templates in the store. These are in a separate repository in this organisation.

@alexellis alexellis closed this Feb 21, 2022
@alexellis
Copy link
Member

/add label: abandoned

@derek derek bot added the abandoned label Feb 21, 2022
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.

Support binary payloads and responses, and non-UTF8 string payloads for Python functions
3 participants