robustness: `with_suffix` validation expression has surprising operator precedence and accepts empty suffix silently
- 主要語言
- Python
- 星號
- 1.5k
- 分支
- 215
- 平均合併
- 1 天 2 分鐘
- 30 天內合併 PR
- 13
描述
## Problem
The guard `if suffix and not suffix[0] == "." or suffix == "." or "/" in suffix:` parses as `(suffix and not suffix[0] == ".") or (suffix == ".") or ("/" in suffix)`. The leading `suffix and ...` short-circuit means an empty string `""` falls through every clause and is accepted as a valid "suffix", silently triggering `name[:-len("")] + ""` later (line 1419), i.e. a no-op. If empty-suffix-means-strip is intentional that should be a documented branch; if it is not, it is a quiet correctness bug. Separately, `not suffix[0] == "."` (relying on `not` binding tighter than `==`) is the kind of expression reviewers misread — `suffix[0] != "."` is unambiguous.
## Why This Matters
The function's docstring promises "suffix (file extension of name) replaced" and the error message is "Invalid suffix"; silently accepting `""` is a behavioural surprise that callers will write tests around without realising. Rewriting the condition once makes the intent explicit and lets the maintainer decide deliberately whether empty is permitted.
## Suggested Fix
Decide on the empty-string semantics (either explicitly allow with a comment, or reject), and rewrite for clarity:
```python
if not suffix or suffix[0] != "." or suffix == "." or "/" in suffix:
raise ValueError(f"Invalid suffix {suffix!r}")
```
If empty-means-strip is a feature, branch on it before the validator:
```python
if suffix == "":
# explicit no-op / strip case
...
elif suffix[0] != "." or suffix == "." or "/" in suffix:
raise ValueError(f"Invalid suffix {suffix!r}")
```
Either way, a test should be added documenting the chosen behaviour.
## Details
| | |
|---|---|
| **Severity** | 🟡 Medium |
| **Category** | robustness |
| **Location** | `yarl/_url.py:1412` |
| **Effort** | ⚡ Quick fix |
---
🤖 Created by Kōan from audit session
貢獻指南
評估
這個 Issue 還沒有評估資料。