Repository: serverless/serverless-azure-functions (ts)

Issue:
Invoke throws 400 error when using `-p` option
Invoke works fine when using `-d` and passing the stringified JSON, but fails when using a path to sample data

Pull request:
fix: Fix failing invoke with data path
Moves data path initialization outside of the constructor and does not replace the `data` property within options. Instead, passes the contents of the file path to the invoke service.

Fixes #320 

Patch:
diff --git a/README.md b/README.md
index 19a2994..c816b05 100644
--- a/README.md
+++ b/README.md
@@ -90,7 +90,19 @@ $ sls invoke -f <functionName>
 After deploying template function app, run
 
 ```bash
-$ sls invoke -f hello '{"name": "Azure"}'
+$ sls invoke -f hello -d '{"name": "Azure"}'
+```
+
+If you have a JSON object in a file, you could run
+
+```bash
+$ sls invoke -f hello -p data.json
+```
+
+If you have your service running locally (in another terminal), you can run:
+
+```bash
+$ sls invoke local -f hello -p data.json
 ```
 
 ### Roll Back Your Function App
diff --git a/package-lock.json b/package-lock.json
index 570ce26..6ab0ba6 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,6 +1,6 @@
 {
   "name": "serverless-azure-functions",
-  "version": "1.0.0-41",
+  "version": "1.0.0-44",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
diff --git a/package.json b/package.json
index 2af0d9b..411ca0e 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "serverless-azure-functions",
-  "version": "1.0.0-41",
+  "version": "1.0.0-44",
   "description": "Provider plugin for the Serverless Framework v1.x which adds support for Azure Functions.",
   "license": "MIT",
   "main": "./lib/index.js",
diff --git a/src/armTemplates/compositeArmTemplate.ts b/src/armTemplates/compositeArmTemplate.ts
index d79c084..199ba04 100644
--- a/src/armTemplates/compositeArmTemplate.ts
+++ b/src/armTemplates/compositeArmTemplate.ts
@@ -41,7 +41,6 @@ export class CompositeArmTemplate implements ArmResourceTemplateGenerator {
         ...parameters,
         ...resource.getParameters(config),
         location: {
-          type: ArmParamType.String,
           value: AzureNamingService.getNormalizedRegionName(config.provider.region)
         }
       };
diff --git a/src/armTemplates/resources/apim.ts b/src/armTemplates/resources/apim.ts
index 3544fba..2576046 100644
--- a/src/armTemplates/resources/apim.ts
+++ b/src/armTemplates/resources/apim.ts
@@ -74,23 +74,18 @@ export class ApimResource implements ArmResourceTemplateGenerator {
 
     return {
       apiManagementName: {
-        type: ArmParamType.String,
         value: ApimResource.getResourceName(config),
       },
       apimSkuName: {
-        type: ArmParamType.String,
         value: apimConfig.sku.name,
       },
       apimSkuCapacity: {
-        type: ArmParamType.Int,
         value: apimConfig.sku.capacity,
       },
       apimPublisherEmail: {
-        type: ArmParamType.String,
         value: apimConfig.publisherEmail,
       },
       apimPublisherName: {
-        type: ArmParamType.String,
         value: apimConfig.publisherName,
       }
     };
diff --git a/src/armTemplates/resources/appInsights.ts b/src/armTemplates/resources/appInsights.ts
index bba0480..0a4498a 100644
--- a/src/armTemplates/resources/appInsights.ts
+++ b/src/armTemplates/resources/appInsights.ts
@@ -46,7 +46,6 @@ export class AppInsightsResource implements ArmResourceTemplateGenerator {
   public getParameters(config: ServerlessAzureConfig): ArmParameters {
     return {
       appInsightsName: {
-        type: ArmParamType.String,
         value: AppInsightsResource.getResourceName(config),
       }
     };
diff --git a/src/armTemplates/resources/appServicePlan.ts b/src/armTemplates/resources/appServicePlan.ts
index 05406f4..670aee7 100644
--- a/src/armTemplates/resources/appServicePlan.ts
+++ b/src/armTemplates/resources/appServicePlan.ts
@@ -65,15 +65,12 @@ export class AppServicePlanResource implements ArmResourceTemplateGenerator {
 
     return {
       appServicePlanName: {
-        type: ArmParamType.String,
         value: AppServicePlanResource.getResourceName(config),
       },
       appServicePlanSkuName: {
-        type: ArmParamType.String,
         value: resourceConfig.sku.name,
       },
       appServicePlanSkuTier: {
-        type: ArmParamType.String,
         value: resourceConfig.sku.tier,
       }
     }
diff --git a/src/armTemplates/resources/functionApp.ts b/src/armTemplates/resources/functionApp.ts
index 1ce2985..9d16299 100644
--- a/src/armTemplates/resources/functionApp.ts
+++ b/src/armTemplates/resources/functionApp.ts
@@ -126,19 +126,15 @@ export class FunctionAppResource implements ArmResourceTemplateGenerator {
 
     return {
       functionAppName: {
-        type: ArmParamType.String,
         value: FunctionAppResource.getResourceName(config),
       },
       functionAppNodeVersion: {
-        type: ArmParamType.String,
         value: resourceConfig.nodeVersion,
       },
       functionAppWorkerRuntime: {
-        type: ArmParamType.String,
         value: resourceConfig.workerRuntime,
       },
       functionAppExtensionVersion: {
-        type: ArmParamType.String,
         value: resourceConfig.extensionVersion,
       }
     };
diff --git a/src/armTemplates/resources/hostingEnvironment.ts b/src/armTemplates/resources/hostingEnvironment.ts
index f2b650f..06a08c9 100644
--- a/src/armTemplates/resources/hostingEnvironment.ts
+++ b/src/armTemplates/resources/hostingEnvironment.ts
@@ -68,7 +68,6 @@ export class HostingEnvironmentResource implements ArmResourceTemplateGenerator
   public getParameters(config: ServerlessAzureConfig): ArmParameters {
     return {
       hostingEnvironmentName: {
-        type: ArmParamType.String,
         value: HostingEnvironmentResource.getResourceName(config)
       }
     }
diff --git a/src/armTemplates/resources/storageAccount.ts b/src/armTemplates/resources/storageAccount.ts
index 734fd60..cf8ea2f 100644
--- a/src/armTemplates/resources/storageAccount.ts
+++ b/src/armTemplates/resources/storageAccount.ts
@@ -66,15 +66,12 @@ export class StorageAccountResource implements ArmResourceTemplateGenerator {
 
     return {
       storageAccountName: {
-        type: ArmParamType.String,
         value: StorageAccountResource.getResourceName(config),
       },
       storageAccountSkuName: {
-        type: ArmParamType.String,
         value: resourceConfig.sku.name,
       },
       storageAccoutSkuTier: {
-        type: ArmParamType.String,
         value: resourceConfig.sku.tier,
       }
     };
diff --git a/src/armTemplates/resources/virtualNetwork.ts b/src/armTemplates/resources/virtualNetwork.ts
index 9092eb8..921d658 100644
--- a/src/armTemplates/resources/virtualNetwork.ts
+++ b/src/armTemplates/resources/virtualNetwork.ts
@@ -86,7 +86,6 @@ export class VirtualNetworkResource implements ArmResourceTemplateGenerator {
   public getParameters(config: ServerlessAzureConfig): ArmParameters {
     return {
       virtualNetworkName: {
-        type: ArmParamType.String,
         value: VirtualNetworkResource.getResourceName(config),
       }
     }
diff --git a/src/models/armTemplates.ts b/src/models/armTemplates.ts
index 778d666..6f10135 100644
--- a/src/models/armTemplates.ts
+++ b/src/models/armTemplates.ts
@@ -46,7 +46,7 @@ export interface ArmResourceTemplate {
 
 export interface ArmParameters {
   [key: string]: {
-    type: ArmParamType;
+    type?: ArmParamType;
     value?: string | number;
     defaultValue?: string | number;
   };
diff --git a/src/plugins/invoke/azureInvokePlugin.ts b/src/plugins/invoke/azureInvokePlugin.ts
index ae2f117..91e9829 100644
--- a/src/plugins/invoke/azureInvokePlugin.ts
+++ b/src/plugins/invoke/azureInvokePlugin.ts
@@ -3,25 +3,12 @@ import { isAbsolute, join } from "path";
 import Serverless from "serverless";
 import { InvokeService } from "../../services/invokeService";
 import { AzureBasePlugin } from "../azureBasePlugin";
+import path from "path";
 
 export class AzureInvokePlugin extends AzureBasePlugin {
 
   public constructor(serverless: Serverless, options: Serverless.Options) {
     super(serverless, options);
-    const path = this.options["path"];
-
-    if (path) {
-      const absolutePath = isAbsolute(path)
-        ? path
-        : join(this.serverless.config.servicePath, path);
-      this.log(this.serverless.config.servicePath);
-      this.log(path);
-
-      if (!fs.existsSync(absolutePath)) {
-        throw new Error("The file you provided does not exist.");
-      }
-      this.options["data"] = fs.readFileSync(absolutePath).toString();
-    }
 
     this.commands = {
       invoke: {
@@ -108,7 +95,6 @@ export class AzureInvokePlugin extends AzureBasePlugin {
 
   private async invoke(local: boolean = false) {
     const functionName = this.options["function"];
-    const data = this.options["data"];
     const method = this.options["method"] || "GET";
     if (!functionName) {
       this.log("Need to provide a name of function to invoke");
@@ -116,9 +102,21 @@ export class AzureInvokePlugin extends AzureBasePlugin {
     }
 
     const invokeService = new InvokeService(this.serverless, this.options, local);
-    const response = await invokeService.invoke(method, functionName, data);
+    const response = await invokeService.invoke(method, functionName, this.getData());
     if (response) {
       this.log(JSON.stringify(response.data));
     }
   }
+
+  private getData() {
+    let dataPath = this.getOption("path");
+    if (dataPath) {
+      dataPath = (isAbsolute(dataPath)) ? dataPath : path.join(this.serverless.config.servicePath, dataPath);
+      if (!fs.existsSync(dataPath)) {
+        throw new Error("The file you provided does not exist.");
+      }
+      return fs.readFileSync(dataPath).toString();
+    }
+    return this.getOption("data");
+  }
 }
diff --git a/src/services/armService.ts b/src/services/armService.ts
index 9a06782..a574c01 100644
--- a/src/services/armService.ts
+++ b/src/services/armService.ts
@@ -95,13 +95,19 @@ export class ArmService extends BaseService {
     this.applyEnvironmentVariables(deployment);
 
     const resourceService = new ResourceService(this.serverless, this.options);
-    const previous = await resourceService.getLastDeploymentTemplate();
+    const previous = await resourceService.getPreviousDeploymentTemplate();
 
     if (this.areDeploymentsEqual(deployment, previous)) {
       this.log("Generated template same as previous. Skipping ARM deployment");
       return;
     }
 
+    for (const key of Object.keys(deployment.parameters)) {
+      if (!deployment.parameters[key].value) {
+        delete deployment.parameters[key];
+      }
+    }
+
     // Construct deployment object
     const armDeployment: Deployment = {
       properties: {
@@ -124,11 +130,14 @@ export class ArmService extends BaseService {
       this.log("-> ARM deployment complete");
       return result;
     } catch (err) {
-      const lastDeployment = await resourceService.getLastDeployment();
-      const errorDetails: DeploymentExtendedError = lastDeployment.properties["error"];
-      if (errorDetails) {
-        throw new Error(this.deploymentErrorToString(errorDetails));
+      const previousDeployment = await resourceService.getPreviousDeployment();
+      if (previousDeployment) {
+        const errorDetails: DeploymentExtendedError = previousDeployment.properties["error"];
+        if (errorDetails) {
+          throw new Error(this.deploymentErrorToString(errorDetails));
+        }
       }
+      throw err;
     }
   }
 
diff --git a/src/services/offlineService.ts b/src/services/offlineService.ts
index e085c63..2664e82 100644
--- a/src/services/offlineService.ts
+++ b/src/services/offlineService.ts
@@ -4,6 +4,7 @@ import Serverless from "serverless";
 import configConstants from "../config";
 import { BaseService } from "./baseService";
 import { PackageService } from "./packageService";
+import path from "path";
 
 export class OfflineService extends BaseService {
 
@@ -58,6 +59,15 @@ export class OfflineService extends BaseService {
    * @param spawnArgs Array of arguments for CLI command
    */
   private spawn(command: string, spawnArgs?: string[]): Promise<void> {
+    // Run command from local node_modules
+    command = path.join(
+      this.serverless.config.servicePath,
+      "node_modules",
+      ".bin",
+      command
+    );
+    
+    // Append .cmd if running on windows
     if (process.platform === "win32") {
       command += ".cmd";
     }
diff --git a/src/services/resourceService.ts b/src/services/resourceService.ts
index dac7acf..6a03265 100644
--- a/src/services/resourceService.ts
+++ b/src/services/resourceService.ts
@@ -29,7 +29,7 @@ export class ResourceService extends BaseService {
   /**
    * Get the most recent resource group deployment
    */
-  public async getLastDeployment() {
+  public async getPreviousDeployment() {
     const deployments = await this.getDeployments();
     if (deployments && deployments.length) {
       return deployments[0];
@@ -39,8 +39,8 @@ export class ResourceService extends BaseService {
   /**
    * Get template from last resource group deployment
    */
-  public async getLastDeploymentTemplate(): Promise<ArmDeployment> {
-    const deployment = await this.getLastDeployment();
+  public async getPreviousDeploymentTemplate(): Promise<ArmDeployment> {
+    const deployment = await this.getPreviousDeployment();
     if (!deployment || deployment.properties.provisioningState !== ArmTemplateProvisioningState.SUCCEEDED) {
       return;
     }


Test patch:
diff --git a/src/plugins/invoke/azureInvokePlugin.test.ts b/src/plugins/invoke/azureInvokePlugin.test.ts
index e4710d4..bceffa4 100644
--- a/src/plugins/invoke/azureInvokePlugin.test.ts
+++ b/src/plugins/invoke/azureInvokePlugin.test.ts
@@ -50,14 +50,17 @@ describe("Azure Invoke Plugin", () => {
     options["function"] = "testApp";
     options["path"] = "testFile.json";
     options["method"] = "GET";
-    const plugin = new AzureInvokePlugin(sls, options);
+    const plugin = new AzureInvokePlugin(sls, {
+      function: "testApp",
+      path: "testFile.json",
+    } as any);
     await invokeHook(plugin, "invoke:invoke");
     expect(invoke).toBeCalledWith(options["method"], options["function"], fileContent);
     expect(sls.cli.log).toBeCalledWith(JSON.stringify(expectedResult.data));
 
   });
 
-  it("calls the invoke hook with file path", async () => {
+  it("fails when data file path does not exist", async () => {
     const invoke = jest.fn();
     InvokeService.prototype.invoke = invoke;
     const sls = MockFactory.createTestServerless();
@@ -65,7 +68,11 @@ describe("Azure Invoke Plugin", () => {
     options["function"] = "testApp";
     options["path"] = "notExist.json";
     options["method"] = "GET";
-    expect(() => new AzureInvokePlugin(sls, options)).toThrow();
+    const plugin = new AzureInvokePlugin(sls, {
+      function: "testApp",
+      path: "notExist.json",
+    } as any);
+    await expect(invokeHook(plugin, "invoke:invoke")).rejects.toThrow();
   });
 
   it("Function invoked with no data", async () => {
diff --git a/src/services/armService.test.ts b/src/services/armService.test.ts
index 361f17c..e458276 100644
--- a/src/services/armService.test.ts
+++ b/src/services/armService.test.ts
@@ -327,7 +327,7 @@ describe("Arm Service", () => {
 
     it("Throws more detailed error message upon failed ARM deployment", async () => {
       Deployments.prototype.createOrUpdate = jest.fn(() => Promise.reject(null));
-      const lastDeploymentError: DeploymentExtendedError = {
+      const previousDeploymentError: DeploymentExtendedError = {
         code: "DeploymentFailed",
         message: "At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-debug for usage details.",
         details: [
@@ -341,9 +341,9 @@ describe("Arm Service", () => {
           }
         ]
       }
-      ResourceService.prototype.getLastDeployment = jest.fn(() => Promise.resolve({
+      ResourceService.prototype.getPreviousDeployment = jest.fn(() => Promise.resolve({
         properties: {
-          error: lastDeploymentError
+          error: previousDeploymentError
         }
       })) as any;
       const deployment: ArmDeployment = {
@@ -351,7 +351,7 @@ describe("Arm Service", () => {
         template: MockFactory.createTestArmTemplate()
       };
       deployment.parameters.param1.value = "3"
-      const { code, message, details } = lastDeploymentError;
+      const { code, message, details } = previousDeploymentError;
       let errorPattern = [
         code,
         message,
@@ -366,5 +366,77 @@ describe("Arm Service", () => {
           new RegExp(`.*${errorPattern}.*`,"s")
         );
     });
+
+    it("Does not try to include paramaters with a value that is undefined", async () => {
+      sls.service.provider.runtime = "nodejs10.x";
+      const deployment = await service.createDeploymentFromType(ArmTemplateType.Consumption);
+
+      expect(deployment.parameters.functionAppExtensionVersion).not.toBeUndefined();
+      expect(deployment.parameters.functionAppExtensionVersion.value).toBeUndefined();
+
+      await service.deployTemplate(deployment);
+
+      expect(deployment.parameters.functionAppExtensionVersion).toBeUndefined();
+
+      const paramKeys = Object.keys(deployment.parameters);
+      paramKeys.forEach((key) => {
+        const paramValue = deployment.parameters[key];
+        if (paramValue) {
+          expect(paramValue.value).not.toBeUndefined();
+        }
+      })
+
+      const expectedResourceGroup = sls.service.provider["resourceGroup"];
+      const expectedDeploymentName = sls.service.provider["deploymentName"] || `${this.resourceGroup}-deployment`;
+      const expectedDeploymentNameRegex = new RegExp(expectedDeploymentName + "-t([0-9]+)")
+      const expectedDeployment: Deployment = {
+        properties: {
+          mode: "Incremental",
+          ...deployment
+        },
+      };
+
+      const call = (Deployments.prototype.createOrUpdate as any).mock.calls[0];
+      expect(call[0]).toEqual(expectedResourceGroup);
+      expect(call[1]).toMatch(expectedDeploymentNameRegex);
+      expect(call[2]).toEqual(expectedDeployment);
+    });
+    
+    it("Throws original error when there has not been a previous deployment", async () => {
+      const originalError = new Error("original error message");
+      Deployments.prototype.createOrUpdate = jest.fn(() => Promise.reject(originalError));
+      const previousDeploymentError: DeploymentExtendedError = {
+        code: "DeploymentFailed",
+        message: "At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-debug for usage details.",
+        details: [
+          {
+            code: "ServiceAlreadyExists",
+            message: "Api service already exists: abc-123-apim"
+          },
+          {
+            code: "StorageAccountAlreadyTaken",
+            message: "The storage account named ABC123 is already taken."
+          }
+        ]
+      }
+      ResourceService.prototype.getPreviousDeployment = jest.fn(() => Promise.resolve(undefined)) as any;
+      const deployment: ArmDeployment = {
+        parameters: MockFactory.createTestParameters(),
+        template: MockFactory.createTestArmTemplate()
+      };
+      deployment.parameters.param1.value = "3"
+      const { code, message, details } = previousDeploymentError;
+      let errorPattern = [
+        code,
+        message,
+        details[0].code,
+        details[0].message,
+        details[1].code,
+        details[1].message
+      ].join(".*")
+      await expect(service.deployTemplate(deployment))
+        .rejects
+        .toThrowError(originalError);
+    });
   });
 });
diff --git a/src/services/offlineService.test.ts b/src/services/offlineService.test.ts
index a26fb24..bee3dc9 100644
--- a/src/services/offlineService.test.ts
+++ b/src/services/offlineService.test.ts
@@ -126,7 +126,7 @@ describe("Offline Service", () => {
     const calls = mySpawn.calls;
     expect(calls).toHaveLength(1);
     const call = calls[0];
-    expect(call.command).toEqual("func");
+    expect(call.command).toEqual(path.join("node_modules", ".bin", "func"));
     expect(call.args).toEqual(["host", "start"]);
   });
 
@@ -142,7 +142,7 @@ describe("Offline Service", () => {
     const calls = mySpawn.calls;
     expect(calls).toHaveLength(1);
     const call = calls[0];
-    expect(call.command).toEqual("func.cmd");
+    expect(call.command).toEqual(path.join("node_modules", ".bin", "func.cmd"));
     expect(call.args).toEqual(["host", "start"]);
   });
 
@@ -174,7 +174,7 @@ describe("Offline Service", () => {
     const calls = mySpawn.calls;
     expect(calls).toHaveLength(1);
     const call = calls[0];
-    expect(call.command).toEqual("func.cmd");
+    expect(call.command).toEqual(path.join("node_modules", ".bin", "func.cmd"));
     expect(call.args).toEqual(["host", "start"]);
 
     const processOnCalls = processOnSpy.mock.calls;
@@ -230,7 +230,7 @@ describe("Offline Service", () => {
     const calls = mySpawn.calls;
     expect(calls).toHaveLength(1);
     const call = calls[0];
-    expect(call.command).toEqual("func.cmd");
+    expect(call.command).toEqual(path.join("node_modules", ".bin", "func.cmd"));
     expect(call.args).toEqual(["host", "start"]);
 
     const processOnCalls = processOnSpy.mock.calls;