From f614d14474f2ee18eedfadb0d9afab4a7de9e445 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 | 38 ++++++++++++----- pkg/apiaccess/keys_integration_test.go | 57 ++++++++++++++++++++++++++ pkg/apiaccess/types.go | 10 +++++ pkg/testhelpers/helpers.go | 6 +++ 4 files changed, 101 insertions(+), 10 deletions(-) diff --git a/pkg/apiaccess/keys.go b/pkg/apiaccess/keys.go index e2b7641a..4df77262 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,26 @@ func (a *APIAccess) DeleteAPIAccessKeyWithContext(ctx context.Context, keys APIA return resp.APIAccessDeleteKeys.DeletedKeys, nil } -func formatAPIAccessKeyErrors(errs []APIAccessKeyErrorInterface) string { - errorString := "" +func formatAPIAccessKeyErrors(errs []APIAccessKeyErrorResponse) string { + errorString := "The following errors have been thrown.\n" 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 + // In the case of create, it is made an empty string to generalize the usage of e.Id + 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 +172,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 +206,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..25d53a58 100644 --- a/pkg/apiaccess/keys_integration_test.go +++ b/pkg/apiaccess/keys_integration_test.go @@ -133,3 +133,60 @@ 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) +} + +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) +} + +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) +} + +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) +} 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 == "" {