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.

Sign up for GitHub

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

Jump to bottom

Simplify server errors and drop some unused code #1095

Merged
merged 5 commits into from
Mar 18, 2024
Merged

Simplify server errors and drop some unused code #1095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f3babaf
Unify some error types that brought no value
imobachgs Mar 15, 2024
f72393e
More errors simplification
imobachgs Mar 15, 2024
c4878b8
Remove unneeded code from the localization D-Bus API
imobachgs Mar 15, 2024
f2848d0
Unify ApiError and Error
imobachgs Mar 15, 2024
80ac668
Convert LocaleError into Error
imobachgs Mar 15, 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
35 changes: 29 additions & 6 deletions rust/agama-server/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,25 @@
use zbus_macros::DBusError;
use agama_lib::error::ServiceError;
use axum::{
http::StatusCode,
response::{IntoResponse, Response},
Json,
};
use serde_json::json;

#[derive(DBusError, Debug)]
#[dbus_error(prefix = "org.opensuse.Agama1.Locale")]
use crate::{l10n::web::LocaleError, questions::QuestionsError};

#[derive(thiserror::Error, Debug)]
pub enum Error {
#[dbus_error(zbus_error)]
ZBus(zbus::Error),
#[error("D-Bus error: {0}")]
DBus(#[from] zbus::Error),
#[error("Generic error: {0}")]
Anyhow(String),
#[error("Agama service error: {0}")]
Service(#[from] ServiceError),
#[error("Questions service error: {0}")]
Questions(QuestionsError),
#[error("Software service error: {0}")]
Locale(#[from] LocaleError),
}

// This would be nice, but using it for a return type
Expand All @@ -22,6 +36,15 @@ impl From<anyhow::Error> for Error {

impl From<Error> for zbus::fdo::Error {
fn from(value: Error) -> zbus::fdo::Error {
zbus::fdo::Error::Failed(format!("Localization error: {value}"))
zbus::fdo::Error::Failed(format!("D-Bus error: {value}"))
}
}

impl IntoResponse for Error {
fn into_response(self) -> Response {
let body = json!({
"error": self.to_string()
});
(StatusCode::BAD_REQUEST, Json(body)).into_response()
}
}
83 changes: 12 additions & 71 deletions rust/agama-server/src/l10n.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub mod web;

use crate::error::Error;
use agama_locale_data::{KeymapId, LocaleId};
use anyhow::Context;

use keyboard::KeymapsDatabase;
use locale::LocalesDatabase;
use regex::Regex;
Expand All @@ -32,38 +32,18 @@ pub struct Locale {

#[dbus_interface(name = "org.opensuse.Agama1.Locale")]
impl Locale {
/// Gets the supported locales information.
///
/// Each element of the list has these parts:
///
/// * The locale code (e.g., "es_ES.UTF-8").
/// * The name of the language according to the language defined by the
/// UILocale property.
/// * The name of the territory according to the language defined by the
/// UILocale property.
fn list_locales(&self) -> Result<Vec<(String, String, String)>, Error> {
let locales = self
.locales_db
.entries()
.iter()
.map(|l| {
(
l.id.to_string(),
l.language.to_string(),
l.territory.to_string(),
)
})
.collect::<Vec<_>>();
Ok(locales)
}

#[dbus_interface(property)]
fn locales(&self) -> Vec<String> {
self.locales.to_owned()
}

#[dbus_interface(property)]
fn set_locales(&mut self, locales: Vec<String>) -> zbus::fdo::Result<()> {
if locales.is_empty() {
return Err(zbus::fdo::Error::Failed(format!(
"The locales list cannot be empty"
)));
}
for loc in &locales {
if !self.locales_db.exists(loc.as_str()) {
return Err(zbus::fdo::Error::Failed(format!(
Expand All @@ -89,22 +69,6 @@ impl Locale {
Ok(self.translate(&locale)?)
}

/// Returns a list of the supported keymaps.
///
/// Each element of the list contains:
///
/// * The keymap identifier (e.g., "es" or "es(ast)").
/// * The name of the keyboard in language set by the UILocale property.
fn list_keymaps(&self) -> Result<Vec<(String, String)>, Error> {
let keymaps = self
.keymaps_db
.entries()
.iter()
.map(|k| (k.id.to_string(), k.localized_description()))
.collect();
Ok(keymaps)
}

#[dbus_interface(property)]
fn keymap(&self) -> String {
self.keymap.to_string()
Expand All @@ -125,32 +89,6 @@ impl Locale {
Ok(())
}

/// Returns a list of the supported timezones.
///
/// Each element of the list contains:
///
/// * The timezone identifier (e.g., "Europe/Berlin").
/// * A list containing each part of the name in the language set by the
/// UILocale property.
/// * The name, in the language set by UILocale, of the main country
/// associated to the timezone (typically, the name of the city that is
/// part of the identifier) or empty string if there is no country.
fn list_timezones(&self) -> Result<Vec<(String, Vec<String>, String)>, Error> {
let timezones: Vec<_> = self
.timezones_db
.entries()
.iter()
.map(|tz| {
(
tz.code.to_string(),
tz.parts.clone(),
tz.country.clone().unwrap_or_default(),
)
})
.collect();
Ok(timezones)
}

#[dbus_interface(property)]
fn timezone(&self) -> &str {
self.timezone.as_str()
Expand All @@ -169,22 +107,25 @@ impl Locale {
}

// TODO: what should be returned value for commit?
fn commit(&mut self) -> Result<(), Error> {
fn commit(&mut self) -> zbus::fdo::Result<()> {
const ROOT: &str = "/mnt";

Command::new("/usr/bin/systemd-firstboot")
.args([
"--root",
ROOT,
"--force",
"--locale",
self.locales.first().context("missing locale")?.as_str(),
self.locales.first().unwrap_or(&"en_US.UTF-8".to_string()),
"--keymap",
&self.keymap.to_string(),
"--timezone",
&self.timezone,
])
.status()
.context("Failed to execute systemd-firstboot")?;
.map_err(|e| {
zbus::fdo::Error::Failed(format!("Could not apply the l10n configuration: {e}"))
})?;

Ok(())
}
Expand Down
38 changes: 13 additions & 25 deletions rust/agama-server/src/l10n/web.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,40 +9,25 @@ use crate::{
use agama_locale_data::{InvalidKeymap, LocaleId};
use axum::{
extract::State,
http::StatusCode,
response::{IntoResponse, Response},
routing::{get, put},
Json, Router,
};
use serde::{Deserialize, Serialize};
use serde_json::json;
use std::{
process::Command,
sync::{Arc, RwLock},
};
use thiserror::Error;

#[derive(Error, Debug)]
#[derive(thiserror::Error, Debug)]
pub enum LocaleError {
#[error("Unknown locale code: {0}")]
UnknownLocale(String),
#[error("Unknown timezone: {0}")]
UnknownTimezone(String),
#[error("Invalid keymap: {0}")]
InvalidKeymap(#[from] InvalidKeymap),
#[error("Cannot translate: {0}")]
OtherError(#[from] Error),
#[error("Cannot change the local keymap: {0}")]
CouldNotSetKeymap(#[from] std::io::Error),
}

impl IntoResponse for LocaleError {
fn into_response(self) -> Response {
let body = json!({
"error": self.to_string()
});
(StatusCode::BAD_REQUEST, Json(body)).into_response()
}
#[error("Could not apply the changes")]
Commit(#[from] std::io::Error),
}

#[derive(Clone)]
Expand Down Expand Up @@ -119,14 +104,14 @@ async fn keymaps(State(state): State<LocaleState>) -> Json<Vec<Keymap>> {
async fn set_config(
State(state): State<LocaleState>,
Json(value): Json<LocaleConfig>,
) -> Result<Json<()>, LocaleError> {
) -> Result<Json<()>, Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

generic comment for whole this PR.

  1. I like reduction of anyhow errors
  2. I do not like this change in method signature. I think it is better to return specific error and use upper Error only when it can return more types of error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The advantage of using the same error is that you only need to implement the IntoResponse trait once. Initially, I called it ApiError, but in the end, I decided to use a single type.

Copy link
Contributor Author

@imobachgs imobachgs Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

However, if you prefer, I could keep the LocaleError and implement IntoResponse only for that kind. It looks good to me too.

Copy link
Contributor

Choose a reason for hiding this comment

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

you cannot have IntoResponse for whole Enum? If it is implemented for Error, I kind of expect it can process also element of Enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe some kind of trait can help there?

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 is implemented for the whole enum (the crate::error::Error) and that's the reason I am returning Error instead of LocaleError. After all, LocaleError knows nothing about Error.

Adding any trait is overcomplicating things, in my opinion. So I would go for:

  1. keep it as it is (returning Error) or...
  2. returning LocaleError and implementing IntoResponse.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, reason why I do not like it is because now it really see only use case for respond, but if it will be used in other parts of rust, then user of this function needs to handle any generic error, while we know that it can product only LocaleError.
So how I do see it is, that if we want to keep extensibility and allow in future to other types of Error in this function, lets change it to Error. But if we know that we always produce only LocaleError ( which I kind of expect ), then I prefer to return minimal abstraction it uses.

But ( and it is big BUT ) I do not care much in web functions that is short and really serve only as connection to real code and do some error translation. But this specific function is quite big and I would say many of its part is not web specific. So maybe if it lives elsewhere and this web function just call other method and translate its errortype into Error. Then it would much nicer.

And to conclude it, I still see this PR as improvement, so feel free to merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I fully agree, this code does not belong here but it is a step in the right direction.

I have added a note to this Trello card so it can be addressed later.

let mut data = state.locale.write().unwrap();
let mut changes = LocaleConfig::default();

if let Some(locales) = &value.locales {
for loc in locales {
if !data.locales_db.exists(loc.as_str()) {
return Err(LocaleError::UnknownLocale(loc.to_string()));
return Err(LocaleError::UnknownLocale(loc.to_string()))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

why it needs ? here? it looks kind to strange to return Err and using ?for returning it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it returns a LocaleError, not a Error itself. In the next iteration, my idea is to put most of this code in a separate struct, and keep this function to just do the arguments lifting. At that point, we could get rid of this return Err and the code will look better.

}
}
data.locales = locales.clone();
Expand All @@ -135,14 +120,14 @@ async fn set_config(

if let Some(timezone) = &value.timezone {
if !data.timezones_db.exists(timezone) {
return Err(LocaleError::UnknownTimezone(timezone.to_string()));
return Err(LocaleError::UnknownTimezone(timezone.to_string()))?;
}
data.timezone = timezone.to_owned();
changes.timezone = Some(data.timezone.clone());
}

if let Some(keymap_id) = &value.keymap {
data.keymap = keymap_id.parse()?;
data.keymap = keymap_id.parse().map_err(LocaleError::InvalidKeymap)?;
changes.keymap = Some(keymap_id.clone());
}

Expand All @@ -161,14 +146,17 @@ async fn set_config(
}

if let Some(ui_keymap) = &value.ui_keymap {
data.ui_keymap = ui_keymap.parse()?;
// data.ui_keymap = ui_keymap.parse().into::<Result<KeymapId, LocaleError>>()?;
data.ui_keymap = ui_keymap.parse().map_err(LocaleError::InvalidKeymap)?;
Command::new("/usr/bin/localectl")
.args(["set-x11-keymap", &ui_keymap])
.output()?;
.output()
.map_err(LocaleError::Commit)?;
Command::new("/usr/bin/setxkbmap")
.arg(&ui_keymap)
.env("DISPLAY", ":0")
.output()?;
.output()
.map_err(LocaleError::Commit)?;
}

_ = state.events.send(Event::L10nConfigChanged(changes));
Expand Down
25 changes: 4 additions & 21 deletions rust/agama-server/src/manager/web.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,10 @@ use agama_lib::{
};
use axum::{
extract::State,
http::StatusCode,
response::{IntoResponse, Response},
routing::{get, post},
Json, Router,
};
use serde::Serialize;
use serde_json::json;
use thiserror::Error;
use tokio_stream::{Stream, StreamExt};

use crate::{
Expand All @@ -37,19 +33,6 @@ pub struct ManagerState<'a> {
manager: ManagerClient<'a>,
}

#[derive(Error, Debug)]
pub enum ManagerError {
#[error("Manager service error: {0}")]
Error(#[from] ServiceError),
}

impl IntoResponse for ManagerError {
fn into_response(self) -> Response {
let body = json!({});
(StatusCode::BAD_REQUEST, Json(body)).into_response()
}
}

/// Holds information about the manager's status.
#[derive(Clone, Serialize, utoipa::ToSchema)]
pub struct InstallerStatus {
Expand Down Expand Up @@ -115,7 +98,7 @@ pub async fn manager_service(dbus: zbus::Connection) -> Result<Router, ServiceEr
#[utoipa::path(get, path = "/api/manager/probe", responses(
(status = 200, description = "The probing process was started.")
))]
async fn probe_action(State(state): State<ManagerState<'_>>) -> Result<(), ManagerError> {
async fn probe_action(State(state): State<ManagerState<'_>>) -> Result<(), Error> {
state.manager.probe().await?;
Ok(())
}
Expand All @@ -124,7 +107,7 @@ async fn probe_action(State(state): State<ManagerState<'_>>) -> Result<(), Manag
#[utoipa::path(get, path = "/api/manager/install", responses(
(status = 200, description = "The installation process was started.")
))]
async fn install_action(State(state): State<ManagerState<'_>>) -> Result<(), ManagerError> {
async fn install_action(State(state): State<ManagerState<'_>>) -> Result<(), Error> {
state.manager.install().await?;
Ok(())
}
Expand All @@ -133,7 +116,7 @@ async fn install_action(State(state): State<ManagerState<'_>>) -> Result<(), Man
#[utoipa::path(get, path = "/api/manager/install", responses(
(status = 200, description = "The installation tasks are executed.")
))]
async fn finish_action(State(state): State<ManagerState<'_>>) -> Result<(), ManagerError> {
async fn finish_action(State(state): State<ManagerState<'_>>) -> Result<(), Error> {
state.manager.finish().await?;
Ok(())
}
Expand All @@ -144,7 +127,7 @@ async fn finish_action(State(state): State<ManagerState<'_>>) -> Result<(), Mana
))]
async fn installer_status(
State(state): State<ManagerState<'_>>,
) -> Result<Json<InstallerStatus>, ManagerError> {
) -> Result<Json<InstallerStatus>, Error> {
let status = InstallerStatus {
phase: state.manager.current_installation_phase().await?,
busy: state.manager.busy_services().await?,
Expand Down
Loading
Loading