fix: Default operation names no longer cause collisions (#332)

APIM uses the operation name as the key when upserting operations into an API. By default we were using the function name as the operation name. During default configuration inference with multiple HTTP methods this caused a collision resulting in the last operation in always one.

This fix generates a unique operation name taking into account the function name + the HTTP method.

src/armTemplates/compositeArmTemplate.ts

@@ -1,4 +1,4 @@
-import { ArmResourceTemplate, ArmResourceTemplateGenerator, ArmParameters, ArmParamType } from "../models/armTemplates";
+import { ArmResourceTemplate, ArmResourceTemplateGenerator, ArmParameters } from "../models/armTemplates";
 import { ServerlessAzureConfig } from "../models/serverless";
 import { AzureNamingService } from "../services/namingService";
 import { Guard } from "../shared/guard";

src/plugins/invoke/azureInvokePlugin.ts

@@ -1,5 +1,5 @@
 import fs from "fs";
-import { isAbsolute, join } from "path";
+import { isAbsolute } from "path";
 import Serverless from "serverless";
 import { InvokeService } from "../../services/invokeService";
 import { AzureBasePlugin } from "../azureBasePlugin";

src/services/apimService.test.ts

@@ -387,6 +387,11 @@ describe("APIM Service", () => {
 
     it("infers APIM operation configuration from HTTP binding", async () => {
       const functions = MockFactory.createTestSlsFunctionConfig();
+
+      // Remove APIM operation configuration
+      delete functions.hello.apim;
+      delete functions.goodbye.apim;
+
       Object.assign(serverless.service, { functions });
 
       let apimResource: ApiManagementServiceResource = {
@@ -425,11 +430,12 @@ describe("APIM Service", () => {
         resourceGroupName,
         serviceName,
         apiName,
-        "hello",
+        "GET-hello",
         {
-          displayName: "hello",
+          name: "GET-hello",
+          displayName: "hello (GET)",
           description: "",
-          method: "get",
+          method: "GET",
           urlTemplate: "hello",
           templateParameters: [],
           responses: [],
@@ -439,11 +445,12 @@ describe("APIM Service", () => {
         resourceGroupName,
         serviceName,
         apiName,
-        "goodbye",
+        "GET-goodbye",
         {
-          displayName: "goodbye",
+          name: "GET-goodbye",
+          displayName: "goodbye (GET)",
           description: "",
-          method: "get",
+          method: "GET",
           urlTemplate: "goodbye",
           templateParameters: [],
           responses: [],

src/services/apimService.ts

@@ -116,7 +116,7 @@ export class ApimService extends BaseService {
       return null;
     }
 
-    this.log("-> Deploying API Operations");
+    this.log(`-> Deploying API Operations: ${this.apimConfig.name}`);
 
     const deployApiTasks = this.serverless.service
       .getAllFunctions()
@@ -144,12 +144,17 @@ export class ApimService extends BaseService {
 
     const httpConfig = httpEvent["x-azure-settings"];
 
+    // Set to GET method by default
+    if (!httpConfig.methods) {
+      httpConfig.methods = ["GET"];
+    }
+
     // Infer APIM operation configuration from HTTP event if not already set
     if (!functionConfig.apim) {
       const operations = httpConfig.methods.map((method) => {
         return {
-          name: functionConfig.name,
-          displayName: functionConfig.name,
+          name: `${method}-${functionConfig.name}`,
+          displayName: `${functionConfig.name} (${method})`,
           urlTemplate: httpConfig.route || functionConfig.name,
           method: method,
           templateParameters: this.getTemplateParameters(httpConfig.route)
@@ -214,7 +219,7 @@ export class ApimService extends BaseService {
       });
 
       if (this.apimConfig.cors) {
-        this.log("-> Deploying CORS policy");
+        this.log(`-> Deploying CORS policy: ${apiContract.name}`);
 
         await this.apimClient.apiPolicy.createOrUpdate(this.resourceGroup, this.apimConfig.name, apiContract.name, {
           format: "rawxml",
@@ -273,6 +278,7 @@ export class ApimService extends BaseService {
       const client = new ApiManagementClient(this.credentials, this.subscriptionId);
 
       const operationConfig: OperationContract = {
+        name: operation.name || functionName,
         displayName: operation.displayName || functionName,
         description: operation.description || "",
         method: operation.method,
@@ -284,17 +290,17 @@ export class ApimService extends BaseService {
       // Ensure a single path seperator in the operation path
       const operationPath = `/${api.path}/${operationConfig.urlTemplate}`.replace(/\/+/g, "/");
       const operationUrl = `${resource.gatewayUrl}${operationPath}`;
-      this.log(`--> ${functionName}: [${operationConfig.method.toUpperCase()}] ${operationUrl}`);
+      this.log(`--> ${operationConfig.name}: [${operationConfig.method.toUpperCase()}] ${operationUrl}`);
 
       const result = await client.apiOperation.createOrUpdate(
         this.resourceGroup,
         this.apimConfig.name,
         api.name,
-        functionName,
+        operationConfig.name,
         operationConfig,
       );
 
-      await client.apiOperationPolicy.createOrUpdate(this.resourceGroup, this.apimConfig.name, api.name, functionName, {
+      await client.apiOperationPolicy.createOrUpdate(this.resourceGroup, this.apimConfig.name, api.name, operationConfig.name, {
         format: "rawxml",
         value: this.createApiOperationXmlPolicy(backend.name),
       });

src/services/armService.test.ts

@@ -363,7 +363,7 @@ describe("Arm Service", () => {
       await expect(service.deployTemplate(deployment))
         .rejects
         .toThrowError(
-          new RegExp(`.*${errorPattern}.*`,"s")
+          new RegExp(`.*${errorPattern}.*`, "s")
         );
     });
 
@@ -401,39 +401,18 @@ describe("Arm Service", () => {
       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);