Fix error message panic in command result parsing (#502)

* Fix error message panic in command result parsing * split out error message parsing in own method * add tests for error message parsing
databricks · Feb 11, 2021 · 0e4a7f8 · 0e4a7f8
1 parent 67c7713
commit 0e4a7f8
Show file tree

Hide file tree

Showing 2 changed files with 130 additions and 26 deletions.
diff --git a/compute/commands.go b/compute/commands.go
@@ -22,8 +22,10 @@ var (
 	tagRE = regexp.MustCompile(`<[^>]*>`)
 	// just exception content without exception name
 	exceptionRE = regexp.MustCompile(`.*Exception: (.*)`)
+	// execution errors resulting from http errors are sometimes hidden in these keys
+	executionErrorRE = regexp.MustCompile(`ExecutionError: ([\s\S]*)\n(StatusCode=[0-9]*)\n(StatusDescription=.*)\n`)
 	// usual error message explanation is hidden in this key
-	errorMessageRE = regexp.MustCompile(`ErrorMessage=(.*)\n`)
+	errorMessageRE = regexp.MustCompile(`ErrorMessage=(.+)\n`)
 )
 
 // NewCommandsAPI creates CommandsAPI instance from provider meta
@@ -91,27 +93,37 @@ func (a CommandsAPI) parseCommandResults(command Command) (result string, err er
 		return
 	case "error":
 		log.Printf("[DEBUG] error caused by command: %s", command.Results.Cause)
-		summary := tagRE.ReplaceAllLiteralString(command.Results.Summary, "")
-		summary = html.UnescapeString(summary)
-		exceptionMatches := exceptionRE.FindStringSubmatch(summary)
-		if len(exceptionMatches) == 2 {
-			summary = strings.ReplaceAll(exceptionMatches[1], "; nested exception is:", "")
-			summary = strings.TrimRight(summary, " ")
-			err = errors.New(summary)
-			return
-		}
-		errorMessageMatches := errorMessageRE.FindStringSubmatch(command.Results.Cause)
-		if len(errorMessageMatches) == 2 {
-			err = errors.New(errorMessageMatches[1])
-			return
-		}
-		err = errors.New(summary)
+		err = a.getCommandResultErrorMessage(command)
 		return
 	}
 	err = fmt.Errorf("Unknown result type %s: %v", command.Results.ResultType, command.Results.Data)
 	return
 }
 
+func (a CommandsAPI) getCommandResultErrorMessage(command Command) error {
+	summary := tagRE.ReplaceAllLiteralString(command.Results.Summary, "")
+	summary = html.UnescapeString(summary)
+
+	exceptionMatches := exceptionRE.FindStringSubmatch(summary)
+	if len(exceptionMatches) == 2 {
+		summary = strings.ReplaceAll(exceptionMatches[1], "; nested exception is:", "")
+		summary = strings.TrimRight(summary, " ")
+		return errors.New(summary)
+	}
+
+	executionErrorMatches := executionErrorRE.FindStringSubmatch(command.Results.Cause)
+	if len(executionErrorMatches) == 4 {
+		return errors.New(strings.Join(executionErrorMatches[1:], "\n"))
+	}
+
+	errorMessageMatches := errorMessageRE.FindStringSubmatch(command.Results.Cause)
+	if len(errorMessageMatches) == 2 {
+		return errors.New(errorMessageMatches[1])
+	}
+
+	return errors.New(summary)
+}
+
 type genericCommandRequest struct {
 	CommandID string `json:"commandId,omitempty" url:"commandId,omitempty"`
 	Language  string `json:"language,omitempty" url:"language,omitempty"`

diff --git a/compute/commands_test.go b/compute/commands_test.go
@@ -14,8 +14,8 @@ import (
 // Test interface compliance
 var _ common.CommandExecutor = (*CommandsAPI)(nil)
 
-func TestSomeCommands(t *testing.T) {
-	client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{
+func commonFixtureWithStatusResponse(response Command) []qa.HTTPFixture {
+	return []qa.HTTPFixture{
 		{
 			Method:       "GET",
 			ReuseRequest: true,
@@ -66,13 +66,7 @@ func TestSomeCommands(t *testing.T) {
 			Method:       "GET",
 			ReuseRequest: true,
 			Resource:     "/api/1.2/commands/status?clusterId=abc&commandId=234&contextId=123",
-			Response: Command{
-				Status: "Finished",
-				Results: &CommandResults{
-					ResultType: "text",
-					Data:       "done",
-				},
-			},
+			Response:     response,
 		},
 		{
 			Method:   "POST",
@@ -82,7 +76,105 @@ func TestSomeCommands(t *testing.T) {
 				ContextID: "123",
 			},
 		},
-	})
+	}
+}
+
+func TestCommandWithExecutionError(t *testing.T) {
+	client, server, err := qa.HttpFixtureClient(t, commonFixtureWithStatusResponse(Command{
+		Status: "Finished",
+		Results: &CommandResults{
+			ResultType: "error",
+			Cause: `
+---
+ExecutionError: An error occurred
+StatusCode=400
+StatusDescription=BadRequest
+---
+			`,
+		},
+	}))
+	defer server.Close()
+	require.NoError(t, err)
+	ctx := context.Background()
+	commands := NewCommandsAPI(ctx, client)
+
+	_, err = commands.Execute("abc", "python", `print("done")`)
+	assert.Equal(t, `An error occurred
+StatusCode=400
+StatusDescription=BadRequest`, err.Error())
+}
+
+func TestCommandWithEmptyErrorMessageUsesSummary(t *testing.T) {
+	client, server, err := qa.HttpFixtureClient(t, commonFixtureWithStatusResponse(Command{
+		Status: "Finished",
+		Results: &CommandResults{
+			ResultType: "error",
+			Cause: `
+---
+ErrorCode=
+ErrorMessage=
+    other text
+---
+			`,
+			Summary: "Proper error",
+		},
+	}))
+	defer server.Close()
+	require.NoError(t, err)
+	ctx := context.Background()
+	commands := NewCommandsAPI(ctx, client)
+
+	_, err = commands.Execute("abc", "python", `print("done")`)
+	assert.Equal(t, "Proper error", err.Error())
+}
+
+func TestCommandWithErrorMessage(t *testing.T) {
+	client, server, err := qa.HttpFixtureClient(t, commonFixtureWithStatusResponse(Command{
+		Status: "Finished",
+		Results: &CommandResults{
+			ResultType: "error",
+			Cause: `
+---
+ErrorCode=
+ErrorMessage=An error occurred
+---
+			`,
+		},
+	}))
+	defer server.Close()
+	require.NoError(t, err)
+	ctx := context.Background()
+	commands := NewCommandsAPI(ctx, client)
+
+	_, err = commands.Execute("abc", "python", `print("done")`)
+	assert.Equal(t, "An error occurred", err.Error())
+}
+
+func TestCommandWithExceptionMessage(t *testing.T) {
+	client, server, err := qa.HttpFixtureClient(t, commonFixtureWithStatusResponse(Command{
+		Status: "Finished",
+		Results: &CommandResults{
+			ResultType: "error",
+			Summary:    "Exception: An error occurred",
+		},
+	}))
+	defer server.Close()
+	require.NoError(t, err)
+	ctx := context.Background()
+	commands := NewCommandsAPI(ctx, client)
+
+	_, err = commands.Execute("abc", "python", `print("done")`)
+	assert.Equal(t, "An error occurred", err.Error())
+}
+
+func TestSomeCommands(t *testing.T) {
+	client, server, err := qa.HttpFixtureClient(t, commonFixtureWithStatusResponse(Command{
+		Status: "Finished",
+		Results: &CommandResults{
+			ResultType: "text",
+			Data:       "done",
+		},
+	}))
 	defer server.Close()
 	require.NoError(t, err)
 	ctx := context.Background()