Merge branch 'master' into advanced-permissions-phase-2

4 files changed, 144 insertions, 98 deletions

diff --git a/model/client.go b/model/client.go
index b48d1ac37..317374d36 100644
--- a/model/client.go
+++ b/model/client.go
@@ -831,8 +831,10 @@ func (c *Client) Command(channelId string, command string) (*Result, *AppError)
 		return nil, err
 	} else {
 		defer closeBody(r)
+
+		response, _ := CommandResponseFromJson(r.Body)
 		return &Result{r.Header.Get(HEADER_REQUEST_ID),
-			r.Header.Get(HEADER_ETAG_SERVER), CommandResponseFromJson(r.Body)}, nil
+			r.Header.Get(HEADER_ETAG_SERVER), response}, nil
 	}
 }
 
diff --git a/model/client4.go b/model/client4.go
index 260d75df6..387ca038f 100644
--- a/model/client4.go
+++ b/model/client4.go
@@ -2991,7 +2991,9 @@ func (c *Client4) ExecuteCommand(channelId, command string) (*CommandResponse, *
 		return nil, BuildErrorResponse(r, err)
 	} else {
 		defer closeBody(r)
-		return CommandResponseFromJson(r.Body), BuildResponse(r)
+
+		response, _ := CommandResponseFromJson(r.Body)
+		return response, BuildResponse(r)
 	}
 }
 
@@ -3007,7 +3009,9 @@ func (c *Client4) ExecuteCommandWithTeam(channelId, teamId, command string) (*Co
 		return nil, BuildErrorResponse(r, err)
 	} else {
 		defer closeBody(r)
-		return CommandResponseFromJson(r.Body), BuildResponse(r)
+
+		response, _ := CommandResponseFromJson(r.Body)
+		return response, BuildResponse(r)
 	}
 }
 
diff --git a/model/command_response.go b/model/command_response.go
index cac7e8452..1ed5286de 100644
--- a/model/command_response.go
+++ b/model/command_response.go
@@ -8,6 +8,8 @@ import (
 	"io"
 	"io/ioutil"
 	"strings"
+
+	"github.com/mattermost/mattermost-server/utils/jsonutils"
 )
 
 const (
@@ -31,14 +33,14 @@ func (o *CommandResponse) ToJson() string {
 	return string(b)
 }
 
-func CommandResponseFromHTTPBody(contentType string, body io.Reader) *CommandResponse {
+func CommandResponseFromHTTPBody(contentType string, body io.Reader) (*CommandResponse, error) {
 	if strings.TrimSpace(strings.Split(contentType, ";")[0]) == "application/json" {
 		return CommandResponseFromJson(body)
 	}
 	if b, err := ioutil.ReadAll(body); err == nil {
-		return CommandResponseFromPlainText(string(b))
+		return CommandResponseFromPlainText(string(b)), nil
 	}
-	return nil
+	return nil, nil
 }
 
 func CommandResponseFromPlainText(text string) *CommandResponse {
@@ -47,15 +49,19 @@ func CommandResponseFromPlainText(text string) *CommandResponse {
 	}
 }
 
-func CommandResponseFromJson(data io.Reader) *CommandResponse {
-	decoder := json.NewDecoder(data)
-	var o CommandResponse
+func CommandResponseFromJson(data io.Reader) (*CommandResponse, error) {
+	b, err := ioutil.ReadAll(data)
+	if err != nil {
+		return nil, err
+	}
 
-	if err := decoder.Decode(&o); err != nil {
-		return nil
+	var o CommandResponse
+	err = json.Unmarshal(b, &o)
+	if err != nil {
+		return nil, jsonutils.HumanizeJsonError(err, b)
 	}
 
 	o.Attachments = StringifySlackFieldValue(o.Attachments)
 
-	return &o
+	return &o, nil
 }
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)
+				}
+			}
+		})
 	}
 }