Skip to content

Added text_mime_types argument#277

Merged
jordaneremieff merged 7 commits intoKludex:mainfrom
georgebv:mime-types-arg
Oct 16, 2022
Merged

Added text_mime_types argument#277
jordaneremieff merged 7 commits intoKludex:mainfrom
georgebv:mime-types-arg

Conversation

@georgebv
Copy link
Copy Markdown
Contributor

@georgebv georgebv commented Oct 1, 2022

Added optional text_mime_types argument to:

  • Mangum adapter's __init__ method
  • handle_base64_response_body function (this function is the target recipient of the text_mime_types list)

This argument is optional and by default references the same DEFAULT_TEXT_MIME_TYPES list.

WARNING: old solutions involving editing the DEFAULT_TEXT_MIME_TYPES list are now broken because this list was moved from mangum/handlers/utils.py to mangum/adapter.py.

resolves #275

Copy link
Copy Markdown
Collaborator

@jordaneremieff jordaneremieff left a comment

Choose a reason for hiding this comment

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

Thanks @georgebv! Just a few requests related to how the configuration and default are organized now.

The original intention of the LambdaConfig was for things like this to avoid having to pass things explicitly to the handler __call__ methods.

Also, could you add one new test demonstrating the new configuration being used?

Comment thread mangum/adapter.py Outdated
Comment thread mangum/handlers/utils.py Outdated
Comment thread mangum/adapter.py Outdated
@georgebv
Copy link
Copy Markdown
Contributor Author

georgebv commented Oct 2, 2022

Regarding new tests, I wasn't sure where to put this. Technically, this is an integration test where we use adapter itself. But, since handlers are so different, I put the test to API gateway.

Comment thread mangum/adapter.py Outdated
@jordaneremieff
Copy link
Copy Markdown
Collaborator

Regarding new tests, I wasn't sure where to put this. Technically, this is an integration test where we use adapter itself. But, since handlers are so different, I put the test to API gateway.

Yep that’s fine, thank you.

@jordaneremieff
Copy link
Copy Markdown
Collaborator

Hi @georgebv, are you still interested in working on this feature?

@georgebv
Copy link
Copy Markdown
Contributor Author

@jordaneremieff absolutely, didn't have time these last 2 weeks - was planning to wrap this up in the next few days

@georgebv
Copy link
Copy Markdown
Contributor Author

I had to bring back the [*DEFAULT_TEXT_MIME_TYPES] because tests testing it were failing due to global list being modified by handler.config["text_mime_types"].append(content_type.decode()). Let me know if you have a better solution for this, but I think this is the best way to handle this - we use global list as default and we make a copy of it on adapter instantiation. Without this the __init__ method of Mangum would have this side effect of mutating global object.

@jordaneremieff
Copy link
Copy Markdown
Collaborator

I had to bring back the [*DEFAULT_TEXT_MIME_TYPES] because tests testing it were failing due to global list being modified by handler.config["text_mime_types"].append(content_type.decode()).

Yep, that's fine then, thanks for explaining.

Copy link
Copy Markdown
Collaborator

@jordaneremieff jordaneremieff left a comment

Choose a reason for hiding this comment

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

Looks good, great work!

@jordaneremieff jordaneremieff merged commit f872706 into Kludex:main Oct 16, 2022
khamaileon pushed a commit to khamaileon/mangum that referenced this pull request Jan 13, 2024
* added vscode files to gitignore

* added text_mime_types argument

* updated text mime type definition and propagation

* copy default text mime types inside mangum

* api gateway test

* moved instance attributes to config

* added tests for custom text mime types
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.

Allow providing extra DEFAULT_TEXT_MIME_TYPES

2 participants