From a7d3661f6c53b24ae1c21e93f94209f3af243349 Mon Sep 17 00:00:00 2001
From: Matt <48867283+laidbackware@users.noreply.github.com>
Date: Wed, 19 May 2021 10:51:45 +0100
Subject: [PATCH] fix: insert error handling in list-all & download plugin
 scripts (#881)

---
 lib/commands/command-install.bash             | 39 ++++++++++---------
 lib/commands/command-list-all.bash            | 33 ++++++++++++++--
 .../fixtures/dummy_broken_plugin/bin/download |  4 ++
 test/fixtures/dummy_broken_plugin/bin/install |  3 ++
 .../fixtures/dummy_broken_plugin/bin/list-all |  5 +++
 test/fixtures/dummy_plugin/bin/list-all       |  2 +
 test/install_command.bats                     | 10 +++++
 test/list_command.bats                        | 29 +++++++++++++-
 test/test_helpers.bash                        | 10 +++++
 9 files changed, 111 insertions(+), 24 deletions(-)
 create mode 100755 test/fixtures/dummy_broken_plugin/bin/download
 create mode 100644 test/fixtures/dummy_broken_plugin/bin/install
 create mode 100644 test/fixtures/dummy_broken_plugin/bin/list-all

diff --git a/lib/commands/command-install.bash b/lib/commands/command-install.bash
index 31f57831b..89163419c 100644
--- a/lib/commands/command-install.bash
+++ b/lib/commands/command-install.bash
@@ -173,24 +173,27 @@ install_tool_version() {
       )
     fi
 
-    (
-      # shellcheck disable=SC2031
-      export ASDF_INSTALL_TYPE=$install_type
-      # shellcheck disable=SC2031
-      export ASDF_INSTALL_VERSION=$version
-      # shellcheck disable=SC2031
-      export ASDF_INSTALL_PATH=$install_path
-      # shellcheck disable=SC2031
-      export ASDF_DOWNLOAD_PATH=$download_path
-      # shellcheck disable=SC2031
-      export ASDF_CONCURRENCY=$concurrency
-      mkdir "$install_path"
-      asdf_run_hook "pre_asdf_install_${plugin_name}" "$full_version"
-      bash "${plugin_path}"/bin/install
-    )
-
-    local exit_code=$?
-    if [ $exit_code -eq 0 ]; then
+    local download_exit_code=$?
+    if [ $download_exit_code -eq 0 ]; then
+      (
+        # shellcheck disable=SC2031
+        export ASDF_INSTALL_TYPE=$install_type
+        # shellcheck disable=SC2031
+        export ASDF_INSTALL_VERSION=$version
+        # shellcheck disable=SC2031
+        export ASDF_INSTALL_PATH=$install_path
+        # shellcheck disable=SC2031
+        export ASDF_DOWNLOAD_PATH=$download_path
+        # shellcheck disable=SC2031
+        export ASDF_CONCURRENCY=$concurrency
+        mkdir "$install_path"
+        asdf_run_hook "pre_asdf_install_${plugin_name}" "$full_version"
+        bash "${plugin_path}"/bin/install
+      )
+    fi
+
+    local install_exit_code=$?
+    if [ $install_exit_code -eq 0 ] && [ $download_exit_code -eq 0 ]; then
       # Remove download directory if --keep-download flag or always_keep_download config setting are not set
       always_keep_download=$(get_asdf_config_value "always_keep_download")
       if [ ! "$keep_download" = "true" ] && [ ! "$always_keep_download" = "yes" ] && [ -d "$download_path" ]; then
diff --git a/lib/commands/command-list-all.bash b/lib/commands/command-list-all.bash
index e05a4fac6..06a938d6d 100644
--- a/lib/commands/command-list-all.bash
+++ b/lib/commands/command-list-all.bash
@@ -4,23 +4,48 @@ list_all_command() {
   local plugin_name=$1
   local query=$2
   local plugin_path
+  local std_out
+  local std_err
   plugin_path=$(get_plugin_path "$plugin_name")
   check_if_plugin_exists "$plugin_name"
 
-  local versions
-  versions=$(bash "${plugin_path}/bin/list-all")
+  # Capture return code to allow error handling
+  return_code=0 && split_outputs std_out std_err "bash ${plugin_path}/bin/list-all" || return_code=$?
+
+  if [[ $return_code -ne 0 ]]; then
+    # Printing all output to allow plugin to handle error formatting
+    printf "Plugin %s's list-all callback script failed with output:\\n" "${plugin_name}" >&2
+    printf "%s\\n" "${std_err}" >&2
+    printf "%s\\n" "${std_out}" >&2
+    exit 1
+  fi
 
   if [[ $query ]]; then
-    versions=$(tr ' ' '\n' <<<"$versions" |
+    std_out=$(tr ' ' '\n' <<<"$std_out" |
       grep -E "^\\s*$query" |
       tr '\n' ' ')
   fi
 
-  IFS=' ' read -r -a versions_list <<<"$versions"
+  IFS=' ' read -r -a versions_list <<<"$std_out"
 
   for version in "${versions_list[@]}"; do
     printf "%s\\n" "${version}"
   done
 }
 
+# This function splits stdout from std error, whilst preserving the return core
+function split_outputs() {
+  {
+    IFS=$'\n' read -r -d '' "${1}"
+    IFS=$'\n' read -r -d '' "${2}"
+    (
+      IFS=$'\n' read -r -d '' _ERRNO_
+      return "${_ERRNO_}"
+    )
+  } < <((printf '\0%s\0%d\0' "$( ( ( ({
+    ${3}
+    printf "%s\n" ${?} 1>&3-
+  } | tr -d '\0' 1>&4-) 4>&2- 2>&1- | tr -d '\0' 1>&4-) 3>&1- | exit "$(cat)") 4>&1-)" "${?}" 1>&2) 2>&1)
+}
+
 list_all_command "$@"
diff --git a/test/fixtures/dummy_broken_plugin/bin/download b/test/fixtures/dummy_broken_plugin/bin/download
new file mode 100755
index 000000000..6ac8f6828
--- /dev/null
+++ b/test/fixtures/dummy_broken_plugin/bin/download
@@ -0,0 +1,4 @@
+#!/usr/bin/env bash
+
+echo "Download failed!"
+exit 1
diff --git a/test/fixtures/dummy_broken_plugin/bin/install b/test/fixtures/dummy_broken_plugin/bin/install
new file mode 100644
index 000000000..4244823d5
--- /dev/null
+++ b/test/fixtures/dummy_broken_plugin/bin/install
@@ -0,0 +1,3 @@
+#!/usr/bin/env bash
+
+echo "Unused script"
diff --git a/test/fixtures/dummy_broken_plugin/bin/list-all b/test/fixtures/dummy_broken_plugin/bin/list-all
new file mode 100644
index 000000000..31bc3607d
--- /dev/null
+++ b/test/fixtures/dummy_broken_plugin/bin/list-all
@@ -0,0 +1,5 @@
+#!/usr/bin/env bash
+
+echo "Attempting to list versions"
+echo "List-all failed!" >&2
+exit 1
diff --git a/test/fixtures/dummy_plugin/bin/list-all b/test/fixtures/dummy_plugin/bin/list-all
index 1b9e8dd9e..efec9526d 100644
--- a/test/fixtures/dummy_plugin/bin/list-all
+++ b/test/fixtures/dummy_plugin/bin/list-all
@@ -2,3 +2,5 @@
 
 versions_list=(1.0.0 1.1.0 2.0.0)
 echo "${versions_list[@]}"
+# Sending message to STD error to ensure that it is ignored
+echo "ignore this error" >&2
diff --git a/test/install_command.bats b/test/install_command.bats
index 5c3341824..127a23ecc 100644
--- a/test/install_command.bats
+++ b/test/install_command.bats
@@ -6,6 +6,7 @@ setup() {
   setup_asdf_dir
   install_dummy_legacy_plugin
   install_dummy_plugin
+  install_dummy_broken_plugin
 
   PROJECT_DIR=$HOME/project
   mkdir $PROJECT_DIR
@@ -258,3 +259,12 @@ EOM
   [ -d $ASDF_DIR/downloads/dummy/1.1.0 ]
   [ $(cat $ASDF_DIR/installs/dummy/1.1.0/version) = "1.1.0" ]
 }
+
+@test "install_command fails when download script exits with non-zero code" {
+  run asdf install dummy-broken 1.0.0
+  echo $output
+  [ "$status" -eq 1 ]
+  [ ! -d $ASDF_DIR/downloads/dummy-broken/1.1.0 ]
+  [ ! -d $ASDF_DIR/installs/dummy-broken/1.1.0 ]
+  [ "$output" == "Download failed!" ]
+}
diff --git a/test/list_command.bats b/test/list_command.bats
index 6ee094b42..56836946b 100644
--- a/test/list_command.bats
+++ b/test/list_command.bats
@@ -5,6 +5,7 @@ load test_helpers
 setup() {
   setup_asdf_dir
   install_dummy_plugin
+  install_dummy_broken_plugin
 }
 
 teardown() {
@@ -15,7 +16,8 @@ teardown() {
   run asdf install dummy 1.0.0
   run asdf install dummy 1.1.0
   run asdf list
-  [ "$(echo -e "dummy\n  1.0.0\n  1.1.0")" == "$output" ]
+  [[ "$output" == "$(echo -e "dummy\n  1.0.0\n  1.1.0")"* ]]
+  [[ "$output" == *"$(echo -e "dummy-broken\n  No versions installed")" ]]
   [ "$status" -eq 0 ]
 }
 
@@ -26,7 +28,10 @@ teardown() {
   run asdf install dummy 1.0.0
   run asdf install tummy 2.0.0
   run asdf list
-  [ "$(echo -e "dummy\n  1.0.0\nmummy\n  No versions installed\ntummy\n  2.0.0")" == "$output" ]
+  [[ "$output" == "$(echo -e "dummy\n  1.0.0")"* ]]
+  [[ "$output" == *"$(echo -e "dummy-broken\n  No versions installed")"* ]]
+  [[ "$output" == *"$(echo -e "mummy\n  No versions installed")"* ]]
+  [[ "$output" == *"$(echo -e "tummy\n  2.0.0")" ]]
   [ "$status" -eq 0 ]
 }
 
@@ -49,3 +54,23 @@ teardown() {
   [ "$(echo -e "1.0.0\n1.1.0")" == "$output" ]
   [ "$status" -eq 0 ]
 }
+
+@test "list_all_command fails when list-all script exits with non-zero code" {
+  run asdf list-all dummy-broken
+  echo $output
+  [ "$status" -eq 1 ]
+  [[ "$output" == "Plugin dummy-broken's list-all callback script failed with output:"* ]]
+}
+
+@test "list_all_command displays stderr then stdout when failing" {
+  run asdf list-all dummy-broken
+  echo $output
+  [[ "$output" == *"List-all failed!"* ]]
+  [[ "$output" == *"Attempting to list versions" ]]
+}
+
+
+@test "list_all_command ignores stderr when completing successfully" {
+  run asdf list-all dummy
+  [[ "$output" != *"ignore this error"* ]]
+}
diff --git a/test/test_helpers.bash b/test/test_helpers.bash
index 0e0fb4f27..8505bde07 100644
--- a/test/test_helpers.bash
+++ b/test/test_helpers.bash
@@ -33,6 +33,12 @@ install_mock_legacy_plugin() {
   cp -r "$BATS_TEST_DIRNAME/fixtures/dummy_legacy_plugin" "$location/plugins/$plugin_name"
 }
 
+install_mock_broken_plugin() {
+  local plugin_name=$1
+  local location="${2:-$ASDF_DIR}"
+  cp -r "$BATS_TEST_DIRNAME/fixtures/dummy_broken_plugin" "$location/plugins/$plugin_name"
+}
+
 install_mock_plugin_repo() {
   local plugin_name=$1
   local location="${BASE_DIR}/repo-${plugin_name}"
@@ -59,6 +65,10 @@ install_dummy_legacy_plugin() {
   install_mock_legacy_plugin "legacy-dummy"
 }
 
+install_dummy_broken_plugin() {
+  install_mock_broken_plugin "dummy-broken"
+}
+
 install_dummy_version() {
   install_mock_plugin_version "dummy" "$1"
 }