refactor(api): Simplify JSON tan handling

Author: alixinne

src/api/json.rs

@@ -9,7 +9,7 @@ use crate::{
     component::ComponentName,
     global::{Global, InputMessage, InputMessageData, InputSourceHandle, Message},
     image::{RawImage, RawImageError},
-    instance::{InstanceHandle, InstanceHandleError},
+    instance::{InstanceHandle, InstanceHandleError, StartEffectError},
 };
 
 /// Schema definitions as Serde serializable structures and enums
@@ -27,11 +27,13 @@ pub enum JsonApiError {
     #[error("error validating request: {0}")]
     Validation(#[from] validator::ValidationErrors),
     #[error("error receiving system response: {0}")]
-    Recv(#[from] tokio::sync::oneshot::error::RecvError),
+    Recv(#[from] oneshot::error::RecvError),
     #[error("error accessing the current instance: {0}")]
     Instance(#[from] InstanceHandleError),
     #[error("no current instance found")]
     InstanceNotFound,
+    #[error(transparent)]
+    StartEffect(#[from] StartEffectError),
 }
 
 /// A client connected to the JSON endpoint
@@ -76,7 +78,7 @@ impl ClientConnection {
         &mut self,
         request: HyperionMessage,
         global: &Global,
-    ) -> Result<Option<HyperionResponse>, JsonApiError> {
+    ) -> Result<HyperionResponse, JsonApiError> {
         request.validate()?;
 
         match request.command {
@@ -146,40 +148,23 @@ impl ClientConnection {
             }) => {
                 // TODO: Handle origin, python_script, image_data
 
-                match self.current_instance(global).await {
-                    Ok(instance) => {
-                        let (tx, rx) = oneshot::channel();
-
-                        instance
-                            .send(InputMessage::new(
-                                self.source.id(),
-                                ComponentName::All,
-                                InputMessageData::Effect {
-                                    priority,
-                                    duration: duration
-                                        .map(|ms| chrono::Duration::milliseconds(ms as _)),
-                                    effect: effect.into(),
-                                    response: Arc::new(Mutex::new(Some(tx))),
-                                },
-                            ))
-                            .await?;
-
-                        return Ok(match rx.await {
-                            Ok(result) => match result {
-                                Ok(_) => None,
-                                Err(err) => Some(HyperionResponse::error(request.tan, err)),
-                            },
-                            Err(_) => Some(HyperionResponse::error(
-                                request.tan,
-                                "effect request dropped",
-                            )),
-                        });
-                    }
-
-                    Err(err) => {
-                        return Ok(Some(HyperionResponse::error(request.tan, err)));
-                    }
-                }
+                let instance = self.current_instance(global).await?;
+                let (tx, rx) = oneshot::channel();
+
+                instance
+                    .send(InputMessage::new(
+                        self.source.id(),
+                        ComponentName::All,
+                        InputMessageData::Effect {
+                            priority,
+                            duration: duration.map(|ms| chrono::Duration::milliseconds(ms as _)),
+                            effect: effect.into(),
+                            response: Arc::new(Mutex::new(Some(tx))),
+                        },
+                    ))
+                    .await?;
+
+                return Ok(rx.await?.map(|_| HyperionResponse::success())?);
             }
 
             HyperionCommand::ServerInfo(message::ServerInfoRequest { subscribe: _ }) => {
@@ -209,46 +194,37 @@ impl ClientConnection {
                     .await;
 
                 // Just answer the serverinfo request, no need to update state
-                return Ok(Some(
-                    global
-                        .read_config(|config| {
-                            let instances = config
-                                .instances
-                                .iter()
-                                .map(|instance_config| (&instance_config.1.instance).into())
-                                .collect();
-
-                            HyperionResponse::server_info(
-                                request.tan,
-                                priorities,
-                                adjustments,
-                                effects,
-                                instances,
-                            )
-                        })
-                        .await,
-                ));
+                return Ok(global
+                    .read_config(|config| {
+                        let instances = config
+                            .instances
+                            .iter()
+                            .map(|instance_config| (&instance_config.1.instance).into())
+                            .collect();
+
+                        HyperionResponse::server_info(priorities, adjustments, effects, instances)
+                    })
+                    .await);
             }
 
             HyperionCommand::Authorize(message::Authorize { subcommand, .. }) => match subcommand {
                 message::AuthorizeCommand::AdminRequired => {
                     // TODO: Perform actual authentication flow
-                    return Ok(Some(HyperionResponse::admin_required(request.tan, false)));
+                    return Ok(HyperionResponse::admin_required(false));
                 }
                 message::AuthorizeCommand::TokenRequired => {
                     // TODO: Perform actual authentication flow
-                    return Ok(Some(HyperionResponse::token_required(request.tan, false)));
+                    return Ok(HyperionResponse::token_required(false));
                 }
                 _ => {
                     return Err(JsonApiError::NotImplemented);
                 }
             },
 
             HyperionCommand::SysInfo => {
-                return Ok(Some(HyperionResponse::sys_info(
-                    request.tan,
+                return Ok(HyperionResponse::sys_info(
                     global.read_config(|config| config.uuid()).await,
-                )));
+                ));
             }
 
             HyperionCommand::Instance(message::Instance {
@@ -258,18 +234,18 @@ impl ClientConnection {
             }) => {
                 if global.get_instance(id).await.is_some() {
                     self.set_current_instance(id);
-                    return Ok(Some(HyperionResponse::switch_to(request.tan, Some(id))));
+                    return Ok(HyperionResponse::switch_to(Some(id)));
                 } else {
                     // Note: it's an "Ok" but should be an Err. Find out how to represent errors
                     // better
-                    return Ok(Some(HyperionResponse::switch_to(request.tan, None)));
+                    return Ok(HyperionResponse::switch_to(None));
                 }
             }
 
             _ => return Err(JsonApiError::NotImplemented),
         };
 
-        Ok(None)
+        Ok(HyperionResponse::success())
     }
 }
 

src/api/json/message.rs

@@ -703,95 +703,91 @@ pub enum HyperionResponseInfo {
 }
 
 impl HyperionResponse {
-    fn success_info(tan: Option<i32>, info: HyperionResponseInfo) -> Self {
+    pub fn with_tan(mut self, tan: Option<i32>) -> Self {
+        self.tan = tan;
+        self
+    }
+
+    fn success_info(info: HyperionResponseInfo) -> Self {
         Self {
             success: true,
-            tan,
+            tan: None,
             error: None,
             info: Some(info),
         }
     }
 
     /// Return a success response
-    pub fn success(tan: Option<i32>) -> Self {
+    pub fn success() -> Self {
         Self {
             success: true,
-            tan,
+            tan: None,
             error: None,
             info: None,
         }
     }
 
     /// Return an error response
-    pub fn error(tan: Option<i32>, error: impl std::fmt::Display) -> Self {
+    pub fn error(error: impl std::fmt::Display) -> Self {
         Self {
             success: false,
-            tan,
+            tan: None,
             error: Some(error.to_string()),
             info: None,
         }
     }
 
     /// Return an error response
-    pub fn error_info(
-        tan: Option<i32>,
-        error: impl std::fmt::Display,
-        info: HyperionResponseInfo,
-    ) -> Self {
+    pub fn error_info(error: impl std::fmt::Display, info: HyperionResponseInfo) -> Self {
         Self {
             success: false,
-            tan,
+            tan: None,
             error: Some(error.to_string()),
             info: Some(info),
         }
     }
 
     /// Return a server information response
     pub fn server_info(
-        tan: Option<i32>,
         priorities: Vec<PriorityInfo>,
         adjustment: Vec<ChannelAdjustment>,
         effects: Vec<EffectDefinition>,
         instances: Vec<InstanceInfo>,
     ) -> Self {
-        Self::success_info(
-            tan,
-            HyperionResponseInfo::ServerInfo(ServerInfo {
-                priorities,
-                // TODO: Actual autoselect value
-                priorities_autoselect: true,
-                adjustment,
-                effects,
-                led_devices: LedDevicesInfo::new(),
-                grabbers: GrabbersInfo::new(),
-                // TODO: Actual video mode
-                video_mode: VideoMode::Mode2D,
-                instances,
-                hostname: hostname(),
-            }),
-        )
+        Self::success_info(HyperionResponseInfo::ServerInfo(ServerInfo {
+            priorities,
+            // TODO: Actual autoselect value
+            priorities_autoselect: true,
+            adjustment,
+            effects,
+            led_devices: LedDevicesInfo::new(),
+            grabbers: GrabbersInfo::new(),
+            // TODO: Actual video mode
+            video_mode: VideoMode::Mode2D,
+            instances,
+            hostname: hostname(),
+        }))
     }
 
-    pub fn admin_required(tan: Option<i32>, admin_required: bool) -> Self {
-        Self::success_info(tan, HyperionResponseInfo::AdminRequired { admin_required })
+    pub fn admin_required(admin_required: bool) -> Self {
+        Self::success_info(HyperionResponseInfo::AdminRequired { admin_required })
     }
 
-    pub fn token_required(tan: Option<i32>, required: bool) -> Self {
-        Self::success_info(tan, HyperionResponseInfo::TokenRequired { required })
+    pub fn token_required(required: bool) -> Self {
+        Self::success_info(HyperionResponseInfo::TokenRequired { required })
     }
 
-    pub fn sys_info(tan: Option<i32>, id: uuid::Uuid) -> Self {
+    pub fn sys_info(id: uuid::Uuid) -> Self {
         // TODO: Properly fill out this response
-        Self::success_info(tan, HyperionResponseInfo::SysInfo(SysInfo::new(id)))
+        Self::success_info(HyperionResponseInfo::SysInfo(SysInfo::new(id)))
     }
 
-    pub fn switch_to(tan: Option<i32>, id: Option<i32>) -> Self {
+    pub fn switch_to(id: Option<i32>) -> Self {
         if let Some(id) = id {
             // Switch successful
-            Self::success_info(tan, HyperionResponseInfo::SwitchTo { instance: Some(id) })
+            Self::success_info(HyperionResponseInfo::SwitchTo { instance: Some(id) })
         } else {
             Self::error_info(
-                tan,
                 "selected hyperion instance not found",
                 HyperionResponseInfo::SwitchTo { instance: None },
             )

src/servers/json.rs

@@ -57,14 +57,14 @@ pub async fn handle_client(
                 Err(error) => Err(JsonServerError::from(error)),
             }
         } {
-            Ok(None) => json::message::HyperionResponse::success(tan),
-            Ok(Some(response)) => response,
+            Ok(response) => response,
             Err(error) => {
                 error!(error = %error, "error processing request");
 
-                json::message::HyperionResponse::error(tan, &error)
+                json::message::HyperionResponse::error(&error)
             }
-        };
+        }
+        .with_tan(tan);
 
         trace!(response = ?reply, "sending response");
 

src/web/session.rs

@@ -116,21 +116,20 @@ impl Session {
         let api = match self.json_api(global).await {
             Ok(api) => api,
             Err(error) => {
-                return HyperionResponse::error(tan, &error);
+                return HyperionResponse::error(&error).with_tan(tan);
             }
         };
 
         let response = match api.handle_request(request, global).await {
-            Ok(None) => HyperionResponse::success(tan),
-            Ok(Some(response)) => response,
+            Ok(response) => response,
             Err(error) => {
                 error!(error =  %error, "error processing request");
-                HyperionResponse::error(tan, &error)
+                HyperionResponse::error(&error)
             }
         };
 
         trace!(response = ?response, "ws response");
-        response
+        response.with_tan(tan)
     }
 }