Fix params.defineList cors parsed as undefined at deploy#1813
Fix params.defineList cors parsed as undefined at deploy#1813HassanBahati wants to merge 4 commits intomasterfrom
Conversation
Summary of ChangesHello @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 Highlights
🧠 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
Activity
Using Gemini Code AssistThe 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
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 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
|
There was a problem hiding this comment.
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.
|
/gemini review |
There was a problem hiding this comment.
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.
|
/gemini review |
There was a problem hiding this comment.
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.
|
/gemini review |
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
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;
}
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
Testing
The fix is covered by these existing tests in spec/v2/providers/https.spec.ts:
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.
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.