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

Improved error handling on Beachfront adapter #873

Merged
merged 8 commits into from
May 20, 2019
Merged

Improved error handling on Beachfront adapter #873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1eb50e8
Removed a redundant error message that was causing some confusion.
Apr 8, 2019
ac3d6da
trying to turn '[]' into null and 200 inti 404
Apr 9, 2019
73fc7f0
not a goot path
Apr 9, 2019
b2a73f1
looking at the status codes
Apr 9, 2019
8422a0b
I think I have covers all these bases.
Apr 9, 2019
43a85af
checking for empty array response a failing sanely
Apr 9, 2019
bf20d8b
removed an attempt to pointlessly reset the status code.
Apr 9, 2019
963ddd6
corrected and added test cases
Apr 9, 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
57 changes: 46 additions & 11 deletions adapters/beachfront/beachfront.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ package beachfront
import (
"encoding/json"
"errors"
"fmt"
"github.com/prebid/prebid-server/errortypes"
"net/http"
"strconv"
"strings"

"github.com/prebid/prebid-server/adapters"
"github.com/prebid/prebid-server/errortypes"
"github.com/prebid/prebid-server/openrtb_ext"

"github.com/mxmCherry/openrtb"
Expand All @@ -22,7 +24,7 @@ const VideoEndpoint = "https://reachms.bfmio.com/bid.json?exchange_id="
const VideoEndpointSuffix = "&prebidserver"

const beachfrontAdapterName = "BF_PREBID_S2S"
const beachfrontAdapterVersion = "0.2.2"
const beachfrontAdapterVersion = "0.3.0"

type BeachfrontAdapter struct {
}
Expand Down Expand Up @@ -140,15 +142,14 @@ func (a *BeachfrontAdapter) MakeRequests(request *openrtb.BidRequest) ([]*adapte
var beachfrontRequests BeachfrontRequests
var reqJSON []byte
var uri string
var errs = make([]error, 0)
var errs = make([]error, 0, len(request.Imp))
var err error
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it's uncommon to have three or more Imps in a given request but, according to the code flow, I believe We'll have at most two Error object elements in this slice, so maybe we are wastiong memory by assigning more than we need whenever we have three or more Imps in a request

var imps int

uri = getEndpoint(request)

beachfrontRequests, errs, imps = preprocess(request, uri)

// These are fatal errors -------------
if uri == VideoEndpoint {
reqJSON, err = json.Marshal(beachfrontRequests.Video)
uri = uri + beachfrontRequests.Video.AppId + VideoEndpointSuffix
Expand All @@ -175,6 +176,15 @@ func (a *BeachfrontAdapter) MakeRequests(request *openrtb.BidRequest) ([]*adapte
headers.Add("Content-Type", "application/json;charset=utf-8")
headers.Add("Accept", "application/json")

if request.Device != nil {
addHeaderIfNonEmpty(headers, "User-Agent", request.Device.UA)
addHeaderIfNonEmpty(headers, "X-Forwarded-For", request.Device.IP)
addHeaderIfNonEmpty(headers, "Accept-Language", request.Device.Language)
if request.Device.DNT != nil {
addHeaderIfNonEmpty(headers, "DNT", strconv.Itoa(int(*request.Device.DNT)))
}
}

return []*adapters.RequestData{{
Method: "POST",
Uri: uri,
Expand Down Expand Up @@ -288,9 +298,6 @@ func getBannerRequest(req *openrtb.BidRequest) (BeachfrontBannerRequest, []error
return beachfrontReq, errs, imps
}

/*
Prepare the request that has been received from Prebid.js, translating it to the beachfront format
*/
func getVideoRequest(req *openrtb.BidRequest) (BeachfrontVideoRequest, []error, int) {
var videoImpsIndex = 0
var beachfrontReq = NewBeachfrontVideoRequest()
Expand Down Expand Up @@ -324,7 +331,7 @@ func getVideoRequest(req *openrtb.BidRequest) (BeachfrontVideoRequest, []error,
The req could contain banner,audio,native and video imps when It arrives here. I am only
interested in video

The beach front video endpoint is only capable of returning a single nurl and price, wrapped in
The beachfront video endpoint is only capable of returning a single nurl and price, wrapped in
an openrtb format, so even though I'm building a request here that will include multiple video
impressions, only a single URL will be returned. Hopefully the beachfront endpoint can be modified
in the future to return multiple video ads
Expand Down Expand Up @@ -394,12 +401,33 @@ func getVideoRequest(req *openrtb.BidRequest) (BeachfrontVideoRequest, []error,
func (a *BeachfrontAdapter) MakeBids(internalRequest *openrtb.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
var bids []openrtb.Bid
var bidtype = getBidType(internalRequest)

/*
Beachfront is now sending an empty array and 200 as their "no results" response. This should catch that.
*/

if response.StatusCode == http.StatusOK && len(response.Body) <= 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we can't just check len(response.Body) == 0 rather than len(response.Body) <= 2 ?

return nil, nil
}

if response.StatusCode == http.StatusNoContent {
return nil, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reduce:

409     if response.StatusCode == http.StatusOK && len(response.Body) <= 2 {
410         return nil, nil
411     }
412
413     if response.StatusCode == http.StatusNoContent {
414         return nil, nil
415     }
416

To a single statement?

if (response.StatusCode == http.StatusNoContent) || (response.StatusCode == http.StatusOK && len(response.Body) <= 2) {
      return nil, nil
}

if response.StatusCode == http.StatusBadRequest {
return nil, []error{&errortypes.BadInput{
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", response.StatusCode),
}}
}

if response.StatusCode != http.StatusOK {
return nil, []error{fmt.Errorf("Unexpected status code: %d. Run with request.debug = 1 for more info", response.StatusCode)}
}

bids, errs := postprocess(response, externalRequest, internalRequest.ID, bidtype)

if len(errs) != 0 {
return nil, append(errs, &errortypes.BadServerResponse{
Message: "Failed to process the beachfront response",
})
return nil, errs
}

bidResponse := adapters.NewBidderResponseWithBidsCapacity(BidCapacity)
Expand Down Expand Up @@ -499,6 +527,13 @@ func extractVideoCrid(nurl string) string {
return strings.TrimSuffix(chunky[2], ":")
}

// Thank you, brightroll.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I ask what's the purpose of this comment? If it's just a leftover of some sort, can we please remove it? Thanks!

func addHeaderIfNonEmpty(headers http.Header, headerName string, headerValue string) {
if len(headerValue) > 0 {
headers.Add(headerName, headerValue)
}
}

func NewBeachfrontBidder() *BeachfrontAdapter {
return &BeachfrontAdapter{}
}
2 changes: 1 addition & 1 deletion adapters/beachfront/beachfronttest/exemplary/minimal-banner.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
"dnt": 0,
"ua": "",
"adapterName": "BF_PREBID_S2S",
"adapterVersion": "0.2.2",
"adapterVersion": "0.3.0",
"user": {
}
}
Expand Down
1 change: 1 addition & 0 deletions adapters/beachfront/beachfronttest/exemplary/simple-video.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
}
},
"mockResponse": {
"status" : 200,
"body": {
"id":"61b87329-8790-47b7-90dd-c53ae7ce1723",
"seatBid":[
Expand Down
72 changes: 72 additions & 0 deletions adapters/beachfront/beachfronttest/supplemental/minimal-banner-empty_array-200.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
{
"mockBidRequest": {
"id": "some_test_ad",
"site": {
"page": "https://test.opposingviews.com/i/society/republican-sen-collins-may-change-vote-tax-bill?cb=1234534"
},
"imp": [
{
"bidfloor": 0.02,
"banner": {
"format": [
{
"w": 300,
"h": 250
}
]
},
"ext": {
"bidder": {
"bidfloor": 0.02,
"appId": "00000000-1111-2222-3333-11111111111"
}
}
}
]
},

"httpCalls": [
{
"expectedRequest": {
"uri": "https://display.bfmio.com/prebid_display",
"body": {
"slots": [
{
"slot": "",
"id": "00000000-1111-2222-3333-11111111111",
"bidfloor": 0.02,
"sizes": [
{
"w": 300,
"h": 250
}
]
}
],
"domain": "test.opposingviews.com",
"page": "https://test.opposingviews.com/i/society/republican-sen-collins-may-change-vote-tax-bill?cb=1234534",
"referrer": "",
"search": "",
"secure": 0,
"requestId": "some_test_ad",
"isMobile": 0,
"ip": "",
"deviceModel": "",
"deviceOs": "",
"dnt": 0,
"ua": "",
"adapterName": "BF_PREBID_S2S",
"adapterVersion": "0.3.0",
"user": {
}
}
},
"mockResponse": {
"status": 200,
"body": []
}
}
],

"expectedBids": []
}
2 changes: 1 addition & 1 deletion adapters/beachfront/beachfronttest/supplemental/minimal-mobile-banner.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
"dnt": 0,
"ua": "",
"adapterName": "BF_PREBID_S2S",
"adapterVersion": "0.2.2",
"adapterVersion": "0.3.0",
"user": {
}
}
Expand Down
1 change: 1 addition & 0 deletions adapters/beachfront/beachfronttest/supplemental/minimal-mobile-video.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
}
},
"mockResponse": {
"status": 200,
"body": {
"id":"61b87329-8790-47b7-90dd-c53ae7ce1723",
"seatBid":[
Expand Down
2 changes: 1 addition & 1 deletion adapters/beachfront/beachfronttest/supplemental/minimal-site-banner.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
"dnt": 0,
"ua": "",
"adapterName": "BF_PREBID_S2S",
"adapterVersion": "0.2.2",
"adapterVersion": "0.3.0",
"user": {
}
}
Expand Down
2 changes: 1 addition & 1 deletion adapters/beachfront/beachfronttest/supplemental/multi-banner.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@
"dnt": 1,
"ua": "Opera/9.80 (X11; Linux i686; Ubuntu/14.10) Presto/2.12.388 Version/12.16",
"adapterName": "BF_PREBID_S2S",
"adapterVersion": "0.2.2",
"adapterVersion": "0.3.0",
"user": {
"buyeruid": "some-buyer",
"id": "some-user"
Expand Down
1 change: 1 addition & 0 deletions adapters/beachfront/beachfronttest/supplemental/multi-mix.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@
}
},
"mockResponse": {
"status": 200,
"body": {
"id":"61b87329-8790-47b7-90dd-c53ae7ce1723",
"seatBid":[
Expand Down
1 change: 1 addition & 0 deletions adapters/beachfront/beachfronttest/supplemental/multi-video.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
}
},
"mockResponse": {
"status": 200,
"body": {
"id":"61b87329-8790-47b7-90dd-c53ae7ce1723",
"seatBid":[
Expand Down