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

Handle binary responses (encode body) #457

Merged
merged 5 commits into from
Sep 26, 2019
Merged

Handle binary responses (encode body) #457

merged 5 commits into from
Sep 26, 2019

Conversation

victormacko
Copy link
Contributor

@victormacko victormacko commented Sep 18, 2019

I've added this in to allow PDF and Excel documents to be outputted by a lambda function (via the API Gateway), as a http response. (They can already be generated internally and saved/emailed ... just not output as the http response).

Previously a lambda error would occur (from memory this was shown as a JSON error "Internal server error" (or something similar) - it's detailed more in #288.
In summary this was because Bref could not encode binary data as JSON for the response to the lambda invocation endpoint.

I've implemented this by looking at the content-type included in the response, and base64 encoding any non-text response - content types of text/* ... (internally it just checks that the content-type header starts with 'text'.
I had a bit of a shortage of time so wasn't able to get the unit-tests running (I don't have the php-fpm executable installed), but hopefully this shouldn't cause any issues.

No changes to the PHP applications are required (after updating the bref package to include this change), apart from configuring API Gateway to allow Binary responses for the certain content-types which are needed ... '/' can also be used to allow everything. At the time of testing I used / as API Gateway was being difficult with having specific content-types entered ... I have a feeling the 'Accept' header (with the content-type you're requesting) needs to be included if you don't use a wildcard content-type).

The serverless (framework) plugin provides an easy way of setting this;
https://github.com/maciejtreder/serverless-apigw-binary

The approach of using the 'text/*' content-type was borrowed from the laravel-bref-bridge (was mentioned in #288;
https://github.com/stechstudio/laravel-bref-bridge/blob/9b4762f09cd6a8384fb6273506d04ee903d0bbec/src/Services/Bootstrap.php#L266-L278

@mnapoli
Copy link
Member

mnapoli commented Sep 19, 2019

The serverless (framework) plugin provides an easy way of setting this;
https://github.com/maciejtreder/serverless-apigw-binary

That's interesting thanks for sharing!

If I understand correctly base64 encoding will only work with the mime types specified explicitly in serverless.yml, for example:

plugins:
 - serverless-apigw-binary

custom:
  apigwBinary:
    types:           #list of mime-types
      - 'image/jpeg'
      - 'text/html'

Here we should only base64 encode image/jpeg responses and text/html responses. All other kind of responses should not be encoded. Even if they need to be, Bref encoding them will not be enough because API Gateway will not support them.

That leads me to this: should Bref automatically encode in base64 the same mimetypes that exist in serverless.yml?

The question would then be: how do we get the list of mime types…

@mnapoli
Copy link
Member

mnapoli commented Sep 19, 2019

Also Bref needs to work without being tied to Serverless (e.g. people can use SAM or CloudFormation)…

@mnapoli
Copy link
Member

mnapoli commented Sep 19, 2019

It seems that there is no need for a plugin anymore, it is supported natively in serverless.yml: https://serverless.com/framework/docs/providers/aws/events/apigateway/#binary-media-types

(sorry for posting 3 times in a row ;))

@mnapoli
Copy link
Member

mnapoli commented Sep 19, 2019

OK we had a quick chat with @Guillaume-Rossignol and here is a summary:

Enabling API Gateway support for binary responses seems simple (https://serverless.com/framework/docs/providers/aws/events/apigateway/#binary-media-types):

provider:
  apiGateway:
    binaryMediaTypes:
      - '*/*'

I can add that to the documentation after merging this PR. By recommending to use */* that's one less thing to worry about for users (it's simpler).

On top of that, this PR does the job: Bref will automatically base64-encode the responses that are NOT text-based. The whitelist is currently: text/*. We think it should also contain application/json. Do we want to add anything else to the list? We should cover common mime types, it's OK if some less common mime types are encoded in base64, it's not optimum but it should still work.

Finally one last question to make this work: are we sure that setting */* in API Gateway will still work with non-base64 encoded responses?

@victormacko
Copy link
Contributor Author

Ignore the last commit :)

@victormacko
Copy link
Contributor Author

victormacko commented Sep 19, 2019

Enabling API Gateway support for binary responses seems simple

Ooo ... very nice spotting there - thank you for for that!

Also Bref needs to work without being tied to Serverless (e.g. people can use SAM or CloudFormation)…

I agree -- my thoughts as well ... also if the serverless.yml is in a non-standard location, etc.

We think it should also contain application/json.

Sounds good -- done. Will keep an eye out for any other additions to the list.

Finally one last question to make this work: are we sure that setting */* in API Gateway will still work with non-base64 encoded responses?

Yes ... looks like it didn't come out properly in my initial description on this PR.
I've been using that setting */* in my API Gateway for testing this PR and it works without any issues (even application/json data) ... I have a feeling API Gateway 'allows' the base64encoded binary data when it matches that content-type, but it doesn't have any adverse affect either way it it comes through without being base64encoded (eg. text/html, etc). ... that is just my assumption though!

@mnapoli
Copy link
Member

mnapoli commented Sep 19, 2019

Perfect! I can take care of the documentation. I'll try also to see if it's possible to cover that with tests. You can try to have a look, but I'm afraid those tests are not the easiest to get started with.

@victormacko
Copy link
Contributor Author

Ace, thank you!
Thanks again for all your work on this btw!

@mnapoli
Copy link
Member

mnapoli commented Sep 26, 2019

Finally one last question to make this work: are we sure that setting */* in API Gateway will still work with non-base64 encoded responses?

Answering this myself: yes it seems to work perfectly!

@mnapoli
Copy link
Member

mnapoli commented Sep 26, 2019

OK I've written some documentation. I'm looking for advices.

Do you think the section I've highlighted is necessary?

Capture d’écran 2019-09-26 à 14 06 00

I am wondering because if you want to return a binary file in PHP, you have to set the content-type anyway. Here the documentation makes it seem like there is an extra step involved, but there isn't. What do you think?

@mnapoli
Copy link
Member

mnapoli commented Sep 26, 2019

@Guillaume-Rossignol did a few tests and it turns out this part of the docs is useful: some web browsers can handle some files without the content types. I will be leaving it.

@victormacko
Copy link
Contributor Author

I think it's good practice to keep it in there.

From past experience without it, the browser caches the response type ... so one request to the url which returns HTML, will continue to be interpreted as HTML unless it's told otherwise (eg. with the content-type to say the next response is actually an image, or something else).

It also maximises proper handling of binary files by the browser, without having to leave the browser to figure out what type of file is actually getting downloaded.

mnapoli added a commit that referenced this pull request Sep 26, 2019
mnapoli added a commit that referenced this pull request Sep 26, 2019
@mnapoli mnapoli merged commit 8ec337d into brefphp:master Sep 26, 2019
mnapoli added a commit that referenced this pull request Sep 26, 2019
mnapoli added a commit that referenced this pull request Sep 26, 2019
mnapoli added a commit that referenced this pull request Sep 26, 2019
mnapoli added a commit that referenced this pull request Sep 26, 2019
mnapoli added a commit that referenced this pull request Sep 26, 2019
@mnapoli
Copy link
Member

mnapoli commented Sep 26, 2019

Despite trying for a few hours I didn't manage to write an automated test for that. I tested things manually for now.

Thank you @victormacko for pushing this!

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.

3 participants