-
Notifications
You must be signed in to change notification settings - Fork 75
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
refactor(jans-cedarling): improve WASM compatibility #10331
Draft
rmarinn
wants to merge
6
commits into
main
Choose a base branch
from
jans-cedarling-10330
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+277
−152
Draft
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6f9eb61
refactor(jans-cedarling): improve WASM compatiblity
rmarinn a4d5b4d
Merge remote-tracking branch 'origin/main' into jans-cedarling-10330
rmarinn 11e6048
chore(jans-cedarling): replace time crate usage with chrono
rmarinn 5e60b02
refactor(jans-cedarling): remove std::time usage in tests
rmarinn 34ee782
refactor(jans-cedarling): remove some more usage of std::time
rmarinn 7385886
refactor(jans-cedarling): make SparKV use chrono::Duration
rmarinn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,24 +5,63 @@ | |
* Copyright (c) 2024, Gluu, Inc. | ||
*/ | ||
|
||
use reqwest::blocking::Client; | ||
use std::{thread::sleep, time::Duration}; | ||
|
||
/// A wrapper around `reqwest::blocking::Client` providing HTTP request functionality | ||
use reqwest::Client; | ||
use serde::de::DeserializeOwned; | ||
use tokio::{ | ||
runtime::{Builder as RtBuilder, Runtime}, | ||
time::Duration, | ||
}; | ||
|
||
/// A wrapper around [`reqwest::Client`] providing HTTP request functionality | ||
/// with retry logic. | ||
/// | ||
/// The `HttpClient` struct allows for sending GET requests with a retry mechanism | ||
/// that attempts to fetch the requested resource up to a maximum number of times | ||
/// if an error occurs. | ||
#[derive(Debug)] | ||
pub struct HttpClient { | ||
client: reqwest::blocking::Client, | ||
client: reqwest::Client, | ||
max_retries: u32, | ||
retry_delay: Duration, | ||
rt: Runtime, | ||
} | ||
|
||
/// A wrapper around [`reqwest::Response`] | ||
#[derive(Debug)] | ||
pub struct Response<'rt> { | ||
rt: &'rt Runtime, | ||
resp: reqwest::Response, | ||
} | ||
|
||
impl Response<'_> { | ||
/// Deserializes the response into <T> from JSON. | ||
pub fn json<T>(self) -> Result<T, HttpClientError> | ||
where | ||
T: DeserializeOwned, | ||
{ | ||
let resp_json = self | ||
.rt | ||
.block_on(async { self.resp.json::<T>().await }) | ||
.map_err(HttpClientError::DeserializeJson)?; | ||
Ok(resp_json) | ||
} | ||
|
||
/// Deserializes the response into a String. | ||
pub fn text(self) -> Result<String, HttpClientError> { | ||
let resp_text = self | ||
.rt | ||
.block_on(async { self.resp.text().await }) | ||
.map_err(HttpClientError::DeserializeJson)?; | ||
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. maybe it should be better to use |
||
Ok(resp_text) | ||
} | ||
} | ||
|
||
impl HttpClient { | ||
pub fn new(max_retries: u32, retry_delay: Duration) -> Result<Self, HttpClientError> { | ||
let rt = RtBuilder::new_current_thread() | ||
.enable_all() | ||
.build() | ||
.expect("Failed to create Tokio runtime"); | ||
let client = Client::builder() | ||
.build() | ||
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. It's a good practice to add a timeout in during an http request like:
|
||
.map_err(HttpClientError::Initialization)?; | ||
|
@@ -31,38 +70,43 @@ impl HttpClient { | |
client, | ||
max_retries, | ||
retry_delay, | ||
rt, | ||
}) | ||
} | ||
|
||
/// Sends a GET request to the specified URI with retry logic. | ||
/// | ||
/// This method will attempt to fetch the resource up to 3 times, with an increasing delay | ||
/// between each attempt. | ||
pub fn get(&self, uri: &str) -> Result<reqwest::blocking::Response, HttpClientError> { | ||
pub fn get(&self, uri: &str) -> Result<Response, HttpClientError> { | ||
// Fetch the JWKS from the jwks_uri | ||
let mut attempts = 0; | ||
let response = loop { | ||
match self.client.get(uri).send() { | ||
// Exit loop on success | ||
Ok(response) => break response, | ||
|
||
Err(e) if attempts < self.max_retries => { | ||
attempts += 1; | ||
// TODO: pass this message to the logger | ||
eprintln!( | ||
"Request failed (attempt {} of {}): {}. Retrying...", | ||
attempts, self.max_retries, e | ||
); | ||
sleep(self.retry_delay * attempts); | ||
}, | ||
// Exit if max retries exceeded | ||
Err(e) => return Err(HttpClientError::MaxHttpRetriesReached(e)), | ||
let response = self.rt.block_on(async { | ||
loop { | ||
match self.client.get(uri).send().await { | ||
// Exit loop on success | ||
Ok(response) => return Ok(response), | ||
|
||
Err(e) if attempts < self.max_retries => { | ||
attempts += 1; | ||
// TODO: pass this message to the logger | ||
eprintln!( | ||
"Request failed (attempt {} of {}): {}. Retrying...", | ||
attempts, self.max_retries, e | ||
); | ||
tokio::time::sleep(self.retry_delay * attempts).await | ||
}, | ||
// Exit if max retries exceeded | ||
Err(e) => return Err(HttpClientError::MaxHttpRetriesReached(e)), | ||
} | ||
} | ||
}; | ||
})?; | ||
|
||
response | ||
let resp = response | ||
.error_for_status() | ||
.map_err(HttpClientError::HttpStatus) | ||
.map_err(HttpClientError::HttpStatus)?; | ||
|
||
Ok(Response { rt: &self.rt, resp }) | ||
} | ||
} | ||
|
||
|
@@ -75,21 +119,24 @@ pub enum HttpClientError { | |
/// Indicates an HTTP error response received from an endpoint. | ||
#[error("Received error HTTP status: {0}")] | ||
HttpStatus(#[source] reqwest::Error), | ||
|
||
/// Indicates a failure to reach the endpoint after 3 attempts. | ||
#[error("Could not reach endpoint after trying 3 times: {0}")] | ||
MaxHttpRetriesReached(#[source] reqwest::Error), | ||
/// Indicates a failure to deserialize the http response into JSON. | ||
#[error("Failed to deserialize response into JSON: {0}")] | ||
DeserializeJson(#[source] reqwest::Error), | ||
/// Indicates a failure to deserialize the http response into JSON. | ||
#[error("Failed to deserialize response into a String: {0}")] | ||
DeserializeText(#[source] reqwest::Error), | ||
} | ||
|
||
#[cfg(test)] | ||
mod test { | ||
use crate::jwt::http_client::HttpClientError; | ||
|
||
use super::HttpClient; | ||
use crate::jwt::http_client::{HttpClient, HttpClientError}; | ||
use mockito::Server; | ||
use serde_json::json; | ||
use std::time::Duration; | ||
use serde_json::{json, Value}; | ||
use test_utils::assert_eq; | ||
use tokio::time::Duration; | ||
|
||
#[test] | ||
fn can_fetch() { | ||
|
@@ -108,17 +155,16 @@ mod test { | |
.expect(1) | ||
.create(); | ||
|
||
let client = | ||
HttpClient::new(3, Duration::from_millis(1)).expect("Should create HttpClient."); | ||
let client = HttpClient::new(3, Duration::new(0, 10)).expect("Should create HttpClient."); | ||
|
||
let response = client | ||
.get(&format!( | ||
"{}/.well-known/openid-configuration", | ||
mock_server.url() | ||
)) | ||
.expect("Should get response") | ||
.json::<serde_json::Value>() | ||
.expect("Should deserialize JSON response."); | ||
.json::<Value>() | ||
.expect("Should deserialize response to JSON"); | ||
|
||
assert_eq!( | ||
response, expected, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
In general it's better to use async then block_on. Here you can do something like :
Moreover, you can remove the lifetime parameter <'rt> from Response.