Skip to content

Fix params.defineList cors parsed as undefined at deploy#1813

Open
HassanBahati wants to merge 4 commits intomasterfrom
@invertase/fix-params-defineList-cors-request-time
Open

Fix params.defineList cors parsed as undefined at deploy#1813
HassanBahati wants to merge 4 commits intomasterfrom
@invertase/fix-params-defineList-cors-request-time

Conversation

@HassanBahati
Copy link
Member

Description

Fixes deploy failure when params.defineList() is used as the cors option in https.onRequest or https.onCall. Previously, the SDK resolved the param at definition time (when the deploy analyzer loads the code), so process.env.ALLOWED_ORIGINS was unset and ListParam.runtimeValue() did JSON.parse(undefined), causing SyntaxError: "undefined" is not valid JSON and blocking deploy. This change defers cors Expression resolution to request time, so the param is only read when the function runs and env is set.

Related Issues

Resolves #1773

Changes Made

  • onRequest & onCall: When cors is an Expression (e.g. defineList(...)), stop calling .value() at definition time. Use CORS origin as a function that calls corsExpression.runtimeValue() only when the cors middleware runs (per request), so the param is resolved at request time.
  • ListParam.runtimeValue(): If the param’s env var is missing or empty, throw a clear “parameter not set” error instead of letting JSON.parse(undefined) throw, so deploy-time mistakes give an actionable message.

Testing

The fix is covered by these existing tests in spec/v2/providers/https.spec.ts:

  • onRequest – cors with defineList
    Test: it("should allow cors params", ...) at line 231 (test body: lines 231–266), inside describe("onRequest", ...).
    Uses defineList("ORIGINS") and cors: origins in https.onRequest, sets process.env.ORIGINS before calling onRequest, then sends an OPTIONS request and asserts CORS headers. With the fix, the origin callback runs at request time and the test passes.
  • onCall – cors with defineList
    Test: it("should allow cors params", ...) at line 488 (test body: lines 488–508), inside describe("onCall", ...).
    Uses origins = defineList("ORIGINS") (with process.env.ORIGINS set in beforeEach) and https.onCall({ cors: origins }, () => 42), then sends a request and checks CORS. With the fix, the param is resolved at request time and the test passes.
    No new tests were added; both tests above were already in the repo and continue to pass with the deferred resolution.

Additional Notes

The cors package supports origin as a function (reqOrigin, callback) => void, which is used for deferred resolution.
ListParam extends Param extends Expression, so opts.cors instanceof Expression is true for defineList(...) and the deferred path is used.
The enableCors debug flag is still honored in both onRequest and onCall when cors is an Expression.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @HassanBahati, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical deployment failure that occurred when using params.defineList() for CORS configuration in https.onRequest or https.onCall functions. The core issue was that the CORS parameter was being resolved prematurely during deployment, leading to an attempt to parse an undefined environment variable. The solution involves deferring the resolution of these parameters until the function is actually invoked at runtime, ensuring that environment variables are properly set and accessible. Additionally, the error messaging for unset list parameters has been significantly improved for clearer diagnostics.

Highlights

  • Deferred CORS Expression Resolution: Modified https.onRequest and https.onCall to defer the resolution of cors options when they are Expression types (like params.defineList()). This prevents JSON.parse(undefined) errors during deployment by resolving the parameter at request time instead of definition time.
  • Improved Parameter Not Set Error: Enhanced ListParam.runtimeValue() to explicitly check if an environment variable is undefined or empty. If so, it now throws a clear error message indicating that the parameter is not set, providing better debugging information than a generic JSON parsing error.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • src/params/types.ts
    • Added a check in ListParam.runtimeValue() to throw a specific error if the associated environment variable is undefined or empty, preventing JSON.parse(undefined) errors.
  • src/v2/providers/https.ts
    • Refactored onRequest and onCall functions to handle cors options of type Expression by deferring their resolution to request time using a dynamic origin function.
    • Ensured that the enableCors debug flag and array-based origin handling are correctly applied within the deferred resolution logic.
Activity
  • The author, HassanBahati, has implemented a fix for a deployment issue related to CORS parameter resolution.
  • The changes include modifications to parameter handling and HTTP function providers.
  • Existing tests for onRequest and onCall with defineList are cited as covering the fix, indicating no new tests were deemed necessary.
  • The PR resolves issue params.defineList is always parsed as undefined in v6 #1773.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively addresses a crash during deployment when params.defineList() is used for CORS configuration. Deferring the resolution of the CORS expression to request-time is the correct approach. The improved error message in ListParam.runtimeValue() is also a welcome addition for better developer experience. My review includes a couple of suggestions to improve code conciseness and maintainability by refactoring duplicated logic.

@HassanBahati
Copy link
Member Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively addresses a deployment failure caused by the premature resolution of params.defineList() for CORS options. Deferring the resolution to request time is the correct approach. The added error handling for list parameters is also a valuable improvement. However, the changes introduce a bug in how non-parameterized CORS origins are handled when they are a RegExp or an array, due to an incorrect type cast. I've included review comments with suggestions to fix this issue while maintaining the function-based approach for CORS configuration.

@HassanBahati
Copy link
Member Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively resolves a deployment failure caused by the premature resolution of params.defineList in CORS configurations. By deferring the parameter resolution to request time, the core issue is addressed correctly. The added error handling for missing list parameters is also a valuable improvement. My review includes a suggestion to further enhance error handling for malformed list parameters and a refactoring proposal to eliminate duplicated code in the CORS logic for better maintainability.

@HassanBahati
Copy link
Member Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively resolves a deployment failure caused by the premature resolution of params.defineList() when used for CORS configuration. Deferring the parameter resolution to request-time is the correct approach, and the implementation using buildStaticCorsOriginCallback and buildCorsOriginFromExpression is clean and well-structured. The enhancement to ListParam.runtimeValue() to provide more explicit error messages is also a valuable improvement for developer experience. I've identified a minor inconsistency in how the enableCors debug flag is handled in onCall compared to onRequest, and I've provided a suggestion to align their behaviors. Overall, this is a solid and important fix.

Comment on lines +517 to +523
let cors: string | boolean | RegExp | Array<string | RegExp> | undefined;
if ("cors" in opts) {
cors = opts.cors as string | boolean | RegExp | Array<string | RegExp>;
} else {
cors = opts.cors;
cors = true;
}
} else {
cors = true;
}

let origin = isDebugFeatureEnabled("enableCors") ? true : cors;
// Arrays cause the access-control-allow-origin header to be dynamic based
// on the origin header of the request. If there is only one element in the
// array, this is unnecessary.
if (Array.isArray(origin) && origin.length === 1) {
origin = origin[0];
let origin = isDebugFeatureEnabled("enableCors") ? true : cors;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There's an inconsistency in how the enableCors debug flag interacts with cors: false between onCall and onRequest. In onRequest, cors: false is respected even when the debug flag is on, correctly disabling CORS. However, in onCall, the current logic allows the debug flag to override cors: false, which enables CORS contrary to the explicit configuration. To ensure consistent behavior across both function types, I suggest refactoring this logic to align with the implementation in onRequest.

    let origin = "cors" in opts ? opts.cors : true;
    if (isDebugFeatureEnabled("enableCors")) {
      // Respect `cors: false` to turn off cors even if debug feature is enabled.
      origin = opts.cors === false ? false : true;
    }

@HassanBahati HassanBahati requested a review from cabljac February 4, 2026 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

params.defineList is always parsed as undefined in v6

1 participant