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

BaseHTTPClient: better constructor API, Default #1517

Merged
merged 3 commits into from
Aug 1, 2024

Conversation

mvidner
Copy link
Contributor

@mvidner mvidner commented Jul 31, 2024

to enable

  • connecting to alternative servers, either production or test mocks
  • unauthenticated calls, either for the initial auth or test mocks

This is needed for both #1438 and #1495

In the end, the API changes only by adding a Default trait implementation which does no authentication.
Connecting to a mock server is achieved by assigning to the public base_url field.

to enable
- connecting to alternative servers, either production or test mocks
- unauthenticated calls, either for the initial auth or test mocsk
@@ -21,15 +21,36 @@ use crate::{auth::AuthToken, error::ServiceError};
/// }
/// ```
pub struct BaseHTTPClient {
client: Client,
pub client: reqwest::Client,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this field public? IMHO, BaseHTTPClient should offer the API that fulfills our needs, and the reqwest::Client should be just an implementation detail. If we want to replace reqwest with something else (e.g., ureq) we can do it easily (or if reqwest API changes it might be easy to adapt).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It enables me constructing a mock client like this:

        let mock_client =  BaseHTTPClient {
            base_url: mock_url,
            ..Default::default()
        };

I chose this way because I did not want to come up with a special name for it.

Alternatively we can make it private again, remove the Default trait and make a set of specialized constructors, like new_unauthenticated_with_url here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, I consider BaseHTTPClient pretty volatile, subject to quick rewrite as its users need it

Copy link
Contributor

Choose a reason for hiding this comment

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

I was playing with an idea to implement some traits here and than do necessary changes in behavior in uderlying structs (which can even cover differences in using authorization token or not)

/// A `default` client
/// - is NOT authenticated (maybe you want to call `new` instead)
/// - uses `localhost`
fn default() -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

In my POV default (or bare in my PR) are the best names ...
... I was thinking about something like new -> authorized and default / bare -> nonauthorized or something more fancy with similar meaning. Simply because behavior of new is not that clear (that it expects authorization token somewhere already)

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

LGTM. As @mvidner said, this BaseHTTPClient is pretty volatile and we need to experiment a bit more. But by now, it looks good enough.

@mvidner mvidner changed the title BaseHTTPClient: fields made public, better constructor API, Default BaseHTTPClient: better constructor API, Default Aug 1, 2024
@mvidner mvidner merged commit 83ee1d3 into master Aug 1, 2024
2 checks passed
@mvidner mvidner deleted the base-http-client-unauthenticated-url branch August 1, 2024 14:47
@imobachgs imobachgs mentioned this pull request Sep 20, 2024
imobachgs added a commit that referenced this pull request Sep 20, 2024
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.

3 participants