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.

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

engagebdr adapter #966

Merged
merged 13 commits into from
Sep 11, 2019
Merged
139 changes: 139 additions & 0 deletions adapters/engagebdr/engagebdr.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
package engagebdr

import (
"encoding/json"
"github.com/prebid/prebid-server/openrtb_ext"
"net/http"

"fmt"

"github.com/mxmCherry/openrtb"
"github.com/prebid/prebid-server/adapters"
"github.com/prebid/prebid-server/errortypes"
)

type EngageBDRAdapter struct {
http *adapters.HTTPAdapter
URI string
}

func (adapter *EngageBDRAdapter) MakeRequests(request *openrtb.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) {

errors := make([]error, 0)

// EngageBDR uses different sspid parameters for banner and video.
sspidImps := make(map[string][]openrtb.Imp)
for _, imp := range request.Imp {
if imp.Native != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this if condition be probably changed to if imp.Native != nil && imp.Site == nil && imp.App == nil in case of possible multiformat requests?

// filter native imps from bid request
continue
}
var bidderExt adapters.ExtImpBidder
if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil {
errors = append(errors, &errortypes.BadInput{
Message: fmt.Sprintf("Ignoring imp id=%s, error while decoding extImpBidder, err: %s", imp.ID, err),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a continue statement make sense after this line? If we have an error for some reason, I believe the following if statement found in line 39 will most likely also print an error (if not an NULL pointer de-reference in line 38) and we'll end up with a stack of errors that have one common source.

Breakpoint 1 set at 0x1626558 for github.com/prebid/prebid-server/adapters/engagebdr.(*EngageBDRAdapter).MakeRequests() ./engagebdr.go:32
(dlv) c
> github.com/prebid/prebid-server/adapters/engagebdr.(*EngageBDRAdapter).MakeRequests() ./engagebdr.go:32 (hits goroutine(21):1 total:1) (PC: 0x1626558)
    27:                 if imp.Native != nil {
    28:                         // filter native imps from bid request
    29:                         continue
    30:                 }
    31:                 var bidderExt adapters.ExtImpBidder
=>  32:                 if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil {
    33:                         errors = append(errors, &errortypes.BadInput{
    34:                                 Message: fmt.Sprintf("Ignoring imp id=%s, error while decoding extImpBidder, err: %s", imp.ID, err),
    35:                         })
    36:                 }
    37:                 impExt := openrtb_ext.ExtImpEngageBDR{}
    38:					err := json.Unmarshal(bidderExt.Bidder, &impExt)
    39:					if err != nil {
(dlv) p bidderExt.Bidder
encoding/json.RawMessage len: 0, cap: 0, nil

}
impExt := openrtb_ext.ExtImpEngageBDR{}
err := json.Unmarshal(bidderExt.Bidder, &impExt)
if err != nil {
errors = append(errors, &errortypes.BadInput{
Message: fmt.Sprintf("Ignoring imp id=%s, error while decoding impExt, err: %s", imp.ID, err),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as line 35 above. Seems like a continue statement would be useful because we'll de-reference impExt in line 44 and there's a possibility that the Unmarshal didn't go well by then.

}
if impExt.Sspid == "" {
errors = append(errors, &errortypes.BadInput{
Message: fmt.Sprintf("Ignoring imp id=%s, no sspid present", imp.ID),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't put a continue statement here, we'd be mapping an empty impExt.Sspid value into sspidImps

}
sspidImps[impExt.Sspid] = append(sspidImps[impExt.Sspid], imp)
}

Copy link
Contributor

@mansinahar mansinahar Jul 19, 2019

Choose a reason for hiding this comment

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

Just a suggestion: Since you're adapter only supports banner and video ads, might be a good idea to add a check somewhere here in this function to discard imps of any other type. I am sure this must be something that you'd be already handling on your end but it's always better to check and fail early.

var adapterRequests []*adapters.RequestData

headers := http.Header{}
headers.Add("Content-Type", "application/json;charset=utf-8")

for sspid, imps := range sspidImps {
if len(imps) > 0 {
// Make a copy as we don't want to change the original request
reqCopy := *request
reqCopy.Imp = imps
reqJSON, err := json.Marshal(reqCopy)
if err != nil {
errors = append(errors, err)
return nil, errors
}
adapterReq := adapters.RequestData{
Method: "POST",
Uri: adapter.URI + "?zoneid=" + sspid,
Body: reqJSON,
Headers: headers,
}
adapterRequests = append(adapterRequests, &adapterReq)
}
}

if len(adapterRequests) == 0 {
errors = append(errors, &errortypes.BadInput{Message: fmt.Sprintf("No imps present")})
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this error message a little bit misleading? There's a possibility that Imp objects were actually present in the request.Imp array but all of them were invalid

return nil, errors
}

return adapterRequests, errors
}

func (adapter *EngageBDRAdapter) MakeBids(internalRequest *openrtb.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
if response.StatusCode == http.StatusNoContent {
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{&errortypes.BadServerResponse{
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", response.StatusCode),
}}
}

var bidResp openrtb.BidResponse
if err := json.Unmarshal(response.Body, &bidResp); err != nil {
return nil, []error{err}
}

bidResponse := adapters.NewBidderResponseWithBidsCapacity(5)

for _, sb := range bidResp.SeatBid {
for i := range sb.Bid {
bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{
Bid: &sb.Bid[i],
BidType: getMediaTypeForImp(sb.Bid[i].ImpID, internalRequest.Imp),
})
}
}
return bidResponse, nil
}

func getMediaTypeForImp(impId string, imps []openrtb.Imp) openrtb_ext.BidType {
mediaType := openrtb_ext.BidTypeBanner
for _, imp := range imps {
if imp.ID == impId {
if imp.Banner == nil && imp.Video != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The Imp object also has an Audio and Native fields which are the other two data types supported by prebid-server but this particular adapter, according to static/bidder-info/engagebdr.yaml
does not support them. Therefore, the following scenarios might set the mediaType to openrtb_ext.BidTypeBanner. Is the behavior we want? Maybe we want to modify the if statement

Imp[i].Banner | Imp[i].Video | Imp[i].Audio | Imp[i].Native  |  mediaType 
--------------|--------------|--------------|----------------|-------------
    nil       |     nil      |    not nil   |     nil        |openrtb_ext.BidTypeBanner
    nil       |     nil      |      nil     |     nil        |openrtb_ext.BidTypeBanner

mediaType = openrtb_ext.BidTypeVideo
}
return mediaType
}
}
return mediaType
}

func NewEngageBDRBidder(client *http.Client, endpoint string) *EngageBDRAdapter {
adapter := &adapters.HTTPAdapter{Client: client}
return &EngageBDRAdapter{
http: adapter,
URI: endpoint,
}
}
11 changes: 11 additions & 0 deletions adapters/engagebdr/engagebdr_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package engagebdr

import (
"github.com/prebid/prebid-server/adapters/adapterstest"
"net/http"
"testing"
)

func TestJsonSamples(t *testing.T) {
adapterstest.RunJSONBidderTest(t, "engagebdrtest", NewEngageBDRBidder(new(http.Client), "http://dsp.bnmla.com/hb"))
}
89 changes: 89 additions & 0 deletions adapters/engagebdr/engagebdrtest/exemplary/banner.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
{
"mockBidRequest": {
"id": "test-request-id",
"imp": [
{
"id": "test-imp-id",
"banner": {
"w": 300,
"h": 250
},
"ext": {
"bidder": {
"sspid": "99999"
}
}
}
]
},

"httpCalls": [
{
"expectedRequest": {
"uri": "http://dsp.bnmla.com/hb?zoneid=99999",
"body":{
"id": "test-request-id",
"imp": [{
"id": "test-imp-id",
"banner": {
"w": 300,
"h": 250
},
"ext": {
"bidder": {
"sspid":"99999"
}
}
}]
}
},
"mockResponse": {
"status": 200,
"body": {
"id": "test-request-id",
"seatbid": [
{
"bid": [
],
"seat": "test-request-id"
}
],
"bidid": "test-request-id",
"cur": "USD"
}
}
}
],

"expectedBidResponses": [
{
"id": "test-request-id",
"seatbid": [
{
"bid": [
{
"id": "test-imp-id",
"impid": "test-imp-id",
"price": 9.81,
"adid": "abcde-12345",
"adm": "<div><img src=\"https://cdn0.bnmla.com/0b1c6e85e9376e3092df8c9fc8ab9095.gif \" width=350 height=250 /></div>",
"adomain":[
"advertiserdomain.com"
],
"iurl": "http://match.bnmla.com/usersync?sspid=59&redir=",
"cid": "campaign1",
"crid": "abcde-12345",
"w": 300,
"h": 250
}
],
"seat": "test-request-id"
}
],
"bidid": "test-request-id",
"cur": "USD"

}
]
}

91 changes: 91 additions & 0 deletions adapters/engagebdr/engagebdrtest/exemplary/video.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
{
"mockBidRequest": {
"id": "test-request-id",
"imp": [
{
"id": "test-imp-id",
"video": {
"w": 300,
"mimes": null,
"h": 250
},
"ext": {
"bidder": {
"sspid":"99998"
}
}
}
]
},

"httpCalls": [
{
"expectedRequest": {
"uri": "http://dsp.bnmla.com/hb?zoneid=99998",
"body":{
"id": "test-request-id",
"imp": [{
"id": "test-imp-id",
"video": {
"w": 300,
"mimes": null,
"h": 250
},
"ext": {
"bidder": {
"sspid":"99998"
}
}
}]
}
},
"mockResponse": {
"status": 200,
"body": {
"id": "test-request-id",
"seatbid": [
{
"bid": [
],
"seat": "test-request-id"
}
],
"bidid": "test-request-id",
"cur": "USD"
}
}
}
],

"expectedBidResponses": [
{
"id": "test-request-id",
"seatbid": [
{
"bid": [
{
"id": "test-imp-id",
"impid": "test-imp-id",
"price": 9.81,
"adid": "abcde-12345",
"adm": "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<VAST version=\"3.0\"><Ad id=\"static\"><InLine><AdSystem>Static VAST</AdSystem><AdTitle>Static VAST Tag</AdTitle><Error><![CDATA[https://video.bnmla.com/count?vTracking=ERROR&vStatus=[ERRORCODE]&vBaId=6&vZId=9100&VGuid=eng-c719ae10-2349-4c6f-9b93-1bb872e2f13d]]></Error><Impression><![CDATA[https://video.bnmla.com/count?vTracking=IMPRESSION&vStatus=61&vBaId=6&vZId=9100&VGuid=eng-c719ae10-2349-4c6f-9b93-1bb872e2f13d]]></Impression><Creatives><Creative><Linear><Duration><![CDATA[00:00:15]]></Duration><TrackingEvents><Tracking event=\"start\" /><Tracking event=\"firstQuartile\" /><Tracking event=\"midpoint\" /><Tracking event=\"thirdQuartile\" /><Tracking event=\"complete\" /><Tracking event=\"pause\" /><Tracking event=\"mute\" /><Tracking event=\"fullscreen\" /><Tracking event=\"start\"><![CDATA[https://video.bnmla.com/count?vTracking=START&vStatus=61&vBaId=6&vZId=9100&VGuid=eng-c719ae10-2349-4c6f-9b93-1bb872e2f13d]]></Tracking></TrackingEvents><VideoClicks><ClickThrough><![CDATA[http://www.engagebdr.com/]]></ClickThrough><ClickTracking /><ClickTracking><![CDATA[https://video.bnmla.com/count?vTracking=VIDEOCLICKING&vStatus=61&vBaId=6&vZId=9100&VGuid=eng-c719ae10-2349-4c6f-9b93-1bb872e2f13d]]></ClickTracking></VideoClicks><MediaFiles><MediaFile delivery=\"progressive\" type=\"video/mp4\" bitrate=\"350\" width=\"428\" height=\"240\"><![CDATA[http://cdnin.bnmla.com/vsbmedia/AEX_Outdoors_HD_ENG_15_exp2016-01-21_sm.mp4]]></MediaFile><MediaFile delivery=\"progressive\" type=\"video/mp4\" bitrate=\"700\" width=\"640\" height=\"360\"><![CDATA[http://cdnin.bnmla.com/vsbmedia/AEX_Outdoors_HD_ENG_15_exp2016-01-21_md.mp4]]></MediaFile><MediaFile delivery=\"progressive\" type=\"video/mp4\" bitrate=\"1600\" width=\"1024\" height=\"576\"><![CDATA[http://cdnin.bnmla.com/vsbmedia/AEX_Outdoors_HD_ENG_15_exp2016-01-21_lg.mp4]]></MediaFile></MediaFiles></Linear></Creative></Creatives></InLine></Ad></VAST>",
"adomain":[
"advertiserdomain.com"
],
"iurl": "https://cdn0.bnmla.com/vtest.xml",
"cid": "campaign1",
"crid": "abcde-12345",
"w": 300,
"h": 250
}
],
"seat": "test-request-id"
}
],
"bidid": "test-request-id",
"cur": "USD"

}
]
}

3 changes: 3 additions & 0 deletions adapters/engagebdr/engagebdrtest/params/race/banner.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"zoneid": "10589"
}
3 changes: 3 additions & 0 deletions adapters/engagebdr/engagebdrtest/params/race/video.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"zoneid": "10592"
}
43 changes: 43 additions & 0 deletions adapters/engagebdr/params_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package engagebdr

import (
"encoding/json"
"testing"

"github.com/prebid/prebid-server/openrtb_ext"
)

func TestValidParams(t *testing.T) {
validator, err := openrtb_ext.NewBidderParamsValidator("../../static/bidder-params")
if err != nil {
t.Fatalf("Failed to fetch the json-schemas. %v", err)
}

for _, validParam := range validParams {
if err := validator.Validate(openrtb_ext.BidderEngageBDR, json.RawMessage(validParam)); err != nil {
t.Errorf("Schema rejected beachfront params: %s", validParam)
}
}
}

func TestInvalidParams(t *testing.T) {
validator, err := openrtb_ext.NewBidderParamsValidator("../../static/bidder-params")
if err != nil {
t.Fatalf("Failed to fetch the json-schemas. %v", err)
}

for _, invalidParam := range invalidParams {
if err := validator.Validate(openrtb_ext.BidderEngageBDR, json.RawMessage(invalidParam)); err == nil {
t.Errorf("Schema allowed unexpected params: %s", invalidParam)
}
}
}

var validParams = []string{
`{"sspid":"12345"}`,
}

var invalidParams = []string{
`{"sspid":null}`,
mansinahar marked this conversation as resolved.
Show resolved Hide resolved
`{"appId":"11bc5dd5-7421-4dd8-c926-40fa653bec76"}`,
}
11 changes: 11 additions & 0 deletions adapters/engagebdr/usersync.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package engagebdr

import (
"github.com/prebid/prebid-server/adapters"
"github.com/prebid/prebid-server/usersync"
"text/template"
)

func NewEngageBDRSyncer(temp *template.Template) usersync.Usersyncer {
return adapters.NewSyncer("engagebdr", 62, temp, adapters.SyncTypeIframe)
}
Loading