refactor(jans-cedarling): improve WASM compatibility #10331
Conversation
rmarinn
requested review from
djellemah,
nynymike,
olehbozhok,
duttarnab and
abaghinyan
DryRun Security SummaryThis GitHub Pull Request enhances the Janssen Project's development and contribution processes by implementing comprehensive security practices, standardized issue templates, automated dependency updates, and robust GitHub Actions workflows to improve code quality and project security. Expand for full summarySummary: This GitHub Pull Request (PR) contains a series of changes across multiple files, primarily focused on improving the development and contribution processes for the Janssen Project. The changes cover a wide range of areas, including issue templates, code linting, dependency management, and GitHub Actions workflows. From an application security perspective, the changes demonstrate a strong commitment to security best practices and a proactive approach to maintaining the overall security and integrity of the codebase. Key security-focused aspects include:
Overall, the changes in this PR demonstrate a strong focus on security and a commitment to maintaining a secure and reliable application. The combination of standardized processes, automated tooling, and security-conscious practices should help to improve the overall security posture of the Janssen Project. Files Changed:
Code AnalysisWe ran |
- replace std::time using with the chrono crate to improve WASM compalibility - use reqwest::Client instead of reqwest::blocking::Client Signed-off-by: rmarinn <[email protected]>
mo-auto
added
the
kind-enhancement
rmarinn
force-pushed
the
jans-cedarling-10330
branch
from
3df6e1d to
6f9eb61
Compare
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
Signed-off-by: rmarinn <[email protected]>
- make SparKV use chrono::Duration instead of std::time::Duration to allow compilation to WASM. Signed-off-by: rmarinn <[email protected]>
There was a problem hiding this comment.
Locally, there are build failures with cargo test --target wasm32-wasip1 and cargo test --target wasm32-unknown-unknown. Is that expected right now?
Looks to me the cause is jsonwebkey, which pulls in an older version of jsonwebtoken which then pulls in an older version of ring.
Yeah... turns out we need to change more things (other than the
we're only really using |
| let resp_text = self | ||
| .rt | ||
| .block_on(async { self.resp.text().await }) | ||
| .map_err(HttpClientError::DeserializeJson)?; |
There was a problem hiding this comment.
maybe it should be better to use DeserializeText instead of DeserializeJson here
| 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.
It's a good practice to add a timeout in during an http request like:
let client = Client::builder()
.timeout(Duration::from_sec(20))
.build()
.map_err(HttpClientError::Initialization)?;
| { | ||
| let resp_json = self | ||
| .rt | ||
| .block_on(async { self.resp.json::<T>().await }) |
There was a problem hiding this comment.
In general it's better to use async then block_on. Here you can do something like :
let resp_json = self
.resp
.json::<T>()
.await
.map_err(HttpClientError::DeserializeJson)?;
Ok(resp_json)
Moreover, you can remove the lifetime parameter <'rt> from Response.
moabu
force-pushed
the
jans-cedarling-10330
branch
from
7385886 to
8155f2b
Compare
moabu
force-pushed
the
main
branch
2 times, most recently
from
5126af2 to
aa1b2ed
Compare
moabu
force-pushed
the
jans-cedarling-10330
branch
from
8155f2b to
799ec30
Compare
Prepare
Description
This PR improves WASM compatibility by:
chronocrate which has WASM support instead ofstd::timefor getting the time.reqwest::blocking::Clientwithreqwest::Client.Target issue
target issue: refactor(jans-cedarling): improve WASM compatibility #10330
closes #10330
Implementation Details
HttpClientcreate will store a reference to atokioruntime which it will use to make async requests so compiling to WASM would be possible.Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.