From 336d48c778c384696db6aa410c69cf208fb655a0 Mon Sep 17 00:00:00 2001 From: pranav-new-relic Date: Thu, 30 Mar 2023 19:03:22 +0530 Subject: [PATCH] fix(apiaccess): corrects incorrect errors thrown by failing API Access Key creation --- pkg/apiaccess/keys.go | 40 ++++++--- pkg/apiaccess/keys_integration_test.go | 116 ++++++++++++++++++++++++- pkg/apiaccess/types.go | 10 +++ pkg/testhelpers/helpers.go | 6 ++ 4 files changed, 158 insertions(+), 14 deletions(-) diff --git a/pkg/apiaccess/keys.go b/pkg/apiaccess/keys.go index e2b7641a..c2ff5e38 100644 --- a/pkg/apiaccess/keys.go +++ b/pkg/apiaccess/keys.go @@ -3,7 +3,7 @@ package apiaccess import ( "context" "errors" - + "fmt" "github.com/newrelic/newrelic-client-go/v2/internal/http" ) @@ -145,10 +145,28 @@ func (a *APIAccess) DeleteAPIAccessKeyWithContext(ctx context.Context, keys APIA return resp.APIAccessDeleteKeys.DeletedKeys, nil } -func formatAPIAccessKeyErrors(errs []APIAccessKeyErrorInterface) string { - errorString := "" +var AccessKeyErrorPrefix = "The following errors have been thrown.\n" + +func formatAPIAccessKeyErrors(errs []APIAccessKeyErrorResponse) string { + errorString := AccessKeyErrorPrefix for _, e := range errs { - errorString += e.GetError().Error() + "\n" + IDAsString := "" + if len(e.Id) != 0 { + // Id is returned in the 'error' block only in the case of update and delete but not with create. + // So; in the case of create, it is made an empty string to generalize the usage of IDAsString. + IDAsString = fmt.Sprintf("%s: ", e.Id) + } + if e.Type == "USER" { + errorString += fmt.Sprintf("%s: %s%s\n", e.UserKeyErrorType, IDAsString, e.Message) + } else if e.Type == "INGEST" { + errorString += fmt.Sprintf("%s: %s%s\n", e.IngestKeyErrorType, IDAsString, e.Message) + } else if len(e.Type) == 0 { + // When Ingest Keys are deleted, the "type" attribute is currently null in the response sent, + // in the "errors" attribute - hence, this condition. However, this is not the case with User Keys. + errorString += fmt.Sprintf("%s: %s%s\n", e.IngestKeyErrorType, IDAsString, e.Message) + } else { + errorString += e.Message + } } return errorString } @@ -156,16 +174,16 @@ func formatAPIAccessKeyErrors(errs []APIAccessKeyErrorInterface) string { // apiAccessKeyCreateResponse represents the JSON response returned from creating key(s). type apiAccessKeyCreateResponse struct { APIAccessCreateKeys struct { - CreatedKeys []APIKey `json:"createdKeys"` - Errors []APIAccessKeyErrorInterface `json:"errors"` + CreatedKeys []APIKey `json:"createdKeys"` + Errors []APIAccessKeyErrorResponse `json:"errors,omitempty"` } `json:"apiAccessCreateKeys"` } // apiAccessKeyUpdateResponse represents the JSON response returned from updating key(s). type apiAccessKeyUpdateResponse struct { APIAccessUpdateKeys struct { - UpdatedKeys []APIKey `json:"updatedKeys"` - Errors []APIAccessKeyErrorInterface `json:"errors"` + UpdatedKeys []APIKey `json:"updatedKeys"` + Errors []APIAccessKeyErrorResponse `json:"errors,omitempty"` } `json:"apiAccessUpdateKeys"` } @@ -190,9 +208,11 @@ type apiAccessKeySearchResponse struct { http.GraphQLErrorResponse } -// apiAccessKeyDeleteResponse represents the JSON response returned from creating key(s). type apiAccessKeyDeleteResponse struct { - APIAccessDeleteKeys APIAccessDeleteKeyResponse `json:"apiAccessDeleteKeys"` + APIAccessDeleteKeys struct { + DeletedKeys []APIAccessDeletedKey `json:"deletedKeys,omitempty"` + Errors []APIAccessKeyErrorResponse `json:"errors,omitempty"` + } `json:"apiAccessDeleteKeys"` } const ( diff --git a/pkg/apiaccess/keys_integration_test.go b/pkg/apiaccess/keys_integration_test.go index b017eaad..65a72509 100644 --- a/pkg/apiaccess/keys_integration_test.go +++ b/pkg/apiaccess/keys_integration_test.go @@ -4,11 +4,11 @@ package apiaccess import ( - "testing" - - "github.com/stretchr/testify/require" - mock "github.com/newrelic/newrelic-client-go/v2/pkg/testhelpers" + "github.com/stretchr/testify/require" + "regexp" + "strings" + "testing" ) func TestIntegrationAPIAccess_IngestKeys(t *testing.T) { @@ -133,3 +133,111 @@ func TestIntegrationAPIAccess_UserKeys(t *testing.T) { require.NoError(t, err) require.NotNil(t, deleteResult) } + +func TestIntegrationAPIAccess_UpdateIngestKeyError(t *testing.T) { + t.Parallel() + client := newIntegrationTestClient(t) + mockIngestKeyID, _ := mock.GetNonExistentIDs() + _, err := client.UpdateAPIAccessKeys(APIAccessUpdateInput{ + Ingest: []APIAccessUpdateIngestKeyInput{ + { + KeyID: mockIngestKeyID, + Name: "Lorem Ipsum", + Notes: "Lorem Ipsum", + }, + }, + }) + require.Error(t, err) + require.Equal(t, validateAPIAccessKeyError(err, "INGEST"), true) +} + +func TestIntegrationAPIAccess_UpdateUserKeyError(t *testing.T) { + t.Parallel() + client := newIntegrationTestClient(t) + _, mockUserKeyID := mock.GetNonExistentIDs() + _, err := client.UpdateAPIAccessKeys(APIAccessUpdateInput{ + User: []APIAccessUpdateUserKeyInput{ + { + KeyID: mockUserKeyID, + Name: "Lorem Ipsum", + Notes: "Lorem Ipsum", + }, + }, + }) + require.Error(t, err) + require.Equal(t, validateAPIAccessKeyError(err, "USER"), true) +} + +func TestIntegrationAPIAccess_DeleteIngestKeyError(t *testing.T) { + t.Parallel() + client := newIntegrationTestClient(t) + mockIngestKeyIDOne, mockIngestKeyIDTwo := mock.GetNonExistentIDs() + _, err := client.DeleteAPIAccessKey(APIAccessDeleteInput{ + IngestKeyIDs: []string{ + mockIngestKeyIDOne, + mockIngestKeyIDTwo, + }, + }) + require.Error(t, err) + require.Equal(t, validateAPIAccessKeyError(err, "INGEST"), true) +} + +func TestIntegrationAPIAccess_DeleteUserKeyError(t *testing.T) { + t.Parallel() + client := newIntegrationTestClient(t) + _, mockUserKeyID := mock.GetNonExistentIDs() + _, err := client.DeleteAPIAccessKey(APIAccessDeleteInput{ + UserKeyIDs: []string{ + mockUserKeyID, + }, + }) + require.Error(t, err) + require.Equal(t, validateAPIAccessKeyError(err, "USER"), true) +} + +var possibleIngestKeyErrors = []string{ + string(APIAccessIngestKeyErrorTypeTypes.NOT_FOUND), + string(APIAccessIngestKeyErrorTypeTypes.INVALID), + string(APIAccessIngestKeyErrorTypeTypes.FORBIDDEN), +} + +var possibleUserKeyErrors = []string{ + string(APIAccessUserKeyErrorTypeTypes.NOT_FOUND), + string(APIAccessUserKeyErrorTypeTypes.INVALID), + string(APIAccessUserKeyErrorTypeTypes.FORBIDDEN), +} + +func validateAPIAccessKeyError(err error, errorType string) (status bool) { + errorMessage := err.Error() + listOfErrorMessages := strings.Split(errorMessage, "\n") + listOfErrorMessages = listOfErrorMessages[1 : len(listOfErrorMessages)-1] + validatedErrorMessages := 0 + + if errorType == "USER" { + for errorIndex := 0; errorIndex < len(listOfErrorMessages); errorIndex++ { + for listIndex := 0; listIndex < len(possibleUserKeyErrors); listIndex++ { + match, _ := regexp.MatchString(possibleUserKeyErrors[listIndex], listOfErrorMessages[errorIndex]) + if match { + validatedErrorMessages += 1 + break + } + } + } + } else if errorType == "INGEST" { + for errorIndex := 0; errorIndex < len(listOfErrorMessages); errorIndex++ { + for listIndex := 0; listIndex < len(possibleIngestKeyErrors); listIndex++ { + match, _ := regexp.MatchString(possibleIngestKeyErrors[listIndex], listOfErrorMessages[errorIndex]) + if match { + validatedErrorMessages += 1 + break + } + } + } + } + + if validatedErrorMessages == len(listOfErrorMessages) { + return true + } + + return false +} diff --git a/pkg/apiaccess/types.go b/pkg/apiaccess/types.go index 96d83fd6..2c0b931e 100644 --- a/pkg/apiaccess/types.go +++ b/pkg/apiaccess/types.go @@ -648,6 +648,16 @@ type APIAccessKeyErrorInterface interface { GetError() error } +type APIAccessKeyErrorResponse struct { + // The message with the error cause. + Message string `json:"message,omitempty"` + // Type of error. + Type string `json:"type,omitempty"` + UserKeyErrorType APIAccessUserKeyErrorType `json:"userErrorType,omitempty"` + IngestKeyErrorType APIAccessIngestKeyErrorType `json:"ingestErrorType,omitempty"` + Id string `json:"id,omitempty"` +} + // UnmarshalAPIAccessKeyErrorInterface unmarshals the interface into the correct type // based on __typename provided by GraphQL func UnmarshalAPIAccessKeyErrorInterface(b []byte) (*APIAccessKeyErrorInterface, error) { diff --git a/pkg/testhelpers/helpers.go b/pkg/testhelpers/helpers.go index 0d17ab91..eae53d7b 100644 --- a/pkg/testhelpers/helpers.go +++ b/pkg/testhelpers/helpers.go @@ -38,6 +38,12 @@ func GetTestAccountID() (int, error) { return getEnvInt("NEW_RELIC_ACCOUNT_ID") } +// GetNonExistentIDs returns two non-existent IDs that can be used to test errors raised +// when Ingest/User keys which do not exist are updated or deleted. +func GetNonExistentIDs() (string, string) { + return "00000FF000F0FFFF000F0F0000FF0FF0000000F000F00F0FF000FFFFF0FF00F0", "00000HH000H0HHHH000H0H0000HH0HH0000000H000H00H0HH000HHHHH0HH00H0" +} + // getEnvInt helper to DRY up the other env get calls for integers func getEnvInt(name string) (int, error) { if name == "" {