fix(v2): validate timeoutSeconds per trigger type (addition) #1877
fix(v2): validate timeoutSeconds per trigger type (addition) #1877IzaakGough wants to merge 5 commits intomasterfrom
Conversation
The v2 SDK accepts any timeoutSeconds value at function-definition
time and leaves the rejection to the Cloud Functions control plane
at deploy time. v1 has had assertRuntimeOptionsValid for a long time
but v2 was missing the equivalent.
Add per-trigger-type validation so misconfigured values fail locally
instead of mid-deploy:
- 540s for event-handling functions (firestore, database, pubsub,
storage, scheduler, eventarc, testLab, remoteConfig, alerts,
dataconnect)
- 3600s for HTTPS and callable functions (onRequest, onCall,
onCallGenkit, identity, dataconnect/graphql, ai)
- 1800s for task queue functions (onTaskDispatched)
Implementation notes:
- New internal helper assertTimeoutSecondsValid(opts, kind) and
MAX_{EVENT,HTTPS,TASK}_TIMEOUT_SECONDS constants in
src/v2/options.ts.
- optionsToEndpoint and optionsToTriggerAnnotations gained an
optional kind argument. When omitted they behave exactly as
before, so existing callers are unaffected.
- Expression<number> and RESET_VALUE are passed through untouched;
the SDK cannot know the concrete value in those cases.
- For providers whose __endpoint is a lazy getter (storage) the
throw happens the first time the manifest is read instead of at
function-definition time; a regression test covers this path.
Fixes #1737
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds validation for timeoutSeconds across all v2 trigger types, ensuring that misconfigured values are caught during function definition rather than at deployment. It introduces specific limits for event-handling (540s), HTTPS/callable (3600s), and task queue (1800s) functions. Reviewer feedback suggests strengthening the validation logic to explicitly handle non-numeric types (like strings or booleans) and clarifying whether a 0-second timeout is valid according to platform documentation.
| opts: GlobalOptions | EventHandlerOptions | HttpsOptions, | ||
| kind: TimeoutKind | ||
| ): void { | ||
| const timeoutSeconds = opts?.timeoutSeconds ?? getGlobalOptions().timeoutSeconds; |
There was a problem hiding this comment.
The current implementation of assertTimeoutSecondsValid skips validation if timeoutSeconds is not a literal number. While this correctly handles Expression and RESET_VALUE, it also silently ignores invalid types like strings or booleans that might be passed by JavaScript users. Consider adding a check to ensure that if the value is not a number, it is at least a valid Expression or ResetValue before returning.
| label = "event-handling"; | ||
| break; | ||
| } | ||
| if (timeoutSeconds < 0 || timeoutSeconds > max) { |
There was a problem hiding this comment.
The validation allows timeoutSeconds to be 0. However, the documentation for GlobalOptions states that the minimum timeout for a 2nd gen function is 1s. If 0 is not intended to be a valid literal value (e.g., if it's not a sentinel for 'use default'), it should be rejected to prevent deployment failures.
References
- Validation logic should align with the documented constraints of the platform to provide accurate early feedback.
IzaakGough
changed the title
3 participants
supersedes #1874