merge: #3463

3463: Do not allow panics in the dal by default (ENG-2411) r=nickgerace a=nickgerace

## Description

This PR disallows panics in the `dal` by default. It fixes or permits existing panic locations along the way.

<img src="https://media1.giphy.com/media/HUkOv6BNWc1HO/giphy.gif"/>

Co-authored-by: Nick Gerace <nick@systeminit.com>

Author: si-bors-ng[bot]

lib/dal/src/code_view.rs

@@ -88,13 +88,14 @@ impl CodeView {
         };
 
         let code_gen_entry: CodeGenerationEntry = serde_json::from_value(func_execution)?;
-        if code_gen_entry.code.is_none() || code_gen_entry.format.is_none() {
-            return Ok(None);
-        }
-
-        // Safe unwraps because of the above check
-        let format = code_gen_entry.format.as_ref().unwrap();
-        let code = code_gen_entry.code.as_ref().unwrap();
+        let format = match code_gen_entry.format {
+            Some(found) => found,
+            None => return Ok(None),
+        };
+        let code = match code_gen_entry.code {
+            Some(found) => found,
+            None => return Ok(None),
+        };
 
         let language = if format.is_empty() {
             CodeLanguage::Unknown

lib/dal/src/component.rs

@@ -97,10 +97,14 @@ pub enum ComponentError {
     LayerDb(#[from] si_layer_cache::LayerDbError),
     #[error("component {0} missing attribute value for code")]
     MissingCodeValue(ComponentId),
+    #[error("missing controlling func data for parent attribute value id: {0}")]
+    MissingControllingFuncDataForParentAttributeValue(AttributeValueId),
     #[error("component {0} missing attribute value for qualifications")]
     MissingQualificationsValue(ComponentId),
     #[error("component {0} missing attribute value for root")]
     MissingRootProp(ComponentId),
+    #[error("more than one schema variant found for component: {0}")]
+    MoreThanOneSchemaVariantFound(ComponentId),
     #[error("found multiple parents for component: {0}")]
     MultipleParentsForComponent(ComponentId),
     #[error("found multiple root attribute values ({0} and {1}, at minimum) for component: {2}")]
@@ -607,12 +611,12 @@ impl Component {
                 let content_hash_discriminants: ContentAddressDiscriminants =
                     content.content_address().into();
                 if let ContentAddressDiscriminants::SchemaVariant = content_hash_discriminants {
-                    // TODO(nick): consider creating a new edge weight kind to make this easier.
-                    // We also should use a proper error here.
                     schema_variant_id = match schema_variant_id {
                         None => Some(content.id().into()),
                         Some(_already_found_schema_variant_id) => {
-                            panic!("already found a schema variant")
+                            return Err(ComponentError::MoreThanOneSchemaVariantFound(
+                                component_id,
+                            ));
                         }
                     };
                 }
@@ -1120,8 +1124,9 @@ impl Component {
             // if av has a parent and parent is controlled by dynamic func, that's the controller
             // else av controls itself
             let controlling_tuple = if let Some(parent_av_id) = maybe_parent_av_id {
-                // We can unwrap because if we're on the child, we've added the parent to result
-                let parent_controlling_data = *result.get(&parent_av_id).unwrap();
+                let parent_controlling_data = *result.get(&parent_av_id).ok_or(
+                    ComponentError::MissingControllingFuncDataForParentAttributeValue(parent_av_id),
+                )?;
 
                 if parent_controlling_data.is_dynamic_func {
                     parent_controlling_data

lib/dal/src/context.rs

@@ -183,6 +183,7 @@ impl ConnectionState {
         matches!(self, Self::Connections(_))
     }
 
+    #[allow(clippy::panic)]
     fn txns(&mut self) -> &mut Transactions {
         match self {
             Self::Transactions(txns) => txns,
@@ -625,20 +626,19 @@ impl DalContext {
         new
     }
 
-    pub async fn parent_is_head(&self) -> bool {
+    pub async fn parent_is_head(&self) -> Result<bool, TransactionsError> {
         if let Some(workspace_pk) = self.tenancy.workspace_pk() {
             let workspace = Workspace::get_by_pk(self, &workspace_pk)
                 .await
-                .unwrap()
-                .unwrap();
-            workspace.default_change_set_id()
-                == self
-                    .change_set_pointer()
-                    .unwrap()
-                    .base_change_set_id
-                    .unwrap()
+                .map_err(|err| TransactionsError::Workspace(err.to_string()))?
+                .ok_or(TransactionsError::WorkspaceNotFound(workspace_pk))?;
+            let change_set = self.change_set_pointer()?;
+            let base_change_set_id = change_set
+                .base_change_set_id
+                .ok_or(TransactionsError::NoBaseChangeSet(change_set.id))?;
+            Ok(workspace.default_change_set_id() == base_change_set_id)
         } else {
-            false
+            Ok(false)
         }
     }
 
@@ -1028,6 +1028,8 @@ pub enum TransactionsError {
     JobQueueProcessor(#[from] JobQueueProcessorError),
     #[error(transparent)]
     Nats(#[from] NatsError),
+    #[error("no base change set for change set: {0}")]
+    NoBaseChangeSet(ChangeSetId),
     #[error(transparent)]
     Pg(#[from] PgError),
     #[error(transparent)]

lib/dal/src/func.rs

@@ -32,6 +32,8 @@ pub enum FuncError {
     Base64Decode(#[from] base64::DecodeError),
     #[error("change set error: {0}")]
     ChangeSet(#[from] ChangeSetPointerError),
+    #[error("chrono parse error: {0}")]
+    ChronoParse(#[from] chrono::ParseError),
     #[error("edge weight error: {0}")]
     EdgeWeight(#[from] EdgeWeightError),
     #[error("cannot find intrinsic func {0}")]

lib/dal/src/func/backend/js_action.rs

@@ -38,7 +38,8 @@ impl FuncDispatch for FuncBackendJsAction {
             execution_id: "ayrtonsennajscommand".to_string(),
             handler: handler.into(),
             code_base64: code_base64.into(),
-            args: serde_json::to_value(args).unwrap(),
+            args: serde_json::to_value(args)
+                .expect("should be impossible to fail serialization here"),
             before,
         };
 

lib/dal/src/func/backend/js_reconciliation.rs

@@ -51,7 +51,8 @@ impl FuncDispatch for FuncBackendJsReconciliation {
             execution_id: "freeronaldinhogauchojsreconciliation".to_string(),
             handler: handler.into(),
             code_base64: code_base64.into(),
-            args: serde_json::to_value(args).unwrap(),
+            args: serde_json::to_value(args)
+                .expect("should be impossible to fail serialization here"),
             before,
         };
 

lib/dal/src/func/intrinsics.rs

@@ -27,10 +27,9 @@ impl IntrinsicFunc {
         let mut builder = PkgSpec::builder();
         builder.name("si-intrinsic-funcs");
         builder.version("2023-05-24");
-        builder.created_at(
-            DateTime::parse_from_rfc2822("Wed, 24 May 2023 00:00:00 PST")
-                .expect("able to parse default datetime"),
-        );
+        builder.created_at(DateTime::parse_from_rfc2822(
+            "Wed, 24 May 2023 00:00:00 PST",
+        )?);
         builder.created_by("System Initiative");
         for instrinsic in IntrinsicFunc::iter() {
             builder.func(instrinsic.to_spec()?);

lib/dal/src/lib.rs

@@ -1,5 +1,12 @@
 //! The Data Access Layer (DAL) for System Initiative.
 
+#![warn(
+    clippy::panic,
+    clippy::panic_in_result_fn,
+    clippy::unwrap_in_result,
+    clippy::unwrap_used
+)]
+
 use std::time::Duration;
 
 use rand::Rng;

lib/dal/src/pkg.rs

@@ -85,6 +85,8 @@ pub enum PkgError {
     SchemaVariant(#[from] SchemaVariantError),
     #[error("json serialization error: {0}")]
     SerdeJson(#[from] serde_json::Error),
+    #[error("taking output socket as input for a prop is unsupported for name ({0}) and socket name ({1})")]
+    TakingOutputSocketAsInputForPropUnsupported(String, String),
     #[error("transactions error: {0}")]
     Transactions(#[from] TransactionsError),
     #[error(transparent)]

lib/dal/src/pkg/import.rs

@@ -3069,9 +3069,13 @@ async fn create_attr_proto_arg(
                 .await?;
             apa_id
         }
-        _ => {
-            // xxx: make this an error
-            panic!("unsupported taking output socket as input for prop");
+        SiPkgAttrFuncInputView::OutputSocket {
+            name, socket_name, ..
+        } => {
+            return Err(PkgError::TakingOutputSocketAsInputForPropUnsupported(
+                name.to_owned(),
+                socket_name.to_owned(),
+            ));
         }
     })
 }