feat: add custom middleware #3502
Conversation
|
Sorry for not replying to the initial issue! Yes we can support. What do you think about the branch in https://github.com/astral-sh/uv/compare/konsti/custom-middleware ? It's similar to yours but encapsulates the stack selection into Am i correct to assume you don't care about middleware for the offline case? |
|
Hi Konsti, that looks a lot better actually! Good question about the offline, our middleware does not account for it yet, but if you feel it's better to add it to the offline case as well, that seems totally valid! |
|
(force-pushing because i had to rebase on main, your initial commit should be preserved) |
konstin
force-pushed
the
feat/custom-middleware
branch
from
95fec31
to
7b88899
Compare
| @@ -113,7 +115,7 @@ pub(crate) async fn pip_compile( | |||
| let client_builder = BaseClientBuilder::new() | |||
| .connectivity(connectivity) | |||
| .native_tls(native_tls) | |||
| .keyring(keyring_provider); | |||
| .middleware_stack(MiddlewareStack::new(3, keyring_provider)); | |||
There was a problem hiding this comment.
Not sure about where to put the retries constant
|
@zanieb - do you want to review this? |
|
Sure |
konstin
force-pushed
the
feat/custom-middleware
branch
from
7b88899
to
ec5f16f
Compare
| .keyring(keyring_provider) | ||
| .middleware_stack(MiddlewareStack::new(3, keyring_provider)) |
There was a problem hiding this comment.
Hey @tdejager sorry I've been slow to get to this.
I don't love this new API. Could we instead:
- Only create the retry middleware if retries is non-zero
- Replace the
Keyringoption withOption<AuthMiddleware> - Add a
extra_middleware: Vec<Arc<dyn Middleware>>option
I think this should get you the same functionality, right?
There was a problem hiding this comment.
Sounds like a good improvement to me, I can also make the changes early next week if you are busy @konstin :)
There was a problem hiding this comment.
I'll be out next week fyi.
I missed that @konstin introduced the MiddlewareStack — let's make sure he's onboard with my proposal :)
There was a problem hiding this comment.
Yeah maybe having the AuthMiddleware in the stack is enough. I thought you might want to be more explicit about enabling/disabling the auth middleware. Let see what konsti thinks :)
There was a problem hiding this comment.
I think we can just have the AuthMiddleware populated by default and ya'll can pass None to explicitly remove it.
There was a problem hiding this comment.
I changed the API so it's now .with methods and checks for > 0 retries, but if we don't take a keyring it becomes even more verbose because we have a number of callsites, and also KeyringProviderType lives in uv-configuration so it can't be Into<AuthMiddleware>.
konstin
force-pushed
the
feat/custom-middleware
branch
from
ec5f16f
to
8869efc
Compare
Summary
This adds the ability to the
RegistryClientandBaseClientfor the consumer of the library to add custom middleware. The problem is that we have different authentication methods/requirements for pypi registries and would like to use the middleware provided by https://github.com/mamba-org/rattlerI've decided that if you do this, it overrides the normal middleware. This makes the
retriesoption, and maybe some others, not really do anything anymore. Which is a bit weird, but couldn't directly think of a better way to do it, as you might end up with 'duplicated' middleware maybe.This should close the issue/question I made: #3400
I'll make it a draft for now because I do not know how you feel about such a change and maybe there is a different design you have in mid.
Test Plan
Once, I can update to the latest uv version, I'll try and test things out directly in: https://github.com/prefix-dev/pixi. I'm happy to add rust test here if you know a good place for it.