From 0ee0a1e239bddaac39e798dcaa6ee3c75e2f585c Mon Sep 17 00:00:00 2001
From: Erik Erikson <erik.m.erikson@nordstrom.com>
Date: Sat, 24 Feb 2018 09:34:50 -0800
Subject: [PATCH] Only disable and restore dependent service resolution methods
 once.  Otherwise, subsequent removals may cache the previous replacements. 
 If they restore last then they will restore with the replacements, breaking
 standard usage. Add tests for this guarantee.

---
 lib/classes/Variables.js      | 23 +++++++++++++----------
 lib/classes/Variables.test.js | 29 +++++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/lib/classes/Variables.js b/lib/classes/Variables.js
index 4926f31de..764b34924 100644
--- a/lib/classes/Variables.js
+++ b/lib/classes/Variables.js
@@ -72,25 +72,24 @@ class Variables {
   // #############
   // ## SERVICE ##
   // #############
-  prepopulateObject(objectToPopulate) {
+  disableDepedentServices(func) {
     const dependentServices = [
       { name: 'CloudFormation', method: 'getValueFromCf', original: this.getValueFromCf },
       { name: 'S3', method: 'getValueFromS3', original: this.getValueFromS3 },
       { name: 'SSM', method: 'getValueFromSsm', original: this.getValueFromSsm },
     ];
-    const dependencyMessage = (configName, configValue, serviceName) =>
-      `Variable Failure: value ${configName} set to '${configValue}' references ${
-        serviceName} which requires a ${configName} value for use.`;
+    const dependencyMessage = (configValue, serviceName) =>
+      `Variable dependency failure: variable '${configValue}' references service ${
+        serviceName} but using that service requires a concrete value to be called.`;
     // replace and then restore the methods for obtaining values from dependent services.  the
     // replacement naturally rejects dependencies on these services that occur during prepopulation.
     // prepopulation is, of course, the process of obtaining the required configuration for using
     // these services.
     dependentServices.forEach((dependentService) => { // knock out
       this[dependentService.method] = (variableString) => BbPromise.reject(
-        dependencyMessage(objectToPopulate.name, variableString, dependentService.name));
+        dependencyMessage(variableString, dependentService.name));
     });
-    return this.populateValue(objectToPopulate.value, true) // populate
-      .then(populated => _.assign(objectToPopulate, { populated }))
+    return func()
       .finally(() => {
         dependentServices.forEach((dependentService) => { // restore
           this[dependentService.method] = dependentService.original;
@@ -100,12 +99,16 @@ class Variables {
   prepopulateService() {
     const provider = this.serverless.getProvider('aws');
     if (provider) {
-      const requiredConfig = [
+      const requiredConfigs = [
         _.assign({ name: 'region' }, provider.getRegionSourceValue()),
         _.assign({ name: 'stage' }, provider.getStageSourceValue()),
       ];
-      const prepopulations = requiredConfig.map(this.prepopulateObject.bind(this));
-      return this.assignProperties(provider, prepopulations);
+      return this.disableDepedentServices(() => {
+        const prepopulations = requiredConfigs.map(config =>
+          this.populateValue(config.value, true) // populate
+            .then(populated => _.assign(config, { populated })));
+        return this.assignProperties(provider, prepopulations);
+      });
     }
     return BbPromise.resolve();
   }
diff --git a/lib/classes/Variables.test.js b/lib/classes/Variables.test.js
index 8962814e3..5d523092a 100644
--- a/lib/classes/Variables.test.js
+++ b/lib/classes/Variables.test.js
@@ -169,7 +169,7 @@ describe('Variables', () => {
           it(`should reject ${config.name} variables in ${property.name} values`, () => {
             awsProvider.options[property.name] = config.value;
             return serverless.variables.populateService()
-              .should.be.rejectedWith('Variable Failure');
+              .should.be.rejectedWith('Variable dependency failure');
           });
           it(`should reject recursively dependent ${config.name} service dependencies`, () => {
             serverless.variables.service.custom = {
@@ -177,11 +177,36 @@ describe('Variables', () => {
             };
             awsProvider.options.region = '${self:custom.settings.region}';
             return serverless.variables.populateService()
-              .should.be.rejectedWith('Variable Failure');
+              .should.be.rejectedWith('Variable dependency failure');
           });
         });
       });
     });
+    describe('dependent service non-interference', () => {
+      const stateCombinations = [
+        { region: 'foo', state: 'bar' },
+        { region: 'foo', state: '${self:bar, "bar"}' },
+        { region: '${self:foo, "foo"}', state: 'bar' },
+        { region: '${self:foo, "foo"}', state: '${self:bar, "bar"}' },
+      ];
+      stateCombinations.forEach((combination) => {
+        it('must leave the dependent services in their original state', () => {
+          const dependentMethods = [
+            { name: 'getValueFromCf', original: serverless.variables.getValueFromCf },
+            { name: 'getValueFromS3', original: serverless.variables.getValueFromS3 },
+            { name: 'getValueFromSsm', original: serverless.variables.getValueFromSsm },
+          ];
+          awsProvider.options.region = combination.region;
+          awsProvider.options.state = combination.state;
+          return serverless.variables.populateService().should.be.fulfilled
+            .then(() => {
+              dependentMethods.forEach((method) => {
+                expect(serverless.variables[method.name]).to.equal(method.original);
+              });
+            });
+        });
+      });
+    });
   });
 
   describe('#getProperties', () => {