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 for HTTP API #602 #604

Merged
merged 23 commits into from
Jun 11, 2020
Merged

Conversation

victormacko
Copy link
Contributor

@victormacko victormacko commented Mar 29, 2020

I've added in support for HTTP v2.0 payloads primarily to have Bref give a "works out of the box" experience when linking Lambda with the new HTTP API via the AWS Console, rather than having to switch back to 1.0 in the integrations section in the console.

Also - i'm pretty keen to start using the new API Gateway HTTP option so hopefully this helps getting Bref one more step closer to it :) ... this should also help with issue #602

I'd started looking at the tests to add but was wanting some guidance on it first -- I think the best approach is to add another FpmHandlerTest class for the v2 payloads as most of the test-cases are probably going to be worth testing...

The changes i've made cover;

  • Adding a 'payloadVersion' in the request & response classes (the response classes needed it as the multi-header option doesn't exist in v2)
  • Handling cookies in the FpmHandler & HttpRequestEvent classes (they're passed in separately and not as a HTTP Header as far as I can tell)
  • Updated how the method is obtained (var names have changed)
  • Updated how the path is obtained (var names have changed)
  • Updated how the query-string is obtained (var names have changed) (the multiValueQueryStringParameters no longer exists either in v2)

The unit-tests all pass, but they haven't as yet been updated to test for the v2 payloads -- need some guidance (as above) on this before spending time on getting that sorted.

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

Thank you, this looks really great! (I have added quite a few of inline comments, but these are mostly details, the whole PR looks really good!)

As for the tests, yes, this is a tricky one. Let me have a look at what's best 🤔

src/Event/Http/HttpResponse.php Outdated Show resolved Hide resolved
src/Event/Http/HttpRequestEvent.php Show resolved Hide resolved
src/Event/Http/HttpRequestEvent.php Outdated Show resolved Hide resolved
src/Event/Http/HttpRequestEvent.php Outdated Show resolved Hide resolved
src/Event/Http/HttpRequestEvent.php Outdated Show resolved Hide resolved
@mnapoli
Copy link
Member

mnapoli commented Apr 5, 2020

Regarding the tests, here is what I noted:

  • Bref\Test\Event\Http\HttpResponseTest: unit test, should be easy to update
    You can duplicate existing tests and name them with v1 and v2 in the test name to differentiate. If some tests are identicals (the behavior is identical between v1 and v2), then don't duplicate the test.
  • Bref\Test\Event\Http\CommonHttpTest: another unit test for the requests, this one is a bit more complex because it's an abstract class.
    This class uses JSON examples for the tests. What you can do is duplicate all JSON files to have them in v1 and v2 formats.

I have pushed a new commit to your pull request to start adding tests in CommonHttpTest. Hopefully that should help you move forward. If you still don't understand, let me know and I'll try to find time to complete them.

@victormacko
Copy link
Contributor Author

Ace, thanks so much for your help on those tests -- i'll have a proper look through it once i've made the other code changes and got them committed.

@mnapoli
Copy link
Member

mnapoli commented Apr 5, 2020

Awesome thanks!

@victormacko
Copy link
Contributor Author

RE tests @mnapoli , the Bref\Test\Handler\FpmHandlerTest doesn't have the 'version' set on any of the data it's testing ... do you have a preferred approach ... default to v1 in the HttpRequestEvent or just add in the 'version' to each of the tests? (not sure if you wanted to keep the tests small or not).

I'll finish tackling the v2 JSON files tomorrow -- i'm almost done :)

@mnapoli
Copy link
Member

mnapoli commented Apr 6, 2020

Regarding FpmHandlerTest yes keep v1, that's definitely sufficient given how much is covered elsewhere. I'll have a look at the rest ASAP, that's awesome.

@mnapoli
Copy link
Member

mnapoli commented Apr 6, 2020

At first look that sounds really good. The build seems to be failing, but we are definitely on the right track.

@victormacko
Copy link
Contributor Author

Ace!
I've updated the FPM tests so there's only 4 overall failing (just 2 issues really);

  • 2 for the multi-headers (AWS concatenates multi-headers a single string, separated by a ',' (not url-encoding ... so commas inside user-agent headers, etc may get handled incorrectly if we were to try and separate them out just using the ',')
  • 2 for multi-query-string (eg. a=abc&a=def) ... the tests expect the rawQueryString to be url-encoded ... and using parse_str & http_build_query together don't handle int-indexed arrays as well as the unit-tests expect :/

Hope that helps -- not quite sure on what approach to take for those two.

- the query string encoding should not make any practical difference, because both encoding are semantically identical
- the multi-value header seems unfortunately like an issue we cannot solve in v2, because all values are flatten in a string with a `,` as a separator: splitting that string again is prone to mistakes, because commas can appear in header values (see https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html); I have asked for confirmation from AWS, we'll see
@mnapoli
Copy link
Member

mnapoli commented Jun 11, 2020

OK @victormacko I think we are good here!

@mnapoli mnapoli merged commit b91ffc7 into brefphp:master Jun 11, 2020
@mnapoli
Copy link
Member

mnapoli commented Jun 11, 2020

Thank you so much for all that work, that is massive. Time to try that!

@mnapoli mnapoli mentioned this pull request Jun 11, 2020
mnapoli added a commit that referenced this pull request Feb 14, 2023
mnapoli added a commit that referenced this pull request Feb 14, 2023
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.

None yet

3 participants