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 support for multiple HTTP events and custom function name #18

Merged
merged 4 commits into from
Dec 13, 2018

Conversation

LuciCondescu
Copy link
Contributor

This pull-request introduces:

  • support for multiple HTTP events on the same lambda function
  • allows custom name for the lambda function

Copy link
Owner

@DianaIonita DianaIonita left a comment

Choose a reason for hiding this comment

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

Hey @LuciCondescu, thanks for submitting this.
Looks like path parameter caching isn't being configured properly for the second endpoint. Here's the config I tried:

    events:
      - http:
          path: /cats/{pawId}
          method: get
          caching:
            enabled: true
            ttlInSeconds: 60
            perKeyInvalidation:
              requireAuthorization: true
              handleUnauthorizedRequests: Ignore
            cacheKeyParameters:
              - name: request.path.pawId
              - name: request.header.Accept-Language
      - http:
          path: /cats/{pawId}
          method: delete
          caching:
            enabled: true
            ttlInSeconds: 60
            perKeyInvalidation:
              requireAuthorization: true
              handleUnauthorizedRequests: Ignore
            cacheKeyParameters:
              - name: request.path.pawId
              - name: request.header.Accept-Language

When I looked in API Gateway, I found that the DELETE method didn't have the cache check box ticked for the pawId path parameter and there was no mention of the Accept-Language header.

It's probably because of this piece of code:

// returns the first method found which depends on this lambda
  const methods = getResourcesByType('AWS::ApiGateway::Method', serverless);
  for (let method of methods) {
    let stringified = JSON.stringify(method);
    if (stringified.lastIndexOf(`"${lambdaFunctionResource.name}"`) != -1) {
      return method;
    }
  }

which just finds the first method - in my case the GET - and attaches path parameter config to it. It probably needs to be smarter to take into account the HTTP method as well.

src/stageCache.js Outdated Show resolved Hide resolved
src/stageCache.js Show resolved Hide resolved
src/stageCache.js Outdated Show resolved Hide resolved
@LuciCondescu
Copy link
Contributor Author

@DianaIonita,

Thanks for pointing me into the right direction. Indeed, I missed a few things.

After you suggestion, I realized the I need to find the specific ApiGateway::Method based on the HTTP method and the endpoint path (because there can be cases when the same lambda has 2 HTTP events of the same type). That's why I had to change the way ApiGateway::Methods where looked up into the CloudFormation template. Now, I am computing the name of the method resource and I am searching directly for it.

I did quite a few tests and everything looks good. However, I couldn't test with your exact serverless template, so it would be great if you can take a second look.

Luci

@minghuijiang
Copy link

IMHO, you can use the function name without specify custom_function name
In pathParametersCashe.js getApiGatewayMethodFor
const fullFunctionName = ${serverless.service.service}-${stage}-${functionName};
can be change to
const fullFunctionName = serverless.service.functions[functionName]['name']

@LuciCondescu
Copy link
Contributor Author

IMHO, you can use the function name without specify custom_function name
In pathParametersCashe.js getApiGatewayMethodFor
const fullFunctionName = ${serverless.service.service}-${stage}-${functionName};
can be change to
const fullFunctionName = serverless.service.functions[functionName]['name']

Hi @minghuijiang,

Actually with the last commit I've changed the way the Gateway::Method is searched for and I had to adapt that function.

Luci

Copy link
Owner

@DianaIonita DianaIonita left a comment

Choose a reason for hiding this comment

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

Hey @LuciCondescu,
Apologies for the delay. PR looks good, I tested it on my code and path parameters are correctly configured.
Thank you for your contribution!

@DianaIonita DianaIonita merged commit be3a590 into DianaIonita:develop Dec 13, 2018
@LuciCondescu
Copy link
Contributor Author

No worries, thank you to for creating this plugin in the first place :)

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

3 participants