Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(FIR-33629): Parsing structured error from response body #108

Merged
merged 2 commits into from
Jun 20, 2024
Merged

feat(FIR-33629): Parsing structured error from response body #108

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95503e0
Parsing structured error from response body
ptiurin Jun 19, 2024
fca680e
more efficient error parsing
ptiurin Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions client_base.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,13 @@ func request(
infolog.Println(err)
return response{nil, 0, nil, ConstructNestedError("error during reading a request response", err)}
}
// Error might be in the response body, despite the status code 200
var queryResponse QueryResponse
if err = json.Unmarshal(body, &queryResponse); err == nil {
if queryResponse.Errors != nil {
return response{nil, resp.StatusCode, nil, NewStructuredError(queryResponse.Errors)}
}
}

if !(resp.StatusCode >= 200 && resp.StatusCode < 300) {
if err = checkErrorResponse(body); err != nil {
Expand Down
19 changes: 19 additions & 0 deletions driver_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,3 +301,22 @@ func TestServiceAccountAuthentication(t *testing.T) {
t.Errorf("Authentication didn't return an error with correct message, got: %s", err.Error())
}
}

func TestIncorrectQueryThrowingStructuredError(t *testing.T) {
db, err := sql.Open("firebolt", dsnSystemEngineMock)
if err != nil {
t.Errorf("failed unexpectedly with %v", err)
}
_, err = db.Query("SET advanced_mode=1")
raiseIfError(t, err)
_, err = db.Query("SET enable_json_error_output_format=true")
raiseIfError(t, err)
_, err = db.Query("SELECT 'blue'::int")
if err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also verify that err is a StructuredError, otherwise it seems like plain error here whould also pass

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately in go our error class becomes masked due to the ConstructNestedError which converts the error to string before throwing it up the stack.
However, looking through the code other error paths prepend their own prefixes. e.g. request returned non ok status code if the error code is not 200 and request returned an error in some other cases. Essentially in this test by checking the exact prefix is error during query execution: error during query request: Cannot parse string we verify the error path here by the process of elimination.
I don't think we can do better here, unless we prepend a specific prefix similar to the above. Not sure if we should do it as our errors are already pretty verbose.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, I believe we can be sure that if engine returns a json string and we don't parse it, the test will fail. This test is only false positive in case the backend still returns a plaintext despite of the setting, which I believe we might not account for in our testing. We can probably leave it as is

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is only false positive in case the backend still returns a plaintext despite of the setting

I don't think it will, since the StructuredError will not be raised so the error will be raised elsewhere in the code e.g. here we prepend the request returned non ok status code and in the test we specifically check for a prefix that doesn't contain this string. Therefore I believe the plain error will make this test fail.
In fact, I've removed the set parameters, run it and got an error:

--- FAIL: TestIncorrectQueryThrowingStructuredError (0.11s)
    /Users/petrotiurin/Development/firebolt-go-sdk/driver_integration_test.go:320: Query didn't return an error with correct message, got: error during query execution: error during query request: request returned non ok status code: 400, Cannot parse string 'blue' as integer: syntax error at beginning of string
FAIL

t.Errorf("Query didn't return an error, although it should")
}

if !strings.HasPrefix(err.Error(), "error during query execution: error during query request: Cannot parse string 'blue' as integer") {
t.Errorf("Query didn't return an error with correct message, got: %s", err.Error())
}
}
18 changes: 18 additions & 0 deletions statement.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,29 @@ type Column struct {
Type string `json:"type"`
}

type Location struct {
FailingLine int `json:"failingLine"`
StartOffset int `json:"startOffset"`
EndOffset int `json:"endOffset"`
}

type ErrorDetails struct {
Code string `json:"code"`
Name string `json:"name"`
Severity string `json:"severity"`
Source string `json:"source"`
Description string `json:"description"`
Resolution string `json:"resolution"`
HelpLink string `json:"helpLink"`
Location Location `json:"location"`
}

type QueryResponse struct {
Query interface{} `json:"query"`
Meta []Column `json:"meta"`
Data [][]interface{} `json:"data"`
Rows int `json:"rows"`
Errors []ErrorDetails `json:"errors"`
Statistics interface{} `json:"statistics"`
}

Expand Down
61 changes: 61 additions & 0 deletions utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,3 +216,64 @@ func valueToNamedValue(args []driver.Value) []driver.NamedValue {
}
return namedValues
}

type StructuredError struct {
Message string
}

func (e StructuredError) Error() string {
return e.Message
}

func NewStructuredError(errorDetails []ErrorDetails) *StructuredError {
// "{severity}: {name} ({code}) - {source}, {description}, resolution: {resolution} at {location} see {helpLink}"
message := strings.Builder{}
for _, error := range errorDetails {
if message.Len() > 0 {
message.WriteString("\n")
}
formatErrorDetails(&message, error)
}
return &StructuredError{
Message: message.String(),
}
}
func formatErrorDetails(message *strings.Builder, error ErrorDetails) string {
if error.Severity != "" {
message.WriteString(fmt.Sprintf("%s: ", error.Severity))
}
if error.Name != "" {
message.WriteString(fmt.Sprintf("%s ", error.Name))
}
if error.Code != "" {
message.WriteString(fmt.Sprintf("(%s) ", error.Code))
}
if error.Description != "" {
addDelimiterIfNotEmpty(message, "-")
message.WriteString(error.Description)
}
if error.Source != "" {
addDelimiterIfNotEmpty(message, ",")
message.WriteString(error.Source)
}
if error.Resolution != "" {
addDelimiterIfNotEmpty(message, ",")
message.WriteString(fmt.Sprintf("resolution: %s", error.Resolution))
}
if error.Location.FailingLine != 0 || error.Location.StartOffset != 0 || error.Location.EndOffset != 0 {
addDelimiterIfNotEmpty(message, " at")
message.WriteString(fmt.Sprintf("%+v", error.Location))
}
if error.HelpLink != "" {
addDelimiterIfNotEmpty(message, ",")
message.WriteString(fmt.Sprintf("see %s", error.HelpLink))
}
return message.String()
}

func addDelimiterIfNotEmpty(message *strings.Builder, delimiter string) {
if message.Len() > 0 {
message.WriteString(delimiter)
message.WriteString(" ")
}
}
75 changes: 75 additions & 0 deletions utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,3 +196,78 @@ func TestValueToNamedValue(t *testing.T) {
assert(namedValues[0].Value, 2, t, "namedValues value is wrong")
assert(namedValues[1].Value, "string", t, "namedValues value is wrong")
}
func TestNewStructuredError(t *testing.T) {
errorDetails := ErrorDetails{
Severity: "error",
Name: "TestError",
Code: "123",
Description: "This is a test error",
Source: "TestSource",
Resolution: "Please fix the error",
Location: Location{
FailingLine: 10,
StartOffset: 20,
EndOffset: 30,
},
HelpLink: "https://example.com",
}

expectedMessage := "error: TestError (123) - This is a test error, TestSource, resolution: Please fix the error at {FailingLine:10 StartOffset:20 EndOffset:30}, see https://example.com"

err := NewStructuredError([]ErrorDetails{errorDetails})

if err.Message != expectedMessage {
t.Errorf("NewStructuredError returned incorrect error message, got: %s, want: %s", err.Message, expectedMessage)
}
}

func TestStructuredErrorWithMissingFields(t *testing.T) {
errorDetails := ErrorDetails{
Severity: "error",
Name: "TestError",
Code: "123",
Description: "This is a test error",
}

expectedMessage := "error: TestError (123) - This is a test error"

err := NewStructuredError([]ErrorDetails{errorDetails})

if err.Message != expectedMessage {
t.Errorf("NewStructuredError returned incorrect error message, got: %s, want: %s", err.Message, expectedMessage)
}
}

func TestStructuredErrorWithMultipleErrors(t *testing.T) {
errorDetails := ErrorDetails{
Severity: "error",
Name: "TestError",
Code: "123",
Description: "This is a test error",
Source: "TestSource",
Resolution: "Please fix the error",
Location: Location{
FailingLine: 10,
StartOffset: 20,
EndOffset: 30,
},
HelpLink: "https://example.com",
}

errorDetails2 := ErrorDetails{
Severity: "error",
Name: "TestError",
Code: "123",
Description: "This is a test error",
Source: "TestSource",
Resolution: "Please fix the error",
}

expectedMessage := "error: TestError (123) - This is a test error, TestSource, resolution: Please fix the error at {FailingLine:10 StartOffset:20 EndOffset:30}, see https://example.com\nerror: TestError (123) - This is a test error, TestSource, resolution: Please fix the error"

err := NewStructuredError([]ErrorDetails{errorDetails, errorDetails2})

if err.Message != expectedMessage {
t.Errorf("NewStructuredError returned incorrect error message, got: %s, want: %s", err.Message, expectedMessage)
}
}
Loading