From 9789c3c455778b079650588035a8e55fba72b882 Mon Sep 17 00:00:00 2001 From: Bohdan Ohorodnii Date: Tue, 17 Dec 2024 01:21:58 +0200 Subject: [PATCH] refactor: API to have a consistent response format --- api/src/errors.rs | 8 ++ api/src/handlers/allowed_versions.rs | 36 +++--- api/src/handlers/compile.rs | 41 +++---- api/src/handlers/process.rs | 54 +++++---- api/src/handlers/scarb_test.rs | 41 +++---- api/src/handlers/scarb_version.rs | 43 +++---- api/src/handlers/types.rs | 164 +++++++++++++++++++++++---- api/src/handlers/utils.rs | 13 +-- 8 files changed, 254 insertions(+), 146 deletions(-) diff --git a/api/src/errors.rs b/api/src/errors.rs index dddbe03c..a8c998d1 100644 --- a/api/src/errors.rs +++ b/api/src/errors.rs @@ -42,6 +42,14 @@ pub enum ApiError { InvalidRequest, #[error("Version not allowed")] VersionNotAllowed, + #[error("Failed to get Scarb version: {0}")] + ScarbVersionNotFound(String), + #[error("Not found")] + NotFound(String), + #[error("Failed to fetch releases")] + FailedToFetchReleases(reqwest::Error), + #[error("Failed to parse releases")] + FailedToParseReleases(reqwest::Error), } pub type Result = std::result::Result; diff --git a/api/src/handlers/allowed_versions.rs b/api/src/handlers/allowed_versions.rs index 9940dd61..f1822080 100644 --- a/api/src/handlers/allowed_versions.rs +++ b/api/src/handlers/allowed_versions.rs @@ -1,7 +1,5 @@ -use crate::errors::Result; +use crate::errors::{ApiError, Result}; use lazy_static::lazy_static; -use rocket::http::Status; -use rocket::serde::json::Json; use semver::Version; use serde::{Deserialize, Serialize}; use std::collections::HashMap; @@ -9,6 +7,8 @@ use std::sync::RwLock; use std::time::SystemTime; use tracing::instrument; +use super::types::ApiResponse; + #[derive(Debug, Deserialize)] struct GitHubRelease { tag_name: String, @@ -27,15 +27,17 @@ lazy_static! { }); } -async fn fetch_github_releases() -> Result, Box> { +async fn fetch_github_releases() -> Result> { let client = reqwest::Client::new(); let releases: Vec = client .get("https://api.github.com/repos/starkware-libs/cairo/releases") .header("User-Agent", "starknet-remix-plugin") .send() - .await? + .await + .map_err(ApiError::FailedToFetchReleases)? .json() - .await?; + .await + .map_err(ApiError::FailedToParseReleases)?; let mut version_map: HashMap<(u64, u64), Version> = HashMap::new(); @@ -90,19 +92,19 @@ pub async fn start_version_updater() { #[instrument] #[get("/allowed-versions")] -pub async fn get_allowed_versions() -> (Status, Json>) { - match do_get_allowed_versions().await { - Ok(versions) => (Status::Ok, versions), - Err(e) => (Status::InternalServerError, Json(vec![e.to_string()])), - } +pub async fn get_allowed_versions() -> ApiResponse> { + do_get_allowed_versions().await } pub async fn is_version_allowed(version: &str) -> bool { - let allowed_versions = do_get_allowed_versions().await.unwrap_or(Json(vec![])); - allowed_versions.contains(&version.to_string()) + let allowed_versions = do_get_allowed_versions().await; + allowed_versions + .data + .unwrap_or_default() + .contains(&version.to_string()) } -pub async fn do_get_allowed_versions() -> Result>> { +pub async fn do_get_allowed_versions() -> ApiResponse> { let should_fetch = { let cache = CACHED_VERSIONS.read().unwrap(); cache.versions.is_empty() @@ -113,11 +115,11 @@ pub async fn do_get_allowed_versions() -> Result>> { let mut cache = CACHED_VERSIONS.write().unwrap(); cache.versions = versions; cache.last_updated = SystemTime::now(); - return Ok(Json(cache.versions.clone())); + return ApiResponse::ok(cache.versions.clone()); } - return Ok(Json(vec![])); + return ApiResponse::ok(vec![]); } let cache = CACHED_VERSIONS.read().unwrap(); - Ok(Json(cache.versions.clone())) + ApiResponse::ok(cache.versions.clone()) } diff --git a/api/src/handlers/compile.rs b/api/src/handlers/compile.rs index 24061c12..70197897 100644 --- a/api/src/handlers/compile.rs +++ b/api/src/handlers/compile.rs @@ -2,21 +2,19 @@ use crate::errors::{ApiError, Result}; use crate::handlers::allowed_versions::is_version_allowed; use crate::handlers::process::{do_process_command, fetch_process_result}; use crate::handlers::types::{ApiCommand, ApiCommandResult}; -use crate::handlers::types::{FileContentMap, ScarbCompileResponse}; +use crate::handlers::types::{CompileResponse, FileContentMap}; use crate::handlers::utils::{get_files_recursive, init_directories, AutoCleanUp}; use crate::handlers::{STATUS_COMPILATION_FAILED, STATUS_SUCCESS, STATUS_UNKNOWN_ERROR}; use crate::metrics::Metrics; use crate::rate_limiter::RateLimited; use crate::worker::WorkerEngine; -use rocket::http::Status; -use rocket::serde::json; use rocket::serde::json::Json; use rocket::{tokio, State}; use std::path::PathBuf; use std::process::{Command, Stdio}; use tracing::instrument; -use super::types::CompilationRequest; +use super::types::{ApiResponse, CompilationRequest}; pub fn scarb_toml_with_version(version: &str) -> String { format!( @@ -45,7 +43,7 @@ pub async fn compile_async( request_json: Json, _rate_limited: RateLimited, engine: &State, -) -> String { +) -> ApiResponse { tracing::info!("/compile/{:?}", request_json.0.file_names()); do_process_command( ApiCommand::Compile { @@ -56,26 +54,18 @@ pub async fn compile_async( } #[instrument(skip(engine))] -#[get("/compile-result/")] +#[get("/compile-async/")] pub async fn get_compile_result( process_id: &str, engine: &State, -) -> (Status, String) { +) -> ApiResponse { tracing::info!("/compile-result/{:?}", process_id); fetch_process_result(process_id, engine, |result| match result { - Ok(ApiCommandResult::Compile(compile_result)) => ( - Status::Ok, - json::to_string(&compile_result) - .unwrap_or_else(|e| format!("Failed to fetch result: {:?}", e)), - ), - Err(err) => ( - Status::InternalServerError, - format!("Failed to fetch result: {:?}", err), - ), - _ => ( - Status::InternalServerError, - "Result is not available".to_string(), - ), + Ok(ApiCommandResult::Compile(compile_result)) => ApiResponse::ok(compile_result.clone()), + Err(err) => { + ApiResponse::internal_server_error(format!("Failed to fetch result: {:?}", err)) + } + _ => ApiResponse::internal_server_error("Result is not available".to_string()), }) } @@ -127,7 +117,7 @@ async fn ensure_scarb_toml( pub async fn do_compile( compilation_request: CompilationRequest, _metrics: &Metrics, -) -> Result> { +) -> Result { // Verify version is in the allowed versions if !is_version_allowed(compilation_request.version.as_deref().unwrap_or("")).await { return Err(ApiError::VersionNotAllowed); @@ -180,9 +170,8 @@ pub async fn do_compile( auto_clean_up.clean_up().await; - Ok(Json(ScarbCompileResponse { - artifacts: file_content_map_array, - message, - status, - })) + Ok(ApiResponse::ok(file_content_map_array) + .with_status(status) + .with_code(200) + .with_message(message)) } diff --git a/api/src/handlers/process.rs b/api/src/handlers/process.rs index c328b676..d297fafe 100644 --- a/api/src/handlers/process.rs +++ b/api/src/handlers/process.rs @@ -1,60 +1,59 @@ use crate::errors::ApiError; use crate::handlers::types::{ApiCommand, ApiCommandResult}; use crate::worker::{ProcessState, WorkerEngine}; -use rocket::http::Status; use rocket::State; +use serde::Serialize; use tracing::{info, instrument}; use uuid::Uuid; +use super::types::ApiResponse; + #[instrument(skip(engine))] #[get("/process_status/")] -pub async fn get_process_status(process_id: String, engine: &State) -> String { +pub async fn get_process_status( + process_id: String, + engine: &State, +) -> ApiResponse { info!("/process_status/{:?}", process_id); // get status of process by ID match Uuid::parse_str(&process_id) { Ok(process_uuid) => { if engine.arc_process_states.contains_key(&process_uuid) { - format!( - "{:}", + ApiResponse::ok( engine .arc_process_states .get(&process_uuid) .unwrap() .value() + .to_string(), ) } else { - // TODO can we return HTTP status code here? - format!("Process with id={} not found", process_id) + ApiResponse::not_found(format!("Process with id={} not found", process_id)) } } - Err(e) => { - // TODO can we return HTTP status code here? - e.to_string() - } + Err(e) => ApiResponse::bad_request(e.to_string()), } } -pub fn do_process_command(command: ApiCommand, engine: &State) -> String { +pub fn do_process_command( + command: ApiCommand, + engine: &State, +) -> ApiResponse { // queue the new Scarb command match engine.enqueue_command(command) { - Ok(uuid) => { - // return the process ID - format!("{}", uuid) - } - Err(e) => { - // TODO can we return HTTP status code here? - e - } + Ok(uuid) => ApiResponse::ok(uuid.to_string()), + Err(e) => ApiResponse::internal_server_error(e.to_string()), } } -pub fn fetch_process_result( +pub fn fetch_process_result( process_id: &str, engine: &State, handle_result: F, -) -> (Status, String) +) -> ApiResponse where - F: FnOnce(Result<&ApiCommandResult, &ApiError>) -> (Status, String), + F: FnOnce(Result<&ApiCommandResult, &ApiError>) -> ApiResponse, + T: Serialize, { // get status of process by ID match Uuid::parse_str(process_id) { @@ -68,15 +67,14 @@ where { ProcessState::Completed(result) => handle_result(Ok(result)), ProcessState::Error(e) => handle_result(Err(e)), - _ => ( - Status::InternalServerError, - "Result not available".to_string(), - ), + _ => { + handle_result(Err(&ApiError::NotFound("Result not available".to_string()))) + } } } else { - (Status::NotFound, "Process id not found".to_string()) + handle_result(Err(&ApiError::NotFound("Process id not found".to_string()))) } } - Err(e) => (Status::BadRequest, e.to_string()), + Err(e) => handle_result(Err(&ApiError::NotFound(e.to_string()))), } } diff --git a/api/src/handlers/scarb_test.rs b/api/src/handlers/scarb_test.rs index 82a0e5d4..4fb0856b 100644 --- a/api/src/handlers/scarb_test.rs +++ b/api/src/handlers/scarb_test.rs @@ -1,56 +1,45 @@ use crate::errors::ApiError; use crate::errors::Result; use crate::handlers::process::{do_process_command, fetch_process_result}; -use crate::handlers::types::{ApiCommand, ApiCommandResult, ScarbTestResponse}; +use crate::handlers::types::{ApiCommand, ApiCommandResult, TestResponse}; use crate::rate_limiter::RateLimited; use crate::utils::lib::get_file_path; use crate::worker::WorkerEngine; -use rocket::http::Status; -use rocket::serde::json; -use rocket::serde::json::Json; use rocket::State; use std::path::PathBuf; use std::process::{Command, Stdio}; use tracing::{debug, info, instrument}; +use super::types::ApiResponse; + #[instrument(skip(engine, _rate_limited))] -#[get("/scarb-test-async/")] +#[post("/scarb-test-async/")] pub async fn scarb_test_async( remix_file_path: PathBuf, engine: &State, _rate_limited: RateLimited, -) -> String { +) -> ApiResponse { info!("/scarb-test-async/{:?}", remix_file_path); do_process_command(ApiCommand::ScarbTest { remix_file_path }, engine) } #[instrument(skip(engine))] -#[get("/scarb-test-result/")] +#[get("/scarb-test-async/")] pub async fn get_scarb_test_result( process_id: &str, engine: &State, -) -> (Status, String) { - info!("/scarb-test-result/{:?}", process_id); +) -> TestResponse { + info!("/scarb-test-async/{:?}", process_id); fetch_process_result(process_id, engine, |result| match result { - Ok(ApiCommandResult::ScarbTest(scarb_result)) => ( - Status::Ok, - json::to_string(&scarb_result) - .unwrap_or_else(|e| format!("Failed to fetch result: {:?}", e)), - ), - Err(err) => ( - Status::InternalServerError, - format!("Failed to fetch result: {:?}", err), - ), - _ => ( - Status::InternalServerError, - "Result not available".to_string(), - ), + Ok(ApiCommandResult::Test(test_result)) => test_result.clone(), + Err(e) => ApiResponse::not_found(e.to_string()), + _ => ApiResponse::internal_server_error("Result not available".to_string()), }) } /// Run Scarb to test a project /// -pub async fn do_scarb_test(remix_file_path: PathBuf) -> Result> { +pub async fn do_scarb_test(remix_file_path: PathBuf) -> Result { let remix_file_path = remix_file_path .to_str() .ok_or(ApiError::FailedToParseString)? @@ -100,5 +89,9 @@ pub async fn do_scarb_test(remix_file_path: PathBuf) -> Result, _rate_limited: RateLimited, -) -> String { +) -> ApiResponse { info!("/scarb_version_async"); do_process_command(ApiCommand::ScarbVersion, engine) } #[instrument(skip(engine))] -#[get("/scarb-version-result/")] -pub async fn get_scarb_version_result( +#[get("/scarb-version-async/")] +pub fn get_scarb_version_result( process_id: &str, engine: &State, -) -> (Status, String) { +) -> ApiResponse { + info!("/scarb-version-async/{:?}", process_id); fetch_process_result(process_id, engine, |result| match result { - Ok(ApiCommandResult::ScarbVersion(version)) => (Status::Ok, version.to_string()), - Err(err) => ( - Status::InternalServerError, - format!("Failed to fetch result: {:?}", err), - ), - _ => ( - Status::InternalServerError, - "Result not available".to_string(), - ), + Ok(ApiCommandResult::ScarbVersion(response)) => response.clone(), + Err(e) => ApiResponse::not_found(e.to_string()), + _ => ApiResponse::internal_server_error("Result not available".to_string()), }) } -/// Run Cairo --version to return Cairo version string +/// Run Scarb --version to return Scarb version string /// /// ## Note -/// (default Cairo version will be used) -pub fn do_cairo_version() -> Result { +/// (default Scarb version will be used) +pub fn do_scarb_version() -> Result> { let version_caller = Command::new("scarb") .arg("--version") .stdout(Stdio::piped()) @@ -56,9 +52,16 @@ pub fn do_cairo_version() -> Result { if output.status.success() { let result = String::from_utf8_lossy(&output.stdout).to_string(); - Ok(result.trim().to_string()) + + let result_with_stderr = String::from_utf8_lossy(&output.stderr).to_string(); + + Ok(ApiResponse::ok(result.clone()) + .with_message(format!("{}{}", result_with_stderr, result)) + .with_success(true) + .with_status("Success".to_string()) + .with_code(200)) } else { - error!("Failed to get cairo version: {:?}", output); - Err(ApiError::CairoVersionNotFound(output.status.to_string())) + error!("Failed to get Scarb version: {:?}", output); + Err(ApiError::ScarbVersionNotFound(output.status.to_string())) } } diff --git a/api/src/handlers/types.rs b/api/src/handlers/types.rs index a6004277..de37f796 100644 --- a/api/src/handlers/types.rs +++ b/api/src/handlers/types.rs @@ -1,3 +1,4 @@ +use rocket::http::{ContentType, Status}; use serde::{Deserialize, Serialize}; use std::path::PathBuf; @@ -5,17 +6,139 @@ pub trait Successful { fn is_successful(&self) -> bool; } -#[derive(Debug, Deserialize, Serialize)] -#[serde(crate = "rocket::serde")] -pub struct CompileResponse { +#[derive(Debug, Serialize, Deserialize, Clone)] +pub struct ApiResponse { + pub success: bool, pub status: String, + pub code: u16, pub message: String, - pub file_content: String, + pub data: Option, + pub error: Option, + pub timestamp: String, + pub request_id: String, } -impl Successful for CompileResponse { - fn is_successful(&self) -> bool { - self.status == "Success" +impl<'r, T: serde::Serialize> rocket::response::Responder<'r, 'static> for ApiResponse { + fn respond_to(self, _: &'r rocket::Request<'_>) -> rocket::response::Result<'static> { + let json = rocket::serde::json::to_string(&self).unwrap(); + + rocket::Response::build() + .sized_body(json.len(), std::io::Cursor::new(json)) + .header(ContentType::JSON) + .status(Status::from_code(self.code).unwrap_or(Status::InternalServerError)) + .ok() + } +} + +impl Default for ApiResponse { + fn default() -> Self { + Self { + success: false, + status: "".to_string(), + code: 0, + message: "".to_string(), + data: None, + error: None, + timestamp: chrono::Utc::now().to_rfc3339(), + request_id: "".to_string(), + } + } +} + +impl ApiResponse { + pub fn internal_server_error(error: String) -> Self { + Self { + status: "InternalServerError".to_string(), + code: 500, + error: Some(error), + ..Default::default() + } + } + + pub fn not_found(error: String) -> Self { + Self { + status: "NotFound".to_string(), + code: 404, + error: Some(error), + ..Default::default() + } + } + + pub fn bad_request(error: String) -> Self { + Self { + status: "BadRequest".to_string(), + code: 400, + error: Some(error), + ..Default::default() + } + } + + pub fn ok(data: T) -> Self { + Self { + success: true, + status: "Ok".to_string(), + code: 200, + data: Some(data), + ..Default::default() + } + } + + pub fn not_available(message: String) -> Self { + Self { + status: "NotAvailable".to_string(), + code: 404, + message, + ..Default::default() + } + } + + pub fn with_message(mut self, message: String) -> Self { + self.message = message; + self + } + + pub fn with_data(mut self, data: T) -> Self { + self.data = Some(data); + self + } + + pub fn with_error(mut self, error: String) -> Self { + self.error = Some(error); + self + } + + pub fn with_timestamp(mut self, timestamp: String) -> Self { + self.timestamp = timestamp; + self + } + + pub fn with_request_id(mut self, request_id: String) -> Self { + self.request_id = request_id; + self + } + + pub fn with_status(mut self, status: String) -> Self { + self.status = status; + self + } + + pub fn with_code(mut self, code: u16) -> Self { + self.code = code; + self + } + + pub fn with_success(mut self, success: bool) -> Self { + self.success = success; + self + } + + pub fn not_allowed(error: String) -> Self { + Self { + status: "NotAllowed".to_string(), + code: 403, + error: Some(error), + ..Default::default() + } } } @@ -25,25 +148,18 @@ pub struct FileContentMap { pub file_content: String, } -#[derive(Debug, Serialize, Deserialize)] -pub struct ScarbCompileResponse { - pub status: String, - pub message: String, - pub artifacts: Vec, -} +pub type CompileResponse = ApiResponse>; + +pub type TestResponse = ApiResponse<()>; -impl Successful for ScarbCompileResponse { +pub type VersionResponse = ApiResponse; + +impl Successful for ApiResponse { fn is_successful(&self) -> bool { - self.status == "Success" + self.success } } -#[derive(Debug, Serialize, Deserialize)] -pub struct ScarbTestResponse { - pub status: String, - pub message: String, -} - #[derive(serde::Serialize, serde::Deserialize, Clone, Debug)] #[serde(crate = "rocket::serde")] pub struct CompilationRequest { @@ -78,9 +194,9 @@ pub enum ApiCommand { #[derive(Debug)] pub enum ApiCommandResult { - ScarbVersion(String), - Compile(ScarbCompileResponse), - ScarbTest(ScarbTestResponse), + ScarbVersion(VersionResponse), + Compile(CompileResponse), + Test(TestResponse), #[allow(dead_code)] Shutdown, } diff --git a/api/src/handlers/utils.rs b/api/src/handlers/utils.rs index d10a52ee..26ce0e03 100644 --- a/api/src/handlers/utils.rs +++ b/api/src/handlers/utils.rs @@ -1,4 +1,3 @@ -use rocket::serde::json::Json; use rocket::tokio; use std::path::Path; use std::time::Instant; @@ -8,11 +7,11 @@ use tracing::{info, instrument}; use crate::errors::{ApiError, Result}; use crate::metrics::{Metrics, COMPILATION_LABEL_VALUE}; +use super::scarb_version::do_scarb_version; use super::types::{CompilationRequest, FileContentMap, Successful}; use super::{ compile::do_compile, scarb_test::do_scarb_test, - scarb_version::do_cairo_version, types::{ApiCommand, ApiCommandResult}, }; @@ -23,10 +22,10 @@ pub async fn on_plugin_launched() { } pub(crate) async fn do_metered_action( - action: impl Future>>, + action: impl Future>, action_label_value: &str, metrics: &Metrics, -) -> Result> { +) -> Result { let start_time = Instant::now(); let result = action.await; let elapsed_time = start_time.elapsed().as_secs_f64(); @@ -108,13 +107,13 @@ impl AutoCleanUp<'_> { pub async fn dispatch_command(command: ApiCommand, metrics: &Metrics) -> Result { match command { - ApiCommand::ScarbVersion => match do_cairo_version() { + ApiCommand::ScarbVersion => match do_scarb_version() { Ok(result) => Ok(ApiCommandResult::ScarbVersion(result)), Err(e) => Err(e), }, ApiCommand::Shutdown => Ok(ApiCommandResult::Shutdown), ApiCommand::ScarbTest { remix_file_path } => match do_scarb_test(remix_file_path).await { - Ok(result) => Ok(ApiCommandResult::ScarbTest(result.into_inner())), + Ok(result) => Ok(ApiCommandResult::Test(result)), Err(e) => Err(e), }, ApiCommand::Compile { @@ -126,7 +125,7 @@ pub async fn dispatch_command(command: ApiCommand, metrics: &Metrics) -> Result< ) .await { - Ok(result) => Ok(ApiCommandResult::Compile(result.into_inner())), + Ok(result) => Ok(ApiCommandResult::Compile(result)), Err(e) => Err(e), }, }