fix(friendshipper): more error handling (#88)

* Handle an uncaught exception in the frontend related to null commit data
* Handle an invalid unwrap() when fetching artifacts from s3. Just return an empty list in this
  case to the caller and log the error.

core/src/storage/entry.rs

@@ -206,7 +206,7 @@ impl ArtifactEntry {
         storage: &ArtifactStorage,
     ) -> Result<Self, CoreError> {
         let commit = self.commit.clone();
-        let path = storage.resolve_path(config)?;
+        let path = storage.resolve_path(config);
         // This only works for V1 schema
         Ok(Self::new(format!(
             "{}{}.json",

core/src/storage/mock/mod.rs

@@ -20,6 +20,10 @@ impl MockArtifactProvider {
 
 #[async_trait]
 impl ArtifactProvider for MockArtifactProvider {
+    fn get_method_prefix(&self) -> MethodPrefix {
+        "file://".into()
+    }
+
     async fn get_artifact_by_prefix(&self, prefix: &str) -> Result<String, CoreError> {
         // TODO: Send back an actual longtail .json file to test with.
         match prefix.contains("fail") {
@@ -112,7 +116,7 @@ impl ArtifactProvider for MockArtifactProvider {
                 fake_entries.push(ArtifactEntry::new(format!("{}{}", path, "test/file/4")));
             }
         }
-        Ok(("file://".into(), fake_entries))
+        Ok((self.get_method_prefix(), fake_entries))
     }
 }
 

core/src/storage/mod.rs

@@ -4,6 +4,7 @@ use std::sync::Arc;
 use anyhow::{anyhow, Result};
 use async_trait::async_trait;
 use serde::{Deserialize, Serialize};
+use tracing::error;
 
 pub use crate::storage::config::ArtifactBuildConfig;
 pub use crate::storage::config::ArtifactConfig;
@@ -24,11 +25,14 @@ pub mod s3;
 // Trait implementing artifact lookup different storage providers
 #[async_trait]
 pub trait ArtifactProvider: std::fmt::Debug + Send + Sync {
+    fn get_method_prefix(&self) -> MethodPrefix;
+
     // Pulls the artifact listing for the given path from the provider
     async fn get_artifact_list(
         &self,
         path: &str,
     ) -> Result<(MethodPrefix, Vec<ArtifactEntry>), CoreError>;
+
     // Get a single artifact matching the given prefix, erroring if there is more than one match
     async fn get_artifact_by_prefix(&self, prefix: &str) -> Result<String, CoreError>;
 }
@@ -75,7 +79,7 @@ impl ArtifactStorage {
         artifact_config: ArtifactConfig,
         short_sha: &str,
     ) -> Result<String, CoreError> {
-        let path = self.resolve_path(&artifact_config)?;
+        let path = self.resolve_path(&artifact_config);
 
         self.provider
             .get_artifact_by_prefix(&format!("{}{}", path, short_sha))
@@ -84,32 +88,41 @@ impl ArtifactStorage {
 
     // Use the provider to get an artifact listing by constructing the lookup path.
     pub async fn artifact_list(&self, artifact_config: ArtifactConfig) -> ArtifactList {
-        let (method_prefix, entries) = self
+        let artifact_list_result = self
             .provider
-            .get_artifact_list(&self.resolve_path(&artifact_config).unwrap())
-            .await
-            .unwrap();
-        let mut list = ArtifactList::new(artifact_config, method_prefix);
-        list.entries = entries;
-        list.sort_by_last_modified();
-        list
+            .get_artifact_list(&self.resolve_path(&artifact_config))
+            .await;
+
+        match artifact_list_result {
+            Err(e) => {
+                let method_prefix: MethodPrefix = self.provider.get_method_prefix();
+                error!("Caught error retrieving artifact list with artifact config {:?} and method prefix {:?}. {}", artifact_config, method_prefix, e);
+                ArtifactList::new(artifact_config, method_prefix)
+            }
+            Ok((method_prefix, entries)) => {
+                let mut list = ArtifactList::new(artifact_config, method_prefix);
+                list.entries = entries;
+                list.sort_by_last_modified();
+                list
+            }
+        }
     }
 
-    fn resolve_path(&self, artifact_config: &ArtifactConfig) -> Result<String> {
+    fn resolve_path(&self, artifact_config: &ArtifactConfig) -> String {
         match &self.schema_version {
             StorageSchemaVersion::V1 => Self::resolve_path_v1(artifact_config),
         }
     }
     // This is the v1 path resolver with full support for the new enums
-    fn resolve_path_v1(artifact_config: &ArtifactConfig) -> Result<String> {
+    fn resolve_path_v1(artifact_config: &ArtifactConfig) -> String {
         let path = format!(
             "v1/{}/{}/{}/{}/",
             artifact_config.project,
             artifact_config.artifact_kind.to_path_string(),
             artifact_config.platform.to_path_string(),
             artifact_config.artifact_build_config.to_path_string(),
         );
-        Ok(path)
+        path
     }
 }
 
@@ -126,7 +139,7 @@ mod test {
             Platform::Win64,
         );
         assert_eq!(
-            ArtifactStorage::resolve_path_v1(&ac).unwrap(),
+            ArtifactStorage::resolve_path_v1(&ac),
             "v1/believerco-gameprototypemp/client/win64/development/"
         );
 
@@ -138,7 +151,7 @@ mod test {
         );
 
         assert_eq!(
-            ArtifactStorage::resolve_path_v1(&ac).unwrap(),
+            ArtifactStorage::resolve_path_v1(&ac),
             "v1/believerco-gameprototypemp/engine/win64/development/"
         );
     }

core/src/storage/s3/mod.rs

@@ -1,6 +1,7 @@
 use anyhow::{anyhow, Result};
 use async_trait::async_trait;
 use aws_sdk_s3::Client;
+use tracing::warn;
 
 use crate::storage::{entry::ArtifactEntry, list::MethodPrefix, ArtifactProvider};
 use crate::types::errors::CoreError;
@@ -24,6 +25,10 @@ impl S3ArtifactProvider {
 
 #[async_trait]
 impl ArtifactProvider for S3ArtifactProvider {
+    fn get_method_prefix(&self) -> MethodPrefix {
+        format!("s3://{}/", self.s3_bucket.clone()).into()
+    }
+
     async fn get_artifact_by_prefix(&self, prefix: &str) -> Result<String, CoreError> {
         self.aws_client.check_config().await?;
         let client = Client::new(&self.aws_client.get_sdk_config().await);
@@ -75,7 +80,7 @@ impl ArtifactProvider for S3ArtifactProvider {
 
         while let Some(resp) = paginator.next().await {
             if resp.is_err() {
-                println!("Error getting list of objects from S3: [{:?}", resp);
+                warn!("Error getting list of objects from S3: [{:?}", resp);
                 return Err(CoreError::from(anyhow!(
                     "Error getting list of objects from S3"
                 )));
@@ -87,10 +92,7 @@ impl ArtifactProvider for S3ArtifactProvider {
         }
 
         entry_list.sort_by(|a, b| b.last_modified.partial_cmp(&a.last_modified).unwrap());
-        Ok((
-            format!("s3://{}/", self.s3_bucket.clone()).into(),
-            entry_list,
-        ))
+        Ok((self.get_method_prefix(), entry_list))
     }
 }
 

friendshipper/src/lib/components/home/ContributorLayout.svelte

@@ -23,6 +23,7 @@
 		GameServerResult,
 		MergeQueue,
 		MergeQueueEntry,
+		Commit,
 		Playtest,
 		SyncClientRequest
 	} from '$lib/types';
@@ -209,7 +210,7 @@
 		}
 	};
 
-	const getCommitMessage = (commit): string => {
+	const getCommitMessage = (commit: Commit): string => {
 		if (commit != null) {
 			if (commit.message != null) {
 				const trimmed: string = commit.message.split('\n')[0];
@@ -221,6 +222,18 @@
 		return 'No message';
 	};
 
+	const getCommitAuthor = (commit: Commit): string => {
+		if (commit != null) {
+			if (commit.author != null) {
+				if (commit.author.name != null) {
+					return commit.author.name;
+				}
+			}
+		}
+
+		return '';
+	};
+
 	onMount(() => {
 		void refresh();
 
@@ -365,7 +378,7 @@
 										{getCommitMessage(node.headCommit)}
 									</TableBodyCell>
 									<TableBodyCell class="p-2 text-center"
-										>{node.headCommit.author.name}</TableBodyCell
+										>{getCommitAuthor(node.headCommit)}</TableBodyCell
 									>
 									<TableBodyCell class="p-2 text-center"
 										>{new Date(node.enqueuedAt).toLocaleString()}</TableBodyCell