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

Added new adapter for CPMStar ad network banners and video #1159

Merged
merged 6 commits into from
Jan 16, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
149 changes: 149 additions & 0 deletions adapters/cpmstar/cpmstar.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
package cpmstar

import (
"encoding/json"
"fmt"
"net/http"

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

type Adapter struct {
endpoint string
}

func (a *Adapter) MakeRequests(request *openrtb.BidRequest, unused *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) {
var errs []error
var adapterRequests []*adapters.RequestData

if err := preprocess(request); err != nil {
errs = append(errs, err)
return nil, errs
}

adapterReq, err := a.makeRequest(request)
if err != nil {
errs = append(errs, err)
return nil, errs
}

adapterRequests = append(adapterRequests, adapterReq)

return adapterRequests, errs
}

func (a *Adapter) makeRequest(request *openrtb.BidRequest) (*adapters.RequestData, error) {
var err error

jsonBody, err := json.Marshal(request)
if err != nil {
return nil, err
}

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

return &adapters.RequestData{
Method: "POST",
Uri: a.endpoint,
Body: jsonBody,
Headers: headers,
}, nil
}

func preprocess(request *openrtb.BidRequest) error {
for i := 0; i < len(request.Imp); i++ {
var imp = request.Imp[i]
var bidderExt adapters.ExtImpBidder

if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil {
return &errortypes.BadInput{
Message: err.Error(),
}
}

if err := validateImp(&request.Imp[i]); err != nil {
JoshuaMGoldstein marked this conversation as resolved.
Show resolved Hide resolved
return err
}

var extImp openrtb_ext.ExtImpCpmstar
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you're using json.Unmarshal to validate the extension content is valid json. Consider passing it through as it and letting your servers reject invalid extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error responses are going to be a lot more informative here if the user uses a quoted string for placementId instead of a number, for example. So we'd prefer to maintain the unmarshalling here. I've removed the re-marshalling, per your other comment.

if err := json.Unmarshal(bidderExt.Bidder, &extImp); err != nil {
return &errortypes.BadInput{
Message: err.Error(),
}
}

request.Imp[i].Ext = bidderExt.Bidder
}

return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

If the request *openrtb.BidRequest argument passed to this function came with an empty Imp array, we could probably be sending out an empty request. Are sure we don't want to return an error in case len(request.Imp) == 0?

}

func validateImp(imp *openrtb.Imp) error {
if imp.Banner == nil && imp.Video == nil {
return &errortypes.BadInput{
Message: "Only Banner and Video bid-types are supported at this time",
}
}
return nil
}

// MakeBids based on cpmstar server response
func (a *Adapter) MakeBids(bidRequest *openrtb.BidRequest, unused *adapters.RequestData, responseData *adapters.ResponseData) (*adapters.BidderResponse, []error) {
if responseData.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.

Other adapters return a BadInput error when response.StatusCode == http.StatusBadRequest do we want that in our context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done on the next line where it checks if responseData.StatusCode != http.StatusOK {

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

var bidResponse openrtb.BidResponse

if err := json.Unmarshal(responseData.Body, &bidResponse); err != nil {
return nil, []error{err}
Copy link
Contributor

Choose a reason for hiding this comment

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

Other adapters also throw an errortypes.BadServerResponse error when Unmarshal goes wrong here, would that also be desirable?

}

rv := adapters.NewBidderResponseWithBidsCapacity(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we only allocate capacity for one bid? Do we not expect more than one matching bid inside the bidResponse?

var errors []error

for _, seatbid := range bidResponse.SeatBid {
for _, bid := range seatbid.Bid {
foundMatchingBid := false
bidType := openrtb_ext.BidTypeBanner
for _, imp := range bidRequest.Imp {
if imp.ID == bid.ImpID {
foundMatchingBid = true
if imp.Banner != nil {
bidType = openrtb_ext.BidTypeBanner
} else if imp.Video != nil {
bidType = openrtb_ext.BidTypeVideo
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to break the for loop once the matching bid we are expecting is found?

}
}

if foundMatchingBid {
rv.Bids = append(rv.Bids, &adapters.TypedBid{
Bid: &bid,
BidType: bidType,
})
} else {
errors = append(errors, &errortypes.BadServerResponse{
Message: fmt.Sprintf("bid id='%s' could not find valid impid='%s'", bid.ID, bid.ImpID),
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be desirable to set a test case up cover the scenario where no matching bid was found. Maybe even when a request returning a StatusCode different than StatusOK
image

}
}
return rv, errors
}

func NewCpmstarBidder(endpoint string) *Adapter {
return &Adapter{
endpoint: endpoint,
}
}
11 changes: 11 additions & 0 deletions adapters/cpmstar/cpmstar_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package cpmstar

import (
"testing"

"github.com/prebid/prebid-server/adapters/adapterstest"
)

func TestJsonSamples(t *testing.T) {
adapterstest.RunJSONBidderTest(t, "cpmstartest", NewCpmstarBidder("//host"))
}
154 changes: 154 additions & 0 deletions adapters/cpmstar/cpmstartest/exemplary/banner-and-video.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
{
"mockBidRequest": {
"id": "test-request-id",
"imp": [
{
"id": "test-banner-imp-id",
"banner": {
"format": [
{
"w": 300,
"h": 250
}
]
},
"ext": {
"bidder": {
"placementId": 154,
"subpoolId": 123
}
}
},
{
"id": "test-video-imp-id",
"video": {
"mimes": [
"video/mp4"
],
"protocols": [
2,
5
],
"w": 640,
"h": 480
},
"ext": {
"bidder": {
"placementId": 154,
"subpoolId": 654
}
}
}
],
"site": {
"id": "fake-site-id"
}
},
"httpCalls": [
{
"expectedRequest": {
"uri": "//host",
"body": {
"id": "test-request-id",
"imp": [
{
"id": "test-banner-imp-id",
"banner": {
"format": [
{
"h": 250,
"w": 300
}
]
},
"ext": {
"placementId": 154,
"subpoolId": 123
}
},
{
"id": "test-video-imp-id",
"video": {
"h": 480,
"mimes": [
"video/mp4"
],
"protocols": [
2,
5
],
"w": 640
},
"ext": {
"placementId": 154,
"subpoolId": 654
}
}
],
"site": {
"id": "fake-site-id"
}
}
},
"mockResponse": {
"status": 200,
"body": {
"id": "test-request-id",
"seatbid": [
{
"seat": "cpmstar",
"bid": [
{
"id": "8ee514f1-b2b8-4abb-89fd-084437d1e800",
"impid": "test-video-imp-id",
"price": 0.5,
"adm": "some-test-ad",
"crid": "crid_10",
"h": 250,
"w": 300
}
]
}
],
"cur": "USD"
}
}
}
],
"expectedBids": [
{
"bid": {
"id": "7706636740145184841",
"impid": "test-imp-id",
"price": 0.5,
"adm": "some-test-ad",
"adid": "29681110",
"adomain": [
"sample.com"
],
"cid": "958",
"crid": "29681110",
"w": 1024,
"h": 576
},
"type": "banner"
},
{
"bid": {
"id": "8ee514f1-b2b8-4abb-89fd-084437d1e800",
"impid": "test-imp-video-id",
"price": 0.5,
"adm": "some-test-ad",
"adid": "29484110",
"adomain": [
"sample.com"
],
"cid": "958",
"crid": "29484110",
"w": 1024,
"h": 576
},
"type": "video"
}
]
}
Loading