From c3d2fc64f863fbb1adc602f78e866193dcd280da Mon Sep 17 00:00:00 2001
From: Vincent Bernat <vincent@bernat.ch>
Date: Tue, 30 Aug 2022 20:32:45 +0200
Subject: [PATCH] helpers/mapstructure: turn panic while decoding to an error
 message

While there is more helpful information in a panic, this is confusing
to the user. With the amount of code using reflection, it seems better
to have clearer messages to help the user find the faulty section if
any.
---
 common/helpers/mapstructure.go      | 29 +++++++++++++++-----
 common/helpers/mapstructure_test.go | 42 ++++++++++++++++++++++++++++-
 console/data/docs/99-changelog.md   |  1 +
 3 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/common/helpers/mapstructure.go b/common/helpers/mapstructure.go
index f48722d5..4f7075ff 100644
--- a/common/helpers/mapstructure.go
+++ b/common/helpers/mapstructure.go
@@ -4,6 +4,8 @@
 package helpers
 
 import (
+	"fmt"
+	"reflect"
 	"strings"
 
 	"github.com/mitchellh/mapstructure"
@@ -26,16 +28,31 @@ func GetMapStructureDecoderConfig(config interface{}, hooks ...mapstructure.Deco
 		ErrorUnused:      true,
 		WeaklyTypedInput: true,
 		MatchName:        MapStructureMatchName,
-		DecodeHook: mapstructure.ComposeDecodeHookFunc(
-			mapstructure.ComposeDecodeHookFunc(hooks...),
-			mapstructure.ComposeDecodeHookFunc(mapstructureUnmarshallerHookFuncs...),
-			mapstructure.TextUnmarshallerHookFunc(),
-			mapstructure.StringToTimeDurationHookFunc(),
-			mapstructure.StringToSliceHookFunc(","),
+		DecodeHook: ProtectedDecodeHookFunc(
+			mapstructure.ComposeDecodeHookFunc(
+				mapstructure.ComposeDecodeHookFunc(hooks...),
+				mapstructure.ComposeDecodeHookFunc(mapstructureUnmarshallerHookFuncs...),
+				mapstructure.TextUnmarshallerHookFunc(),
+				mapstructure.StringToTimeDurationHookFunc(),
+				mapstructure.StringToSliceHookFunc(","),
+			),
 		),
 	}
 }
 
+// ProtectedDecodeHookFunc wraps a DecodeHookFunc to recover and returns an error on panic.
+func ProtectedDecodeHookFunc(hook mapstructure.DecodeHookFunc) mapstructure.DecodeHookFunc {
+	return func(from, to reflect.Value) (v interface{}, err error) {
+		defer func() {
+			if r := recover(); r != nil {
+				v = nil
+				err = fmt.Errorf("internal error while parsing: %s", r)
+			}
+		}()
+		return mapstructure.DecodeHookExec(hook, from, to)
+	}
+}
+
 // MapStructureMatchName tells if map key and field names are equal.
 func MapStructureMatchName(mapKey, fieldName string) bool {
 	key := strings.ToLower(strings.ReplaceAll(mapKey, "-", ""))
diff --git a/common/helpers/mapstructure_test.go b/common/helpers/mapstructure_test.go
index 3a1be5a6..e389fc6d 100644
--- a/common/helpers/mapstructure_test.go
+++ b/common/helpers/mapstructure_test.go
@@ -3,7 +3,15 @@
 
 package helpers
 
-import "testing"
+import (
+	"errors"
+	"reflect"
+	"strings"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+	"github.com/mitchellh/mapstructure"
+)
 
 func TestMapStructureMatchName(t *testing.T) {
 	cases := []struct {
@@ -27,3 +35,35 @@ func TestMapStructureMatchName(t *testing.T) {
 		}
 	}
 }
+
+func TestProtectedDecodeHook(t *testing.T) {
+	var configuration struct {
+		A string
+		B string
+	}
+	panicHook := func(from, to reflect.Type, data interface{}) (interface{}, error) {
+		if from.Kind() == reflect.String {
+			panic(errors.New("noooo"))
+		}
+		return data, nil
+	}
+	decoder, err := mapstructure.NewDecoder(GetMapStructureDecoderConfig(&configuration, panicHook))
+	if err != nil {
+		t.Fatalf("NewDecoder() error:\n%+v", err)
+	}
+	err = decoder.Decode(gin.H{"A": "hello", "B": "bye"})
+	if err == nil {
+		t.Fatal("Decode() did not error")
+	} else {
+		got := strings.Split(err.Error(), "\n")
+		expected := []string{
+			`2 error(s) decoding:`,
+			``,
+			`* error decoding 'A': internal error while parsing: noooo`,
+			`* error decoding 'B': internal error while parsing: noooo`,
+		}
+		if diff := Diff(got, expected); diff != "" {
+			t.Fatalf("Decode() error:\n%s", diff)
+		}
+	}
+}
diff --git a/console/data/docs/99-changelog.md b/console/data/docs/99-changelog.md
index c55ec0a0..f94d66ab 100644
--- a/console/data/docs/99-changelog.md
+++ b/console/data/docs/99-changelog.md
@@ -15,6 +15,7 @@ identified with a specific icon:
 
 - 🩹 *orchestrator*: validate configuration of other services on start
 - 🩹 *inlet*: correctly parse `inlet.snmp.communities` when it is just a string
+- 🌱 *cmd*: print a shorter message when an internal error happens when parsing configuration
 - 🌱 *inlet*: add `inlet.snmp.ports` to configure SNMP exporter ports
 
 ## 1.5.7 - 2022-08-23