From bcb5747b9e7a9adc0614e7365406d64d4e15f7d9 Mon Sep 17 00:00:00 2001 From: Laurentiu Badea Date: Fri, 16 Oct 2020 01:35:49 +0000 Subject: [PATCH 1/7] Add http api for fetching accounts This works the same way as the API for stored requests, preserving the ability to query multiple ids, so the same API endpoint can be extended to support accounts. --- config/config_test.go | 3 +- config/stored_requests.go | 3 - .../backends/http_fetcher/fetcher.go | 91 ++++++++++++++++++- .../backends/http_fetcher/fetcher_test.go | 82 ++++++++++++++--- 4 files changed, 158 insertions(+), 21 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index d2e80c7e14c..e11dac08ebd 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -652,8 +652,7 @@ func TestValidateAccountsConfigRestrictions(t *testing.T) { cfg.Accounts.Postgres.ConnectionInfo.Database = "accounts" errs := cfg.validate() - assert.Len(t, errs, 2) - assert.Contains(t, errs, errors.New("accounts.http: retrieving accounts via http not available, use accounts.files")) + assert.Len(t, errs, 1) assert.Contains(t, errs, errors.New("accounts.postgres: retrieving accounts via postgres not available, use accounts.files")) } diff --git a/config/stored_requests.go b/config/stored_requests.go index 249569461bc..5985afaeed5 100644 --- a/config/stored_requests.go +++ b/config/stored_requests.go @@ -137,9 +137,6 @@ func resolvedStoredRequestsConfig(cfg *Configuration) { } func (cfg *StoredRequests) validate(errs configErrors) configErrors { - if cfg.DataType() == AccountDataType && cfg.HTTP.Endpoint != "" { - errs = append(errs, fmt.Errorf("%s.http: retrieving accounts via http not available, use accounts.files", cfg.Section())) - } if cfg.DataType() == AccountDataType && cfg.Postgres.ConnectionInfo.Database != "" { errs = append(errs, fmt.Errorf("%s.postgres: retrieving accounts via postgres not available, use accounts.files", cfg.Section())) } else { diff --git a/stored_requests/backends/http_fetcher/fetcher.go b/stored_requests/backends/http_fetcher/fetcher.go index d533d5315ab..9de494f51c9 100644 --- a/stored_requests/backends/http_fetcher/fetcher.go +++ b/stored_requests/backends/http_fetcher/fetcher.go @@ -7,6 +7,7 @@ import ( "fmt" "io/ioutil" "net/http" + "net/url" "strings" "github.com/prebid/prebid-server/stored_requests" @@ -19,9 +20,13 @@ import ( // // This file expects the endpoint to satisfy the following API: // +// Stored requests // GET {endpoint}?request-ids=["req1","req2"]&imp-ids=["imp1","imp2","imp3"] // -// This endpoint should return a payload like: +// Accounts +// GET {endpoint}?account-ids=["acc1","acc2"] +// +// The above endpoints should return a payload like: // // { // "requests": { @@ -34,12 +39,20 @@ import ( // "imp3": null // If imp3 is not found // } // } +// or +// { +// "accounts": { +// "acc1": { ... config data for acc1 ... }, +// "acc2": { ... config data for acc2 ... }, +// }, +// } // // func NewFetcher(client *http.Client, endpoint string) *HttpFetcher { // Do some work up-front to figure out if the (configurable) endpoint has a query string or not. // When we build requests, we'll either want to add `?request-ids=...&imp-ids=...` _or_ - // `&request-ids=...&imp-ids=...`, depending. + // `&request-ids=...&imp-ids=...`. + urlPrefix := endpoint if strings.Contains(endpoint, "?") { urlPrefix = urlPrefix + "&" @@ -47,7 +60,11 @@ func NewFetcher(client *http.Client, endpoint string) *HttpFetcher { urlPrefix = urlPrefix + "?" } - glog.Info("Making http_fetcher which calls GET " + urlPrefix + "request-ids=%REQUEST_ID_LIST%&imp-ids=%IMP_ID_LIST%") + if url, err := url.Parse(endpoint); err != nil { + glog.Warningf(`Invalid endpoint "%s": %v`, endpoint, err) + } else { + glog.Infof("Making http_fetcher for endpoint %v", url) + } return &HttpFetcher{ client: client, @@ -81,8 +98,68 @@ func (fetcher *HttpFetcher) FetchRequests(ctx context.Context, requestIDs []stri return } -func (fetcher *HttpFetcher) FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error) { - return nil, []error{stored_requests.NotFoundError{accountID, "Account"}} +// FetchAccounts retrieves account configurations +// +// Request format is similar to the one for requests: +// GET {endpoint}?account-ids=["account1","account2",...] +// +// The endpoint is expected to respond with a JSON map with accountID -> json.RawMessage +// { +// "account1": { ... account json ... } +// } +// The JSON contents of account config is returned as-is (NOT validated) +func (fetcher *HttpFetcher) FetchAccounts(ctx context.Context, accountIDs []string) (accountData map[string]json.RawMessage, errs []error) { + if len(accountIDs) == 0 { + return nil, nil + } + httpReq, err := http.NewRequestWithContext(ctx, "GET", fetcher.Endpoint+"account-ids=[\""+strings.Join(accountIDs, "\",\"")+"\"]", nil) + if err != nil { + return nil, []error{ + fmt.Errorf(`Error fetching accounts "%v" via http: %v`, accountIDs, err), + } + } + httpResp, err := ctxhttp.Do(ctx, fetcher.client, httpReq) + if err != nil { + return nil, []error{ + fmt.Errorf(`Error fetching accounts %v via http: %v`, accountIDs, err), + } + } + defer httpResp.Body.Close() + respBytes, err := ioutil.ReadAll(httpResp.Body) + if err != nil { + return nil, []error{ + fmt.Errorf(`Error fetching accounts %v via http: could not read response`, accountIDs), + } + } + if httpResp.StatusCode != http.StatusOK { + return nil, []error{ + fmt.Errorf(`Error fetching accounts %v via http: unexpected response status %d`, accountIDs, httpResp.StatusCode), + } + } + var responseData accountsResponseContract + if err = json.Unmarshal(respBytes, &responseData); err != nil { + return nil, []error{ + fmt.Errorf(`Error fetching accounts %v via http: malformed response`, accountIDs), + } + } + errs = convertNullsToErrs(responseData.Accounts, "Account", errs) + return responseData.Accounts, errs +} + +// FetchAccount fetchers a single accountID and returns its corresponding json +func (fetcher *HttpFetcher) FetchAccount(ctx context.Context, accountID string) (accountJSON json.RawMessage, errs []error) { + accountData, errs := fetcher.FetchAccounts(ctx, []string{accountID}) + if len(errs) > 0 { + return nil, errs + } + accountJSON, ok := accountData[accountID] + if !ok { + return nil, []error{stored_requests.NotFoundError{ + ID: accountID, + DataType: "Account", + }} + } + return accountJSON, []error{} } func (fetcher *HttpFetcher) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) { @@ -190,3 +267,7 @@ type responseContract struct { Requests map[string]json.RawMessage `json:"requests"` Imps map[string]json.RawMessage `json:"imps"` } + +type accountsResponseContract struct { + Accounts map[string]json.RawMessage `json:"accounts"` +} diff --git a/stored_requests/backends/http_fetcher/fetcher_test.go b/stored_requests/backends/http_fetcher/fetcher_test.go index dc4076fd4d9..e751ac77aee 100644 --- a/stored_requests/backends/http_fetcher/fetcher_test.go +++ b/stored_requests/backends/http_fetcher/fetcher_test.go @@ -9,6 +9,8 @@ import ( "net/http/httptest" "strings" "testing" + + "github.com/stretchr/testify/assert" ) func TestSingleReq(t *testing.T) { @@ -61,14 +63,39 @@ func TestReqsAndImps(t *testing.T) { assertErrLength(t, errs, 0) } -func TestMissingValues(t *testing.T) { - fetcher, close := newEmptyFetcher(t, []string{"req-1", "req-2"}, []string{"imp-1"}) +func TestFetchAccounts(t *testing.T) { + fetcher, close := newTestAccountFetcher(t, []string{"acc-1", "acc-2"}) defer close() - reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1", "req-2"}, []string{"imp-1"}) - assertMapKeys(t, reqData) - assertMapKeys(t, impData) - assertErrLength(t, errs, 3) + accData, errs := fetcher.FetchAccounts(context.Background(), []string{"acc-1", "acc-2"}) + assertMapKeys(t, accData, "acc-1", "acc-2") + assertErrLength(t, errs, 0) +} + +func TestFetchAccountsNoData(t *testing.T) { + fetcher, close := newFetcherBrokenBackend() + defer close() + accData, errs := fetcher.FetchAccounts(context.Background(), []string{"req-1"}) + assertMapKeys(t, accData) + assertErrLength(t, errs, 1) +} + +func TestFetchAccount(t *testing.T) { + fetcher, close := newTestAccountFetcher(t, []string{"acc-1"}) + defer close() + + account, errs := fetcher.FetchAccount(context.Background(), "acc-1") + assert.Empty(t, errs, "Unexpected error fetching existing account") + assert.JSONEq(t, `"acc-1"`, string(account)) +} + +func TestFetchAccountNoData(t *testing.T) { + fetcher, close := newFetcherBrokenBackend() + defer close() + + unknownAccount, errs := fetcher.FetchAccount(context.Background(), "unknown-acc") + assert.NotEmpty(t, errs, "Retrieving unknown account should have returned an error") + assert.Nil(t, unknownAccount) } func TestErrResponse(t *testing.T) { @@ -139,12 +166,12 @@ func newTestFetcher(t *testing.T, expectReqIDs []string, expectImpIDs []string) func newHandler(t *testing.T, expectReqIDs []string, expectImpIDs []string, jsonifier func(string) json.RawMessage) func(w http.ResponseWriter, r *http.Request) { return func(w http.ResponseWriter, r *http.Request) { query := r.URL.Query() - assertMatches(t, query.Get("request-ids"), expectReqIDs) - assertMatches(t, query.Get("imp-ids"), expectImpIDs) - gotReqIDs := richSplit(query.Get("request-ids")) gotImpIDs := richSplit(query.Get("imp-ids")) + assertMatches(t, gotReqIDs, expectReqIDs) + assertMatches(t, gotImpIDs, expectImpIDs) + reqIDResponse := make(map[string]json.RawMessage, len(gotReqIDs)) impIDResponse := make(map[string]json.RawMessage, len(gotImpIDs)) @@ -174,10 +201,43 @@ func newHandler(t *testing.T, expectReqIDs []string, expectImpIDs []string, json } } -func assertMatches(t *testing.T, query string, expected []string) { +func newTestAccountFetcher(t *testing.T, expectAccIDs []string) (fetcher *HttpFetcher, closer func()) { + handler := newAccountHandler(t, expectAccIDs, jsonifyID) + server := httptest.NewServer(http.HandlerFunc(handler)) + return NewFetcher(server.Client(), server.URL), server.Close +} + +func newAccountHandler(t *testing.T, expectAccIDs []string, jsonifier func(string) json.RawMessage) func(w http.ResponseWriter, r *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { + query := r.URL.Query() + gotAccIDs := richSplit(query.Get("account-ids")) + + assertMatches(t, gotAccIDs, expectAccIDs) + + accIDResponse := make(map[string]json.RawMessage, len(gotAccIDs)) + + for _, accID := range gotAccIDs { + if accID != "" { + accIDResponse[accID] = jsonifier(accID) + } + } + + respObj := accountsResponseContract{ + Accounts: accIDResponse, + } + + if respBytes, err := json.Marshal(respObj); err != nil { + t.Errorf("failed to marshal responseContract in test: %v", err) + w.WriteHeader(http.StatusInternalServerError) + } else { + w.Write(respBytes) + } + } +} + +func assertMatches(t *testing.T, queryVals []string, expected []string) { t.Helper() - queryVals := richSplit(query) if len(queryVals) == 1 && queryVals[0] == "" { if len(expected) != 0 { t.Errorf("Expected no query vals, but got %v", queryVals) From 63e646c14be8c191772bbc498dcabce306156b7c Mon Sep 17 00:00:00 2001 From: Laurentiu Badea Date: Mon, 19 Oct 2020 18:55:28 +0000 Subject: [PATCH 2/7] Integrated feedback https://github.com/prebid/prebid-server/pull/1545/files#r507843554 https://github.com/prebid/prebid-server/pull/1545/files#r507846553 https://github.com/prebid/prebid-server/pull/1545/files#r507826627 https://github.com/prebid/prebid-server/pull/1545/files#r507847197 https://github.com/prebid/prebid-server/pull/1545/files#r507851829 https://github.com/prebid/prebid-server/pull/1545/files#r507863404 https://github.com/prebid/prebid-server/pull/1545/files#r507861937 https://github.com/prebid/prebid-server/pull/1545/files#r507864468 --- .../backends/http_fetcher/fetcher.go | 24 +++++++++---------- .../backends/http_fetcher/fetcher_test.go | 19 +++++++++++---- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/stored_requests/backends/http_fetcher/fetcher.go b/stored_requests/backends/http_fetcher/fetcher.go index 9de494f51c9..46d87faf383 100644 --- a/stored_requests/backends/http_fetcher/fetcher.go +++ b/stored_requests/backends/http_fetcher/fetcher.go @@ -53,6 +53,12 @@ func NewFetcher(client *http.Client, endpoint string) *HttpFetcher { // When we build requests, we'll either want to add `?request-ids=...&imp-ids=...` _or_ // `&request-ids=...&imp-ids=...`. + if url, err := url.Parse(endpoint); err != nil { + glog.Warningf(`Invalid endpoint "%s": %v`, endpoint, err) + } else { + glog.Infof("Making http_fetcher for endpoint %v", url) + } + urlPrefix := endpoint if strings.Contains(endpoint, "?") { urlPrefix = urlPrefix + "&" @@ -60,12 +66,6 @@ func NewFetcher(client *http.Client, endpoint string) *HttpFetcher { urlPrefix = urlPrefix + "?" } - if url, err := url.Parse(endpoint); err != nil { - glog.Warningf(`Invalid endpoint "%s": %v`, endpoint, err) - } else { - glog.Infof("Making http_fetcher for endpoint %v", url) - } - return &HttpFetcher{ client: client, Endpoint: urlPrefix, @@ -108,14 +108,14 @@ func (fetcher *HttpFetcher) FetchRequests(ctx context.Context, requestIDs []stri // "account1": { ... account json ... } // } // The JSON contents of account config is returned as-is (NOT validated) -func (fetcher *HttpFetcher) FetchAccounts(ctx context.Context, accountIDs []string) (accountData map[string]json.RawMessage, errs []error) { +func (fetcher *HttpFetcher) FetchAccounts(ctx context.Context, accountIDs []string) (map[string]json.RawMessage, []error) { if len(accountIDs) == 0 { return nil, nil } httpReq, err := http.NewRequestWithContext(ctx, "GET", fetcher.Endpoint+"account-ids=[\""+strings.Join(accountIDs, "\",\"")+"\"]", nil) if err != nil { return nil, []error{ - fmt.Errorf(`Error fetching accounts "%v" via http: %v`, accountIDs, err), + fmt.Errorf(`Error fetching accounts %v via http: build request failed with %v`, accountIDs, err), } } httpResp, err := ctxhttp.Do(ctx, fetcher.client, httpReq) @@ -128,7 +128,7 @@ func (fetcher *HttpFetcher) FetchAccounts(ctx context.Context, accountIDs []stri respBytes, err := ioutil.ReadAll(httpResp.Body) if err != nil { return nil, []error{ - fmt.Errorf(`Error fetching accounts %v via http: could not read response`, accountIDs), + fmt.Errorf(`Error fetching accounts %v via http: error reading response: %v`, accountIDs, err), } } if httpResp.StatusCode != http.StatusOK { @@ -139,10 +139,10 @@ func (fetcher *HttpFetcher) FetchAccounts(ctx context.Context, accountIDs []stri var responseData accountsResponseContract if err = json.Unmarshal(respBytes, &responseData); err != nil { return nil, []error{ - fmt.Errorf(`Error fetching accounts %v via http: malformed response`, accountIDs), + fmt.Errorf(`Error fetching accounts %v via http: failed to parse response: %v`, accountIDs, err), } } - errs = convertNullsToErrs(responseData.Accounts, "Account", errs) + errs := convertNullsToErrs(responseData.Accounts, "Account", nil) return responseData.Accounts, errs } @@ -159,7 +159,7 @@ func (fetcher *HttpFetcher) FetchAccount(ctx context.Context, accountID string) DataType: "Account", }} } - return accountJSON, []error{} + return accountJSON, nil } func (fetcher *HttpFetcher) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) { diff --git a/stored_requests/backends/http_fetcher/fetcher_test.go b/stored_requests/backends/http_fetcher/fetcher_test.go index e751ac77aee..b4bd106b8a2 100644 --- a/stored_requests/backends/http_fetcher/fetcher_test.go +++ b/stored_requests/backends/http_fetcher/fetcher_test.go @@ -63,21 +63,32 @@ func TestReqsAndImps(t *testing.T) { assertErrLength(t, errs, 0) } +func TestMissingValues(t *testing.T) { + fetcher, close := newEmptyFetcher(t, []string{"req-1", "req-2"}, []string{"imp-1"}) + defer close() + + reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1", "req-2"}, []string{"imp-1"}) + assertMapKeys(t, reqData) + assertMapKeys(t, impData) + assertErrLength(t, errs, 3) +} + func TestFetchAccounts(t *testing.T) { fetcher, close := newTestAccountFetcher(t, []string{"acc-1", "acc-2"}) defer close() accData, errs := fetcher.FetchAccounts(context.Background(), []string{"acc-1", "acc-2"}) + assert.Empty(t, errs) assertMapKeys(t, accData, "acc-1", "acc-2") - assertErrLength(t, errs, 0) } func TestFetchAccountsNoData(t *testing.T) { fetcher, close := newFetcherBrokenBackend() defer close() + accData, errs := fetcher.FetchAccounts(context.Background(), []string{"req-1"}) - assertMapKeys(t, accData) - assertErrLength(t, errs, 1) + assert.Len(t, errs, 1) + assert.Nil(t, accData) } func TestFetchAccount(t *testing.T) { @@ -86,7 +97,7 @@ func TestFetchAccount(t *testing.T) { account, errs := fetcher.FetchAccount(context.Background(), "acc-1") assert.Empty(t, errs, "Unexpected error fetching existing account") - assert.JSONEq(t, `"acc-1"`, string(account)) + assert.JSONEq(t, `"acc-1"`, string(account), "Unexpected account data returned") } func TestFetchAccountNoData(t *testing.T) { From e0af3f7eb0a39f36581e3fb11f1632c230265c93 Mon Sep 17 00:00:00 2001 From: Laurentiu Badea Date: Mon, 19 Oct 2020 19:02:41 +0000 Subject: [PATCH 3/7] Add bad json test for FetchAccounts https://github.com/prebid/prebid-server/pull/1545/files#r507867507 --- .../backends/http_fetcher/fetcher_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/stored_requests/backends/http_fetcher/fetcher_test.go b/stored_requests/backends/http_fetcher/fetcher_test.go index b4bd106b8a2..5abb196e05b 100644 --- a/stored_requests/backends/http_fetcher/fetcher_test.go +++ b/stored_requests/backends/http_fetcher/fetcher_test.go @@ -91,6 +91,15 @@ func TestFetchAccountsNoData(t *testing.T) { assert.Nil(t, accData) } +func TestFetchAccountsBadJSON(t *testing.T) { + fetcher, close := newFetcherBadJSON() + defer close() + + accData, errs := fetcher.FetchAccounts(context.Background(), []string{"req-1"}) + assert.Len(t, errs, 1) + assert.Nil(t, accData) +} + func TestFetchAccount(t *testing.T) { fetcher, close := newTestAccountFetcher(t, []string{"acc-1"}) defer close() @@ -162,6 +171,14 @@ func newFetcherBrokenBackend() (fetcher *HttpFetcher, closer func()) { return NewFetcher(server.Client(), server.URL), server.Close } +func newFetcherBadJSON() (fetcher *HttpFetcher, closer func()) { + handler := func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`broken JSON`)) + } + server := httptest.NewServer(http.HandlerFunc(handler)) + return NewFetcher(server.Client(), server.URL), server.Close +} + func newEmptyFetcher(t *testing.T, expectReqIDs []string, expectImpIDs []string) (fetcher *HttpFetcher, closer func()) { handler := newHandler(t, expectReqIDs, expectImpIDs, jsonifyToNull) server := httptest.NewServer(http.HandlerFunc(handler)) From 16cc796b1dc0d437f962b168c316cf38fb25c1d6 Mon Sep 17 00:00:00 2001 From: Laurentiu Badea Date: Mon, 19 Oct 2020 19:18:44 +0000 Subject: [PATCH 4/7] (refactor) use built-in assert.Empty/assert.Len where applicable. --- .../backends/http_fetcher/fetcher_test.go | 34 +++++++------------ 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/stored_requests/backends/http_fetcher/fetcher_test.go b/stored_requests/backends/http_fetcher/fetcher_test.go index 5abb196e05b..8e2375a362c 100644 --- a/stored_requests/backends/http_fetcher/fetcher_test.go +++ b/stored_requests/backends/http_fetcher/fetcher_test.go @@ -19,8 +19,8 @@ func TestSingleReq(t *testing.T) { reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1"}, nil) assertMapKeys(t, reqData, "req-1") - assertMapKeys(t, impData) - assertErrLength(t, errs, 0) + assert.Empty(t, impData) + assert.Empty(t, errs) } func TestSeveralReqs(t *testing.T) { @@ -29,8 +29,8 @@ func TestSeveralReqs(t *testing.T) { reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1", "req-2"}, nil) assertMapKeys(t, reqData, "req-1", "req-2") - assertMapKeys(t, impData) - assertErrLength(t, errs, 0) + assert.Empty(t, impData) + assert.Empty(t, errs) } func TestSingleImp(t *testing.T) { @@ -38,9 +38,9 @@ func TestSingleImp(t *testing.T) { defer close() reqData, impData, errs := fetcher.FetchRequests(context.Background(), nil, []string{"imp-1"}) - assertMapKeys(t, reqData) + assert.Empty(t, reqData) assertMapKeys(t, impData, "imp-1") - assertErrLength(t, errs, 0) + assert.Empty(t, errs) } func TestSeveralImps(t *testing.T) { @@ -48,9 +48,9 @@ func TestSeveralImps(t *testing.T) { defer close() reqData, impData, errs := fetcher.FetchRequests(context.Background(), nil, []string{"imp-1", "imp-2"}) - assertMapKeys(t, reqData) + assert.Empty(t, reqData) assertMapKeys(t, impData, "imp-1", "imp-2") - assertErrLength(t, errs, 0) + assert.Empty(t, errs) } func TestReqsAndImps(t *testing.T) { @@ -60,7 +60,7 @@ func TestReqsAndImps(t *testing.T) { reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1"}, []string{"imp-1"}) assertMapKeys(t, reqData, "req-1") assertMapKeys(t, impData, "imp-1") - assertErrLength(t, errs, 0) + assert.Empty(t, errs) } func TestMissingValues(t *testing.T) { @@ -68,9 +68,9 @@ func TestMissingValues(t *testing.T) { defer close() reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1", "req-2"}, []string{"imp-1"}) - assertMapKeys(t, reqData) - assertMapKeys(t, impData) - assertErrLength(t, errs, 3) + assert.Empty(t, reqData) + assert.Empty(t, impData) + assert.Len(t, errs, 3) } func TestFetchAccounts(t *testing.T) { @@ -124,7 +124,7 @@ func TestErrResponse(t *testing.T) { reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1"}, []string{"imp-1"}) assertMapKeys(t, reqData) assertMapKeys(t, impData) - assertErrLength(t, errs, 1) + assert.Len(t, errs, 1) } func assertSameContents(t *testing.T, expected map[string]json.RawMessage, actual map[string]json.RawMessage) { @@ -338,11 +338,3 @@ func assertMapKeys(t *testing.T, m map[string]json.RawMessage, keys ...string) { } } } - -func assertErrLength(t *testing.T, errs []error, expected int) { - t.Helper() - - if len(errs) != expected { - t.Errorf("Expected %d errors. Got: %v", expected, errs) - } -} From 0c921bd424441205cb5fa02b6cf908b22628b8c0 Mon Sep 17 00:00:00 2001 From: Laurentiu Badea Date: Tue, 20 Oct 2020 16:31:31 +0000 Subject: [PATCH 5/7] Error out early if event endpoint url cannot be parsed --- stored_requests/backends/http_fetcher/fetcher.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stored_requests/backends/http_fetcher/fetcher.go b/stored_requests/backends/http_fetcher/fetcher.go index 46d87faf383..4a148377664 100644 --- a/stored_requests/backends/http_fetcher/fetcher.go +++ b/stored_requests/backends/http_fetcher/fetcher.go @@ -54,7 +54,7 @@ func NewFetcher(client *http.Client, endpoint string) *HttpFetcher { // `&request-ids=...&imp-ids=...`. if url, err := url.Parse(endpoint); err != nil { - glog.Warningf(`Invalid endpoint "%s": %v`, endpoint, err) + glog.Errorf(`Invalid endpoint "%s": %v`, endpoint, err) } else { glog.Infof("Making http_fetcher for endpoint %v", url) } From 11540067c546c53d652d6c3099f9c5411452810f Mon Sep 17 00:00:00 2001 From: Laurentiu Badea Date: Wed, 21 Oct 2020 18:04:58 +0000 Subject: [PATCH 6/7] Address feedback Bail out if http endpoint isn't parseable. Add custom messages to test assertions. Check error assertions before the data. --- .../backends/http_fetcher/fetcher.go | 4 +- .../backends/http_fetcher/fetcher_test.go | 38 +++++++++---------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/stored_requests/backends/http_fetcher/fetcher.go b/stored_requests/backends/http_fetcher/fetcher.go index 4a148377664..203150c0689 100644 --- a/stored_requests/backends/http_fetcher/fetcher.go +++ b/stored_requests/backends/http_fetcher/fetcher.go @@ -54,7 +54,7 @@ func NewFetcher(client *http.Client, endpoint string) *HttpFetcher { // `&request-ids=...&imp-ids=...`. if url, err := url.Parse(endpoint); err != nil { - glog.Errorf(`Invalid endpoint "%s": %v`, endpoint, err) + glog.Fatalf(`Invalid endpoint "%s": %v`, endpoint, err) } else { glog.Infof("Making http_fetcher for endpoint %v", url) } @@ -142,7 +142,7 @@ func (fetcher *HttpFetcher) FetchAccounts(ctx context.Context, accountIDs []stri fmt.Errorf(`Error fetching accounts %v via http: failed to parse response: %v`, accountIDs, err), } } - errs := convertNullsToErrs(responseData.Accounts, "Account", nil) + errs := convertNullsToErrs(responseData.Accounts, "Account", []error{}) return responseData.Accounts, errs } diff --git a/stored_requests/backends/http_fetcher/fetcher_test.go b/stored_requests/backends/http_fetcher/fetcher_test.go index 8e2375a362c..dc8efe5cccf 100644 --- a/stored_requests/backends/http_fetcher/fetcher_test.go +++ b/stored_requests/backends/http_fetcher/fetcher_test.go @@ -18,9 +18,9 @@ func TestSingleReq(t *testing.T) { defer close() reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1"}, nil) + assert.Empty(t, errs, "Unexpected errors fetching known requests") assertMapKeys(t, reqData, "req-1") - assert.Empty(t, impData) - assert.Empty(t, errs) + assert.Empty(t, impData, "Unexpected imps returned fetching just requests") } func TestSeveralReqs(t *testing.T) { @@ -28,9 +28,9 @@ func TestSeveralReqs(t *testing.T) { defer close() reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1", "req-2"}, nil) + assert.Empty(t, errs, "Unexpected errors fetching known requests") assertMapKeys(t, reqData, "req-1", "req-2") - assert.Empty(t, impData) - assert.Empty(t, errs) + assert.Empty(t, impData, "Unexpected imps returned fetching just requests") } func TestSingleImp(t *testing.T) { @@ -38,9 +38,9 @@ func TestSingleImp(t *testing.T) { defer close() reqData, impData, errs := fetcher.FetchRequests(context.Background(), nil, []string{"imp-1"}) - assert.Empty(t, reqData) + assert.Empty(t, errs, "Unexpected errors fetching known imps") + assert.Empty(t, reqData, "Unexpected requests returned fetching just imps") assertMapKeys(t, impData, "imp-1") - assert.Empty(t, errs) } func TestSeveralImps(t *testing.T) { @@ -48,9 +48,9 @@ func TestSeveralImps(t *testing.T) { defer close() reqData, impData, errs := fetcher.FetchRequests(context.Background(), nil, []string{"imp-1", "imp-2"}) - assert.Empty(t, reqData) + assert.Empty(t, errs, "Unexpected errors fetching known imps") + assert.Empty(t, reqData, "Unexpected requests returned fetching just imps") assertMapKeys(t, impData, "imp-1", "imp-2") - assert.Empty(t, errs) } func TestReqsAndImps(t *testing.T) { @@ -58,9 +58,9 @@ func TestReqsAndImps(t *testing.T) { defer close() reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1"}, []string{"imp-1"}) + assert.Empty(t, errs, "Unexpected errors fetching known reqs and imps") assertMapKeys(t, reqData, "req-1") assertMapKeys(t, impData, "imp-1") - assert.Empty(t, errs) } func TestMissingValues(t *testing.T) { @@ -68,9 +68,9 @@ func TestMissingValues(t *testing.T) { defer close() reqData, impData, errs := fetcher.FetchRequests(context.Background(), []string{"req-1", "req-2"}, []string{"imp-1"}) - assert.Empty(t, reqData) - assert.Empty(t, impData) - assert.Len(t, errs, 3) + assert.Empty(t, reqData, "Fetching unknown reqs should return no reqs") + assert.Empty(t, impData, "Fetching unknown imps should return no imps") + assert.Len(t, errs, 3, "Fetching 3 unknown reqs+imps should return 3 errors") } func TestFetchAccounts(t *testing.T) { @@ -78,7 +78,7 @@ func TestFetchAccounts(t *testing.T) { defer close() accData, errs := fetcher.FetchAccounts(context.Background(), []string{"acc-1", "acc-2"}) - assert.Empty(t, errs) + assert.Empty(t, errs, "Unexpected error fetching known accounts") assertMapKeys(t, accData, "acc-1", "acc-2") } @@ -87,8 +87,8 @@ func TestFetchAccountsNoData(t *testing.T) { defer close() accData, errs := fetcher.FetchAccounts(context.Background(), []string{"req-1"}) - assert.Len(t, errs, 1) - assert.Nil(t, accData) + assert.Len(t, errs, 1, "Fetching unknown account should have returned an error") + assert.Nil(t, accData, "Fetching unknown account should return nil account map") } func TestFetchAccountsBadJSON(t *testing.T) { @@ -96,8 +96,8 @@ func TestFetchAccountsBadJSON(t *testing.T) { defer close() accData, errs := fetcher.FetchAccounts(context.Background(), []string{"req-1"}) - assert.Len(t, errs, 1) - assert.Nil(t, accData) + assert.Len(t, errs, 1, "Fetching account with broken json should have returned an error") + assert.Nil(t, accData, "Fetching account with broken json should return nil account map") } func TestFetchAccount(t *testing.T) { @@ -106,7 +106,7 @@ func TestFetchAccount(t *testing.T) { account, errs := fetcher.FetchAccount(context.Background(), "acc-1") assert.Empty(t, errs, "Unexpected error fetching existing account") - assert.JSONEq(t, `"acc-1"`, string(account), "Unexpected account data returned") + assert.JSONEq(t, `"acc-1"`, string(account), "Unexpected account data fetching existing account") } func TestFetchAccountNoData(t *testing.T) { @@ -115,7 +115,7 @@ func TestFetchAccountNoData(t *testing.T) { unknownAccount, errs := fetcher.FetchAccount(context.Background(), "unknown-acc") assert.NotEmpty(t, errs, "Retrieving unknown account should have returned an error") - assert.Nil(t, unknownAccount) + assert.Nil(t, unknownAccount, "Retrieving unknown account should return nil json.RawMessage") } func TestErrResponse(t *testing.T) { From e0db826122a48e803e94b9f6e2cfdaec971d00af Mon Sep 17 00:00:00 2001 From: Laurentiu Badea Date: Wed, 21 Oct 2020 20:36:26 +0000 Subject: [PATCH 7/7] Add coverage code supplied by @bsardo --- .../backends/http_fetcher/fetcher.go | 5 +-- .../backends/http_fetcher/fetcher_test.go | 43 ++++++++++++++++++- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/stored_requests/backends/http_fetcher/fetcher.go b/stored_requests/backends/http_fetcher/fetcher.go index 203150c0689..bc12caecb98 100644 --- a/stored_requests/backends/http_fetcher/fetcher.go +++ b/stored_requests/backends/http_fetcher/fetcher.go @@ -53,11 +53,10 @@ func NewFetcher(client *http.Client, endpoint string) *HttpFetcher { // When we build requests, we'll either want to add `?request-ids=...&imp-ids=...` _or_ // `&request-ids=...&imp-ids=...`. - if url, err := url.Parse(endpoint); err != nil { + if _, err := url.Parse(endpoint); err != nil { glog.Fatalf(`Invalid endpoint "%s": %v`, endpoint, err) - } else { - glog.Infof("Making http_fetcher for endpoint %v", url) } + glog.Infof("Making http_fetcher for endpoint %v", endpoint) urlPrefix := endpoint if strings.Contains(endpoint, "?") { diff --git a/stored_requests/backends/http_fetcher/fetcher_test.go b/stored_requests/backends/http_fetcher/fetcher_test.go index dc8efe5cccf..30933181e1d 100644 --- a/stored_requests/backends/http_fetcher/fetcher_test.go +++ b/stored_requests/backends/http_fetcher/fetcher_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -100,6 +101,37 @@ func TestFetchAccountsBadJSON(t *testing.T) { assert.Nil(t, accData, "Fetching account with broken json should return nil account map") } +func TestFetchAccountsNoIDsProvided(t *testing.T) { + fetcher, close := newTestAccountFetcher(t, []string{"acc-1", "acc-2"}) + defer close() + + accData, errs := fetcher.FetchAccounts(nil, []string{}) + assert.Empty(t, errs, "Unexpected error fetching empty account list") + assert.Nil(t, accData, "Fetching empty account list should return nil") +} + +// Force build request failure by not providing a context +func TestFetchAccountsFailedBuildRequest(t *testing.T) { + fetcher, close := newTestAccountFetcher(t, []string{"acc-1", "acc-2"}) + defer close() + + accData, errs := fetcher.FetchAccounts(nil, []string{"acc-1"}) + assert.Len(t, errs, 1, "Fetching accounts without context should result in error ") + assert.Nil(t, accData, "Fetching accounts without context should return nil") +} + +// Force http error via request timeout +func TestFetchAccountsContextTimeout(t *testing.T) { + fetcher, close := newTestAccountFetcher(t, []string{"acc-1", "acc-2"}) + defer close() + + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(0)) + defer cancel() + accData, errs := fetcher.FetchAccounts(ctx, []string{"acc-1"}) + assert.Len(t, errs, 1, "Expected context timeout error") + assert.Nil(t, accData, "Unexpected account data returned instead of timeout") +} + func TestFetchAccount(t *testing.T) { fetcher, close := newTestAccountFetcher(t, []string{"acc-1"}) defer close() @@ -114,10 +146,19 @@ func TestFetchAccountNoData(t *testing.T) { defer close() unknownAccount, errs := fetcher.FetchAccount(context.Background(), "unknown-acc") - assert.NotEmpty(t, errs, "Retrieving unknown account should have returned an error") + assert.NotEmpty(t, errs, "Retrieving unknown account should return error") assert.Nil(t, unknownAccount, "Retrieving unknown account should return nil json.RawMessage") } +func TestFetchAccountNoIDProvided(t *testing.T) { + fetcher, close := newTestAccountFetcher(t, nil) + defer close() + + account, errs := fetcher.FetchAccount(context.Background(), "") + assert.Len(t, errs, 1, "Fetching account with empty id should error") + assert.Nil(t, account, "Fetching account with empty id should return nil") +} + func TestErrResponse(t *testing.T) { fetcher, close := newFetcherBrokenBackend() defer close()