Add optional required configuration to parameters

[cdubz]

Changes made in #132 made all cacheKeyParameters values required by default but previously this was not the case. This breaks functions that rely on cache key parameters based on optional headers/query strings/etc.

This is an implementation of @geetchoubey's recommendation from #132 (comment).

This change allows a config like this:

# 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

And the required value defaults to false so backwards compatibility is maintained with versions before this bug was introduced in 1.10.3.

Fixes #133

[geetchoubey]

Thanks for implementing this @cdubz. You may want to update the docs for this as well.

[cdubz]

Documentation added. We should add a test as well.

[DianaIonita]

Hi @cdubz and @geetchoubey,

Thank you so much for raising this PR. I haven't been able to reproduce the issue you're having (for me, endpoints don't error when a cache key parameter is missing), but I think I understand what's going on.

I'm not convinced that adding an extra "required" boolean parameter alongside cache key parameter names is the solution, because there's already an out-of-the-box serverless feature to toggle whether parameters are required.

A sample from this yaml:

functions:
  hello:
    events:
      # REST API endpoint (API Gateway v1)
      - http:
          path: users/create
          method: get
          request:
            parameters:
              paths:
                paramName: true # mark path parameter as required
              headers:
                headerName: true # mark header as required
              querystrings:
                paramName: true # mark query string

I had a quick play around since I found the settings above, and it looks like they're applied after this plugin does its thing. So, if this plugin defaults to "false", then they can be marked as required by code like the above.
Could you please confirm whether these request parameter settings work for you?

[EricVentor]

Any updates on this 👀

[cdubz]

I am intending to test @DianaIonita's recommendation but just haven't had the time yet 😭

In our current config we have some optional parameters that are just not listed in the request config. I'm hoping adding them and marking them as specifically optional will resolve this issue.

Here's an example real endpoint we have configured:

showDetail:
  handler: handler.router
  timeout: 15
  events:
    - http:
        path: shows/{showUuid}
        method: get
        caching:
          enabled: true
          cacheKeyParameters:
            - name: request.header.x-canada-filter
            - name: request.path.showUuid
        private: true

In this case the request.header.x-canada-filter may or may not be present and we fall back on a default value if it's not. I'm hoping that adding this will resolve the issue:

request:
  parameters:
    headers:
      x-canada-filter: false

[EricVentor]

@DianaIonita currently there already is a required: boolean param alongside the cache key parameter. As you can see in this comment from #132. If I'm understanding all this correctly: this PR was aimed to mitigate the (accidental) breaking change caused by #132 ... with I guess another breaking change.

I believe the change you're proposing is a separate breaking change to the current configuration spec.

My 2 cents, merge this as is. It will remedy the breaking change that occurred from #132. Therefore all the people that have been using this package prior to 1.10.3 will be in a good state again. (Sorry to the people that are relying on 1.10.3's default required: true configuration. 🤷)

Then the configuration change @DianaIonita is proposing can perhaps come in a correctly semver'd 2.x.x version?

[DianaIonita]

Hi everyone,

This is my suggestion:

• set the default of not required on cache key parameters
• if anyone wants to require cache key parametres, they can do it via the already existing serverless framework config (events.http.request.parameters)

This way, we avoid the confusion of having two config options that do the same thing. It also doesn't need the developer to remember any special behaviours of this caching plugin.

[josh-feather]

@DianaIonita I created #138 to implement your suggestions.

[DianaIonita]

Thanks everyone. I just created release 1.10.4 which includes @josh-feather's change of making cache key parameter not required by default.