-
Notifications
You must be signed in to change notification settings - Fork 44
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
Ask cli #1457
Ask cli #1457
Changes from all commits
d16b211
fb6b97e
177f5cc
5ba3b2f
59faba9
84120fb
a552420
97fd52f
491acec
6e05a80
1d67e2c
4abaab0
104adf2
4074fa6
f35f94d
f696f41
7eabf7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
use agama_lib::connection; | ||
use agama_lib::proxies::Questions1Proxy; | ||
use anyhow::Context; | ||
use agama_lib::questions::http_client::HTTPClient; | ||
use agama_lib::{connection, error::ServiceError}; | ||
use clap::{Args, Subcommand, ValueEnum}; | ||
|
||
#[derive(Subcommand, Debug)] | ||
|
@@ -19,6 +19,10 @@ pub enum QuestionsCommands { | |
/// Path to a file containing the answers in YAML format. | ||
path: String, | ||
}, | ||
/// prints list of questions that is waiting for answer in YAML format | ||
List, | ||
/// Ask question from stdin in YAML format and print answer when it is answered. | ||
Ask, | ||
} | ||
|
||
#[derive(Args, Debug)] | ||
|
@@ -35,30 +39,54 @@ pub enum Modes { | |
NonInteractive, | ||
} | ||
|
||
async fn set_mode(proxy: Questions1Proxy<'_>, value: Modes) -> anyhow::Result<()> { | ||
// TODO: how to print dbus error in that anyhow? | ||
async fn set_mode(proxy: Questions1Proxy<'_>, value: Modes) -> Result<(), ServiceError> { | ||
proxy | ||
.set_interactive(value == Modes::Interactive) | ||
.await | ||
.context("Failed to set mode for answering questions.") | ||
.map_err(|e| e.into()) | ||
} | ||
|
||
async fn set_answers(proxy: Questions1Proxy<'_>, path: String) -> anyhow::Result<()> { | ||
// TODO: how to print dbus error in that anyhow? | ||
async fn set_answers(proxy: Questions1Proxy<'_>, path: String) -> Result<(), ServiceError> { | ||
proxy | ||
.add_answer_file(path.as_str()) | ||
.await | ||
.context("Failed to set answers from answers file") | ||
.map_err(|e| e.into()) | ||
} | ||
|
||
pub async fn run(subcommand: QuestionsCommands) -> anyhow::Result<()> { | ||
async fn list_questions() -> Result<(), ServiceError> { | ||
let client = HTTPClient::new().await?; | ||
let questions = client.list_questions().await?; | ||
// FIXME: that conversion to anyhow error is nasty, but we do not expect issue | ||
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. We could have a generic error for these cases. But let's keep it as it is now. At some point we should improve our error types. |
||
// when questions are already read from json | ||
// 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 | ||
let questions_json = | ||
serde_json::to_string_pretty(&questions).map_err(Into::<anyhow::Error>::into)?; | ||
println!("{}", questions_json); | ||
Ok(()) | ||
} | ||
|
||
async fn ask_question() -> Result<(), ServiceError> { | ||
let client = HTTPClient::new().await?; | ||
let question = serde_json::from_reader(std::io::stdin())?; | ||
|
||
let created_question = client.create_question(&question).await?; | ||
let answer = client.get_answer(created_question.generic.id).await?; | ||
let answer_json = serde_json::to_string_pretty(&answer).map_err(Into::<anyhow::Error>::into)?; | ||
println!("{}", answer_json); | ||
|
||
client.delete_question(created_question.generic.id).await?; | ||
Ok(()) | ||
} | ||
|
||
pub async fn run(subcommand: QuestionsCommands) -> Result<(), ServiceError> { | ||
let connection = connection().await?; | ||
let proxy = Questions1Proxy::new(&connection) | ||
.await | ||
.context("Failed to connect to Questions service")?; | ||
let proxy = Questions1Proxy::new(&connection).await?; | ||
|
||
match subcommand { | ||
QuestionsCommands::Mode(value) => set_mode(proxy, value.value).await, | ||
QuestionsCommands::Answers { path } => set_answers(proxy, path).await, | ||
QuestionsCommands::List => list_questions().await, | ||
QuestionsCommands::Ask => ask_question().await, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,170 @@ | ||
use reqwest::{header, Client, Response}; | ||
use serde::{de::DeserializeOwned, Serialize}; | ||
|
||
use crate::{auth::AuthToken, error::ServiceError}; | ||
|
||
/// Base that all HTTP clients should use. | ||
/// | ||
/// It provides several features including automatic base URL switching, | ||
/// websocket events listening or object constructions. | ||
/// | ||
/// Usage should be just thin layer in domain specific client. | ||
/// | ||
/// ```no_run | ||
/// use agama_lib::questions::model::Question; | ||
/// use agama_lib::base_http_client::BaseHTTPClient; | ||
/// use agama_lib::error::ServiceError; | ||
/// | ||
/// async fn get_questions() -> Result<Vec<Question>, ServiceError> { | ||
/// let client = BaseHTTPClient::new()?; | ||
/// client.get("/questions").await | ||
/// } | ||
/// ``` | ||
pub struct BaseHTTPClient { | ||
client: Client, | ||
pub base_url: String, | ||
} | ||
|
||
const API_URL: &str = "http://localhost/api"; | ||
|
||
impl BaseHTTPClient { | ||
// if there is need for client without authorization, create new constructor for it | ||
pub fn new() -> Result<Self, ServiceError> { | ||
let token = AuthToken::find().ok_or(ServiceError::NotAuthenticated)?; | ||
|
||
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))?; | ||
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. I am still no convinced of using anyhow, especially in a library. But we can postpone that until we refactor the 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. me neither...I basically use it for trash error which should not happen instead of unwrap |
||
|
||
headers.insert(header::AUTHORIZATION, value); | ||
|
||
let client = Client::builder().default_headers(headers).build()?; | ||
|
||
Ok(Self { | ||
client, | ||
base_url: API_URL.to_string(), // TODO: add support for remote server | ||
}) | ||
} | ||
|
||
/// 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) | ||
} | ||
} | ||
|
||
/// 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. | ||
/// | ||
/// Arguments: | ||
/// | ||
/// * `path`: path relative to HTTP API like `/questions` | ||
pub async fn get_response(&self, path: &str) -> Result<Response, ServiceError> { | ||
self.client | ||
.get(self.url(path)) | ||
.send() | ||
.await | ||
.map_err(|e| e.into()) | ||
} | ||
|
||
fn url(&self, path: &str) -> String { | ||
self.base_url.clone() + path | ||
} | ||
|
||
/// post object to given path and report error if response is not success | ||
jreidinger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// | ||
/// Arguments: | ||
/// | ||
/// * `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> { | ||
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. Stupid question, is it normal that a successful HTTP 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. Well, response is optional in POST. I check what we have in 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. Yes, a POST call does not need to return anything. I am not convinced about using "no text" string. Why not just return an empty body? In any case, we can evolve this API when we use it. |
||
let response = self.post_response(path, object).await?; | ||
if response.status().is_success() { | ||
Ok(()) | ||
} else { | ||
Err(self.build_backend_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. | ||
/// | ||
/// In general unless specific response handling is needed, simple post should be used. | ||
/// | ||
/// Arguments: | ||
/// | ||
/// * `path`: path relative to HTTP API like `/questions` | ||
/// * `object`: Object that can be serialiazed to JSON as body of request. | ||
pub async fn post_response( | ||
jreidinger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
&self, | ||
path: &str, | ||
object: &impl Serialize, | ||
) -> Result<Response, ServiceError> { | ||
self.client | ||
.post(self.url(path)) | ||
.json(object) | ||
.send() | ||
.await | ||
.map_err(|e| e.into()) | ||
} | ||
|
||
/// delete call on given path and report error if failed | ||
jreidinger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// | ||
/// 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) | ||
} | ||
} | ||
|
||
/// delete call on 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. | ||
/// | ||
/// 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. | ||
/// | ||
/// Arguments: | ||
/// | ||
/// * `path`: path relative to HTTP API like `/questions/1` | ||
pub async fn delete_response(&self, path: &str) -> Result<Response, ServiceError> { | ||
self.client | ||
.delete(self.url(path)) | ||
.send() | ||
.await | ||
.map_err(|e| e.into()) | ||
} | ||
|
||
const NO_TEXT: &'static str = "(Failed to extract error text from HTTP response)"; | ||
/// Builds [`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 { | ||
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. Perhaps you might want to implement something like 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. Can it be asynchronous? As it is not simple conversion but it also ask for text. 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. Also it is a bit against documentation of traits which mention that it should be obvious to convert...which I am not sure about this case. 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. OK, async could be a problem. Let's keep it as it is. 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. About when to implement those traits, I am usually unsure 😅 |
||
let code = response.status().as_u16(); | ||
let text = response | ||
.text() | ||
.await | ||
.unwrap_or_else(|_| Self::NO_TEXT.to_string()); | ||
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. "No text" -> 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. I check quite deeply when call to text can fail and to be honest I do not find it either in documentation neither from code. So lets use your suggestion. |
||
ServiceError::BackendError(code, text) | ||
} | ||
} |
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.
I wonder whether we should use JSON for everything.
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.
it is fine for me if agreed on it
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.
reason why I use yaml for it is that in answers file we used for automatic answers we also use yaml.
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.
Yes, I know, we use JSON for profiles and YAML for the answers file. I am not saying that we should fix it now, but let's have into account in the future.