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

Adding image/jpeg as a default binary content type #707

Closed
wants to merge 1 commit into from

Conversation

artkay
Copy link
Contributor

@artkay artkay commented Feb 8, 2018

I am working on a small scale image processing API and I need to support both content types - image/jpg and image/jpeg while Chalice allows me to use image/jpg only.
This PR adds exactly one line - the new content type with the e.

@stealthycoin
Copy link
Contributor

Thanks for the pull request @artkay.

The only issue here is that there is a hard limit right now of 25 binary types that can be assigned to an API Gateway REST API. If a new default type is added it can cause projects already at the 25 limit to be pushed to 26, causing a deployment failure. So this change is potentially not a backwards compatible one.

That being said I am not against adding a few more common ones to the default list as long as the version is bumped accordingly and people are warned in the changelog.

Also to be clear you are not blocked, the default list is just for convenience. Chalice supports modifying this list through the app.api.binary_types property which is documented here and here.

For example this would give you the support you need:

from chalice import Chalice
app = Chalice(app_name='app')
app.api.binary_types.append('image/jpeg')

@jamesls
Copy link
Member

jamesls commented Mar 1, 2018

Yeah, while this does possibly introduce an issue for someone with 25 or more binary content types, it does seem like this is a common enough content type for it to be in the default list.

Perhaps we can also update our docs to suggest that if you're going to modify the binary types that you explicitly control all of them to be protected against new default additions in the future.

@stealthycoin stealthycoin self-assigned this Mar 2, 2018
@stealthycoin
Copy link
Contributor

I'd be happy to take this one. There is another related change I'd like to make to the docs at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants