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

Update router and handler to support advanced use case (routing with API Gateway V1/V2) with event driven runtime #11

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

davide-desio-eleva
Copy link

@davide-desio-eleva davide-desio-eleva commented Apr 15, 2024

Hi, is this package still mantained?

As for Bref documentation we should be able to use this repo if we are running event driven functions for an HTTP Application routed by API Gateway (https://bref.sh/docs/local-development/event-driven-functions#api-gateway-local-development), to emulate API Gateway locally.

Basically if we are in this scenario https://bref.sh/docs/use-cases/http/advanced-use-cases#with-the-event-driven-function-runtime (as an example developing a REST API) using a simple function like

<?php

use Bref\Context\Context;
use Nyholm\Psr7\Response;

require 'vendor/autoload.php';

class Handler implements \Bref\Event\Handler
{
    public function handle($event, ?Context $context): Response
    {
        $attributes = $event->getAttributes();
        $queryParams = $event->getQueryParams();
        $parsedBody = $event->getParsedBody();

        $message = [
            "message" =>'Bref! Your function executed successfully!',
            "context" => $context,
            "input" => [
                "attributes"=>$attributes,
                "queryParams"=>$queryParams,
                "parsedBody"=>$parsedBody
            ]
        ];

        $status = 200;

        return new Response($status, [], json_encode($message));
    }
}

return new Handler();

As it seems to me that I can't get serverless-offline work with Bref (I've tried without success mostly beacuse provided.al2 is not a supported runtime, and I've read a bunch of topic that is not on the roadmap), and as stated by documentation "serverless bref:local is a bit unpractical because you need to manually craft HTTP events in JSON", I've done some changes in this repo source code:

  • In router, I've added a function which parse all events (not only the first one), so that a warmer or other triggers won't be a problem if added before an http event. I've also added a line to recognize "http" events for API Gateway V1 routing, so that serverless.yaml will be parsed also for those (not only for "httpApi" events for API Gateway V2 routing).
  • In router, I've added another function to parse separated file inclusion for serverless.yml if servelress syntax to include file is recognized ( ${file(...)} )
  • In handler, I've passed a second parameter to the handler to get "Context" from "lambda-context" attribute as stated here https://bref.sh/docs/use-cases/http/advanced-use-cases#lambda-event-and-context

This should improve this codebase so that this dev-server is working with a definition like this

Root serverless.yml

## Define atomic functions
functions:
  ## Hello function
  - ${file(./src/function/hello/serverless.yml)}

Separate /src/function/hello/serverless.yml

hello:
  handler: src/function/hello/index.php #function handler
  package: #package patterns
    include:
      - "!**/*"
      - src/function/hello/**
  events: #events
    #keep warm event
    - schedule:
        rate: rate(5 minutes)
        enabled: ${strToBool(${self:custom.scheduleEnabled.${env:STAGE_NAME}})}
        input:
          warmer: true
    #api gateway event
    - http:
        path: /hello #api endpoint path
        method: 'GET' #api endpoint method
        cors: true
        caching: #cache
          enabled: false
        documentation:
          summary: "/hello"
          description: "Just a sample GET to say hello"
          methodResponses:
            - statusCode: 200
              responseBody:
                description: "A successful response"
              responseModels:
                application/json: "HelloResponse"
            - statusCode: 500
              responseBody:
                description: "Internal Server Error"
              responseModels:
                application/json: "ErrorResponse"
            - statusCode: 400
              responseBody:
                description: "Request error"
              responseModels:
                application/json: "BadRequestResponse"

Hope this helps.

ddesioeleva added 7 commits April 15, 2024 12:31
- it should work with http (V1) as well as httpApi (V2) events
- it should work whatever is the index of the http event in events list
- it should work if function is contained is a separate file with serverless syntax
@davide-desio-eleva
Copy link
Author

Updates:

  • I've fixed CS
  • I've added tests for serverless.yml file for function (included in general one)
  • I've fixed routing for path parameters (there was an original test failing due to a wrong match on pattern for routes starting with a slash)

CI should be ok now 🚀

@davide-desio-eleva
Copy link
Author

Other updates:

  • I've added a line to let it work with event driver runtime (which expect an exit as per lambda function proxy integration)
  • I've updated doc per function example

@davide-desio-eleva davide-desio-eleva changed the title Update router and handler Update router and handler to support advanced use case (routing with API Gateway V1/V2) with event driven runtime Apr 16, 2024
@davide-desio-eleva
Copy link
Author

@mnapoli have you seen this PR?

@mnapoli
Copy link
Member

mnapoli commented Apr 27, 2024

oh sorry I completely missed that in my notifications.

This PR is huge, addressing multiple things at once. This is unlikely to be merged as-is unfortunately (like for any other open-source project).

In router, I've added a function which parse all events (not only the first one), so that a warmer or other triggers won't be a problem if added before an http event.

Great!

I've also added a line to recognize "http" events for API Gateway V1 routing, so that serverless.yaml will be parsed also for those (not only for "httpApi" events for API Gateway V2 routing).

If it's just one extra line of code, sounds good to me. That's not something I want to add or maintain, but if it's only 1 line and it's working, then 👍

In router, I've added another function to parse separated file inclusion for serverless.yml if servelress syntax to include file is recognized ( ${file(...)} )

This is too much code to merge unfortunately, I'd rather not have to maintain this.

In handler, I've passed a second parameter to the handler to get "Context" from "lambda-context" attribute as stated here https://bref.sh/docs/use-cases/http/advanced-use-cases#lambda-event-and-context

I don't think that works, PSR-15 handlers don't know about AWS Lambda, they don't have a "context" parameter. Or am I missing something?

I've fixed routing for path parameters (there was an original test failing due to a wrong match on pattern for routes starting with a slash)

Great!

I've added a line to let it work with event driver runtime (which expect an exit as per lambda function proxy integration)

I'm not sure what you mean by that?

I've updated doc per function example

Can you please remove the docs addition? Bref HTTP handlers are not supposed to be written that way, I'd rather not encourage readers to follow it.

@davide-desio-eleva
Copy link
Author

davide-desio-eleva commented Apr 29, 2024

Hi @mnapoli, don't worry, I can close this and have multiple little PRs, if you want me to split them.
Those changes were made in my fork to let me use this dev-server as my local environment, and this PR is just to let you know I've faced some situation which could be useful for others.

Let's try to split this on topics.

Event handling/parsing

  1. Multiple event parsing: if it is ok, I'll do a PR for just this part. It is ok to call it "Parse multiple events in functions"?
  2. Yes its about a single line, in my opinion it should be possible to support both "http" or "httpApi" events as those are just alternative architectures on AWS (even if V1 and V2 seems to suggest V2 is the top choice, v2 offers less capabilities than V1 and a lot of users will continue to choose V1 over V2). Is this ok to include this line in the previous PR?

Separate file handling/parsing
3) Also in this case, is just a matter of a bunch of lines of code (a function) to understand if a "file" has been included. As serverless framework support it and it's a good practice to split files to increase code readability, it's very likely to happen to have a separate file for each function (as you don't want your source code to have a very big serverless file configuring everything). Severless offline is working in this scenario, as an example.
I totally understand you don't want to maintain anything that is out of scope, but imho this will improve the scenario in which dev-server could be used.
Do you want me to make a specific PR to better evaluate it?

Lambda Context
4) Context: you're right, I've probably added something not necessary tricked by the base example. I'll check it but I think I could totally remove this line.

Wrong test
5) What about the fixed test, should I do a separate PR?

Function Runtime / Lambda Proxy Integration
6) This line aa25288. This is because the handler of point 7. In Lambda, anyone can write a function as a lambda proxy integration which return a simple json with specific attributes (statusCode, headers, body). If this is the case (and this is working in the cloud as I've tested it), shouldn't we give a way to use dev-server anyway to support local development?
7) Is there a suggestion on how to write the HTTP handler as you would expect? It would be great to give a structured example so that people understand what to do. Isn't the one included the least complex one for function written as a lambda proxy integration? Would you suggest another implementation?

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.

None yet

2 participants