-
Notifications
You must be signed in to change notification settings - Fork 43
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
use reqwest::{header, Client, Response}; | ||
use reqwest::{header, Response}; | ||
use serde::{de::DeserializeOwned, Serialize}; | ||
|
||
use crate::{auth::AuthToken, error::ServiceError}; | ||
|
@@ -21,15 +21,36 @@ use crate::{auth::AuthToken, error::ServiceError}; | |
/// } | ||
/// ``` | ||
pub struct BaseHTTPClient { | ||
client: Client, | ||
pub client: reqwest::Client, | ||
pub base_url: String, | ||
} | ||
|
||
const API_URL: &str = "http://localhost/api"; | ||
|
||
impl Default for BaseHTTPClient { | ||
/// A `default` client | ||
/// - is NOT authenticated (maybe you want to call `new` instead) | ||
/// - uses `localhost` | ||
fn default() -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my POV |
||
Self { | ||
client: reqwest::Client::new(), | ||
base_url: API_URL.to_owned(), | ||
} | ||
} | ||
} | ||
|
||
impl BaseHTTPClient { | ||
// if there is need for client without authorization, create new constructor for it | ||
/// Uses `localhost`, authenticates with [`AuthToken`]. | ||
pub fn new() -> Result<Self, ServiceError> { | ||
Ok(Self { | ||
client: Self::authenticated_reqwest_client()?, | ||
..Default::default() | ||
}) | ||
} | ||
|
||
fn authenticated_reqwest_client() -> Result<reqwest::Client, ServiceError> { | ||
mvidner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// TODO: this error is subtly misleading, leading me to believe the SERVER said it, | ||
// but in fact it is the CLIENT not finding an auth token | ||
let token = AuthToken::find().ok_or(ServiceError::NotAuthenticated)?; | ||
|
||
let mut headers = header::HeaderMap::new(); | ||
|
@@ -39,12 +60,10 @@ impl BaseHTTPClient { | |
|
||
headers.insert(header::AUTHORIZATION, value); | ||
|
||
let client = Client::builder().default_headers(headers).build()?; | ||
|
||
Ok(Self { | ||
client, | ||
base_url: API_URL.to_string(), // TODO: add support for remote server | ||
}) | ||
let client = reqwest::Client::builder() | ||
.default_headers(headers) | ||
.build()?; | ||
Ok(client) | ||
} | ||
|
||
/// Simple wrapper around [`Response`] to get object from response. | ||
|
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.
Why is this field public? IMHO,
BaseHTTPClient
should offer the API that fulfills our needs, and thereqwest::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).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.
It enables me constructing a mock client like this:
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?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.
Anyway, I consider BaseHTTPClient pretty volatile, subject to quick rewrite as its users need it
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.
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)