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

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

Merged
merged 2 commits into from
Nov 11, 2023

Conversation

geetchoubey
Copy link
Contributor

@geetchoubey geetchoubey commented Nov 8, 2023

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.

@geetchoubey geetchoubey changed the title Change default value of Method RequestParameters from {} to false Change default value of Method RequestParameters from {} to true Nov 8, 2023
@slatermorgan
Copy link

slatermorgan commented Nov 9, 2023

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

@geetchoubey
Copy link
Contributor Author

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 DianaIonita self-requested a review November 11, 2023 18:05
@DianaIonita
Copy link
Owner

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 DianaIonita merged commit afb8117 into DianaIonita:develop Nov 11, 2023
@DianaIonita
Copy link
Owner

Released in this version.

@slatermorgan
Copy link

slatermorgan commented Nov 13, 2023

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
Copy link
Contributor Author

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
  },
}

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.

3 participants