Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make api clients with interceptors cloneable #566

Merged
merged 6 commits into from
Aug 29, 2024

Conversation

sprudel
Copy link
Contributor

@sprudel sprudel commented Aug 25, 2024

Make clients clonable:

  • Remove the chained interceptor
  • Make service account interceptor cloneable by managing token with interior mutablity

Note: this is built on top of #565

@buehler
Copy link
Collaborator

buehler commented Aug 26, 2024

Hi @sprudel

While I like the idea of clonable interceptors, I don't understand why the chained interceptor should be removed. (1) it is a breaking change and (2) it was introduced to remove the need for generic and multiple impls for the client builder.

Since it is duplicate code, I'd love to have only one implementation for the client builder. Otherwise, we need to maintain several implementations and things can and will go wrong when some parts are updated.

@sprudel
Copy link
Contributor Author

sprudel commented Aug 26, 2024

@buehler yes it is a breaking change, but when making the clients cloneable it created a lot of friction.
Compare #543 which created even more complexity by introducing a new trait. (But I didn't have a look if this could have been simplified).
Basically it felt to me not being a good abstraction, as it locks all properties such as Clone not only to the concrete interceptor but also to the impl of the ChainedInterceptor.

As far as I have understood it was only introduced to make the impl of the builder easy, or was there another reason?
For me a good builder should make it easy to construct a type, and not change the result, only because a builder is used.

I have iterated over the implementation and now found a way to have no code duplication by using generics in a smart way.
This also made it very easy to support all possible interceptors, which can be quite handy.

I pushed the modified version to this branch. What do you think of this version?

Still it is a fair argument to say such a breaking change is not worth it. Given #567 however, this will be needed either way.

@sprudel
Copy link
Contributor Author

sprudel commented Aug 29, 2024

@buehler From my perspective this PR would be mergable. However, I'm not sure if using the generics to remove the chained interceptor still fulfills your requirements.

@buehler
Copy link
Collaborator

buehler commented Aug 29, 2024

This is a pretty solid implementation. Thank you @sprudel. To be honest, I didn't know enough about rust types to make this kind of changes. It serves as a good example for me now ;-)

I like the implementation and how it is used.

@buehler buehler merged commit da2dddf into smartive:main Aug 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants