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

Replace D-Bus with HTTP-based clients #1438

Merged
merged 42 commits into from
Aug 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
756fc6a
run zypper --verbose to see where it gets stuck
mvidner Jun 5, 2024
b771383
UsersHttpClient
mvidner Jul 4, 2024
e295016
derive Debug for Question
mvidner Jul 23, 2024
7ea3c0a
Move RootConfig, RootPatchSettings to a model module
mvidner Jul 23, 2024
fc3f848
set root password, set root ssh key (happy path)
mvidner Jul 23, 2024
f3a2d3e
set_first_user: try improving error reporting
mvidner Jul 24, 2024
dd32fc5
UsersHttpClient::set_first_user use Result(<>), issues as Err
mvidner Jul 26, 2024
5abaf51
BaseHTTPClient cleanup
mvidner Jul 26, 2024
298cabc
cleanup
mvidner Jul 26, 2024
e094efb
hint for fixing a failed test run
mvidner Jul 26, 2024
3458c21
consistent spelling: UsersHTTPClient
mvidner Jul 26, 2024
9c71690
Split out agama_lib::localization::model::LocaleConfig
mvidner Jul 29, 2024
074cbb4
use LocalizationHTTPClient
mvidner Jul 29, 2024
6fcbf2f
remove LocalizationClient (D-Bus), no longer used
mvidner Jul 29, 2024
e5cd7b4
chore: fix `cargo doc` links
mvidner Jul 29, 2024
e6ccc55
BaseHTTPClient::unit_or_error factored out
mvidner Jul 30, 2024
b8b3541
test: LocalizationStore and LocalizationHTTPClient on a happy path
mvidner Jul 30, 2024
7369123
test: make it green by running a mock HTTP server
mvidner Jul 31, 2024
7f89dc1
BaseHTTPClient: fields made public, better constructor API, Default
mvidner Jul 31, 2024
4e8eefe
test LocalizationStore test_setting_l10n
mvidner Jul 31, 2024
24e15cb
test: UsersStore::load
mvidner Aug 1, 2024
59f1cac
remove unneeded async/await
mvidner Aug 1, 2024
7e27c36
BaseHTTPClient.client private again, +new_unauthenticated_with_url
mvidner Aug 1, 2024
9dedab9
adapt to changed BaseHTTPClient API: new_unauthenticated_with_url
mvidner Aug 1, 2024
2549764
test UsersStore::store
mvidner Aug 1, 2024
1d4ca4e
BaseHTTPClient: just use the public base_url field
mvidner Aug 1, 2024
85759e6
Merge branch 'master' into dbus-to-http
mvidner Aug 1, 2024
8dad93c
test: factor out setup code
mvidner Aug 1, 2024
3dca6b5
remove unneeded async/await
mvidner Aug 2, 2024
5c649ab
patch_root: pass the u32 from backend
mvidner Aug 4, 2024
686a016
dev dependency: env_logger
mvidner Aug 5, 2024
6da0409
Hint to the user when AuthToken::find fails
mvidner Aug 5, 2024
be37d86
BaseHTTPClient API cleanup
mvidner Aug 5, 2024
2166a9b
fix up mock for u32
mvidner Aug 5, 2024
0171c06
test: better diagnostics in case of failure
mvidner Aug 5, 2024
ebe5210
debug: show how to peek into reqwest::Response bodies
mvidner Aug 5, 2024
b7b75c7
questions::HTTPClient::new remove unneeded async/await
mvidner Aug 7, 2024
80b82e5
test questions::HTTPClient::list_questions
mvidner Aug 7, 2024
d2bff01
test questions::HTTPClient::try_answer
mvidner Aug 7, 2024
b0a6a20
test: questions:HTTPClient::create_question
mvidner Aug 7, 2024
bab32b9
simplify new code as Clippy suggested
mvidner Aug 7, 2024
4817cb0
changelog
mvidner Aug 9, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
432 changes: 427 additions & 5 deletions rust/Cargo.lock

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion rust/agama-cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub enum ConfigCommands {

pub async fn run(subcommand: ConfigCommands) -> anyhow::Result<()> {
let Some(token) = AuthToken::find() else {
println!("You need to login for generating a valid token");
println!("You need to login for generating a valid token: agama auth login");
return Ok(());
};

Expand Down
2 changes: 1 addition & 1 deletion rust/agama-cli/src/profile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub enum ProfileCommands {
/// Evaluate a profile, injecting the hardware information from D-Bus
///
/// For an example of Jsonnet-based profile, see
/// https://github.com/openSUSE/agama/blob/master/rust/agama-lib/share/examples/profile.jsonnet
/// <https://github.com/openSUSE/agama/blob/master/rust/agama-lib/share/examples/profile.jsonnet>
Evaluate {
/// Path to jsonnet file.
path: PathBuf,
Expand Down
6 changes: 3 additions & 3 deletions rust/agama-cli/src/questions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub enum QuestionsCommands {
/// mode or change the answer in automatic mode.
///
/// Please check Agama documentation for more details and examples:
/// https://github.com/openSUSE/agama/blob/master/doc/questions.md
/// <https://github.com/openSUSE/agama/blob/master/doc/questions.md>
Answers {
/// Path to a file containing the answers in YAML format.
path: String,
Expand Down Expand Up @@ -55,7 +55,7 @@ async fn set_answers(proxy: Questions1Proxy<'_>, path: String) -> Result<(), Ser
}

async fn list_questions() -> Result<(), ServiceError> {
let client = HTTPClient::new().await?;
let client = HTTPClient::new()?;
let questions = client.list_questions().await?;
// FIXME: if performance is bad, we can skip converting json from http to struct and then
// serialize it, but it won't be pretty string
Expand All @@ -66,7 +66,7 @@ async fn list_questions() -> Result<(), ServiceError> {
}

async fn ask_question() -> Result<(), ServiceError> {
let client = HTTPClient::new().await?;
let client = HTTPClient::new()?;
let question = serde_json::from_reader(std::io::stdin())?;

let created_question = client.create_question(&question).await?;
Expand Down
4 changes: 4 additions & 0 deletions rust/agama-lib/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,7 @@ curl = { version = "0.4.44", features = ["protocol-ftp"] }
jsonwebtoken = "9.3.0"
chrono = { version = "0.4.38", default-features = false, features = ["now", "std", "alloc", "clock"] }
home = "0.5.9"

[dev-dependencies]
httpmock = "0.7.0"
env_logger = "0.11.5"
2 changes: 1 addition & 1 deletion rust/agama-lib/src/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ impl Display for AuthToken {

/// Claims that are included in the token.
///
/// See https://datatracker.ietf.org/doc/html/rfc7519 for reference.
/// See <https://datatracker.ietf.org/doc/html/rfc7519> for reference.
#[derive(Debug, Serialize, Deserialize)]
pub struct TokenClaims {
pub exp: i64,
Expand Down
189 changes: 131 additions & 58 deletions rust/agama-lib/src/base_http_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ impl BaseHTTPClient {
let mut headers = header::HeaderMap::new();
// just use generic anyhow error here as Bearer format is constructed by us, so failures can come only from token
let value = header::HeaderValue::from_str(format!("Bearer {}", token).as_str())
.map_err(|e| anyhow::Error::new(e))?;
.map_err(anyhow::Error::new)?;

headers.insert(header::AUTHORIZATION, value);

Expand All @@ -66,40 +66,36 @@ impl BaseHTTPClient {
Ok(client)
}

/// Simple wrapper around [`Response`] to get object from response.
///
/// If a complete [`Response`] is needed, use the [`Self::get_response`] method.
///
/// Arguments:
///
/// * `path`: path relative to HTTP API like `/questions`
pub async fn get<T: DeserializeOwned>(&self, path: &str) -> Result<T, ServiceError> {
let response = self.get_response(path).await?;
if response.status().is_success() {
response.json::<T>().await.map_err(|e| e.into())
} else {
Err(self.build_backend_error(response).await)
}
fn url(&self, path: &str) -> String {
self.base_url.clone() + path
}

/// Calls GET method on the given path and returns [`Response`] that can be further
/// processed.
///
/// If only simple object from JSON is required, use method get.
/// Simple wrapper around [`Response`] to get object from response.
///
/// Arguments:
///
/// * `path`: path relative to HTTP API like `/questions`
pub async fn get_response(&self, path: &str) -> Result<Response, ServiceError> {
self.client
pub async fn get<T>(&self, path: &str) -> Result<T, ServiceError>
where
T: DeserializeOwned,
{
let response: Result<_, ServiceError> = self
.client
.get(self.url(path))
.send()
.await
.map_err(|e| e.into())
.map_err(|e| e.into());
self.deserialize_or_error(response?).await
}

fn url(&self, path: &str) -> String {
self.base_url.clone() + path
pub async fn post<T>(&self, path: &str, object: &impl Serialize) -> Result<T, ServiceError>
where
T: DeserializeOwned,
{
let response = self
.request_response(reqwest::Method::POST, path, object)
.await?;
self.deserialize_or_error(response).await
}

/// post object to given path and report error if response is not success
Expand All @@ -108,77 +104,154 @@ impl BaseHTTPClient {
///
/// * `path`: path relative to HTTP API like `/questions`
/// * `object`: Object that can be serialiazed to JSON as body of request.
pub async fn post(&self, path: &str, object: &impl Serialize) -> Result<(), ServiceError> {
let response = self.post_response(path, object).await?;
if response.status().is_success() {
Ok(())
} else {
Err(self.build_backend_error(response).await)
}
pub async fn post_void(&self, path: &str, object: &impl Serialize) -> Result<(), ServiceError> {
let response = self
.request_response(reqwest::Method::POST, path, object)
.await?;
self.unit_or_error(response).await
}

/// post object to given path and returns server response. Reports error only if failed to send
/// request, but if server returns e.g. 500, it will be in Ok result.
/// put object to given path, deserializes the response
///
/// In general unless specific response handling is needed, simple post should be used.
/// Arguments:
///
/// * `path`: path relative to HTTP API like `/users/first`
/// * `object`: Object that can be serialiazed to JSON as body of request.
pub async fn put<T>(&self, path: &str, object: &impl Serialize) -> Result<T, ServiceError>
where
T: DeserializeOwned,
{
let response = self
.request_response(reqwest::Method::PUT, path, object)
.await?;
self.deserialize_or_error(response).await
}

/// put object to given path and report error if response is not success
///
/// Arguments:
///
/// * `path`: path relative to HTTP API like `/questions`
/// * `path`: path relative to HTTP API like `/users/first`
/// * `object`: Object that can be serialiazed to JSON as body of request.
pub async fn put_void(&self, path: &str, object: &impl Serialize) -> Result<(), ServiceError> {
mvidner marked this conversation as resolved.
Show resolved Hide resolved
let response = self
.request_response(reqwest::Method::PUT, path, object)
.await?;
self.unit_or_error(response).await
}

/// patch object at given path and report error if response is not success
///
/// Arguments:
///
/// * `path`: path relative to HTTP API like `/users/first`
/// * `object`: Object that can be serialiazed to JSON as body of request.
pub async fn post_response(
pub async fn patch<T>(&self, path: &str, object: &impl Serialize) -> Result<T, ServiceError>
where
T: DeserializeOwned,
{
let response = self
.request_response(reqwest::Method::PATCH, path, object)
.await?;
self.deserialize_or_error(response).await
}

pub async fn patch_void(
&self,
path: &str,
object: &impl Serialize,
) -> Result<Response, ServiceError> {
self.client
.post(self.url(path))
.json(object)
.send()
.await
.map_err(|e| e.into())
) -> Result<(), ServiceError> {
let response = self
.request_response(reqwest::Method::PATCH, path, object)
.await?;
self.unit_or_error(response).await
}

/// delete call on given path and report error if failed
///
/// Arguments:
///
/// * `path`: path relative to HTTP API like `/questions/1`
pub async fn delete(&self, path: &str) -> Result<(), ServiceError> {
let response = self.delete_response(path).await?;
if response.status().is_success() {
Ok(())
} else {
Err(self.build_backend_error(response).await)
}
pub async fn delete_void(&self, path: &str) -> Result<(), ServiceError> {
let response: Result<_, ServiceError> = self
.client
.delete(self.url(path))
.send()
.await
.map_err(|e| e.into());
self.unit_or_error(response?).await
}

/// delete call on given path and returns server response. Reports error only if failed to send
/// POST/PUT/PATCH an object to a given path and returns server response.
/// Reports Err only if failed to send
/// request, but if server returns e.g. 500, it will be in Ok result.
///
/// In general unless specific response handling is needed, simple delete should be used.
/// TODO: do not need variant with request body? if so, then create additional method.
/// In general unless specific response handling is needed, simple post should be used.
///
/// Arguments:
///
/// * `path`: path relative to HTTP API like `/questions/1`
pub async fn delete_response(&self, path: &str) -> Result<Response, ServiceError> {
/// * `method`: for example `reqwest::Method::PUT`
/// * `path`: path relative to HTTP API like `/questions`
/// * `object`: Object that can be serialiazed to JSON as body of request.
async fn request_response(
&self,
method: reqwest::Method,
path: &str,
object: &impl Serialize,
) -> Result<Response, ServiceError> {
self.client
.delete(self.url(path))
.request(method, self.url(path))
.json(object)
.send()
.await
.map_err(|e| e.into())
}

/// Return deserialized JSON body as `Ok(T)` or an `Err` with [`ServiceError::BackendError`]
async fn deserialize_or_error<T>(&self, response: Response) -> Result<T, ServiceError>
where
T: DeserializeOwned,
{
// DEBUG: This dbg is nice but it omits the body, thus we try harder below
// let response = dbg!(response);

if response.status().is_success() {
// We'd like to simply:
// response.json::<T>().await.map_err(|e| e.into())
// BUT also peek into the response text, in case something is wrong
// so this copies the implementation from the above and adds a debug part

let bytes_r: Result<_, ServiceError> = response.bytes().await.map_err(|e| e.into());
let bytes = bytes_r?;

// DEBUG: (we expect JSON so dbg! would escape too much, eprintln! is better)
// let text = String::from_utf8_lossy(&bytes);
// eprintln!("Response body: {}", text);

serde_json::from_slice(&bytes).map_err(|e| e.into())
} else {
Err(self.build_backend_error(response).await)
}
mvidner marked this conversation as resolved.
Show resolved Hide resolved
}

/// Return `Ok(())` or an `Err` with [`ServiceError::BackendError`]
async fn unit_or_error(&self, response: Response) -> Result<(), ServiceError> {
if response.status().is_success() {
Ok(())
} else {
Err(self.build_backend_error(response).await)
}
}

const NO_TEXT: &'static str = "(Failed to extract error text from HTTP response)";
/// Builds [`BackendError`] from response.
/// Builds [`ServiceError::BackendError`] from response.
///
/// It contains also processing of response body, that is why it has to be async.
///
/// Arguments:
///
/// * `response`: response from which generate error
pub async fn build_backend_error(&self, response: Response) -> ServiceError {
async fn build_backend_error(&self, response: Response) -> ServiceError {
let code = response.status().as_u16();
let text = response
.text()
Expand Down
2 changes: 1 addition & 1 deletion rust/agama-lib/src/dbus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ macro_rules! property_from_dbus {
/// NOTE: we could follow a different approach like building our own type (e.g.
/// using the newtype idiom) and offering a better API.
///
/// * `source`: hash map containing non-onwed values ([zbus::zvariant::Value]).
/// * `source`: hash map containing non-onwed values ([enum@zbus::zvariant::Value]).
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not know about that enum@ prefix.

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's a disambiguation suggested by rustdoc because there is also a #[derive(Value)]

pub fn to_owned_hash(source: &HashMap<&str, Value<'_>>) -> HashMap<String, OwnedValue> {
let mut owned = HashMap::new();
for (key, value) in source.iter() {
Expand Down
2 changes: 1 addition & 1 deletion rust/agama-lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! This library offers an API to interact with Agama services. At this point, the library allows:
//!
//! * Reading and writing [installation settings](install_settings::InstallSettings).
//! * Monitoring the [progress](progress).
//! * Monitoring the [progress].
//! * Triggering actions through the [manager] (e.g., starting installation).
//!
//! ## Handling installation settings
Expand Down
5 changes: 3 additions & 2 deletions rust/agama-lib/src/localization.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
//! Implements support for handling the localization settings

mod client;
mod http_client;
pub mod model;
mod proxies;
mod settings;
mod store;

pub use client::LocalizationClient;
pub use http_client::LocalizationHTTPClient;
pub use proxies::LocaleProxy;
pub use settings::LocalizationSettings;
pub use store::LocalizationStore;
Loading