From ae854cd1396c37dbf52806c22a8f3dd5485fe29b Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Mon, 11 Mar 2024 12:29:25 -0400 Subject: [PATCH 1/9] fix jira error parsing --- integrations/access/jira/client.go | 2 +- integrations/access/jira/types.go | 66 +++++++++++++++++++++++++- integrations/access/jira/types_test.go | 64 +++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 integrations/access/jira/types_test.go diff --git a/integrations/access/jira/client.go b/integrations/access/jira/client.go index e5071aca2c484..f33c33e14c83e 100644 --- a/integrations/access/jira/client.go +++ b/integrations/access/jira/client.go @@ -128,7 +128,7 @@ func NewJiraClient(conf JiraConfig, clusterName, teleportProxyAddr string, statu if resp.IsError() { switch result := resp.Error().(type) { case *ErrorResult: - return trace.Errorf("http error code=%v, errors=[%v]", resp.StatusCode(), strings.Join(result.ErrorMessages, ", ")) + return trace.Errorf("http error code=%v, errors=[%s]", resp.StatusCode(), result) case nil: return nil default: diff --git a/integrations/access/jira/types.go b/integrations/access/jira/types.go index 609999cb2f3b8..a69ce827118cc 100644 --- a/integrations/access/jira/types.go +++ b/integrations/access/jira/types.go @@ -16,11 +16,75 @@ limitations under the License. package jira +import ( + "encoding/json" + "fmt" + "github.com/gravitational/trace" + "strings" +) + // Jira REST API resources +// ErrorResult is used to parse the errors from Jira. +// The JSON Schema is specified here: +// https://docs.atlassian.com/software/jira/docs/api/REST/1000.1223.0/#error-responses +// However JIRA does not consistently respect the schema (especially for old instances). +// We need to support legacy errors as well (array of strings). type ErrorResult struct { ErrorMessages []string `url:"errorMessages"` - Errors []string `url:"errors"` + Errors Errors `url:"errors"` +} + +func (e ErrorResult) String() string { + sb := strings.Builder{} + if len(e.ErrorMessages) > 0 { + sb.WriteString(fmt.Sprintf("error messages: %s ", e.ErrorMessages)) + } + if details := e.Errors.String(); details != "" { + sb.WriteString(fmt.Sprintf("error details: %s", details)) + } + result := sb.String() + if result == "" { + return "Unknown error" + } + return result +} + +// Errors are used to unmarshall inconsistently formatted Jira errors. +type Errors struct { + Errors map[string]string + LegacyErrors []string +} + +func (e *Errors) UnmarshalJSON(data []byte) error { + // Try to parse as a new error + var errors map[string]string + if err := json.Unmarshal(data, &errors); err == nil { + e.Errors = errors + return nil + } + + // Try to parse as a legacy error + var legacyErrors []string + if err := json.Unmarshal(data, &legacyErrors); err == nil { + e.LegacyErrors = legacyErrors + return nil + } + + // Everything failed, we return an unrmarshalling error that contains the data. + // This way, even if everything failed, the user still has the original response in the logs. + return trace.Errorf("Failed to unmarshall Jira error: %q", string(data)) +} + +func (e Errors) String() string { + switch { + case len(e.Errors) > 0: + return fmt.Sprintf("%s", e.Errors) + case len(e.LegacyErrors) > 0: + return fmt.Sprintf("%s", e.LegacyErrors) + default: + return "" + } } type GetMyPermissionsQueryOptions struct { diff --git a/integrations/access/jira/types_test.go b/integrations/access/jira/types_test.go new file mode 100644 index 0000000000000..46b77a27b7809 --- /dev/null +++ b/integrations/access/jira/types_test.go @@ -0,0 +1,64 @@ +package jira + +import ( + "encoding/json" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/stretchr/testify/require" + "testing" +) + +func TestErrorsUnmarshall(t *testing.T) { + t.Parallel() + tests := []struct { + name string + input string + expectedOutput ErrorResult + assertErr require.ErrorAssertionFunc + }{ + { + name: "new error", + input: `{"errorMessages":[], "errors": {"project": "project is required"}}`, + expectedOutput: ErrorResult{ + Errors: Errors{Errors: map[string]string{"project": "project is required"}}, + }, + assertErr: require.NoError, + }, + { + name: "legacy error", + input: `{"errorMessages":["foo"],"errors":["bar", "baz"]}`, + expectedOutput: ErrorResult{ + ErrorMessages: []string{"foo"}, + Errors: Errors{LegacyErrors: []string{"bar", "baz"}}, + }, + assertErr: require.NoError, + }, + { + name: "empty error", + input: `{"errorMessages":["There was an error parsing JSON. Check that your request body is valid."]}`, + expectedOutput: ErrorResult{ + ErrorMessages: []string{"There was an error parsing JSON. Check that your request body is valid."}, + }, + assertErr: require.NoError, + }, + { + name: "malformed error", + input: `{"errorMessages":["Foo"],"errors":"This is a single string"}`, + expectedOutput: ErrorResult{ErrorMessages: []string{"Foo"}}, + assertErr: func(t require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, "This is a single string") + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt := tt + t.Parallel() + data := []byte(tt.input) + var result ErrorResult + tt.assertErr(t, json.Unmarshal(data, &result)) + diff := cmp.Diff(tt.expectedOutput, result, cmpopts.EquateEmpty()) + require.Empty(t, diff) + }) + } +} From d295456556eca480525c9b4a06be3ac283b43c64 Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Mon, 11 Mar 2024 14:27:15 -0400 Subject: [PATCH 2/9] lint --- integrations/access/jira/types.go | 3 ++- integrations/access/jira/types_test.go | 21 ++++++++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/integrations/access/jira/types.go b/integrations/access/jira/types.go index a69ce827118cc..d63afab4f76f5 100644 --- a/integrations/access/jira/types.go +++ b/integrations/access/jira/types.go @@ -19,8 +19,9 @@ package jira import ( "encoding/json" "fmt" - "github.com/gravitational/trace" "strings" + + "github.com/gravitational/trace" ) // Jira REST API resources diff --git a/integrations/access/jira/types_test.go b/integrations/access/jira/types_test.go index 46b77a27b7809..d38046f004968 100644 --- a/integrations/access/jira/types_test.go +++ b/integrations/access/jira/types_test.go @@ -1,11 +1,30 @@ +/* + * Teleport + * Copyright (C) 2023 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + package jira import ( "encoding/json" + "testing" + "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/require" - "testing" ) func TestErrorsUnmarshall(t *testing.T) { From 8fc31ec5110056d7885d3a6003bca23e735f20d4 Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Mon, 11 Mar 2024 15:58:07 -0400 Subject: [PATCH 3/9] Address zac's feedback: imprement error + rename structs --- integrations/access/jira/types.go | 21 ++++++++++++--------- integrations/access/jira/types_test.go | 6 +++--- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/integrations/access/jira/types.go b/integrations/access/jira/types.go index d63afab4f76f5..a2d690da2bce7 100644 --- a/integrations/access/jira/types.go +++ b/integrations/access/jira/types.go @@ -32,32 +32,35 @@ import ( // However JIRA does not consistently respect the schema (especially for old instances). // We need to support legacy errors as well (array of strings). type ErrorResult struct { - ErrorMessages []string `url:"errorMessages"` - Errors Errors `url:"errors"` + ErrorMessages []string `url:"errorMessages"` + Details ErrorDetails `url:"errors"` } -func (e ErrorResult) String() string { +// Error implements the error interface and returns a string describing the +// error returned by Jira. +func (e ErrorResult) Error() string { sb := strings.Builder{} if len(e.ErrorMessages) > 0 { sb.WriteString(fmt.Sprintf("error messages: %s ", e.ErrorMessages)) } - if details := e.Errors.String(); details != "" { + if details := e.Details.String(); details != "" { sb.WriteString(fmt.Sprintf("error details: %s", details)) } result := sb.String() if result == "" { - return "Unknown error" + return "Unknown jira error" } return result } -// Errors are used to unmarshall inconsistently formatted Jira errors. -type Errors struct { +// ErrorDetails are used to unmarshall inconsistently formatted Jira errors +// details. +type ErrorDetails struct { Errors map[string]string LegacyErrors []string } -func (e *Errors) UnmarshalJSON(data []byte) error { +func (e *ErrorDetails) UnmarshalJSON(data []byte) error { // Try to parse as a new error var errors map[string]string if err := json.Unmarshal(data, &errors); err == nil { @@ -77,7 +80,7 @@ func (e *Errors) UnmarshalJSON(data []byte) error { return trace.Errorf("Failed to unmarshall Jira error: %q", string(data)) } -func (e Errors) String() string { +func (e ErrorDetails) String() string { switch { case len(e.Errors) > 0: return fmt.Sprintf("%s", e.Errors) diff --git a/integrations/access/jira/types_test.go b/integrations/access/jira/types_test.go index d38046f004968..8bfb02b72b8f8 100644 --- a/integrations/access/jira/types_test.go +++ b/integrations/access/jira/types_test.go @@ -27,7 +27,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestErrorsUnmarshall(t *testing.T) { +func TestErrorResultUnmarshall(t *testing.T) { t.Parallel() tests := []struct { name string @@ -39,7 +39,7 @@ func TestErrorsUnmarshall(t *testing.T) { name: "new error", input: `{"errorMessages":[], "errors": {"project": "project is required"}}`, expectedOutput: ErrorResult{ - Errors: Errors{Errors: map[string]string{"project": "project is required"}}, + Details: ErrorDetails{Errors: map[string]string{"project": "project is required"}}, }, assertErr: require.NoError, }, @@ -48,7 +48,7 @@ func TestErrorsUnmarshall(t *testing.T) { input: `{"errorMessages":["foo"],"errors":["bar", "baz"]}`, expectedOutput: ErrorResult{ ErrorMessages: []string{"foo"}, - Errors: Errors{LegacyErrors: []string{"bar", "baz"}}, + Details: ErrorDetails{LegacyErrors: []string{"bar", "baz"}}, }, assertErr: require.NoError, }, From 1956d94ab55c5b4d0f48d4fc4c99afd215918466 Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Mon, 11 Mar 2024 16:44:49 -0400 Subject: [PATCH 4/9] Update integrations/access/jira/types_test.go Co-authored-by: Zac Bergquist --- integrations/access/jira/types_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integrations/access/jira/types_test.go b/integrations/access/jira/types_test.go index 8bfb02b72b8f8..2e414c33dd11f 100644 --- a/integrations/access/jira/types_test.go +++ b/integrations/access/jira/types_test.go @@ -27,7 +27,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestErrorResultUnmarshall(t *testing.T) { +func TestErrorResultUnmarshal(t *testing.T) { t.Parallel() tests := []struct { name string From 67e5224abbd0c343905d15318d3b0fbac61386c5 Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Tue, 12 Mar 2024 09:48:17 -0400 Subject: [PATCH 5/9] Apply suggestions from code review Co-authored-by: Roman Tkachenko --- integrations/access/jira/types.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integrations/access/jira/types.go b/integrations/access/jira/types.go index a2d690da2bce7..6b9b9bb474d73 100644 --- a/integrations/access/jira/types.go +++ b/integrations/access/jira/types.go @@ -48,7 +48,7 @@ func (e ErrorResult) Error() string { } result := sb.String() if result == "" { - return "Unknown jira error" + return "Unknown Jira error" } return result } @@ -77,7 +77,7 @@ func (e *ErrorDetails) UnmarshalJSON(data []byte) error { // Everything failed, we return an unrmarshalling error that contains the data. // This way, even if everything failed, the user still has the original response in the logs. - return trace.Errorf("Failed to unmarshall Jira error: %q", string(data)) + return trace.Errorf("Failed to unmarshal Jira error: %q", string(data)) } func (e ErrorDetails) String() string { From 562ae87f38ffcf2142756f5d6861d1f8ae319244 Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Tue, 12 Mar 2024 18:05:41 -0400 Subject: [PATCH 6/9] address feedback --- integrations/access/jira/types.go | 23 +++++++++++++++-------- integrations/access/jira/types_test.go | 4 ++-- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/integrations/access/jira/types.go b/integrations/access/jira/types.go index 6b9b9bb474d73..3760d2735b399 100644 --- a/integrations/access/jira/types.go +++ b/integrations/access/jira/types.go @@ -56,22 +56,29 @@ func (e ErrorResult) Error() string { // ErrorDetails are used to unmarshall inconsistently formatted Jira errors // details. type ErrorDetails struct { - Errors map[string]string - LegacyErrors []string + // errors contain object-formatted Jira errors. Usually Jira returns + // errors in an object where keys are single word representing what is + // broken, and values containing text descrption of the issue. + // This is the official return format, according to Jira's docs. + errors map[string]string + // legacyErrors ensures backward compatibility with Jira errors returned as + // a list. It's unclear wich Jira version and which part of jira can return + // such errors, but they existed at some point, and we might still get them. + legacyErrors []string } func (e *ErrorDetails) UnmarshalJSON(data []byte) error { // Try to parse as a new error var errors map[string]string if err := json.Unmarshal(data, &errors); err == nil { - e.Errors = errors + e.errors = errors return nil } // Try to parse as a legacy error var legacyErrors []string if err := json.Unmarshal(data, &legacyErrors); err == nil { - e.LegacyErrors = legacyErrors + e.legacyErrors = legacyErrors return nil } @@ -82,10 +89,10 @@ func (e *ErrorDetails) UnmarshalJSON(data []byte) error { func (e ErrorDetails) String() string { switch { - case len(e.Errors) > 0: - return fmt.Sprintf("%s", e.Errors) - case len(e.LegacyErrors) > 0: - return fmt.Sprintf("%s", e.LegacyErrors) + case len(e.errors) > 0: + return fmt.Sprintf("%s", e.errors) + case len(e.legacyErrors) > 0: + return fmt.Sprintf("%s", e.legacyErrors) default: return "" } diff --git a/integrations/access/jira/types_test.go b/integrations/access/jira/types_test.go index 2e414c33dd11f..2e00fc287fb22 100644 --- a/integrations/access/jira/types_test.go +++ b/integrations/access/jira/types_test.go @@ -39,7 +39,7 @@ func TestErrorResultUnmarshal(t *testing.T) { name: "new error", input: `{"errorMessages":[], "errors": {"project": "project is required"}}`, expectedOutput: ErrorResult{ - Details: ErrorDetails{Errors: map[string]string{"project": "project is required"}}, + Details: ErrorDetails{errors: map[string]string{"project": "project is required"}}, }, assertErr: require.NoError, }, @@ -48,7 +48,7 @@ func TestErrorResultUnmarshal(t *testing.T) { input: `{"errorMessages":["foo"],"errors":["bar", "baz"]}`, expectedOutput: ErrorResult{ ErrorMessages: []string{"foo"}, - Details: ErrorDetails{LegacyErrors: []string{"bar", "baz"}}, + Details: ErrorDetails{legacyErrors: []string{"bar", "baz"}}, }, assertErr: require.NoError, }, From 18acb51960544779f6998f7e1851e3b43f96fced Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Tue, 12 Mar 2024 18:12:44 -0400 Subject: [PATCH 7/9] Update integrations/access/jira/types.go --- integrations/access/jira/types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integrations/access/jira/types.go b/integrations/access/jira/types.go index 3760d2735b399..61892a1c25d12 100644 --- a/integrations/access/jira/types.go +++ b/integrations/access/jira/types.go @@ -62,7 +62,7 @@ type ErrorDetails struct { // This is the official return format, according to Jira's docs. errors map[string]string // legacyErrors ensures backward compatibility with Jira errors returned as - // a list. It's unclear wich Jira version and which part of jira can return + // a list. It's unclear which Jira version and which part of jira can return // such errors, but they existed at some point, and we might still get them. legacyErrors []string } From d825191b97ea8dec927b155c695c59f970f11aaa Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Wed, 13 Mar 2024 11:03:57 -0400 Subject: [PATCH 8/9] address feedback + fix tests --- integrations/access/jira/types.go | 26 +++++++++++++------------- integrations/access/jira/types_test.go | 4 ++-- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/integrations/access/jira/types.go b/integrations/access/jira/types.go index 61892a1c25d12..b72fed6ccb612 100644 --- a/integrations/access/jira/types.go +++ b/integrations/access/jira/types.go @@ -41,7 +41,7 @@ type ErrorResult struct { func (e ErrorResult) Error() string { sb := strings.Builder{} if len(e.ErrorMessages) > 0 { - sb.WriteString(fmt.Sprintf("error messages: %s ", e.ErrorMessages)) + sb.WriteString(fmt.Sprintf("error messages: %s ", strings.Join(e.ErrorMessages, ";"))) } if details := e.Details.String(); details != "" { sb.WriteString(fmt.Sprintf("error details: %s", details)) @@ -56,29 +56,29 @@ func (e ErrorResult) Error() string { // ErrorDetails are used to unmarshall inconsistently formatted Jira errors // details. type ErrorDetails struct { - // errors contain object-formatted Jira errors. Usually Jira returns + // Errors contain object-formatted Jira Errors. Usually Jira returns // errors in an object where keys are single word representing what is - // broken, and values containing text descrption of the issue. + // broken, and values containing text description of the issue. // This is the official return format, according to Jira's docs. - errors map[string]string - // legacyErrors ensures backward compatibility with Jira errors returned as - // a list. It's unclear which Jira version and which part of jira can return + Errors map[string]string + // LegacyErrors ensures backward compatibility with Jira errors returned as + // a list. It's unclear which Jira version and which part of Jira can return // such errors, but they existed at some point, and we might still get them. - legacyErrors []string + LegacyErrors []string } func (e *ErrorDetails) UnmarshalJSON(data []byte) error { // Try to parse as a new error var errors map[string]string if err := json.Unmarshal(data, &errors); err == nil { - e.errors = errors + e.Errors = errors return nil } // Try to parse as a legacy error var legacyErrors []string if err := json.Unmarshal(data, &legacyErrors); err == nil { - e.legacyErrors = legacyErrors + e.LegacyErrors = legacyErrors return nil } @@ -89,10 +89,10 @@ func (e *ErrorDetails) UnmarshalJSON(data []byte) error { func (e ErrorDetails) String() string { switch { - case len(e.errors) > 0: - return fmt.Sprintf("%s", e.errors) - case len(e.legacyErrors) > 0: - return fmt.Sprintf("%s", e.legacyErrors) + case len(e.Errors) > 0: + return fmt.Sprintf("%s", e.Errors) + case len(e.LegacyErrors) > 0: + return fmt.Sprintf("%s", e.LegacyErrors) default: return "" } diff --git a/integrations/access/jira/types_test.go b/integrations/access/jira/types_test.go index 2e00fc287fb22..2e414c33dd11f 100644 --- a/integrations/access/jira/types_test.go +++ b/integrations/access/jira/types_test.go @@ -39,7 +39,7 @@ func TestErrorResultUnmarshal(t *testing.T) { name: "new error", input: `{"errorMessages":[], "errors": {"project": "project is required"}}`, expectedOutput: ErrorResult{ - Details: ErrorDetails{errors: map[string]string{"project": "project is required"}}, + Details: ErrorDetails{Errors: map[string]string{"project": "project is required"}}, }, assertErr: require.NoError, }, @@ -48,7 +48,7 @@ func TestErrorResultUnmarshal(t *testing.T) { input: `{"errorMessages":["foo"],"errors":["bar", "baz"]}`, expectedOutput: ErrorResult{ ErrorMessages: []string{"foo"}, - Details: ErrorDetails{legacyErrors: []string{"bar", "baz"}}, + Details: ErrorDetails{LegacyErrors: []string{"bar", "baz"}}, }, assertErr: require.NoError, }, From dce4741b66db59a77bf9499025c2dbefa0b12237 Mon Sep 17 00:00:00 2001 From: Hugo Shaka Date: Wed, 13 Mar 2024 13:28:22 -0400 Subject: [PATCH 9/9] fix unmarshal --- integrations/access/jira/types.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integrations/access/jira/types.go b/integrations/access/jira/types.go index b72fed6ccb612..617bb9055804c 100644 --- a/integrations/access/jira/types.go +++ b/integrations/access/jira/types.go @@ -32,8 +32,8 @@ import ( // However JIRA does not consistently respect the schema (especially for old instances). // We need to support legacy errors as well (array of strings). type ErrorResult struct { - ErrorMessages []string `url:"errorMessages"` - Details ErrorDetails `url:"errors"` + ErrorMessages []string `url:"errorMessages" json:"errorMessages"` + Details ErrorDetails `url:"errors" json:"errors"` } // Error implements the error interface and returns a string describing the