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

first draft of questions API in http #1091

Merged
merged 13 commits into from
Mar 22, 2024
52 changes: 51 additions & 1 deletion rust/agama-lib/src/proxies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ trait Questions1 {

/// New method
#[dbus_proxy(name = "New")]
fn new_quetion(
fn new_question(
&self,
class: &str,
text: &str,
Expand All @@ -121,3 +121,53 @@ trait Questions1 {
#[dbus_proxy(property)]
fn set_interactive(&self, value: bool) -> zbus::Result<()>;
}

#[dbus_proxy(
interface = "org.opensuse.Agama1.Questions.Generic",
default_service = "org.opensuse.Agama1",
default_path = "/org/opensuse/Agama1/Questions"
)]
trait GenericQuestion {
/// Answer property
#[dbus_proxy(property)]
fn answer(&self) -> zbus::Result<String>;
#[dbus_proxy(property)]
fn set_answer(&self, value: &str) -> zbus::Result<()>;

/// Class property
#[dbus_proxy(property)]
fn class(&self) -> zbus::Result<String>;

/// Data property
#[dbus_proxy(property)]
fn data(&self) -> zbus::Result<std::collections::HashMap<String, String>>;

/// DefaultOption property
#[dbus_proxy(property)]
fn default_option(&self) -> zbus::Result<String>;

/// Id property
#[dbus_proxy(property)]
fn id(&self) -> zbus::Result<u32>;

/// Options property
#[dbus_proxy(property)]
fn options(&self) -> zbus::Result<Vec<String>>;

/// Text property
#[dbus_proxy(property)]
fn text(&self) -> zbus::Result<String>;
}

#[dbus_proxy(
interface = "org.opensuse.Agama1.Questions.WithPassword",
default_service = "org.opensuse.Agama1",
default_path = "/org/opensuse/Agama1/Questions"
)]
trait QuestionWithPassword {
/// Password property
#[dbus_proxy(property)]
fn password(&self) -> zbus::Result<String>;
#[dbus_proxy(property)]
fn set_password(&self, value: &str) -> zbus::Result<()>;
}
1 change: 1 addition & 0 deletions rust/agama-server/src/questions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use log;
use zbus::{dbus_interface, fdo::ObjectManager, zvariant::ObjectPath, Connection};

mod answers;
pub mod web;

#[derive(thiserror::Error, Debug)]
pub enum QuestionsError {
Expand Down
210 changes: 210 additions & 0 deletions rust/agama-server/src/questions/web.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
//! This module implements the web API for the questions module.
//!
//! The module offers two public functions:
//!
//! * `questions_service` which returns the Axum service.
//! * `questions_stream` which offers an stream that emits questions related signals.

use std::collections::HashMap;
use crate::{error::Error, web::Event};
use agama_lib::{
error::ServiceError, proxies::{GenericQuestionProxy, QuestionWithPasswordProxy},
};
use anyhow::Context;
use axum::{
extract::{State, Path},
http::StatusCode,
response::{IntoResponse, Response},
routing::{get, put},
Json, Router,
};
use tokio_stream::{Stream, StreamExt};
use zbus::{fdo::ObjectManagerProxy, names::{InterfaceName, OwnedInterfaceName}};
use zbus::zvariant::OwnedObjectPath;
use zbus::zvariant::ObjectPath;
use thiserror::Error;
use serde::{Deserialize, Serialize};
use serde_json::json;

// TODO: move to lib
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only tricky part with moving to lib is that structs for web does not live there...There are GenericQuestion struct, so after move I will need to implement some Into

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think that the Client should be 1) agnostic from the web layer and 2) live in agama-lib. However, given that we are short on time, we could add a Trello card listing things to improve and refactor. After all, we are still learning how to organize our code.

#[derive(Clone)]
struct QuestionsClient<'a> {
connection: zbus::Connection,
objects_proxy: ObjectManagerProxy<'a>,
}

impl<'a> QuestionsClient<'a> {
pub async fn new(dbus: zbus::Connection) -> Result<Self, zbus::Error> {
Ok(Self {
connection: dbus.clone(),
objects_proxy: ObjectManagerProxy::new(&dbus).await?
})
}

pub async fn questions(&self) -> Result<Vec<Question>, ServiceError> {
let objects = self.objects_proxy.get_managed_objects().await
.context("failed to get managed object with Object Manager")?;
let mut result: Vec<Question> = Vec::with_capacity(objects.len());
let password_interface = OwnedInterfaceName::from(
InterfaceName::from_static_str("org.opensuse.Agama1.Questions.WithPassword")
.context("Failed to create interface name for question with password")?
);
for (path, interfaces_hash) in objects.iter() {
if interfaces_hash.contains_key(&password_interface) {
result.push(self.create_question_with_password(&path).await?)
} else {
result.push(self.create_generic_question(&path).await?)
}
}
Ok(result)
}

async fn create_generic_question(&self, path: &OwnedObjectPath) -> Result<Question, ServiceError> {
let dbus_question = GenericQuestionProxy::builder(&self.connection)
.path(path)?.cache_properties(zbus::CacheProperties::No).build().await?;
let result = Question {
generic: GenericQuestion {
id: dbus_question.id().await?,
class: dbus_question.class().await?,
text: dbus_question.text().await?,
options: dbus_question.options().await?,
default_option: dbus_question.default_option().await?,
data: dbus_question.data().await?
},
with_password: None
};

Ok(result)
}

async fn create_question_with_password(&self, path: &OwnedObjectPath) -> Result<Question, ServiceError> {
let dbus_question = QuestionWithPasswordProxy::builder(&self.connection)
.path(path)?.cache_properties(zbus::CacheProperties::No).build().await?;
let mut result = self.create_generic_question(path).await?;
result.with_password = Some(QuestionWithPassword{
password: dbus_question.password().await?
});

Ok(result)
}

pub async fn answer(&self, id: u32, answer: Answer) -> Result<(), ServiceError> {
let question_path = OwnedObjectPath::from(
ObjectPath::try_from(format!("/org/opensuse/Agama1/Questions/{}", id))
.context("Failed to create dbus path")?
);
if let Some(password) = answer.with_password {
let dbus_password = QuestionWithPasswordProxy::builder(&self.connection)
.path(&question_path)?.cache_properties(zbus::CacheProperties::No).build().await?;
dbus_password.set_password(password.password.as_str()).await?
}
let dbus_generic = GenericQuestionProxy::builder(&self.connection)
.path(&question_path)?.cache_properties(zbus::CacheProperties::No).build().await?;
dbus_generic.set_answer(answer.generic.answer.as_str()).await?;
Ok(())
}
}

#[derive(Error, Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This error do not bring any value. As we already did in the software layer, you can directly use crate::error::Error.

pub enum QuestionsError {
#[error("Question service error: {0}")]
Error(#[from] ServiceError),
}

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

#[derive(Clone)]
struct QuestionsState<'a> {
questions: QuestionsClient<'a>,
}

#[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)]
pub struct Question {
generic: GenericQuestion,
with_password: Option<QuestionWithPassword>,
}

/// Facade of agama_lib::questions::GenericQuestion
/// For fields details see it.
#[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need a facade? If it is because of the utoipa::ToSchema, it is fine with me. I have derived the utoipa::ToSchema for some structs in lib and it feels wrong to me.

However, having to use a facade just because of our documentation library feels wrong too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it is not related to utoipa at all. It is because I do not want to break dbus code and I like more the approach with generic question and composition of question parts. Original code contain QuestionWithPassword that contain link to GenericQuestion, which I do not like much.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, as soon as you do not break the D-Bus external API, you can refactor the internals if you wish. If you decide to keep the facade, please, write down the reason in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, I will write it to comment. In DBus we have attributes and in question is included also answer attribute. Which is not what I want to http API. I have there two parts: 1. question and 2. answer. So original one struct is split into two and the first one is used as output and the second as expected input. At least that is my idea. As said I will write it to comment

pub struct GenericQuestion {
id: u32,
class: String,
text: String,
options: Vec<String>,
default_option: String,
data: HashMap<String, String>
}

/// Facade of agama_lib::questions::WithPassword
/// For fields details see it.
#[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same than above.

pub struct QuestionWithPassword {
password: String
}

#[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)]
pub struct Answer {
generic: GenericAnswer,
with_password: Option<PasswordAnswer>,
}

/// Answer needed for GenericQuestion
#[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)]
pub struct GenericAnswer {
answer: String
}

/// Answer needed for Password specific questions.
#[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)]
pub struct PasswordAnswer {
password: String
}
/// Sets up and returns the axum service for the questions module.
pub async fn questions_service(dbus: zbus::Connection) -> Result<Router, ServiceError> {
let questions = QuestionsClient::new(dbus.clone()).await?;
let state = QuestionsState { questions };
let router = Router::new()
.route("/questions", get(list_questions))
.route("/questions/:id/answer", put(answer))
.with_state(state);
Ok(router)
}

pub async fn questions_stream(dbus: zbus::Connection) -> Result<impl Stream<Item = Event>, Error> {
let proxy = ObjectManagerProxy::new(&dbus).await?;
let add_stream = proxy
.receive_interfaces_added()
.await?
.then(|_| async move {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not know what your plan is here, but to simplify things a bit, you could include the question number (taken from the D-Bus path) and let the client (on the JavaScript side) retrieve the question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, current API does not allow to retrieve single question. That is different to DBus.
Basically my idea about API is:

  1. list all unanswered questions at /questions
  2. answer specific question according to its id with /questions/:id/answer
  3. get event that list of unanswered questions changed
  4. and now when I am thinking about it, we need also way to provide answers file, so some API for it.

Reason why I design it this way is that common workflow is question arise, answer is provided and then another question arise. I think that having multiple unanswered question is quite rare situation, so I optimize API for that single case, but do not prevent in future to extend it e.g. to also get single question if there will be many questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I get the idea. About the answers file, we are not using it right now, so you can skip that part.

Event::QuestionsChanged
});
let remove_stream = proxy
.receive_interfaces_removed()
.await?
.then(|_| async move {
Event::QuestionsChanged
});
Ok(StreamExt::merge(add_stream, remove_stream))
}

async fn list_questions(State(state): State<QuestionsState<'_>>
) -> Result<Json<Vec<Question>>, QuestionsError> {
Ok(Json(state.questions.questions().await?))
}

async fn answer(
State(state): State<QuestionsState<'_>>,
Path(question_id): Path<u32>,
Json(answer): Json<Answer>
) -> Result<(), QuestionsError> {
state.questions.answer(question_id, answer).await?;
Ok(())
}
1 change: 1 addition & 0 deletions rust/agama-server/src/web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::{
l10n::web::l10n_service,
manager::web::{manager_service, manager_stream},
software::web::{software_service, software_stream},
questions::web::{questions_service, questions_stream}
web::common::{progress_stream, service_status_stream},
};
use axum::Router;
Expand Down
1 change: 1 addition & 0 deletions rust/agama-server/src/web/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pub enum Event {
id: String,
},
PatternsChanged(HashMap<String, SelectedBy>),
QuestionsChanged,
InstallationPhaseChanged {
phase: InstallationPhase,
},
Expand Down
Loading