Issue 3821: Only request CloudFormation variables once

[stevearoonie]

What did you implement:

When requesting CloudFormation variables first check if there has already been a request for the same variable and if so return the promise from the first request.

Hotfix for #3821

How did you implement it:

Store the AWS request promises in an instance variable map keyed by stack and variable name. Check the map before going to AWS.

How can we verify it:

To check this, create multiple variables in the serverless.yml all referring to the same CloudFormation variable. e.g.:

custom:
  userPool: ${cf:sls-test-variable.userPool}

functions:
  someFunc1:
    handler: ...
    events:
      - http:
          authorizer: ${self:custom.userPool}
  someFunc2:
    handler: ...
    events:
      - http:
          authorizer: ${self:custom.userPool}
  someFunc3:
    handler: ...
    events:
      - http:
          authorizer: ${self:custom.userPool}

Only one call should be made to AWS.

Todos:

• Write tests
• Write documentation
• Fix linting errors
• Make sure code coverage hasn't dropped
• Provide verification config / commands / resources
• Enable "Allow edits from maintainers" for this PR
• Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

[HyperBrain]

Despite of the lodash and simplification switches, I'm not really sure if this approach is the right solution for the problem, as the promises are cached. Imo the problem should be solved a level higher, so that variable values are cached, and not their fetching promises.
The whole problem does not only affect CF vars (although the symptom becomes visible with them) and we should fix the root of the problem and not the symptoms.

[HyperBrain]

This whole if/if can be turned into a single line with lodash:

_.set(cfVars, `${stackName}${outputLogicalId}`, promise);

This will automatically create all needed path objects if they do not exist.

[stevearoonie]

Thanks for that. You may have guessed I am very new to javascript :)

[HyperBrain]

You should return a promised value here as the function's current signature also returns promises.
return BbPromise.resolve(this.cfVars[stackName][outputLogicalId]);

[stevearoonie]

The cached values are promises so I don't think this is needed.

[HyperBrain]

Can be simplified with lodash:

const cachedVar = _.get(this, `cfVars.${stackName}.${outputLogicalId}`);
if (cachedVar) ...

[stevearoonie]

I initially looked at caching the values themselves instead of the promises but it seemed to me that the code changes required to do this would have been more significant and I wasn't confident I could do it quickly and safely.

Whilst I agree that this is just fixing a symptom I imagine that fixing the underlying problem is a lot more work and this at least allowed me to continue using the CF vars.

[HyperBrain]

I'll remove the "closes" comment in the issue to prevent the issue to disappear. I'm fine with having this as ad-hoc hotfix, so that it is usable again.
We should keep issue #3821 for the deep solution then.

The only thing that's missing right now here is, to add a unit tests that checks that the cache is function (by having a test variable solution that loads multiple variables multiple times and check if the retrieval itself is only called once).

[HyperBrain]

We have a new PR #4499 that is the first part of the solution and introduces a generic caching as first step.
That approach is cleaner because it keep architectural separation, i.e. keeps provider specific implementations out of the generic Variables class.

The AWS specific request optimization and caching should be done within this.serverless.getProvider('aws').request() so that it catches all possible requests to the AWS API.

I'll close this one in favor of the other.