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

Replace reqwest with http and hyper, and tower #297

Merged
merged 124 commits into from
Mar 29, 2023

Conversation

L1ghtman2k
Copy link
Contributor

@L1ghtman2k L1ghtman2k commented Jan 16, 2023

Fixes #99

@L1ghtman2k L1ghtman2k marked this pull request as draft January 16, 2023 03:28
@L1ghtman2k
Copy link
Contributor Author

L1ghtman2k commented Jan 16, 2023

Hey @XAMPPRocky , this is still a WIP, I am just pushing this here to indicate that it is being worked on.

A few I am planning to do:

  • Refactor.
    • There are a few instances where complex calls(ex: &str -> URI) can be handled by a until function
    • ...
  • fix issues related to URL -> URI transition (unwrapping infallible errors, putting "/" in front of routes etc)
  • more robust interface for OctocrabBuilder(should include with_layer function to allow non-default custom layers)
  • Fixing tests
    • Assuming base_uri is now a service layer, I will need to fix/cleanup tests relying on absolute_uri function.
  • Cargo Features(hyper-openssl, hyper-timeout, optional features, etc)

But thus far, I have a question:

  • Does it make sense to move base_uri to a service layer(that is how I implemented it in current PR, and that's how kube-rs implements it), instead of having it be constructed every time on the fly? I see a few pros for this, like how it makes all caller functions a lot cleaner(since you wouldn't need to interpolate strings with base_uri). However, it does introduce a niche(?) issue with the cloning of octocrab client: You wouldn't be able to change base_uri of the cloned client(since it will clone underlying service that was already constructed)

@L1ghtman2k L1ghtman2k changed the title sync changes made so far Replace reqwest with http and hyper, and tower Jan 16, 2023
@XAMPPRocky
Copy link
Owner

You wouldn't be able to change base_uri of the cloned client(since it will clone underlying service that was already constructed)

I'd need to see the service code in kube and this more, but just from your description. My initial thought for the solution to this problem is to have the service wrapped in a ArcSwap, this way we can update the underlying service, while still having all the shallow clone benefits of Arc.

@L1ghtman2k
Copy link
Contributor Author

L1ghtman2k commented Jan 16, 2023

Service in Kube does it via https://docs.rs/tower/latest/tower/buffer/struct.Buffer.html

@L1ghtman2k
Copy link
Contributor Author

another potential improvement is to replace url lib completely with hyper::Uri, and hyper_serde crate, though, it does become a little redundant to annotate Uri fields with

#[serde(deserialize_with = "hyper_serde::deserialize",
serialize_with = "hyper_serde::serialize")] 

annotation... I think for now it is better to keep the URL crate for serialization/deserialization operations. Plus, I am not entirely sure how hyper_serde behaves when uri is wrapped around constructs like Option

@L1ghtman2k
Copy link
Contributor Author

Also, could you re-approve the workflow rerun? I think all works on my end, just want to make sure it compiles in CI

@L1ghtman2k
Copy link
Contributor Author

I worked around the base URI service issue by making it the outer layer, which wraps BufferService inside. This essentially means that we are only full cloning the base_uri service when making .clone() call, everything else is a shallow copy offered by BufferService

@L1ghtman2k
Copy link
Contributor Author

At this point, I think this is almost done. The few things that are left are: implement some retry layer(hidden behind retry feature, just like timeout), and fix docs

@L1ghtman2k
Copy link
Contributor Author

L1ghtman2k commented Jan 19, 2023

seems like openSSL issue on windows. I will try to build it on that target system

edit: ended up using hyper-tls instead

@L1ghtman2k
Copy link
Contributor Author

L1ghtman2k commented Jan 20, 2023

A few things I am not sure what to decide on:

Currently, the OctocrabBuilder::build() implements various hyper-client configurations (tls, timeout), and additional layers(tls, retry, extra-headers) via features that are enabled by default. In addition, OctocrabBuilder contains a few fields that configure some of the settings for these functionalities(e.g. connect_timeout).

This is a rather opinionated approach and leaves little room for customization in terms of adding your own layers directly into the builder.

If the end user would like to introduce some additional layer (say metrics layer before, or after retry), or replace the current implementation with their own (e.g. custom tracing), or caching(#175), they would have to use Octocrab::new() and construct their own service from scratch, which could lead to some issues if developers are not careful(e.g. while Octocrab::new does take care of adding BaseUri layer, it doesn't ensure that service contains necessary layers, like ExtraHeaders for auth).

We could leave it as is, we would just have to provide some good examples(or reference the OctocrabBuilder::build function)

Another way we could go about it is to make the Octocrab Builder completely unopinionated, and let users specify their own layers from scratch(kube-rs way):

    pub fn with_layer<L: Layer<T>>(self, layer: L) -> Self<L::Service> {
        Self {
            service_builder: self.service_builder.layer(layer),
            ..Self
        }
    }

This has a plus since it makes OctocrabBuilder reusable for custom clients. However, this makes the configuration slightly more complicated for users who are new or aren't too interested in customizing their services. This also leaves a lot of room for potential pitfalls(forgetting ExtraHeader Layer for auth, not enabling tls, etc)

There are approaches that could be implemented by combining both:

  • Move important pieces (we would have to decide which ones, I think at the very least ExtraHeader's auth portion) to Octocrab::new(), just like base_uri, and then let clients use with_layer any way they want.
  • Supporting "best practices" layers(e.g. retry, tls, tracing) with opinionated implementation. One way to implement this would be to create an opinionated with_layer_X(ex: with_layer_timeout(write_timeout, read_timeout ...).

I am personally leaning more towards current implementation, and backing it up with better examples on how to construct custom clients

L1ghtman2k and others added 17 commits March 21, 2023 13:49
 - add new_empty to facilitate bare bones octocrab builder. Point fn new to fn default to make changes backward compatible.
 - add base_url function, and point it to base_uri with deprecation notice
# Conflicts:
#	src/api/actions.rs
#	src/api/activity/notifications.rs
#	src/api/apps.rs
#	src/api/events.rs
#	src/api/repos.rs
#	src/api/repos/events.rs
#	src/error.rs
#	src/lib.rs
#	src/page.rs
#	tests/repos_stargazers_tests.rs
#	tests/team_invitations_tests.rs
#	tests/team_members_tests.rs
@L1ghtman2k
Copy link
Contributor Author

L1ghtman2k commented Mar 21, 2023

Wired, GitHub UI merge conflict resolution is a little different than my IDEs "Sync Fork"... Merge conflicts should be resolved now...?

Cargo.toml Outdated Show resolved Hide resolved
@XAMPPRocky
Copy link
Owner

Thank you for your PR, and congrats on your first contribution! 🎉

@XAMPPRocky XAMPPRocky merged commit 16134a8 into XAMPPRocky:master Mar 29, 2023
@maflcko
Copy link
Contributor

maflcko commented Mar 31, 2023

I think there were a few typos in the routes. Hopefully fixed in #320

#[tokio::test]
async fn parametrize_uri_valid() {
//Previously, invalid characters were handled by url lib's parse function.
//Todo: should we handle encoding of uri routes ourselves?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is that label names may be part of the route and can contain any utf-8. See #323

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace reqwest with http
5 participants