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
|
Hi all, just wondering if you guys are happy with the current design, and if rebasing is the only requirement for this PR to be merged? |
|
Sorry, I need to review the latest implementation but am prioritizing things that impact more users right now. |
|
No worries. Thanks @zanieb ! |
|
Is this still needed? |
|
Would still be very useful for us, however, as reqwest-middleware is now a forked version, I'm unsure regarding the compatibilities with the open-source version. However, we could re-vendor the middleware if needed :) |
|
It looks like @konstin added |
|
Sounds good, is there something you like me to change other than a rebase? |
|
Can we drop all the |
|
Not at all computer currently but sounds good to me, let me give it a go at the end of the week! |
tdejager
force-pushed
the
feat/custom-middleware
branch
from
8869efc to
a052418
Compare
|
Closing this as there were so many changes it make sense to make a different PR. Will reference this when I get to it :) |
This PR is a revival of #3502, albeit in a much simpler form. This would allow for different middlewares like authentication and such, useful for if you want to deviate from the keychain authentication methods when using uv as a library. @zanieb I hope I made the changes as you noted you wanted to see them :) Happy to add/change anything you need.
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.