Improve the 403 forbidden hint to suggest ignore-error-codes when applicable#19521
Conversation
uv test inventory changesThis PR changes the tests when compared with the latest
|
|
See #19517 |
| /// Return the prior successful Simple API access for an index that returned `403 Forbidden`. | ||
| pub fn forbidden_access(&self, index_url: &IndexUrl) -> Option<ForbiddenIndexAccess> { |
There was a problem hiding this comment.
This function name and description are really confusing for the actual behaviour here.
I would maybe call this forbidden_access_hint or forbidden_access_pattern. Like, what this is doing is looking at the successful and failed requests and noting whether the failures and successes are disjoint sets, because if they are that indicates all the failures are due to missing packages and we can recommend config to enable fallback.
| .any(|package_name| capabilities.successful_simple_api.contains(package_name)) | ||
| { | ||
| return Some(ForbiddenIndexAccess::SamePackage); |
There was a problem hiding this comment.
It feels weird this is just looking at all queries and that we're not taking a specific package as input.
It also feels weird that if we get a single matching hit we go "well the user doesn't need this suggestion anymore".
Is there any rationale to either of these choices? I'm fine with landing it as a strict improvement either way.
| /// We only store indexes that lack capabilities (i.e., don't support range requests, aren't | ||
| /// authorized). The benefit is that the map is almost always empty, so validating capabilities is | ||
| /// extremely cheap. |
There was a problem hiding this comment.
Are we worried about the cost here? This will add a lot of contention, right? Literally every request has to mutate this now?
There was a problem hiding this comment.
Hm yes that does seem problematic, thanks for pointing that out.
I feel like we should avoid that somehow.
| self.0 | ||
| .write() | ||
| .unwrap() | ||
| .entry(index_url) | ||
| .or_default() | ||
| .successful_simple_api | ||
| .insert(package_name); |
There was a problem hiding this comment.
One possible not-even-sure-it's-an-optimization we could do here since we do have a RWLock: we could first acquire read-only and check if the package name already appears, and early-return if it does.
| ╰─▶ Because idna was not found in the package registry and anyio==4.3.0 depends on idna>=2.8, we can conclude that anyio==4.3.0 cannot be used. | ||
| And because only anyio==4.3.0 is available and your project depends on anyio, we can conclude that your project's requirements are unsatisfiable. | ||
|
|
||
| hint: An index URL (http://[LOCALHOST]/) returned a 403 Forbidden error, but uv received a successful response for another package from the index. If the failing package is not present on this index, consider adding `ignore-error-codes = [403]` to the index's `[[tool.uv.index]]` entry to continue searching across indexes. |
|
I'll defer to @Gankra on the review, I like the hint though. |
3 participants
No description provided.