diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f68266fc..bae53bea3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1 +1,2 @@ - Deprecate typoed function name lessThanorEqualTo (#1284) +- Fix a bug where supplying preserveExternalChanges to scheduled functions would cause deployment failure (#1305). diff --git a/spec/runtime/manifest.spec.ts b/spec/runtime/manifest.spec.ts index 22c5f8af1..eb553643b 100644 --- a/spec/runtime/manifest.spec.ts +++ b/spec/runtime/manifest.spec.ts @@ -1,8 +1,16 @@ import { expect } from "chai"; -import { stackToWire, ManifestStack } from "../../src/runtime/manifest"; +import { + stackToWire, + ManifestStack, + initV2ScheduleTrigger, + initV1ScheduleTrigger, + initTaskQueueTrigger, +} from "../../src/runtime/manifest"; +import { RESET_VALUE } from "../../src/common/options"; import * as params from "../../src/params"; import * as optsv2 from "../../src/v2/options"; import * as v1 from "../../src/v1"; +import { DeploymentOptions } from "../../src/v1"; describe("stackToWire", () => { afterEach(() => { @@ -168,3 +176,90 @@ describe("stackToWire", () => { expect(stackToWire(stack)).to.deep.equal(expected); }); }); + +describe("initTaskQueueTrigger", () => { + it("should init a taskQueueTrigger without preserveExternalChanges", () => { + const tt = initTaskQueueTrigger(); + + expect(tt).to.deep.eq({ + retryConfig: { + maxAttempts: RESET_VALUE, + maxDoublings: RESET_VALUE, + maxBackoffSeconds: RESET_VALUE, + maxRetrySeconds: RESET_VALUE, + minBackoffSeconds: RESET_VALUE, + }, + rateLimits: { + maxConcurrentDispatches: RESET_VALUE, + maxDispatchesPerSecond: RESET_VALUE, + }, + }); + }); + + it("should init a taskQueueTrigger with preserveExternalChanges", () => { + const opts: DeploymentOptions = { preserveExternalChanges: true }; + + const tt = initTaskQueueTrigger(opts); + + expect(tt).to.deep.eq({ + rateLimits: {}, + retryConfig: {}, + }); + }); +}); + +describe("initScheduleTrigger", () => { + it("should init a v1 scheduleTrigger without preserveExternalChanges", () => { + const st = initV1ScheduleTrigger("every 30 minutes"); + + expect(st).to.deep.eq({ + schedule: "every 30 minutes", + timeZone: RESET_VALUE, + retryConfig: { + retryCount: RESET_VALUE, + maxDoublings: RESET_VALUE, + maxRetryDuration: RESET_VALUE, + minBackoffDuration: RESET_VALUE, + maxBackoffDuration: RESET_VALUE, + }, + }); + }); + + it("should init a v1 scheduleTrigger with preserveExternalChanges", () => { + const opts: DeploymentOptions = { preserveExternalChanges: true }; + + const st = initV1ScheduleTrigger("every 30 minutes", opts); + + expect(st).to.deep.eq({ + schedule: "every 30 minutes", + retryConfig: {}, + }); + }); + + it("should init a v2 scheduleTrigger without preserveExternalChanges", () => { + const st = initV2ScheduleTrigger("every 30 minutes"); + + expect(st).to.deep.eq({ + schedule: "every 30 minutes", + timeZone: RESET_VALUE, + retryConfig: { + retryCount: RESET_VALUE, + maxDoublings: RESET_VALUE, + maxRetrySeconds: RESET_VALUE, + minBackoffSeconds: RESET_VALUE, + maxBackoffSeconds: RESET_VALUE, + }, + }); + }); + + it("should init a v2 scheduleTrigger with preserveExternalChanges", () => { + const opts: DeploymentOptions = { preserveExternalChanges: true }; + + const st = initV2ScheduleTrigger("every 30 minutes", opts); + + expect(st).to.deep.eq({ + schedule: "every 30 minutes", + retryConfig: {}, + }); + }); +}); diff --git a/spec/v1/cloud-functions.spec.ts b/spec/v1/cloud-functions.spec.ts index 3a9c99b67..6ee3abc41 100644 --- a/spec/v1/cloud-functions.spec.ts +++ b/spec/v1/cloud-functions.spec.ts @@ -189,6 +189,35 @@ describe("makeCloudFunction", () => { }); }); + it("should setup a scheduleTrigger in __endpoint given a schedule and preserveExternalChanges", () => { + const schedule = { + schedule: "every 5 minutes", + retryConfig: { retryCount: 3 }, + timeZone: "America/New_York", + }; + const cf = makeCloudFunction({ + provider: "mock.provider", + eventType: "mock.event", + service: "service", + triggerResource: () => "resource", + handler: () => null, + options: { + schedule, + preserveExternalChanges: true, + }, + }); + expect(cf.__endpoint).to.deep.equal({ + platform: "gcfv1", + scheduleTrigger: { + ...schedule, + retryConfig: { + ...schedule.retryConfig, + }, + }, + labels: {}, + }); + }); + it("should construct the right context for event", () => { const args: any = { ...cloudFunctionArgs, diff --git a/spec/v1/providers/pubsub.spec.ts b/spec/v1/providers/pubsub.spec.ts index 77b6fe24a..0a7b89ad6 100644 --- a/spec/v1/providers/pubsub.spec.ts +++ b/spec/v1/providers/pubsub.spec.ts @@ -388,6 +388,32 @@ describe("Pubsub Functions", () => { expect(result.__endpoint.availableMemoryMb).to.deep.equal(256); expect(result.__endpoint.timeoutSeconds).to.deep.equal(90); }); + + it("should return an appropriate endpoint when called with preserveExternalChanges", () => { + const result = functions + .region("us-east1") + .runWith({ + timeoutSeconds: 90, + memory: "256MB", + preserveExternalChanges: true, + }) + .pubsub.schedule("every 5 minutes") + .timeZone("America/New_York") + .onRun(() => null); + + expect(result.__endpoint).to.deep.eq({ + platform: "gcfv1", + labels: {}, + region: ["us-east1"], + availableMemoryMb: 256, + timeoutSeconds: 90, + scheduleTrigger: { + schedule: "every 5 minutes", + timeZone: "America/New_York", + retryConfig: {}, + }, + }); + }); }); }); diff --git a/spec/v1/providers/tasks.spec.ts b/spec/v1/providers/tasks.spec.ts index dc0b5e0c6..040e30388 100644 --- a/spec/v1/providers/tasks.spec.ts +++ b/spec/v1/providers/tasks.spec.ts @@ -28,6 +28,7 @@ import { MockRequest } from "../../fixtures/mockrequest"; import { runHandler } from "../../helper"; import { MINIMAL_V1_ENDPOINT } from "../../fixtures"; import { MINIMIAL_TASK_QUEUE_TRIGGER } from "./fixtures"; +import { runWith } from "../../../src/v1"; describe("#onDispatch", () => { it("should return a trigger/endpoint with appropriate values", () => { @@ -67,6 +68,7 @@ describe("#onDispatch", () => { ...MINIMAL_V1_ENDPOINT, platform: "gcfv1", taskQueueTrigger: { + ...MINIMIAL_TASK_QUEUE_TRIGGER, rateLimits: { maxConcurrentDispatches: 30, maxDispatchesPerSecond: 40, @@ -83,6 +85,35 @@ describe("#onDispatch", () => { }); }); + it("should return an endpoint with appropriate values with preserveExternalChanges set", () => { + const result = runWith({ preserveExternalChanges: true }) + .tasks.taskQueue({ + rateLimits: { + maxConcurrentDispatches: 30, + }, + retryConfig: { + maxAttempts: 5, + maxRetrySeconds: 10, + }, + invoker: "private", + }) + .onDispatch(() => undefined); + + expect(result.__endpoint).to.deep.equal({ + platform: "gcfv1", + taskQueueTrigger: { + rateLimits: { + maxConcurrentDispatches: 30, + }, + retryConfig: { + maxAttempts: 5, + maxRetrySeconds: 10, + }, + invoker: ["private"], + }, + }); + }); + it("should allow both region and runtime options to be set", () => { const fn = functions .region("us-east1") @@ -114,6 +145,10 @@ describe("#onDispatch", () => { ...MINIMIAL_TASK_QUEUE_TRIGGER, retryConfig: { maxAttempts: 5, + maxBackoffSeconds: functions.RESET_VALUE, + maxDoublings: functions.RESET_VALUE, + maxRetrySeconds: functions.RESET_VALUE, + minBackoffSeconds: functions.RESET_VALUE, }, }, }); diff --git a/spec/v2/providers/scheduler.spec.ts b/spec/v2/providers/scheduler.spec.ts index 5ca9fb407..4f3e6f984 100644 --- a/spec/v2/providers/scheduler.spec.ts +++ b/spec/v2/providers/scheduler.spec.ts @@ -63,11 +63,13 @@ describe("schedule", () => { expect(schedule.getOpts(options)).to.deep.eq({ schedule: "* * * * *", timeZone: "utc", - retryCount: 3, - maxRetrySeconds: 1, - minBackoffSeconds: 2, - maxBackoffSeconds: 3, - maxDoublings: 4, + retryConfig: { + retryCount: 3, + maxRetrySeconds: 1, + minBackoffSeconds: 2, + maxBackoffSeconds: 3, + maxDoublings: 4, + }, opts: { ...options, memory: "128MiB", @@ -139,6 +141,38 @@ describe("schedule", () => { ]); }); + it("should create a schedule function with preserveExternalChanges", () => { + const schfn = schedule.onSchedule( + { + schedule: "* * * * *", + preserveExternalChanges: true, + }, + () => console.log(1) + ); + + expect(schfn.__endpoint).to.deep.eq({ + platform: "gcfv2", + labels: {}, + scheduleTrigger: { + schedule: "* * * * *", + timeZone: undefined, + retryConfig: { + retryCount: undefined, + maxRetrySeconds: undefined, + minBackoffSeconds: undefined, + maxBackoffSeconds: undefined, + maxDoublings: undefined, + }, + }, + }); + expect(schfn.__requiredAPIs).to.deep.eq([ + { + api: "cloudscheduler.googleapis.com", + reason: "Needed for scheduled functions.", + }, + ]); + }); + it("should have a .run method", async () => { const testObj = { foo: "bar", diff --git a/spec/v2/providers/tasks.spec.ts b/spec/v2/providers/tasks.spec.ts index 4962b342e..473768371 100644 --- a/spec/v2/providers/tasks.spec.ts +++ b/spec/v2/providers/tasks.spec.ts @@ -128,6 +128,73 @@ describe("onTaskDispatched", () => { }); }); + it("should return a minimal endpoint without preserveExternalChanges set", () => { + const result = onTaskDispatched( + { + retryConfig: { + maxAttempts: 4, + maxRetrySeconds: 10, + }, + rateLimits: { + maxDispatchesPerSecond: 10, + }, + }, + () => undefined + ); + + expect(result.__endpoint).to.deep.equal({ + ...MINIMAL_V2_ENDPOINT, + platform: "gcfv2", + labels: {}, + taskQueueTrigger: { + retryConfig: { + maxAttempts: 4, + maxRetrySeconds: 10, + maxBackoffSeconds: options.RESET_VALUE, + maxDoublings: options.RESET_VALUE, + minBackoffSeconds: options.RESET_VALUE, + }, + rateLimits: { + maxDispatchesPerSecond: 10, + maxConcurrentDispatches: options.RESET_VALUE, + }, + }, + }); + }); + + it("should create a complex endpoint with preserveExternalChanges set", () => { + const result = onTaskDispatched( + { + ...FULL_OPTIONS, + retryConfig: { + maxAttempts: 4, + maxRetrySeconds: 10, + }, + rateLimits: { + maxDispatchesPerSecond: 10, + }, + invoker: "private", + preserveExternalChanges: true, + }, + () => undefined + ); + + expect(result.__endpoint).to.deep.equal({ + ...FULL_ENDPOINT, + platform: "gcfv2", + taskQueueTrigger: { + retryConfig: { + maxAttempts: 4, + maxRetrySeconds: 10, + }, + rateLimits: { + maxDispatchesPerSecond: 10, + }, + invoker: ["private"], + }, + }); + }); + it("should merge options and globalOptions", () => { options.setGlobalOptions({ concurrency: 20, diff --git a/spec/v2/providers/testLab.spec.ts b/spec/v2/providers/testLab.spec.ts index 7d59f8bca..15ff77248 100644 --- a/spec/v2/providers/testLab.spec.ts +++ b/spec/v2/providers/testLab.spec.ts @@ -23,6 +23,7 @@ import { expect } from "chai"; import * as testLab from "../../../src/v2/providers/testLab"; import * as options from "../../../src/v2/options"; +import { MINIMAL_V2_ENDPOINT } from "../../fixtures"; describe("onTestMatrixCompleted", () => { afterEach(() => { @@ -33,6 +34,7 @@ describe("onTestMatrixCompleted", () => { const fn = testLab.onTestMatrixCompleted(() => 2); expect(fn.__endpoint).to.deep.eq({ + ...MINIMAL_V2_ENDPOINT, platform: "gcfv2", labels: {}, eventTrigger: { @@ -59,6 +61,7 @@ describe("onTestMatrixCompleted", () => { ); expect(fn.__endpoint).to.deep.eq({ + ...MINIMAL_V2_ENDPOINT, platform: "gcfv2", availableMemoryMb: 512, region: ["us-central1"], diff --git a/src/runtime/manifest.ts b/src/runtime/manifest.ts index 65cb28dec..6ff5fe0f9 100644 --- a/src/runtime/manifest.ts +++ b/src/runtime/manifest.ts @@ -215,17 +215,17 @@ const RESETTABLE_RATE_LIMITS_OPTIONS: ResettableKeys< export function initTaskQueueTrigger( ...opts: ManifestOptions[] ): ManifestEndpoint["taskQueueTrigger"] { - let taskQueueTrigger = {}; + const taskQueueTrigger: ManifestEndpoint["taskQueueTrigger"] = { + retryConfig: {}, + rateLimits: {}, + }; if (opts.every((opt) => !opt?.preserveExternalChanges)) { - const retryConfig = {}; for (const key of Object.keys(RESETTABLE_RETRY_CONFIG_OPTIONS)) { - retryConfig[key] = RESET_VALUE; + taskQueueTrigger.retryConfig[key] = RESET_VALUE; } - const rateLimits = {}; for (const key of Object.keys(RESETTABLE_RATE_LIMITS_OPTIONS)) { - rateLimits[key] = RESET_VALUE; + taskQueueTrigger.rateLimits[key] = RESET_VALUE; } - taskQueueTrigger = { retryConfig, rateLimits }; } return taskQueueTrigger; } @@ -257,13 +257,15 @@ function initScheduleTrigger( schedule: string | Expression, ...opts: ManifestOptions[] ): ManifestEndpoint["scheduleTrigger"] { - let scheduleTrigger: ManifestEndpoint["scheduleTrigger"] = { schedule }; + let scheduleTrigger: ManifestEndpoint["scheduleTrigger"] = { + schedule, + retryConfig: {}, + }; if (opts.every((opt) => !opt?.preserveExternalChanges)) { - const retryConfig = {}; for (const key of Object.keys(resetOptions)) { - retryConfig[key] = RESET_VALUE; + scheduleTrigger.retryConfig[key] = RESET_VALUE; } - scheduleTrigger = { ...scheduleTrigger, timeZone: RESET_VALUE, retryConfig }; + scheduleTrigger = { ...scheduleTrigger, timeZone: RESET_VALUE }; } return scheduleTrigger; } diff --git a/src/v1/function-configuration.ts b/src/v1/function-configuration.ts index 504e392ca..e92971a48 100644 --- a/src/v1/function-configuration.ts +++ b/src/v1/function-configuration.ts @@ -246,6 +246,17 @@ export interface RuntimeOptions { * When false, requests with invalid tokens set context.app to undefiend. */ enforceAppCheck?: boolean; + + /** + * Controls whether function configuration modified outside of function source is preserved. Defaults to false. + * + * @remarks + * When setting configuration available in the underlying platform that is not yet available in the Firebase Functions + * SDK, we highly recommend setting `preserveExternalChanges` to `true`. Otherwise, when the Firebase Functions SDK releases + * a new version of the SDK with support for the missing configuration, your function's manually configured setting + * may inadvertently be wiped out. + */ + preserveExternalChanges?: boolean; } /** @@ -264,14 +275,4 @@ export interface DeploymentOptions extends RuntimeOptions { * Schedule for the scheduled function. */ schedule?: Schedule; - /** - * Controls whether function configuration modified outside of function source is preserved. Defaults to false. - * - * @remarks - * When setting configuration available in the underlying platform that is not yet available in the Firebase Functions - * SDK, we highly recommend setting `preserveExternalChanges` to `true`. Otherwise, when the Firebase Functions SDK releases - * a new version of the SDK with support for the missing configuration, your function's manually configured setting - * may inadvertently be wiped out. - */ - preserveExternalChanges?: boolean; } diff --git a/src/v1/providers/tasks.ts b/src/v1/providers/tasks.ts index 9126588a5..c9bf70849 100644 --- a/src/v1/providers/tasks.ts +++ b/src/v1/providers/tasks.ts @@ -129,8 +129,21 @@ export class TaskQueueBuilder { ...optionsToEndpoint(this.depOpts), taskQueueTrigger: initTaskQueueTrigger(this.depOpts), }; - copyIfPresent(func.__endpoint.taskQueueTrigger, this.tqOpts, "retryConfig"); - copyIfPresent(func.__endpoint.taskQueueTrigger, this.tqOpts, "rateLimits"); + copyIfPresent( + func.__endpoint.taskQueueTrigger.retryConfig, + this.tqOpts?.retryConfig || {}, + "maxAttempts", + "maxBackoffSeconds", + "maxDoublings", + "maxRetrySeconds", + "minBackoffSeconds" + ); + copyIfPresent( + func.__endpoint.taskQueueTrigger.rateLimits, + this.tqOpts?.rateLimits || {}, + "maxConcurrentDispatches", + "maxDispatchesPerSecond" + ); convertIfPresent( func.__endpoint.taskQueueTrigger, this.tqOpts, diff --git a/src/v2/providers/scheduler.ts b/src/v2/providers/scheduler.ts index ec17e10e5..9346c451c 100644 --- a/src/v2/providers/scheduler.ts +++ b/src/v2/providers/scheduler.ts @@ -38,19 +38,21 @@ import * as logger from "../../logger"; import * as options from "../options"; /** @hidden */ -interface ScheduleArgs { +interface SeparatedOpts { schedule: string | Expression; timeZone?: timezone | Expression | ResetValue; - retryCount?: number | Expression | ResetValue; - maxRetrySeconds?: number | Expression | ResetValue; - minBackoffSeconds?: number | Expression | ResetValue; - maxBackoffSeconds?: number | Expression | ResetValue; - maxDoublings?: number | Expression | ResetValue; + retryConfig?: { + retryCount?: number | Expression | ResetValue; + maxRetrySeconds?: number | Expression | ResetValue; + minBackoffSeconds?: number | Expression | ResetValue; + maxBackoffSeconds?: number | Expression | ResetValue; + maxDoublings?: number | Expression | ResetValue; + }; opts: options.GlobalOptions; } /** @internal */ -export function getOpts(args: string | ScheduleOptions): ScheduleArgs { +export function getOpts(args: string | ScheduleOptions): SeparatedOpts { if (typeof args === "string") { return { schedule: args, @@ -60,11 +62,13 @@ export function getOpts(args: string | ScheduleOptions): ScheduleArgs { return { schedule: args.schedule, timeZone: args.timeZone, - retryCount: args.retryCount, - maxRetrySeconds: args.maxRetrySeconds, - minBackoffSeconds: args.minBackoffSeconds, - maxBackoffSeconds: args.maxBackoffSeconds, - maxDoublings: args.maxDoublings, + retryConfig: { + retryCount: args.retryCount, + maxRetrySeconds: args.maxRetrySeconds, + minBackoffSeconds: args.minBackoffSeconds, + maxBackoffSeconds: args.maxBackoffSeconds, + maxDoublings: args.maxDoublings, + }, opts: args as options.GlobalOptions, }; } @@ -194,7 +198,7 @@ export function onSchedule( copyIfPresent(ep.scheduleTrigger, separatedOpts, "timeZone"); copyIfPresent( ep.scheduleTrigger.retryConfig, - separatedOpts, + separatedOpts.retryConfig, "retryCount", "maxRetrySeconds", "minBackoffSeconds", diff --git a/src/v2/providers/tasks.ts b/src/v2/providers/tasks.ts index 6a493fe51..adb0b81a7 100644 --- a/src/v2/providers/tasks.ts +++ b/src/v2/providers/tasks.ts @@ -251,8 +251,21 @@ export function onTaskDispatched( taskQueueTrigger: initTaskQueueTrigger(options.getGlobalOptions(), opts), }; - copyIfPresent(func.__endpoint.taskQueueTrigger, opts, "retryConfig"); - copyIfPresent(func.__endpoint.taskQueueTrigger, opts, "rateLimits"); + copyIfPresent( + func.__endpoint.taskQueueTrigger.retryConfig, + opts.retryConfig, + "maxAttempts", + "maxBackoffSeconds", + "maxDoublings", + "maxRetrySeconds", + "minBackoffSeconds" + ); + copyIfPresent( + func.__endpoint.taskQueueTrigger.rateLimits, + opts.rateLimits, + "maxConcurrentDispatches", + "maxDispatchesPerSecond" + ); convertIfPresent(func.__endpoint.taskQueueTrigger, opts, "invoker", "invoker", convertInvoker); func.__requiredAPIs = [ diff --git a/src/v2/providers/testLab.ts b/src/v2/providers/testLab.ts index 16d95ea6a..d24e6e003 100644 --- a/src/v2/providers/testLab.ts +++ b/src/v2/providers/testLab.ts @@ -20,7 +20,7 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. -import { ManifestEndpoint } from "../../runtime/manifest"; +import { initV2Endpoint, ManifestEndpoint } from "../../runtime/manifest"; import { CloudEvent, CloudFunction } from "../core"; import { EventHandlerOptions, getGlobalOptions, optionsToEndpoint } from "../options"; import { wrapTraceContext } from "../trace"; @@ -195,6 +195,7 @@ export function onTestMatrixCompleted( func.run = handler; const ep: ManifestEndpoint = { + ...initV2Endpoint(getGlobalOptions(), optsOrHandler), platform: "gcfv2", ...baseOpts, ...specificOpts,