MM-10232, MM-10259: Improve error handling from invalid json (#8668)

* MM-10232: improve error handling from malformed slash command responses

Switch to json.Unmarshal, which doesn't obscure JSON parse failures like
json.Decode. The latter is primarily designed for streams of JSON, not
necessarily unmarshalling just a single object.

* rework HumanizedJsonError to expose Line and Character discretely

* MM-10259: pinpoint line and character where json config error occurs

* tweak HumanizeJsonError to accept err first

1 files changed, 120 insertions, 86 deletions

diff --git a/model/command_response_test.go b/model/command_response_test.go
index dde8d032b..894f9d655 100644
--- a/model/command_response_test.go
+++ b/model/command_response_test.go
@@ -6,17 +6,9 @@ package model
 import (
 	"strings"
 	"testing"
-)
-
-func TestCommandResponseJson(t *testing.T) {
-	o := CommandResponse{Text: "test"}
-	json := o.ToJson()
-	ro := CommandResponseFromJson(strings.NewReader(json))
 
-	if o.Text != ro.Text {
-		t.Fatal("Ids do not match")
-	}
-}
+	"github.com/stretchr/testify/assert"
+)
 
 func TestCommandResponseFromHTTPBody(t *testing.T) {
 	for _, test := range []struct {
@@ -29,95 +21,137 @@ func TestCommandResponseFromHTTPBody(t *testing.T) {
 		{"application/json", `{"text": "foo"}`, "foo"},
 		{"application/json; charset=utf-8", `{"text": "foo"}`, "foo"},
 	} {
-		response := CommandResponseFromHTTPBody(test.ContentType, strings.NewReader(test.Body))
-		if response.Text != test.ExpectedText {
-			t.Fatal()
-		}
+		response, err := CommandResponseFromHTTPBody(test.ContentType, strings.NewReader(test.Body))
+		assert.NoError(t, err)
+		assert.Equal(t, test.ExpectedText, response.Text)
 	}
 }
 
 func TestCommandResponseFromPlainText(t *testing.T) {
 	response := CommandResponseFromPlainText("foo")
-	if response.Text != "foo" {
-		t.Fatal("text should be foo")
-	}
+	assert.Equal(t, "foo", response.Text)
 }
 
 func TestCommandResponseFromJson(t *testing.T) {
-	json := `{
-		"response_type": "ephemeral",
-		"text": "response text",
-		"username": "response username",
-		"icon_url": "response icon url",
-		"goto_location": "response goto location",
-		"attachments": [{
-			"text": "attachment 1 text",
-			"pretext": "attachment 1 pretext"
-		},{
-			"text": "attachment 2 text",
-			"fields": [{
-				"title": "field 1",
-				"value": "value 1",
-				"short": true
-			},{
-				"title": "field 2",
-				"value": [],
-				"short": false
-			}]
-		}]
-	}`
-
-	response := CommandResponseFromJson(strings.NewReader(json))
+	t.Parallel()
 
-	if response == nil {
-		t.Fatal("should've received non-nil CommandResponse")
+	sToP := func(s string) *string {
+		return &s
 	}
 
-	if response.ResponseType != "ephemeral" {
-		t.Fatal("should've received correct response type")
-	} else if response.Text != "response text" {
-		t.Fatal("should've received correct response text")
-	} else if response.Username != "response username" {
-		t.Fatal("should've received correct response username")
-	} else if response.IconURL != "response icon url" {
-		t.Fatal("should've received correct response icon url")
-	} else if response.GotoLocation != "response goto location" {
-		t.Fatal("should've received correct response goto location")
+	testCases := []struct {
+		Description             string
+		Json                    string
+		ExpectedCommandResponse *CommandResponse
+		ExpectedError           *string
+	}{
+		{
+			"empty response",
+			"",
+			nil,
+			sToP("parsing error at line 1, character 1: unexpected end of JSON input"),
+		},
+		{
+			"malformed response",
+			`{"text": }`,
+			nil,
+			sToP("parsing error at line 1, character 11: invalid character '}' looking for beginning of value"),
+		},
+		{
+			"invalid response",
+			`{"text": "test", "response_type": 5}`,
+			nil,
+			sToP("parsing error at line 1, character 36: json: cannot unmarshal number into Go struct field CommandResponse.response_type of type string"),
+		},
+		{
+			"ephemeral response",
+			`{
+				"response_type": "ephemeral",
+				"text": "response text",
+				"username": "response username",
+				"icon_url": "response icon url",
+				"goto_location": "response goto location",
+				"attachments": [{
+					"text": "attachment 1 text",
+					"pretext": "attachment 1 pretext"
+				},{
+					"text": "attachment 2 text",
+					"fields": [{
+						"title": "field 1",
+						"value": "value 1",
+						"short": true
+					},{
+						"title": "field 2",
+						"value": [],
+						"short": false
+					}]
+				}]
+			}`,
+			&CommandResponse{
+				ResponseType: "ephemeral",
+				Text:         "response text",
+				Username:     "response username",
+				IconURL:      "response icon url",
+				GotoLocation: "response goto location",
+				Attachments: []*SlackAttachment{
+					{
+						Text:    "attachment 1 text",
+						Pretext: "attachment 1 pretext",
+					},
+					{
+						Text: "attachment 2 text",
+						Fields: []*SlackAttachmentField{
+							{
+								Title: "field 1",
+								Value: "value 1",
+								Short: true,
+							},
+							{
+								Title: "field 2",
+								Value: "[]",
+								Short: false,
+							},
+						},
+					},
+				},
+			},
+			nil,
+		},
+		{
+			"null array items",
+			`{"attachments":[{"fields":[{"title":"foo","value":"bar","short":true}, null]}, null]}`,
+			&CommandResponse{
+				Attachments: []*SlackAttachment{
+					{
+						Fields: []*SlackAttachmentField{
+							{
+								Title: "foo",
+								Value: "bar",
+								Short: true,
+							},
+						},
+					},
+				},
+			},
+			nil,
+		},
 	}
 
-	attachments := response.Attachments
-	if len(attachments) != 2 {
-		t.Fatal("should've received 2 attachments")
-	} else if attachments[0].Text != "attachment 1 text" {
-		t.Fatal("should've received correct first attachment text")
-	} else if attachments[0].Pretext != "attachment 1 pretext" {
-		t.Fatal("should've received correct first attachment pretext")
-	} else if attachments[1].Text != "attachment 2 text" {
-		t.Fatal("should've received correct second attachment text")
-	}
+	for _, testCase := range testCases {
+		testCase := testCase
+		t.Run(testCase.Description, func(t *testing.T) {
+			t.Parallel()
 
-	fields := attachments[1].Fields
-	if len(fields) != 2 {
-		t.Fatal("should've received 2 fields")
-	} else if fields[0].Value.(string) != "value 1" {
-		t.Fatal("should've received correct first attachment value")
-	} else if _, ok := fields[1].Value.(string); !ok {
-		t.Fatal("should've received second attachment value parsed as a string")
-	} else if fields[1].Value.(string) != "[]" {
-		t.Fatal("should've received correct second attachment value")
-	}
-}
-
-func TestCommandResponseNullArrayItems(t *testing.T) {
-	payload := `{"attachments":[{"fields":[{"title":"foo","value":"bar","short":true}, null]}, null]}`
-	cr := CommandResponseFromJson(strings.NewReader(payload))
-	if cr == nil {
-		t.Fatal("CommandResponse should not be nil")
-	}
-	if len(cr.Attachments) != 1 {
-		t.Fatalf("expected one attachment")
-	}
-	if len(cr.Attachments[0].Fields) != 1 {
-		t.Fatalf("expected one field")
+			response, err := CommandResponseFromJson(strings.NewReader(testCase.Json))
+			if testCase.ExpectedError != nil {
+				assert.EqualError(t, err, *testCase.ExpectedError)
+				assert.Nil(t, response)
+			} else {
+				assert.NoError(t, err)
+				if assert.NotNil(t, response) {
+					assert.Equal(t, testCase.ExpectedCommandResponse, response)
+				}
+			}
+		})
 	}
 }