Skip to content

Commit eb65ce6

Browse files
hmhealeyhmhealey
committedJan 11, 2019
MM-13606 Remove consumeAndClose and clean up integration response handling (5.7) (mattermost#10090)
* MM-13606 Remove consumeAndClose and clean up integration response handling (mattermost#10066) * Fix unit tests for 5.7
1 parent 3aed6ef commit eb65ce6

17 files changed

+444
-178
lines changed
 

‎app/command.go

+120-69
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package app
55

66
import (
77
"fmt"
8+
"io"
89
"io/ioutil"
910
"net/http"
1011
"net/url"
@@ -160,7 +161,6 @@ func (a *App) ExecuteCommand(args *model.CommandArgs) (*model.CommandResponse, *
160161
trigger := parts[0][1:]
161162
trigger = strings.ToLower(trigger)
162163
message := strings.Join(parts[1:], " ")
163-
provider := GetCommandProvider(trigger)
164164

165165
clientTriggerId, triggerId, appErr := model.GenerateTriggerId(args.UserId, a.AsymmetricSigningKey())
166166
if appErr != nil {
@@ -169,24 +169,52 @@ func (a *App) ExecuteCommand(args *model.CommandArgs) (*model.CommandResponse, *
169169

170170
args.TriggerId = triggerId
171171

172-
if provider != nil {
173-
if cmd := provider.GetCommand(a, args.T); cmd != nil {
174-
response := provider.DoCommand(a, args, message)
175-
return a.HandleCommandResponse(cmd, args, response, true)
176-
}
172+
cmd, response := a.tryExecuteBuiltInCommand(args, trigger, message)
173+
if cmd != nil && response != nil {
174+
return a.HandleCommandResponse(cmd, args, response, true)
177175
}
178176

179-
cmd, response, appErr := a.ExecutePluginCommand(args)
177+
cmd, response, appErr = a.tryExecutePluginCommand(args)
180178
if appErr != nil {
181179
return nil, appErr
182-
}
183-
if cmd != nil {
180+
} else if cmd != nil && response != nil {
184181
response.TriggerId = clientTriggerId
185182
return a.HandleCommandResponse(cmd, args, response, true)
186183
}
187184

185+
cmd, response, appErr = a.tryExecuteCustomCommand(args, trigger, message)
186+
if appErr != nil {
187+
return nil, appErr
188+
} else if cmd != nil && response != nil {
189+
response.TriggerId = clientTriggerId
190+
return a.HandleCommandResponse(cmd, args, response, false)
191+
}
192+
193+
return nil, model.NewAppError("command", "api.command.execute_command.not_found.app_error", map[string]interface{}{"Trigger": trigger}, "", http.StatusNotFound)
194+
}
195+
196+
// tryExecutePluginCommand attempts to run a built in command based on the given arguments. If no such command can be
197+
// found, returns nil for all arguments.
198+
func (a *App) tryExecuteBuiltInCommand(args *model.CommandArgs, trigger string, message string) (*model.Command, *model.CommandResponse) {
199+
provider := GetCommandProvider(trigger)
200+
if provider == nil {
201+
return nil, nil
202+
}
203+
204+
cmd := provider.GetCommand(a, args.T)
205+
if cmd == nil {
206+
return nil, nil
207+
}
208+
209+
return cmd, provider.DoCommand(a, args, message)
210+
}
211+
212+
// tryExecuteCustomCommand attempts to run a custom command based on the given arguments. If no such command can be
213+
// found, returns nil for all arguments.
214+
func (a *App) tryExecuteCustomCommand(args *model.CommandArgs, trigger string, message string) (*model.Command, *model.CommandResponse, *model.AppError) {
215+
// Handle custom commands
188216
if !*a.Config().ServiceSettings.EnableCommands {
189-
return nil, model.NewAppError("ExecuteCommand", "api.command.disabled.app_error", nil, "", http.StatusNotImplemented)
217+
return nil, nil, model.NewAppError("ExecuteCommand", "api.command.disabled.app_error", nil, "", http.StatusNotImplemented)
190218
}
191219

192220
chanChan := a.Srv.Store.Channel().Get(args.ChannelId, true)
@@ -195,98 +223,121 @@ func (a *App) ExecuteCommand(args *model.CommandArgs) (*model.CommandResponse, *
195223

196224
result := <-a.Srv.Store.Command().GetByTeam(args.TeamId)
197225
if result.Err != nil {
198-
return nil, result.Err
226+
return nil, nil, result.Err
199227
}
200228

201229
tr := <-teamChan
202230
if tr.Err != nil {
203-
return nil, tr.Err
231+
return nil, nil, tr.Err
204232
}
205233
team := tr.Data.(*model.Team)
206234

207235
ur := <-userChan
208236
if ur.Err != nil {
209-
return nil, ur.Err
237+
return nil, nil, ur.Err
210238
}
211239
user := ur.Data.(*model.User)
212240

213241
cr := <-chanChan
214242
if cr.Err != nil {
215-
return nil, cr.Err
243+
return nil, nil, cr.Err
216244
}
217245
channel := cr.Data.(*model.Channel)
218246

247+
var cmd *model.Command
248+
219249
teamCmds := result.Data.([]*model.Command)
220-
for _, cmd := range teamCmds {
221-
if trigger == cmd.Trigger {
222-
mlog.Debug(fmt.Sprintf(utils.T("api.command.execute_command.debug"), trigger, args.UserId))
250+
for _, teamCmd := range teamCmds {
251+
if trigger == teamCmd.Trigger {
252+
cmd = teamCmd
253+
}
254+
}
223255

224-
p := url.Values{}
225-
p.Set("token", cmd.Token)
256+
if cmd == nil {
257+
return nil, nil, nil
258+
}
226259

227-
p.Set("team_id", cmd.TeamId)
228-
p.Set("team_domain", team.Name)
260+
mlog.Debug(fmt.Sprintf(utils.T("api.command.execute_command.debug"), trigger, args.UserId))
229261

230-
p.Set("channel_id", args.ChannelId)
231-
p.Set("channel_name", channel.Name)
262+
p := url.Values{}
263+
p.Set("token", cmd.Token)
232264

233-
p.Set("user_id", args.UserId)
234-
p.Set("user_name", user.Username)
265+
p.Set("team_id", cmd.TeamId)
266+
p.Set("team_domain", team.Name)
235267

236-
p.Set("command", "/"+trigger)
237-
p.Set("text", message)
268+
p.Set("channel_id", args.ChannelId)
269+
p.Set("channel_name", channel.Name)
238270

239-
p.Set("trigger_id", triggerId)
271+
p.Set("user_id", args.UserId)
272+
p.Set("user_name", user.Username)
240273

241-
hook, appErr := a.CreateCommandWebhook(cmd.Id, args)
242-
if appErr != nil {
243-
return nil, model.NewAppError("command", "api.command.execute_command.failed.app_error", map[string]interface{}{"Trigger": trigger}, appErr.Error(), http.StatusInternalServerError)
244-
}
245-
p.Set("response_url", args.SiteURL+"/hooks/commands/"+hook.Id)
246-
247-
var req *http.Request
248-
if cmd.Method == model.COMMAND_METHOD_GET {
249-
req, _ = http.NewRequest(http.MethodGet, cmd.URL, nil)
250-
251-
if req.URL.RawQuery != "" {
252-
req.URL.RawQuery += "&"
253-
}
254-
req.URL.RawQuery += p.Encode()
255-
} else {
256-
req, _ = http.NewRequest(http.MethodPost, cmd.URL, strings.NewReader(p.Encode()))
257-
}
274+
p.Set("command", "/"+trigger)
275+
p.Set("text", message)
258276

259-
req.Header.Set("Accept", "application/json")
260-
req.Header.Set("Authorization", "Token "+cmd.Token)
261-
if cmd.Method == model.COMMAND_METHOD_POST {
262-
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
263-
}
277+
p.Set("trigger_id", args.TriggerId)
264278

265-
resp, err := a.HTTPService.MakeClient(false).Do(req)
266-
if err != nil {
267-
return nil, model.NewAppError("command", "api.command.execute_command.failed.app_error", map[string]interface{}{"Trigger": trigger}, err.Error(), http.StatusInternalServerError)
268-
}
269-
if resp.StatusCode != http.StatusOK {
270-
defer resp.Body.Close()
271-
body, _ := ioutil.ReadAll(resp.Body)
272-
return nil, model.NewAppError("command", "api.command.execute_command.failed_resp.app_error", map[string]interface{}{"Trigger": trigger, "Status": resp.Status}, string(body), http.StatusInternalServerError)
273-
}
279+
hook, appErr := a.CreateCommandWebhook(cmd.Id, args)
280+
if appErr != nil {
281+
return cmd, nil, model.NewAppError("command", "api.command.execute_command.failed.app_error", map[string]interface{}{"Trigger": trigger}, appErr.Error(), http.StatusInternalServerError)
282+
}
283+
p.Set("response_url", args.SiteURL+"/hooks/commands/"+hook.Id)
274284

275-
response, err := model.CommandResponseFromHTTPBody(resp.Header.Get("Content-Type"), resp.Body)
276-
if err != nil {
277-
return nil, model.NewAppError("command", "api.command.execute_command.failed.app_error", map[string]interface{}{"Trigger": trigger}, err.Error(), http.StatusInternalServerError)
278-
}
279-
if response == nil {
280-
return nil, model.NewAppError("command", "api.command.execute_command.failed_empty.app_error", map[string]interface{}{"Trigger": trigger}, "", http.StatusInternalServerError)
281-
}
285+
return a.doCommandRequest(cmd, p)
286+
}
282287

283-
response.TriggerId = clientTriggerId
288+
func (a *App) doCommandRequest(cmd *model.Command, p url.Values) (*model.Command, *model.CommandResponse, *model.AppError) {
289+
// Prepare the request
290+
var req *http.Request
291+
var err error
292+
if cmd.Method == model.COMMAND_METHOD_GET {
293+
req, err = http.NewRequest(http.MethodGet, cmd.URL, nil)
294+
} else {
295+
req, err = http.NewRequest(http.MethodPost, cmd.URL, strings.NewReader(p.Encode()))
296+
}
297+
298+
if err != nil {
299+
return cmd, nil, model.NewAppError("command", "api.command.execute_command.failed.app_error", map[string]interface{}{"Trigger": cmd.Trigger}, err.Error(), http.StatusInternalServerError)
300+
}
284301

285-
return a.HandleCommandResponse(cmd, args, response, false)
302+
if cmd.Method == model.COMMAND_METHOD_GET {
303+
if req.URL.RawQuery != "" {
304+
req.URL.RawQuery += "&"
286305
}
306+
req.URL.RawQuery += p.Encode()
287307
}
288308

289-
return nil, model.NewAppError("command", "api.command.execute_command.not_found.app_error", map[string]interface{}{"Trigger": trigger}, "", http.StatusNotFound)
309+
req.Header.Set("Accept", "application/json")
310+
req.Header.Set("Authorization", "Token "+cmd.Token)
311+
if cmd.Method == model.COMMAND_METHOD_POST {
312+
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
313+
}
314+
315+
// Send the request
316+
resp, err := a.HTTPService.MakeClient(false).Do(req)
317+
if err != nil {
318+
return cmd, nil, model.NewAppError("command", "api.command.execute_command.failed.app_error", map[string]interface{}{"Trigger": cmd.Trigger}, err.Error(), http.StatusInternalServerError)
319+
}
320+
321+
defer resp.Body.Close()
322+
323+
// Handle the response
324+
body := io.LimitReader(resp.Body, MaxIntegrationResponseSize)
325+
326+
if resp.StatusCode != http.StatusOK {
327+
// Ignore the error below because the resulting string will just be the empty string if bodyBytes is nil
328+
bodyBytes, _ := ioutil.ReadAll(body)
329+
330+
return cmd, nil, model.NewAppError("command", "api.command.execute_command.failed_resp.app_error", map[string]interface{}{"Trigger": cmd.Trigger, "Status": resp.Status}, string(bodyBytes), http.StatusInternalServerError)
331+
}
332+
333+
response, err := model.CommandResponseFromHTTPBody(resp.Header.Get("Content-Type"), body)
334+
if err != nil {
335+
return cmd, nil, model.NewAppError("command", "api.command.execute_command.failed.app_error", map[string]interface{}{"Trigger": cmd.Trigger}, err.Error(), http.StatusInternalServerError)
336+
} else if response == nil {
337+
return cmd, nil, model.NewAppError("command", "api.command.execute_command.failed_empty.app_error", map[string]interface{}{"Trigger": cmd.Trigger}, "", http.StatusInternalServerError)
338+
}
339+
340+
return cmd, response, nil
290341
}
291342

292343
func (a *App) HandleCommandResponse(command *model.Command, args *model.CommandArgs, response *model.CommandResponse, builtIn bool) (*model.CommandResponse, *model.AppError) {

‎app/command_test.go

+104
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,15 @@
44
package app
55

66
import (
7+
"io"
8+
"net/http"
9+
"net/http/httptest"
10+
"net/url"
11+
"strings"
712
"testing"
813

914
"github.com/stretchr/testify/assert"
15+
"github.com/stretchr/testify/require"
1016

1117
"github.com/mattermost/mattermost-server/model"
1218
)
@@ -65,3 +71,101 @@ func TestCreateCommandPost(t *testing.T) {
6571
t.Fatal("should have failed - bad post type")
6672
}
6773
}
74+
75+
func TestDoCommandRequest(t *testing.T) {
76+
th := Setup().InitBasic()
77+
defer th.TearDown()
78+
79+
th.App.UpdateConfig(func(cfg *model.Config) {
80+
cfg.ServiceSettings.AllowedUntrustedInternalConnections = model.NewString("127.0.0.1")
81+
cfg.ServiceSettings.EnableCommands = model.NewBool(true)
82+
})
83+
84+
t.Run("with a valid text response", func(t *testing.T) {
85+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
86+
io.Copy(w, strings.NewReader("Hello, World!"))
87+
}))
88+
defer server.Close()
89+
90+
_, resp, err := th.App.doCommandRequest(&model.Command{URL: server.URL}, url.Values{})
91+
require.Nil(t, err)
92+
93+
assert.NotNil(t, resp)
94+
assert.Equal(t, "Hello, World!", resp.Text)
95+
})
96+
97+
t.Run("with a valid json response", func(t *testing.T) {
98+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
99+
w.Header().Add("Content-Type", "application/json")
100+
101+
io.Copy(w, strings.NewReader(`{"text": "Hello, World!"}`))
102+
}))
103+
defer server.Close()
104+
105+
_, resp, err := th.App.doCommandRequest(&model.Command{URL: server.URL}, url.Values{})
106+
require.Nil(t, err)
107+
108+
assert.NotNil(t, resp)
109+
assert.Equal(t, "Hello, World!", resp.Text)
110+
})
111+
112+
t.Run("with a large text response", func(t *testing.T) {
113+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
114+
io.Copy(w, InfiniteReader{})
115+
}))
116+
defer server.Close()
117+
118+
// Since we limit the length of the response, no error will be returned and resp.Text will be a finite string
119+
120+
_, resp, err := th.App.doCommandRequest(&model.Command{URL: server.URL}, url.Values{})
121+
require.Nil(t, err)
122+
require.NotNil(t, resp)
123+
})
124+
125+
t.Run("with a large, valid json response", func(t *testing.T) {
126+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
127+
w.Header().Add("Content-Type", "application/json")
128+
129+
io.Copy(w, io.MultiReader(strings.NewReader(`{"text": "`), InfiniteReader{}, strings.NewReader(`"}`)))
130+
}))
131+
defer server.Close()
132+
133+
_, _, err := th.App.doCommandRequest(&model.Command{URL: server.URL}, url.Values{})
134+
require.NotNil(t, err)
135+
require.Equal(t, "api.command.execute_command.failed.app_error", err.Id)
136+
})
137+
138+
t.Run("with a large, invalid json response", func(t *testing.T) {
139+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
140+
w.Header().Add("Content-Type", "application/json")
141+
142+
io.Copy(w, InfiniteReader{})
143+
}))
144+
defer server.Close()
145+
146+
_, _, err := th.App.doCommandRequest(&model.Command{URL: server.URL}, url.Values{})
147+
require.NotNil(t, err)
148+
require.Equal(t, "api.command.execute_command.failed.app_error", err.Id)
149+
})
150+
151+
// // This test has been commented out because it relies on test logic only available in 5.8+
152+
// t.Run("with a slow response", func(t *testing.T) {
153+
// timeout := 100 * time.Millisecond
154+
155+
// server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
156+
// time.Sleep(timeout + time.Millisecond)
157+
// io.Copy(w, strings.NewReader(`{"text": "Hello, World!"}`))
158+
// }))
159+
// defer server.Close()
160+
161+
// th.App.HTTPService.(*httpservice.HTTPServiceImpl).RequestTimeout = timeout
162+
// defer func() {
163+
// th.App.HTTPService.(*httpservice.HTTPServiceImpl).RequestTimeout = httpservice.RequestTimeout
164+
// }()
165+
166+
// _, _, err := th.App.doCommandRequest(&model.Command{URL: server.URL}, url.Values{})
167+
// require.NotNil(t, err)
168+
// require.Equal(t, "api.command.execute_command.failed.app_error", err.Id)
169+
// })
170+
}
171+

‎app/http_service_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func TestMockHTTPService(t *testing.T) {
3838
client := th.App.HTTPService.MakeClient(false)
3939

4040
resp, err := client.Get(url + "/get")
41-
defer consumeAndClose(resp)
41+
defer resp.Body.Close()
4242

4343
bodyContents, _ := ioutil.ReadAll(resp.Body)
4444

@@ -53,7 +53,7 @@ func TestMockHTTPService(t *testing.T) {
5353

5454
request, _ := http.NewRequest(http.MethodPut, url+"/put", nil)
5555
resp, err := client.Do(request)
56-
defer consumeAndClose(resp)
56+
defer resp.Body.Close()
5757

5858
bodyContents, _ := ioutil.ReadAll(resp.Body)
5959

0 commit comments

Comments
 (0)
Failed to load comments.