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.

Sign up for GitHub

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

Jump to bottom

Use GitHub GraphQL API if token is provided as an optimization #868

Closed
NobodyXu opened this issue Mar 6, 2023 · 8 comments · Fixed by #1124
Closed

Use GitHub GraphQL API if token is provided as an optimization #868

NobodyXu opened this issue Mar 6, 2023 · 8 comments · Fixed by #1124

Comments

@NobodyXu
Copy link
Member

NobodyXu commented Mar 6, 2023

With recent surges of dependabot PRs, our CI was able to hit the rate limit of GitHub API and it takes half an hour for it to complete due to installing crates from source and some might even fail.

As we have previously discussed about this #832 (comment) , it seems that the GitHub GraphQL API allows 50k requests per hour since we only need a few fields out of the entire release API schema:

oh I tested that and we can do something like 50k crates per hour with the quota in there.

@passcod since you have already tested this out, can you work on this please?
@Techcable
Copy link

Techcable commented Mar 9, 2023 •
edited
Loading

I have actually had my web browser get denied connection to GitHub because cargo bininstall is too aggressive with requests...

I don't mean to be rude, but the user experience right now is closer to running a denial-of-service script than an installation tool 😦

I've just started using cargo quickinstall directly. This works fine for me (ideally I'd like to use official releases....)

@NobodyXu
Copy link
Member Author

@Techcable Are you using 0.20.1?
This problem should be migrated in the next version due to use of GitHub Restful API in #832
I was a bit busy but should have enough time to release a new version of cargo-binstall soon.

@passcod
Copy link
Member

passcod commented Mar 11, 2023

Short testimonial of #832 experience: tried install just (on windows) and it made an absurd amount of queries, took over a minute to resolve. Then tried with latest main and it was much faster (though had some strange pauses that I'll investigate later), did much fewer requests (unauthenticated).

@NobodyXu
Copy link
Member Author

That looks great!
I will do a release once after a few (roughly ~2) PRs and this should make cargo-binstall much more usable.

@Techcable
Copy link

Short testimonial of #832 experience: tried install just (on windows) and it made an absurd amount of queries, took over a minute to resolve. Then tried with latest main and it was much faster (though had some strange pauses that I'll investigate later), did much fewer requests (unauthenticated).

I will try this too. I am very excited :)

@NobodyXu
Copy link
Member Author

NobodyXu commented Jun 4, 2023

@passcod Do you know any bug-free lightweight crate for parsing link header?

I am looking for a bug-free lightweight crate for parsing link header.

I've found hyperx and parse_link_header, but their implementation is buggy since they simply split on , and ; and disregards quoting ".

hyperx and parse_link_header also pulls in too many dependencies.

Is there any crate that satisfy my requirement?
If not I might create a new crate under cargo-bins

I've also posted this in reddit but so far found not crate satisfying the condition.

@passcod
Copy link
Member

passcod commented Jun 4, 2023 •
edited
Loading

unfortunately my best idea was http-types (already in our dep tree as it's used by hyper iirc) but they never got around to implement it, though they have for many other header types. it might be worth it to implement under there, possibly forking until they release if they're not active as we've done for other cases

@NobodyXu
Copy link
Member Author

NobodyXu commented Jun 4, 2023

@passcod Thanks, but I just realize that GraphQL doesn't use link header but pageInfo object for pagination.
My bad since I'm not familiar with GraphQL.

NobodyXu added a commit that referenced this issue Jun 4, 2023
@NobodyXu
feat: Add GraphQL support to GhApiClient
ce85ff1 
Fixed #868

 - Add new fn `remote::Client::post`
 - Add new fn `remote::RequestBuilder::body`
 - Re-export `reqwest::Body` in `remote`
 - Add GraphQL to `GhApiClient`, fallback to Restful API if token is not
   provided or authorization failed.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu NobodyXu mentioned this issue Jun 4, 2023
Merged
NobodyXu added a commit that referenced this issue Jun 4, 2023
@NobodyXu
feat: Add GraphQL support to GhApiClient
87fea86 
Fixed #868

 - Add new fn `remote::Client::post`
 - Add new fn `remote::RequestBuilder::body`
 - Re-export `reqwest::Body` in `remote`
 - Add dep percent-encoding v2.2.0 to binstalk-downloader
 - Add GraphQL to `GhApiClient`, fallback to Restful API if token is not
   provided or authorization failed.
 - Fixed `GhReleaseArtifact::try_extract_artifact_from_str`: decode
   percent encoded http url path and add regression tests

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
NobodyXu added a commit that referenced this issue Jun 4, 2023
@NobodyXu
feat: Add GraphQL support to GhApiClient
2422e0f 
Fixed #868

 - Add new fn `remote::Client::post`
 - Add new fn `remote::RequestBuilder::body`
 - Re-export `reqwest::Body` in `remote`
 - Add dep percent-encoding v2.2.0 to binstalk-downloader
 - Add GraphQL to `GhApiClient`, fallback to Restful API if token is not
   provided or authorization failed.
 - Fixed `GhReleaseArtifact::try_extract_artifact_from_str`: decode
   percent encoded http url path and add regression tests

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
NobodyXu added a commit that referenced this issue Jun 4, 2023
@NobodyXu
feat: Add GraphQL support to GhApiClient
f52da2a 
Fixed #868

 - Add new fn `remote::Client::post`
 - Add new fn `remote::RequestBuilder::body`
 - Re-export `reqwest::Body` in `remote`
 - Add dep percent-encoding v2.2.0 to binstalk-downloader
 - Add dep serde-tuple-vec-map v1.0.1 to binstalk-downloader
 - Add GraphQL to `GhApiClient`, fallback to Restful API if token is not
   provided or authorization failed.
 - Fixed `GhReleaseArtifact::try_extract_artifact_from_str`: decode
   percent encoded http url path and add regression tests
 - Added variant `GhApiError::Context` & `GhApiContextError`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
NobodyXu added a commit that referenced this issue Jun 4, 2023
@NobodyXu
feat: Add GraphQL support to GhApiClient
b22f00f 
Fixed #868

 - Add new fn `remote::Client::post`
 - Add new fn `remote::RequestBuilder::body`
 - Re-export `reqwest::Body` in `remote`
 - Add dep percent-encoding v2.2.0 to binstalk-downloader
 - Add dep serde-tuple-vec-map v1.0.1 to binstalk-downloader
 - Add GraphQL to `GhApiClient`, fallback to Restful API if token is not
   provided or authorization failed.
 - Fixed `GhReleaseArtifact::try_extract_artifact_from_str`: decode
   percent encoded http url path and add regression tests
 - Added variant `GhApiError::Context` & `GhApiContextError`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
NobodyXu added a commit that referenced this issue Jun 4, 2023
@NobodyXu
feat: Add GraphQL support to GhApiClient
395af34 
Fixed #868

 - Add new fn `remote::Client::post`
 - Add new fn `remote::RequestBuilder::body`
 - Re-export `reqwest::Body` in `remote`
 - Add dep percent-encoding v2.2.0 to binstalk-downloader
 - Add dep serde-tuple-vec-map v1.0.1 to binstalk-downloader
 - Add GraphQL to `GhApiClient`, fallback to Restful API if token is not
   provided or authorization failed.
 - Fixed `GhReleaseArtifact::try_extract_artifact_from_str`: decode
   percent encoded http url path and add regression tests
 - Added variant `GhApiError::Context` & `GhApiContextError`
 - Added variant `GhApiError::GraphQLErrors` & `GhGraphQLErrors`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
NobodyXu added a commit that referenced this issue Jun 4, 2023
@NobodyXu
feat: Add GraphQL support to GhApiClient
a4f701a 
Fixed #868

 - Add new fn `remote::Client::post`
 - Add new fn `remote::RequestBuilder::body`
 - Re-export `reqwest::Body` in `remote`
 - Add dep percent-encoding v2.2.0 to binstalk-downloader
 - Add dep serde-tuple-vec-map v1.0.1 to binstalk-downloader
 - Add GraphQL to `GhApiClient`, fallback to Restful API if token is not
   provided or authorization failed.
 - Fixed `GhReleaseArtifact::try_extract_artifact_from_str`: decode
   percent encoded http url path and add regression tests
 - Added variant `GhApiError::Context` & `GhApiContextError`
 - Added variant `GhApiError::GraphQLErrors` & `GhGraphQLErrors`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
NobodyXu added a commit that referenced this issue Jun 4, 2023
@NobodyXu
feat: Add GraphQL support to GhApiClient
93661bf 
Fixed #868

 - Add new fn `remote::Client::post`
 - Add new fn `remote::RequestBuilder::body`
 - Re-export `reqwest::Body` in `remote`
 - Add dep percent-encoding v2.2.0 to binstalk-downloader
 - Add dep serde-tuple-vec-map v1.0.1 to binstalk-downloader
 - Add GraphQL to `GhApiClient`, fallback to Restful API if token is not
   provided or authorization failed.
 - Fixed `GhReleaseArtifact::try_extract_artifact_from_str`: decode
   percent encoded http url path and add regression tests
 - Added variant `GhApiError::Context` & `GhApiContextError`
 - Added variant `GhApiError::GraphQLErrors` & `GhGraphQLErrors`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu NobodyXu mentioned this issue Jun 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: Add GraphQL support to GhApiClient cargo-bins/cargo-binstall
3 participants
@passcod @Techcable @NobodyXu