-
Notifications
You must be signed in to change notification settings - Fork 42
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
add http header support #66
add http header support #66
Conversation
Out of curiosity what do you need custom heeaders for? |
Access to authenticated esplora servers. most public ones are heavily rate limited |
Please rebase to pick up changes in #69 that fix CI. |
ab52545
to
9fabcf6
Compare
rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 9fabcf6
@johncantrell97 thanks for your work on this. Our github rules also require commits be signed, can you amend/sign your commit? |
@johncantrell97 hey can you rebase and rework this one now that #75 is merged? hopefully not a big change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
9fabcf6
to
73cc41c
Compare
WalkthroughThe update introduces the ability to handle custom HTTP headers across both asynchronous and blocking clients. This is achieved by adding a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Pull Request Test Coverage Report for Build 8310674431Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/async.rs (2 hunks)
- src/blocking.rs (3 hunks)
- src/lib.rs (7 hunks)
Additional comments: 11
src/blocking.rs (3)
- 39-40: The addition of the
headers
field in theBlockingClient
struct is a straightforward and effective way to support custom HTTP headers. This change aligns with the PR's objective to simplify the process for end-users to include custom HTTP headers in their requests.- 50-50: Correctly transferring the
headers
from theBuilder
to theBlockingClient
during construction ensures that any headers specified during client configuration are preserved. This is a key part of making the feature user-friendly and functional.- 66-70: The implementation of header addition to each request made by the
BlockingClient
is well-done. Looping through theheaders
map and applying each key-value pair as a header to the request object is the correct approach. However, it's important to ensure that the headers being set do not inadvertently overwrite any essential headers thatminreq
might set by default, such asContent-Type
for certain requests.Consider verifying if
minreq
sets any default headers that should not be overwritten by custom headers, or if there should be a mechanism to prevent certain headers from being overwritten by the user.src/async.rs (2)
- 27-27: The import of
reqwest::{header, Client, StatusCode}
is necessary for the changes made to support custom HTTP headers in theAsyncClient
. This ensures that the required types and modules are available for use in the header handling logic.- 52-62: The logic to convert the headers from a
HashMap<String, String>
to areqwest::header::HeaderMap
and setting them as default headers for thereqwest::Client
is correctly implemented. This approach allows for the headers to be included in every request made by theAsyncClient
. It's important to handle potential errors when converting strings toHeaderName
andHeaderValue
, as done here, to ensure that only valid headers are set. Additionally, consider documenting the behavior regarding overwriting default headers set byreqwest
to make it clear to users how custom headers interact with default ones.Verify and document how custom headers provided by users interact with default headers set by
reqwest
, especially in cases where there might be conflicts.src/lib.rs (6)
- 120-121: Adding a
headers
field to theBuilder
struct is a straightforward and effective way to allow users to specify custom HTTP headers for their requests. This change aligns with the PR's objective to simplify the process of including custom headers, especially for authenticated Esplora servers.- 131-131: Initializing the
headers
field with an emptyHashMap
in theBuilder::new
method ensures that users can optionally add headers without having to deal withOption<HashMap<...>>
. This is a good practice for keeping the API simple and user-friendly.- 147-151: The
header
method provides a convenient way for users to add individual headers to their requests. This method's design, which takes key-value pairs as strings and returns the modifiedBuilder
instance, follows the builder pattern effectively, allowing for fluent API usage.- 194-196: The addition of
InvalidHttpHeaderName
andInvalidHttpHeaderValue
error variants to theError
enum is crucial for robust error handling. It allows the library to provide more specific feedback to the user when they attempt to set invalid headers, enhancing the overall usability and reliability of the library.- 274-286: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [277-297]
The
setup_clients_with_headers
function in the test module demonstrates how the new headers functionality can be used in practice. By allowing the function to take aHashMap
of headers and applying it to theBuilder
, this setup is flexible and can be easily extended for additional tests. However, it's important to ensure that the headers used in tests reflect realistic scenarios that users might encounter.
- 881-913: The test
test_get_tx_with_http_header
effectively verifies that custom HTTP headers are correctly applied to requests made by both the blocking and async clients. Using an "Authorization" header in the test is a practical choice, as it represents a common use case for custom headers. This test enhances the confidence in the new functionality's correctness.
I've rebased this on lastest master changes and updated to work with minreq for blocking module and added a simple test with http headers. In the process of rebasing I also signed the commits so github will let us merge it. Note: minreq doesn't include any header key, value validation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Technically an end-user could do this themselves because the builders support passing in pre-built ureq/reqwest clients but this makes it a bit easier for them.
Summary by CodeRabbit
Builder
struct for adding HTTP headers to requests.