moving invoker into HttpsTrigger block and adding more tests (#960)

Author: colerogers

spec/common/encoding.spec.ts

@@ -2,6 +2,18 @@ import { expect } from 'chai';
 import { convertInvoker } from '../../src/common/encoding';
 
 describe('convertInvoker', () => {
+  it('should raise an error on empty array', () => {
+    expect(() => convertInvoker([])).to.throw;
+  });
+
+  it('should raise an error on empty string', () => {
+    expect(() => convertInvoker('')).to.throw;
+  });
+
+  it('should raise an error on empty string with service accounts', () => {
+    expect(() => convertInvoker(['service-account@', ''])).to.throw;
+  });
+
   it('should raise an error on mixing public and service accounts', () => {
     expect(() => convertInvoker(['public', 'service-account@'])).to.throw;
   });

spec/v1/providers/https.spec.ts

@@ -107,12 +107,14 @@ describe('CloudHttpsBuilder', () => {
         .runWith({
           timeoutSeconds: 90,
           memory: '256MB',
+          invoker: 'private',
         })
         .https.onRequest(() => null);
 
       expect(fn.__trigger.regions).to.deep.equal(['us-east1']);
       expect(fn.__trigger.availableMemoryMb).to.deep.equal(256);
       expect(fn.__trigger.timeout).to.deep.equal('90s');
+      expect(fn.__trigger.httpsTrigger.invoker).to.deep.equal(['private']);
     });
   });
 });

spec/v2/providers/https.spec.ts

@@ -111,9 +111,9 @@ describe('onRequest', () => {
       ...FULL_TRIGGER,
       httpsTrigger: {
         allowInsecure: false,
+        invoker: ['service-account1@', 'service-account2@'],
       },
       regions: ['us-west1', 'us-central1'],
-      invoker: ['service-account1@', 'service-account2@'],
     });
   });
 
@@ -141,12 +141,12 @@ describe('onRequest', () => {
       platform: 'gcfv2',
       httpsTrigger: {
         allowInsecure: false,
+        invoker: ['private'],
       },
       concurrency: 20,
       minInstances: 3,
       regions: ['us-west1', 'us-central1'],
       labels: {},
-      invoker: ['private'],
     });
   });
 

src/cloud-functions.ts

@@ -32,7 +32,6 @@ import { warn } from './logger';
 export { Request, Response };
 import {
   convertIfPresent,
-  convertInvoker,
   copyIfPresent,
   Duration,
   durationFromSeconds,
@@ -270,7 +269,9 @@ export interface TriggerAnnotated {
       service: string;
     };
     failurePolicy?: FailurePolicy;
-    httpsTrigger?: {};
+    httpsTrigger?: {
+      invoker?: string[];
+    };
     labels?: { [key: string]: string };
     regions?: string[];
     schedule?: Schedule;
@@ -279,7 +280,6 @@ export interface TriggerAnnotated {
     vpcConnectorEgressSettings?: string;
     serviceAccountEmail?: string;
     ingressSettings?: string;
-    invoker?: string[];
   };
 }
 
@@ -542,7 +542,6 @@ export function optionsToTrigger(options: DeploymentOptions) {
     'serviceAccount',
     serviceAccountFromShorthand
   );
-  convertIfPresent(trigger, options, 'invoker', 'invoker', convertInvoker);
 
   return trigger;
 }

src/common/encoding.ts

@@ -70,12 +70,20 @@ export function convertInvoker(invoker: string | string[]): string[] {
     invoker = [invoker];
   }
 
+  if (invoker.length === 0) {
+    throw new Error('Invalid option for invoker: Must be a non-empty array.');
+  }
+
+  if (invoker.find((inv) => inv.length === 0)) {
+    throw new Error('Invalid option for invoker: Must be a non-empty string.');
+  }
+
   if (
     invoker.length > 1 &&
     invoker.find((inv) => inv === 'public' || inv === 'private')
   ) {
     throw new Error(
-      "Invalid option for invoker. Cannot have 'public' or 'private' in an array of service accounts"
+      "Invalid option for invoker: Cannot have 'public' or 'private' in an array of service accounts."
     );
   }
 

src/providers/https.ts

@@ -23,6 +23,7 @@
 import * as express from 'express';
 
 import { HttpsFunction, optionsToTrigger, Runnable } from '../cloud-functions';
+import { convertIfPresent, convertInvoker } from '../common/encoding';
 import {
   CallableContext,
   FunctionsErrorCode,
@@ -68,6 +69,13 @@ export function _onRequestWithOptions(
     ...optionsToTrigger(options),
     httpsTrigger: {},
   };
+  convertIfPresent(
+    cloudFunction.__trigger.httpsTrigger,
+    options,
+    'invoker',
+    'invoker',
+    convertInvoker
+  );
   // TODO parse the options
   return cloudFunction;
 }

src/v2/core.ts

@@ -32,15 +32,16 @@ export interface TriggerAnnotation {
     service: string;
   };
   failurePolicy?: { retry: boolean };
-  httpsTrigger?: {};
+  httpsTrigger?: {
+    invoker?: string[];
+  };
   labels?: { [key: string]: string };
   regions?: string[];
   timeout?: string;
   vpcConnector?: string;
   vpcConnectorEgressSettings?: string;
   serviceAccountEmail?: string;
   ingressSettings?: string;
-  invoker?: string[];
 
   // TODO: schedule
 }

src/v2/options.ts

@@ -21,7 +21,6 @@
 // SOFTWARE.
 
 import {
-  convertInvoker,
   durationFromSeconds,
   serviceAccountFromShorthand,
 } from '../common/encoding';
@@ -276,7 +275,6 @@ export function optionsToTriggerAnnotations(
       return retry ? { retry: true } : null;
     }
   );
-  convertIfPresent(annotation, opts, 'invoker', 'invoker', convertInvoker);
 
   return annotation;
 }

src/v2/providers/https.ts

@@ -22,6 +22,7 @@
 
 import * as cors from 'cors';
 import * as express from 'express';
+import { convertIfPresent, convertInvoker } from '../../common/encoding';
 
 import {
   CallableRequest,
@@ -104,7 +105,7 @@ export function onRequest(
       const specificOpts = options.optionsToTriggerAnnotations(
         opts as options.GlobalOptions
       );
-      return {
+      const trigger: any = {
         // TODO(inlined): Remove "apiVersion" once the latest version of the CLI
         // has migrated to "platform".
         apiVersion: 2,
@@ -119,6 +120,14 @@ export function onRequest(
           allowInsecure: false,
         },
       };
+      convertIfPresent(
+        trigger.httpsTrigger,
+        opts,
+        'invoker',
+        'invoker',
+        convertInvoker
+      );
+      return trigger;
     },
   });
   return handler as HttpsFunction;