From 1b47f16f4706369ba68dbecd084af0500bc84257 Mon Sep 17 00:00:00 2001 From: Cameron Rice <37162584+camrice@users.noreply.github.com> Date: Wed, 4 Dec 2019 13:23:08 -0800 Subject: [PATCH] Updated handleError arguments to be pointers for video endpoint (#1128) * Updated handleError arguments to be pointers for video endpoint * Removing unneeded pointer to http.ResponseWriter * Adding units test for update to handleError --- endpoints/openrtb2/video_auction.go | 26 +++---- endpoints/openrtb2/video_auction_test.go | 87 ++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 13 deletions(-) diff --git a/endpoints/openrtb2/video_auction.go b/endpoints/openrtb2/video_auction.go index 61f4f3e739f..1f234b85919 100644 --- a/endpoints/openrtb2/video_auction.go +++ b/endpoints/openrtb2/video_auction.go @@ -91,7 +91,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re } requestJson, err := ioutil.ReadAll(lr) if err != nil { - handleError(labels, w, []error{err}, ao) + handleError(&labels, w, []error{err}, &ao) return } @@ -102,27 +102,27 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re if err != nil { if deps.cfg.VideoStoredRequestRequired { - handleError(labels, w, []error{err}, ao) + handleError(&labels, w, []error{err}, &ao) return } } else { storedRequest, errs := deps.loadStoredVideoRequest(context.Background(), storedRequestId) if len(errs) > 0 { - handleError(labels, w, errs, ao) + handleError(&labels, w, errs, &ao) return } //merge incoming req with stored video req resolvedRequest, err = jsonpatch.MergePatch(storedRequest, requestJson) if err != nil { - handleError(labels, w, []error{err}, ao) + handleError(&labels, w, []error{err}, &ao) return } } //unmarshal and validate combined result videoBidReq, errL, podErrors := deps.parseVideoRequest(resolvedRequest) if len(errL) > 0 { - handleError(labels, w, errL, ao) + handleError(&labels, w, errL, &ao) return } @@ -130,7 +130,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re if deps.defaultRequest { if err := json.Unmarshal(deps.defReqJSON, bidReq); err != nil { err = fmt.Errorf("Invalid JSON in Default Request Settings: %s", err) - handleError(labels, w, []error{err}, ao) + handleError(&labels, w, []error{err}, &ao) return } } @@ -154,7 +154,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re } err := errors.New(fmt.Sprintf("all pods are incorrect: %s", strings.Join(resPodErr, "; "))) errL = append(errL, err) - handleError(labels, w, errL, ao) + handleError(&labels, w, errL, &ao) return } @@ -166,7 +166,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re errL = deps.validateRequest(bidReq) if len(errL) > 0 { - handleError(labels, w, errL, ao) + handleError(&labels, w, errL, &ao) return } @@ -194,7 +194,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re if acctIdErr := validateAccount(deps.cfg, labels.PubID); acctIdErr != nil { errL = append(errL, acctIdErr) - handleError(labels, w, errL, ao) + handleError(&labels, w, errL, &ao) return } //execute auction logic @@ -203,7 +203,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re ao.Response = response if err != nil { errL := []error{err} - handleError(labels, w, errL, ao) + handleError(&labels, w, errL, &ao) return } @@ -211,7 +211,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re bidResp, err := buildVideoResponse(response, podErrors) if err != nil { errL := []error{err} - handleError(labels, w, errL, ao) + handleError(&labels, w, errL, &ao) return } if bidReq.Test == 1 { @@ -222,7 +222,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re //resp, err := json.Marshal(response) if err != nil { errL := []error{err} - handleError(labels, w, errL, ao) + handleError(&labels, w, errL, &ao) return } @@ -238,7 +238,7 @@ func cleanupVideoBidRequest(videoReq *openrtb_ext.BidRequestVideo, podErrors []P return videoReq } -func handleError(labels pbsmetrics.Labels, w http.ResponseWriter, errL []error, ao analytics.AuctionObject) { +func handleError(labels *pbsmetrics.Labels, w http.ResponseWriter, errL []error, ao *analytics.AuctionObject) { labels.RequestStatus = pbsmetrics.RequestStatusErr var errors string var status int = http.StatusInternalServerError diff --git a/endpoints/openrtb2/video_auction_test.go b/endpoints/openrtb2/video_auction_test.go index 85285a3032a..6a8e913a0bf 100644 --- a/endpoints/openrtb2/video_auction_test.go +++ b/endpoints/openrtb2/video_auction_test.go @@ -3,12 +3,14 @@ package openrtb2 import ( "context" "encoding/json" + "errors" "io/ioutil" "net/http/httptest" "strings" "testing" "github.com/mxmCherry/openrtb" + "github.com/prebid/prebid-server/analytics" analyticsConf "github.com/prebid/prebid-server/analytics/config" "github.com/prebid/prebid-server/config" "github.com/prebid/prebid-server/exchange" @@ -641,6 +643,91 @@ func TestMergeOpenRTBToVideoRequest(t *testing.T) { assert.Equal(t, videoReq.Site.Page, bidReq.Site.Page, "Device.Site.Page is incorrect") } +func TestHandleError(t *testing.T) { + ao := analytics.AuctionObject{ + Status: 200, + Errors: make([]error, 0), + } + + labels := pbsmetrics.Labels{ + Source: pbsmetrics.DemandUnknown, + RType: pbsmetrics.ReqTypeVideo, + PubID: pbsmetrics.PublisherUnknown, + Browser: "test browser", + CookieFlag: pbsmetrics.CookieFlagUnknown, + RequestStatus: pbsmetrics.RequestStatusOK, + } + + recorder := httptest.NewRecorder() + err1 := errors.New("Error for testing handleError 1") + err2 := errors.New("Error for testing handleError 2") + handleError(&labels, recorder, []error{err1, err2}, &ao) + + assert.Equal(t, pbsmetrics.RequestStatusErr, labels.RequestStatus, "labels.RequestStatus should indicate an error") + assert.Equal(t, 500, recorder.Code, "Error status should be written to writer") + assert.Equal(t, 500, ao.Status, "AnalyticsObject should have error status") + assert.Equal(t, 2, len(ao.Errors), "New errors should be appended to AnalyticsObject Errors") + assert.Equal(t, "Error for testing handleError 1", ao.Errors[0].Error(), "Error in AnalyticsObject should have test error message for first error") + assert.Equal(t, "Error for testing handleError 2", ao.Errors[1].Error(), "Error in AnalyticsObject should have test error message for second error") +} + +func TestHandleErrorMetrics(t *testing.T) { + ex := &mockExchangeVideo{} + reqData, err := ioutil.ReadFile("sample-requests/video/video_invalid_sample.json") + if err != nil { + t.Fatalf("Failed to fetch a valid request: %v", err) + } + reqBody := string(getRequestPayload(t, reqData)) + req := httptest.NewRequest("POST", "/openrtb2/video", strings.NewReader(reqBody)) + recorder := httptest.NewRecorder() + + deps, met, mod := mockDepsWithMetrics(t, ex) + deps.VideoAuctionEndpoint(recorder, req, nil) + + assert.Equal(t, int64(0), met.RequestStatuses[pbsmetrics.ReqTypeVideo][pbsmetrics.RequestStatusOK].Count(), "OK requests count should be 0") + assert.Equal(t, int64(1), met.RequestStatuses[pbsmetrics.ReqTypeVideo][pbsmetrics.RequestStatusErr].Count(), "Error requests count should be 1") + assert.Equal(t, 1, len(mod.auctionObjects), "Mock AnalyticsModule should have 1 AuctionObject") + assert.Equal(t, 500, mod.auctionObjects[0].Status, "AnalyticsObject should have 500 status") + assert.Equal(t, 2, len(mod.auctionObjects[0].Errors), "AnalyticsObject should have Errors length of 2") + assert.Equal(t, "request missing required field: PodConfig.DurationRangeSec", mod.auctionObjects[0].Errors[0].Error(), "First error in AnalyticsObject should have message regarding DurationRangeSec") + assert.Equal(t, "request missing required field: PodConfig.Pods", mod.auctionObjects[0].Errors[1].Error(), "Second error in AnalyticsObject should have message regarding Pods") +} + +func mockDepsWithMetrics(t *testing.T, ex *mockExchangeVideo) (*endpointDeps, *pbsmetrics.Metrics, *mockAnalyticsModule) { + theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList(), config.DisabledMetrics{}) + mockModule := &mockAnalyticsModule{} + edep := &endpointDeps{ + ex, + newParamsValidator(t), + &mockVideoStoredReqFetcher{}, + &mockVideoStoredReqFetcher{}, + empty_fetcher.EmptyFetcher{}, + &config.Configuration{MaxRequestSize: maxSize}, + theMetrics, + mockModule, + map[string]string{}, + false, + []byte{}, + openrtb_ext.BidderMap, + } + + return edep, theMetrics, mockModule +} + +type mockAnalyticsModule struct { + auctionObjects []*analytics.AuctionObject +} + +func (m *mockAnalyticsModule) LogAuctionObject(ao *analytics.AuctionObject) { + m.auctionObjects = append(m.auctionObjects, ao) +} + +func (m *mockAnalyticsModule) LogCookieSyncObject(cso *analytics.CookieSyncObject) { return } + +func (m *mockAnalyticsModule) LogSetUIDObject(so *analytics.SetUIDObject) { return } + +func (m *mockAnalyticsModule) LogAmpObject(ao *analytics.AmpObject) { return } + func mockDeps(t *testing.T, ex *mockExchangeVideo) *endpointDeps { theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList(), config.DisabledMetrics{}) edep := &endpointDeps{