Skip to content

fix(https): don't pass None CORS options to flask-cors; fix ruff PLR0917 - #336

Merged
shettyvarun268 merged 6 commits into
mainfrom
fix/lint-and-mypy-deps
Nov 11, 2026
Merged

shettyvarun268 merged 6 commits into
mainfrom
fix/lint-and-mypy-deps

Conversation

@shettyvarun268

@shettyvarun268 shettyvarun268 commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  1. Fix on_request CORS handling (https_fn.py, options.py):

    • Previously, https_fn.on_request always passed methods=options.cors.cors_methods and origins=options.cors.cors_origins to flask_cors.cross_origin(), even when either field was None.
    • Because flask-cors merges passed keyword arguments into its defaults via options.update(...), passing None overwrote the default value instead of falling back to it—crashing requests at runtime on flask-cors 3.0.10 through 6.0.1 when only one field of CorsOptions was set, and failing to emit CORS headers on flask-cors 6.0.5 (as well as failing mypy against flask-cors 6.0.5's CrossOriginOptionsInput TypedDict).
    • Updated on_request to omit unset CorsOptions fields so they fall back to flask-cors defaults ("*" for origins and all standard HTTP methods for methods), matching on_call's behavior. Passing CorsOptions() with both fields unset now enables default permissive CORS headers across all origins and methods.
    • Refined CorsOptions.cors_origins type annotation from _re.Pattern to _re.Pattern[str] and documented the default behavior when fields are left unset.
    • Note: This fix stands alone on main (and also unblocks the flask-cors 6.0.5 upgrade in chore(deps): bump the version-minor-and-patch group across 1 directory with 14 updates #335).
  2. Ruff PLR0917 (dataconnect_fn.py):

Testing

  • Added test_on_request_cors_options_branches in tests/test_https_fn.py testing preflight OPTIONS requests and asserting Access-Control-Allow-Origin and Access-Control-Allow-Methods headers across all 4 CorsOptions combinations plus a non-matching origin case.
  • Verified ruff check ., ruff format --check ., mypy ., and pytest all pass.

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

Copy link
Copy Markdown
Contributor

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 updates CORS configuration handling in https_fn.py to dynamically build arguments, refines a regex pattern type annotation in options.py, and adds PLR0917 to the global ignore list in pyproject.toml. The review feedback recommends avoiding the global suppression of PLR0917 by using local suppression instead, and suggests refactoring the CORS dictionary construction to maintain type safety without resorting to _typing.Any.

Comment thread pyproject.toml Resolved
Comment thread src/firebase_functions/https_fn.py Resolved
@shettyvarun268
shettyvarun268 marked this pull request as ready for review November 10, 2026 21:13
@shettyvarun268 shettyvarun268 changed the title chore(lint): fix ruff PLR0917 and flask-cors 6.0.5 mypy typing fix(https): don't pass None CORS options to flask-cors; fix ruff PLR0917 Nov 10, 2026

@wandamora wandamora left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A few nits, but overall LGTM. Thanks!

Comment thread tests/test_https_fn.py Resolved
Comment thread tests/test_https_fn.py Resolved
Comment thread src/firebase_functions/options.py Resolved
@shettyvarun268
shettyvarun268 merged commit b3241f4 into main Nov 11, 2026
22 checks passed
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.

2 participants