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

Adds optional base64 encoding status via response headers #343

Closed
wants to merge 2 commits into from
Closed

Adds optional base64 encoding status via response headers #343

wants to merge 2 commits into from

Conversation

shrink
Copy link

@shrink shrink commented Jun 11, 2019

we have a Laravel application served through lambda that I've been able to greatly simplify by switching away from my home-grown solution to bref, which has been a great help towards maintainability, thank you very much! our application serves images and html so we rely on the isBase64Encoded value -- which we explicitly provide through the response headers, as that was the ideal approach with the lambda->laravel implementation.

This pull request allows an application to return a value for isBase64Encoded through the response header: there's no magic required of bref, it defers responsibility to the application. #288 discusses the magic approach, I'm not sure if there's any intention to implement that in future but for now this seems like a good first step.

The test doesn't use fromPsr7Response because (as far as I've been able to tell) in the real world (that is, outside of the tests) header names are all lowercase whereas fromPsr7Response uses ucwords which means the header names do not match what I'm seeing in production. I'm not sure if that's a quirk of API Gateway or my environment but for now I've used the lowercase version in the tests.

I've used a helper method (isBase64Encoded) even though the check is short because the logic is a little confusing if it's all on one line, and potentially if in future a magic method is implemented, the logic for isBase64Encoded can be swapped out.

(bool) ((array) $headers['isbase64encoded'] ?? false) // huh

thanks!

A PHP application can explicitly set the Lambda response `isBase64Encoded` value by returning the value as a header (which will be cast to boolean).
@mnapoli
Copy link
Member

mnapoli commented Jun 18, 2019

Thank you for the PR, I'm not sure I understand it completely though. Could you show an example of how this feature can be used?

which we explicitly provide through the response headers

Do you set the header in the PHP response (Laravel for example) and then the idea here is that Bref will set the flag? Where do you encode the response?

@shrink
Copy link
Author

shrink commented Jun 18, 2019

@mnapoli sure, for context:

The application we run is responsible for archiving websites and then serving those archives serverlessly. Every website we archive has many different types of files, all of which must be served through API Gateway: unlike typical websites, we can't split assets out into a CDN. An example can be seen at azurilland.com.

The application is specifically built for Lambda and API Gateway, it determines when a response needs to be encoded for API Gateway based on the content type and sets the isBase64Encoded header to true. My homegrown Laravel-on-Lambda solution (pre-bref) then used this header value to construct the API Gateway response.

My application already handles all the encoding logic so it felt like this was the lowest friction way to migrate to bref. That said, I do think that giving the application responsibility for encoding is a good approach because serving assets through Lambda shouldn't be encouraged: if bref were to do this automatically developers might not realise that it is expensive and slow. That said, I'm not married to this implementation: if bref handled the encoding that would meet my needs too. I'm happy either way :)

@mnapoli
Copy link
Member

mnapoli commented Jun 18, 2019

Thanks! Could you show a code example on how you use this? Do you do anything different in Laravel?

@shrink
Copy link
Author

shrink commented Jun 18, 2019

As this application is essentially a proxy responsible for processing files it builds responses, so this probably deviates somewhat from how a typical Laravel application might work, I'm not sure if it's representative. This is the relevant parts:

Routes:

use Curs3\Serve\Response;

Route::fallback(function (Request $request) {
    $response = new Response($request->getUri());
    
    return $response->getResponse();
});

Response:

/**
* Get a Response for this page.
*
* @return \Symfony\Component\HttpFoundation\Response
*/
public function getResponse(): HttpResponse
{
    $file = $this->getFileFromUrl($this->url); // At this point the file has been encoded by `file@encode`

    [$body, $status, $headers] = $this->processFile($file);

    return response($body, $status)->withHeaders($headers);
}

File:

/**
* Is the File text?
*
* @return bool
*/
public function isText(): bool
{
    if (! $this->getMimeType()) {
        return false;
    }

    foreach (self::TEXT_TYPES as $pattern) {
        if (fnmatch($pattern, $this->getMimetype())) {
            return true;
        }
    }

    return false;
}

/**
* Is the File binary?
*
* @return bool
*/
public function isBinary(): bool
{
    return ! $this->isText();
}

/**
* Encode the body of the file as base64.
*
* @return void
*/
public function encode(): void
{
    $this->addHeaders(['isBase64Encoded' => true]);
    $this->setBody(base64_encode($this->getBody() ?? ''));
}

@mnapoli
Copy link
Member

mnapoli commented Jun 21, 2019

OK I think I see a little better.

You use a custom HTTP header isBase64Encoded that your PHP app sets, and your PHP app is also responsible of encoding the body.

I'm not 100% certain but I think it would make more sense that the PHP app doesn't do the encoding.

Maybe an alternative approach would be to set the header in PHP, and then Bref will fetch the raw body from PHP-FPM and encode it before passing it to API Gateway. See #288 (comment)

@shrink
Copy link
Author

shrink commented Jun 21, 2019

Maybe an alternative approach would be to set the header in PHP, and then Bref will fetch the raw body from PHP-FPM and encode it before passing it to API Gateway

At that point the application is responsible for identifying which responses that should be encoded so there's little difference in the approach: it would mean that both the application and bref have to be concerned about the need to encode.

I think the options are along these lines:

Responsibility Application Bref Implementation
Mixed x x Bref-Binary header passed to bref to indicate if content should be encoded
Application x Bref passes value of isBase64Encoded header to API Gateway
Bref x Bref identifies based on Content-Type if response should be encoded

If Bref were to determine what to encode based on Content-Type it could accept an optional header value to say do not encode this response to avoid any issues around unintentional encoding if the developer needed more granular control.

I think that either the application should be responsible or Bref, I think the mixed responsibility approach has the same level of developer overhead as the application approach, but introduces complexity in that the developer loses control over the body of their response.

@mnapoli
Copy link
Member

mnapoli commented Sep 26, 2019

Thank you for working on this @shrink. I appreciate the effort, ultimately we went with #457 and we'll see how it goes.

@mnapoli mnapoli closed this Sep 26, 2019
@shrink
Copy link
Author

shrink commented Sep 26, 2019

@mnapoli no problem! glad to see this feature has been added -- I think the chosen solution is a good one. thanks :-)

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.

2 participants