fix(circuit_breaker): normalize handled/ignored exceptions to a validated tuple at construction#8319
Conversation
|
Not all issues are linked correctly. Please link each issue to the PR either manually or using a closing keyword in the format If mentioning more than one issue, separate them with commas: i.e. |
…ated tuple at construction
46f8d51 to
734c6be
Compare
leandrodamascena
left a comment
There was a problem hiding this comment.
Hey @Iamrodos thanks a look for sending this PR.
Since #8316 already merged, that stray ValueError you flagged is on develop now, so let's just fix it here rather than open a follow-up. I checked and it's the only raw ValueError left in the utility, so this one change wraps it up.
Only open question is which exception to use. CircuitBreakerConfigError is really about CircuitBreakerConfig, and CircuitBreakerPersistenceError is for write-path store errors, so neither is a perfect fit for a construction check. I'd go with CircuitBreakerConfigError or the base CircuitBreakerError, but you decide, I'm happy with both. Add a small test and I'll approve.
…eError when key_attr equals sort_key_attr
|
Done. Selected |
Thanks! Pls fix the small error with ty and then we can merge. Let me know if you need any help. |
…rmalize_exceptions
|



Issue number: closes #8317
Summary
Changes
CircuitBreakerConfigstoredhandled_exceptions/ignored_exceptionsverbatim and used them directly inisinstance(exception, self.handled_exceptions). Passing a list constructed fine but raisedTypeError: isinstance() arg 2 must be a type, a tuple of types, or a unionlater — only when a circuit tripped, i.e. only under a real downstream failure. Becausecounts_as_failureraises before the failure is counted, it masks the original error and the circuit never opens.Both params now go through a
_normalize_exceptionsstep in__init__that accepts a single exception type or any iterable of them (normalizing to a tuple), and validates the result is a non-empty tuple of exception types — raisingCircuitBreakerConfigErrorat construction instead of a crypticTypeErrorlater. Mirrors howevent_handler's exception handler already accepts a single class or a list. A baretuple(...)(as in #8318) isn't enough: it regresses the single-type case (tuple(SomeException)raises) and skips validation.User experience
Before:
After: the list just works; genuinely bad input (a non-exception element, a non-iterable, an empty list, a str) fails immediately at construction with a clear
CircuitBreakerConfigError.Tests
New unit tests in
test_config_and_states.py: list/generator → tuple, single type → tuple (the #8318 regression), and rejection of an empty iterable, a non-exception element, a non-iterable, and a str.pytest(unit + functional),make mypy, ruff, and the security/complexity baselines all pass.One thing I noticed while in here — totally non-blocking, just curious: the exceptions have a slightly different feel across the utility. Config validation raises
CircuitBreakerConfigError, butCircuitBreakerDynamoDBPersistence'ssort_key_attr == key_attrcheck raises a bareValueError, soexcept CircuitBreakerErrorcatches every construction/config problem except that one. It's tiny at runtime (fails fast at setup), but since it's alpha it felt like the cheap moment to raise it, before changing it would be a breaking change. Would you want construction-time validation unified underCircuitBreakerError, or is theValueErrordeliberate? Happy to fold it into this PR or do a quick follow-up — whatever suits.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.