DianaIonita / serverless-api-gateway-caching

A plugin for the Serverless framework which helps with configuring caching for API Gateway endpoints.
ISC License
136 stars 35 forks source link

Change default value of Method RequestParameters from {} to true #132

Closed geetchoubey closed 7 months ago

geetchoubey commented 8 months ago

Per the AWS documentation, method.Properties.RequestParameters only accepts a string;boolean pair object. The issue: When using the plugin like:

events:
  - http:
      path: pathPart
      method: get
      cors: true
      caching:
        enabled: true
        cacheKeyParameters:
          - name: request.querystring.limit
          - name: request.header.Accept
          - name: request.header.Authorization

The plugin modifies the cloudformation:

"ApiGatewayMethodMyApiGet": {
  "Type": "AWS::ApiGateway::Method",
  "Properties": {
    "HttpMethod": "GET",
    "RequestParameters": {
      "method.request.querystring.limit": {},
      "method.request.header.Accept": {},
      "method.request.header.Authorization": {}
    },

Which leads to the following error:

Properties validation failed for resource ApiGatewayMethodMyApiGet with message:
#/RequestParameters/method.request.header.Accept: #: no subschema matched out of the total 2 subschemas
#/RequestParameters/method.request.header.Accept: expected type: Boolean, found: JSONObject
#/RequestParameters/method.request.header.Accept: expected type: String, found: JSONObject
#/RequestParameters/method.request.querystring.limit: #: no subschema matched out of the total 2 subschemas
#/RequestParameters/method.request.querystring.limit: expected type: Boolean, found: JSONObject
#/RequestParameters/method.request.querystring.limit: expected type: String, found: JSONObject
#/RequestParameters/method.request.header.Authorization: #: no subschema matched out of the total 2 subschemas
#/RequestParameters/method.request.header.Authorization: expected type: Boolean, found: JSONObject
#/RequestParameters/method.request.header.Authorization: expected type: String, found: JSONObject

This PR changes the default values to true instead of {} and updates related unit tests.

slatermorgan commented 7 months ago

Currently experiencing this in our deployment pipeline. Why is this suddently an issue? have AWS changed their Cloud Formation validation?

geetchoubey commented 7 months ago

Currently experiencing this in our deployment pipeline. Why is this suddently an issue? have AWS changed their Cloud Formation validation?

That is surprising to our team as well. We have had a service running for a long time until this week we noticed the pipeline failing due to this plugin.

DianaIonita commented 7 months ago

Hi @geetchoubey,

Thanks for raising this issue and creating a fix. I haven't been able to reproduce the problem you've seen, but the plugin works fine with your changes, so I'll just create a release and see what happens 😄

DianaIonita commented 7 months ago

Released in this version.

slatermorgan commented 7 months ago

From the AWS docs linked above:

"The value associated with the key is a Boolean flag indicating whether the parameter is required (true) or optional (false). The method request parameter names defined here are available in Integration to be mapped to integration request parameters or templates."

As this PR changed the default value to true, doesnt this mean that optional parameters I want to add to the cacheKeyParameters block are now always going to be required?

I am seeing in my lambda functions, parameters which were previously not required are now required.

geetchoubey commented 7 months ago

From the AWS docs linked above:

"The value associated with the key is a Boolean flag indicating whether the parameter is required (true) or optional (false). The method request parameter names defined here are available in Integration to be mapped to integration request parameters or templates."

As this PR changed the default value to true, doesnt this mean that optional parameters I want to add to the cacheKeyParameters block are now always going to be required?

I am seeing in my lambda functions, parameters which were previously not required are now required.

You make a valid point and I am not entirely sure how it was dealt before, but I tried out an additional flag required and this is how it worked out.

# serverless.yml
cors: true
caching:
  enabled: true
  cacheKeyParameters:
    - name: request.querystring.required # Final value in API GW: True
      required: true
    - name: request.querystring.notrequired # Final value in API GW:  False
      required: false
    - name: request.querystring.letssee # Final value in API GW:  False
// src/cacheKeyParameters.js
const isRequired = cacheKeyParameter.required || false;
...
method.Properties.RequestParameters[`method.${cacheKeyParameter.name}`] = (existingValue == null || existingValue == undefined) ? isRequired : existingValue;
// cloudformation-stack.json
"ApiGatewayMethodAnotherGet": {
  "Type": "AWS::ApiGateway::Method",
  "Properties": {
    "HttpMethod": "GET",
    "RequestParameters": {
      "method.request.querystring.required": true,
      "method.request.querystring.notrequired": false,
      "method.request.querystring.letssee": false
  },
}