Fix #4056: keep blank query values, add token bucket regression test#4069
Fix #4056: keep blank query values, add token bucket regression test#4069MukundaKatta wants to merge 1 commit intoPrefectHQ:mainfrom
Conversation
Addresses three bugs reported in issue PrefectHQ#4056: 1. parse_qs in match_uri_template now passes keep_blank_values=True so that URIs like resource://data/42?format= no longer silently drop the format key. Adds tests covering blank-value and mixed-value cases. 2. Adds a regression test that locks in the correct token-bucket semantics: last_refill must advance on every consume() call, including denied ones. The fix proposed in the issue (move last_refill update inside the success branch) would let a client that retries quickly bypass the configured refill rate by re-counting the same elapsed window on each retry. The env_nested_delimiter concern from the issue is not addressed here: `FASTMCP_DOCKET__CONCURRENCY` (single underscore between prefix and the nested key, double for nesting) already works as documented in docs/more/settings.mdx. The form `FASTMCP__DOCKET__CONCURRENCY` is not the correct pydantic-settings syntax when the parent has env_prefix="FASTMCP_". Closes PrefectHQ#4056.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
marvin-context-protocol
Bot
added
bug
|
Thanks for the report. This issue goes beyond what our contributor guidelines ask for — we just need a short problem description and an MRE. Please see our contributing guidelines and condense this issue. We'll triage it once it's trimmed down. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fae7daa83
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # implementation, last_refill advances on each call, so total | ||
| # accumulated tokens after 0.2s is ~2 (10/s * 0.2s). | ||
| for _ in range(20): | ||
| await limiter.consume(1) |
There was a problem hiding this comment.
This regression test does not actually fail if last_refill is only advanced on successful consumes, because the loop never checks whether consume(1) was denied. Under that buggy behavior, retries start succeeding intermittently (and draining tokens), so the final consume(5) is False assertion can still pass, leaving the clock-freeze regression undetected. Add an assertion on each loop iteration (or control time deterministically) so the test distinguishes the intended behavior.
Useful? React with 👍 / 👎.
The HeadlessOAuth callback handler in src/fastmcp/utilities/tests.py
called parse_qs() without keep_blank_values=True, so an explicitly-empty
query parameter (e.g. ?state= or ?error_description=) was silently
dropped. Real OAuth providers can emit empty values, and downstream code
uses .get("state", [None])[0] to distinguish "not present" (None) from
"present but empty" (""). Without the flag, both cases collapsed to None.
This is the same defect class that PR PrefectHQ#4069 fixed in
src/fastmcp/resources/template.py; this commit applies the same fix to
the remaining parse_qs call site so behavior is consistent across the
code base.
Adds three regression tests in tests/utilities/test_tests.py covering
blank state, missing state, and blank error_description.
1 participant
Why
Issue #4056 reports three minor bugs. After investigation, only one is a real defect that needs a code fix; the other two are addressed in the PR text below.
Closes #4056.
What
Bug 1 (fixed):
parse_qsdrops blank query valuessrc/fastmcp/resources/template.pycallsparse_qs(query_string)withoutkeep_blank_values=True, so URIs likeresource://data/42?format=silently lose theformatkey. The fix addskeep_blank_values=Trueand a brief comment explaining why.Bug 2 (no code change needed):
env_nested_delimiterwith nested modelsThe issue claims
FASTMCP__DOCKET__CONCURRENCYshould work. Withenv_prefix="FASTMCP_"on the parent andenv_nested_delimiter="__", the correct pydantic-settings syntax isFASTMCP_DOCKET__CONCURRENCY(single underscore between the prefix and the nested key, double underscore for nesting). This is exactly whatdocs/more/settings.mdxalready documents:So the documented form works; the form in the issue is not standard pydantic-settings syntax and would not work with the configured prefix. Happy to add a regression test or adjust the docs further if a maintainer wants to nail this down.
Bug 3 (proposed fix is incorrect; defensive test added)
The issue proposes moving
self.last_refill = nowinside theif self.tokens >= tokens:branch inTokenBucketRateLimiter.consume(). That fix is wrong: it freezes the clock during denied requests. A client that retries quickly after a denial would re-count the same elapsed window on each retry, accumulating tokens far faster than the configuredrefill_rateand bypassing the rate limit. This was independently flagged by the Codex reviewer on PR #4057 and the proposer agreed (#4057 (comment)).The current implementation is the standard token bucket algorithm (advance the clock on every
consume()call). To prevent regression, this PR addstest_denied_consumes_do_not_freeze_clockwhich drains the bucket, hammers it with denied calls over ~0.2s, and asserts that no extra burst capacity has accrued.Tested
test_query_param_with_blank_value_is_preserved— verifies?format=round-trips as{"format": ""}.test_query_param_with_blank_and_present_values— mixed blank + non-blank query values.test_denied_consumes_do_not_freeze_clock— locks in correct token bucket semantics so the wrong fix from the issue can't be reintroduced.tests/resources/test_resource_template.pyandtests/server/middleware/test_rate_limiting.pycontinue to apply.