Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updated handleError arguments to be pointers for video endpoint #1128

Merged
merged 3 commits into from
Dec 4, 2019
Merged

Updated handleError arguments to be pointers for video endpoint #1128

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c4395c
Updated handleError arguments to be pointers for video endpoint
Nov 26, 2019
ef3e417
Removing unneeded pointer to http.ResponseWriter
Dec 3, 2019
5f51755
Adding units test for update to handleError
Dec 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions endpoints/openrtb2/video_auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -102,35 +102,35 @@ 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
}

var bidReq = &openrtb.BidRequest{}
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
}
}
Expand All @@ -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
}

Expand All @@ -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
}

Expand Down Expand Up @@ -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
Expand All @@ -203,15 +203,15 @@ 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
}

//build simplified response
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 {
Expand All @@ -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
}

Expand All @@ -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
Expand All @@ -255,9 +255,9 @@ func handleError(labels pbsmetrics.Labels, w http.ResponseWriter, errL []error,
}
errors = fmt.Sprintf("%s %s", errors, er.Error())
}
w.WriteHeader(status)
(*w).WriteHeader(status)
ao.Status = status
fmt.Fprintf(w, "Critical error while running the video endpoint: %v", errors)
fmt.Fprintf(*w, "Critical error while running the video endpoint: %v", errors)
glog.Errorf("/openrtb2/video Critical error: %v", errors)
ao.Errors = append(ao.Errors, errL...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pointers you added are a great catch but I have a question. I tried the following test:

718 func TestHandleError(t *testing.T) {
719     testHandler := func(w http.ResponseWriter, r *http.Request) {
720         fmt.Println("[TestHandleError] function start")
721         labels := pbsmetrics.Labels{
722             Source:        pbsmetrics.DemandUnknown,
723             RType:         pbsmetrics.ReqTypeVideo,
724             PubID:         pbsmetrics.PublisherUnknown,
725             Browser:       pbsmetrics.BrowserSafari,
726             CookieFlag:    pbsmetrics.CookieFlagUnknown,
727             RequestStatus: pbsmetrics.RequestStatusOK,
728         }
729         err := &errortypes.BadInput{Message: "Unable to find required stored request id"}
730         ao := analytics.AuctionObject{
731             Status: http.StatusOK,
732             Errors: make([]error, 0),
733         }
734
735         handleError(&labels, &w, []error{err}, &ao)
736
737         fmt.Printf("[TestHandleError] print labels.RequestStatus = %v \n", labels.RequestStatus)
738         fmt.Printf("[TestHandleError] print ao.Status = %v \n", ao.Status)
739         fmt.Printf("[TestHandleError] print ao.Errors = %v \n", ao.Errors)
740         fmt.Printf("[TestHandleError] print w.Header() = %v \n", w.Header())
741         fmt.Println("[TestHandleError] function end")
742         return
743     }
744     http.HandleFunc("/testEndpoint", testHandler)
745     log.Fatal(http.ListenAndServe(":8080", nil))
746 }
video_auction_test.go

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and I get the following:

╰─➤  dlv test
# github.com/prebid/prebid-server/endpoints/openrtb2 [github.com/prebid/prebid-server/endpoints/openrtb2.test]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Googled this thing and found a post in stackoverflow (https://stackoverflow.com/questions/22157514/passing-http-responsewriter-by-value-or-reference?rq=1) mentioning interfaces are pointers and no pointer is needed in functions similar to handleError. Did you get different results?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I actually did not catch that interfaces do not need explicit pointers. I'll update this in a moment to remove that as it is not necessary from the info you posted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the pointer to the ResponseWriter. Thank you for catching that!

}
Expand Down