Skip to content

fix: accept a single limit string for default_limits/application_limits (fixes #239)#278

Closed
gaoflow wants to merge 1 commit into
laurentS:masterfrom
gaoflow:fix-239-default-limits-string
Closed

fix: accept a single limit string for default_limits/application_limits (fixes #239)#278
gaoflow wants to merge 1 commit into
laurentS:masterfrom
gaoflow:fix-239-default-limits-string

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 9, 2026

Copy link
Copy Markdown

Summary

Limiter(default_limits="1/minute") — passing a single limit as a string rather than a list — crashes when a limit is hit:

AttributeError: 'ValueError' object has no attribute 'detail'

__init__ iterates default_limits (and application_limits), and iterating a string yields its characters, so "1/minute" becomes eight invalid one-character limits ({'1', '/', 'm', 'i', ...}). At request time each fails to parse; the resulting ValueError is then routed to the rate-limit handler, which expects a RateLimitExceeded and reads .detail — masking the real cause.

Fix

Coerce a bare string to a single-element list for both default_limits and application_limits. This is the coercion the issue suggested, and matches the "string or list of strings" the docstring already advertises.

Tests

Added regression tests (run across all three middleware/handler combinations) asserting that a string default_limits/application_limits stores exactly one limit and is enforced (200 then 429) instead of crashing. Full local suite: 109 passed.

@gaoflow gaoflow changed the title Accept a single limit string for default_limits/application_limits (fixes #239) fix: accept a single limit string for default_limits/application_limits (fixes #239) Jun 10, 2026
Limiter(default_limits="1/second") and application_limits accepted a
bare string but crashed later with an opaque AttributeError when a limit
was exceeded, because the string was iterated character-by-character
instead of being treated as one limit. Coerce a str to a single-element
list at parse time so a bare string works the same as a one-item list.

Fixes laurentS#239.
@gaoflow gaoflow force-pushed the fix-239-default-limits-string branch from be77b70 to d15d612 Compare June 10, 2026 14:50
@laurentS

Copy link
Copy Markdown
Owner

Hi @gaoflow I wonder what brought the need for this fix. The default_limits and application_limits both have clear typing, so the "common mistake" mentioned in the comment in extension.py is a bit strange. Type checkers should raise an error on such a mistake, no?

@gaoflow

gaoflow commented Jun 14, 2026

Copy link
Copy Markdown
Author

Good question. The trigger is that both loops iterate the argument directly: for limit in set(default_limits) (and for limit in application_limits). A str is iterable, so default_limits="1/minute" does not fail there: it silently iterates into single characters ("1", "/", "m", ...), building a set of invalid one-character "limits". Nothing raises at init; the failure only surfaces later at request time as the AttributeError: 'ValueError' object has no attribute 'detail' from #239, which gives the user no hint that the real cause was passing a string instead of a list.

You are right that default_limits/application_limits are typed List[StrOrCallableStr], so a type checker does flag a bare string. The argument for handling it at runtime anyway is that (a) the current behaviour is a silent footgun whose crash is far removed from the cause, and (b) much slowapi setup is app glue where people are not running a type checker in CI. #239 itself suggested either coercing the value or raising a clear early error.

I went with coercion as the forgiving option, but if you would rather keep the typing strict and fail fast, I am happy to switch this to an explicit early ValueError in __init__ (e.g. "default_limits should be a list of limit strings; wrap a single limit as ['1/minute']") instead of silently wrapping it. Which would you prefer?

@laurentS

Copy link
Copy Markdown
Owner

OK, AI slop as I thought. Closing

@laurentS laurentS closed this Jun 15, 2026
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