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

RP adapter: use video placement parameter to set size ID #1607

Merged
merged 4 commits into from
Dec 10, 2020
Merged

RP adapter: use video placement parameter to set size ID #1607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dcdf776
RP adapter: use video placement parameter to set size ID
Dec 3, 2020
4ee8062
Fixes after review
Dec 7, 2020
b7d1840
Fixes after review
Dec 9, 2020
083059c
Fixes after review
Dec 9, 2020
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
50 changes: 41 additions & 9 deletions adapters/rubicon/rubicon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,18 @@ func (a *RubiconAdapter) Call(ctx context.Context, req *pbs.PBSRequest, bidder *
rubiReq.Device = &deviceCopy

if thisImp.Video != nil {
videoExt := rubiconVideoExt{Skip: params.Video.Skip, SkipDelay: params.Video.SkipDelay, RP: rubiconVideoExtRP{SizeID: params.Video.VideoSizeID}}

videoSizeId := params.Video.VideoSizeID
if videoSizeId == 0 {
resolvedSizeId, err := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl, thisImp.ID)
if err == nil {
videoSizeId = resolvedSizeId
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why is the error behavior for the legacy auction endpoint different from that of the openrtb endpoint i.e. in this case, even if you get an error while resolving the video size ID, you just let it be 0 and move on with processing as usual but in case of openrtb, you actually discard that imp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if rubiconExt.Video.VideoSizeID == 0 { errs = append(errs, &errortypes.BadInput{ Message: fmt.Sprintf("imp[%d].ext.bidder.rubicon.video.size_id must be defined for video impression", i), }) continue
this was previous logic in non legacy, haven't found similar in legacy

Copy link
Contributor

Choose a reason for hiding this comment

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

I see but if your bidder expects a non-zero sizeID and will return an error for requests with an empty/invalid video sizeID then I'd suggest returning an error here and failing fast as you do for non-legacy rather than sending the request to the bidder and the bidder then returning an error back. That will be inefficient use of resources.

Also, I'd assume the behavior to be the same as both in legacy and non-legacy endpoints unless your system has a way to differentiate between the two.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooops! Looks like I missed your update to continue in an error situation for the legacy endpoint as well. Thanks!

} else {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In line 496, if the error turns out to not be nil, do we want to continue as we do in the check in line 505 below?

491         if thisImp.Video != nil {
492
493             videoSizeId := params.Video.VideoSizeID
494             if videoSizeId == 0 {
495                 resolvedSizeId, err := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl, thisImp.ID)
496                 if err == nil {
497                     videoSizeId = resolvedSizeId
498 -               }
    +               } else { // err != nil
    +                   continue
    +               } 
499             }
500
501             videoExt := rubiconVideoExt{Skip: params.Video.Skip, SkipDelay: params.Video.SkipDelay, RP: rubiconVideoExtRP{SizeID: videoSizeId}}
502             thisImp.Video.Ext, err = json.Marshal(&videoExt)
503         } else {
504             primarySizeID, altSizeIDs, err := parseRubiconSizes(unit.Sizes)
505             if err != nil {
506                 continue
507             }
adapters/rubicon/rubicon.go

}

videoExt := rubiconVideoExt{Skip: params.Video.Skip, SkipDelay: params.Video.SkipDelay, RP: rubiconVideoExtRP{SizeID: videoSizeId}}
thisImp.Video.Ext, err = json.Marshal(&videoExt)
} else {
primarySizeID, altSizeIDs, err := parseRubiconSizes(unit.Sizes)
Expand Down Expand Up @@ -601,6 +612,24 @@ func (a *RubiconAdapter) Call(ctx context.Context, req *pbs.PBSRequest, bidder *
return bids, nil
}

func resolveVideoSizeId(placement openrtb.VideoPlacementType, instl int8, impId string) (sizeID int, err error) {
if placement != 0 {
if placement == 1 {
return 201, nil
}
if placement == 3 {
return 203, nil
}
}

if instl == 1 {
return 202, nil
}
return 0, &errortypes.BadInput{
Message: fmt.Sprintf("video.size_id can not be resolved in impression with id : %s", impId),
}
}

func appendTrackerToUrl(uri string, tracker string) (res string) {
// Append integration method. Adapter init happens once
urlObject, err := url.Parse(uri)
Expand Down Expand Up @@ -771,21 +800,24 @@ func (a *RubiconAdapter) MakeRequests(request *openrtb.BidRequest, reqInfo *adap

isVideo := isVideo(thisImp)
if isVideo {
if rubiconExt.Video.VideoSizeID == 0 {
errs = append(errs, &errortypes.BadInput{
Message: fmt.Sprintf("imp[%d].ext.bidder.rubicon.video.size_id must be defined for video impression", i),
})
continue
videoCopy := *thisImp.Video

videoSizeId := rubiconExt.Video.VideoSizeID
if videoSizeId == 0 {
resolvedSizeId, err := resolveVideoSizeId(thisImp.Video.Placement, thisImp.Instl, thisImp.ID)
if err != nil {
errs = append(errs, err)
continue
}
videoSizeId = resolvedSizeId
}

// if imp.ext.is_rewarded_inventory = 1, set imp.video.ext.videotype = "rewarded"
var videoType = ""
if bidderExt.Prebid != nil && bidderExt.Prebid.IsRewardedInventory == 1 {
videoType = "rewarded"
}

videoCopy := *thisImp.Video
videoExt := rubiconVideoExt{Skip: rubiconExt.Video.Skip, SkipDelay: rubiconExt.Video.SkipDelay, VideoType: videoType, RP: rubiconVideoExtRP{SizeID: rubiconExt.Video.VideoSizeID}}
videoExt := rubiconVideoExt{Skip: rubiconExt.Video.Skip, SkipDelay: rubiconExt.Video.SkipDelay, VideoType: videoType, RP: rubiconVideoExtRP{SizeID: videoSizeId}}
videoCopy.Ext, err = json.Marshal(&videoExt)
thisImp.Video = &videoCopy
thisImp.Banner = nil
Expand Down
49 changes: 48 additions & 1 deletion adapters/rubicon/rubicon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"context"
"encoding/json"
"github.com/prebid/prebid-server/errortypes"
"io/ioutil"
"net/http"
"net/http/httptest"
Expand Down Expand Up @@ -278,7 +279,7 @@ func TestRubiconBasicResponse(t *testing.T) {

bids, err := an.Call(ctx, pbReq, pbReq.Bidders[0])
assert.Nil(t, err, "Should not have gotten an error: %v", err)
assert.Equal(t, 3, len(bids), "Received %d bids instead of 3", len(bids))
assert.Equal(t, 2, len(bids), "Received %d bids instead of 3", len(bids))

for _, bid := range bids {
matched := false
Expand Down Expand Up @@ -524,6 +525,52 @@ func TestAppendTracker(t *testing.T) {
}
}

func TestResolveVideoSizeId(t *testing.T) {
testScenarios := []struct {
placement openrtb.VideoPlacementType
instl int8
impId string
expected int
expectedErr error
}{
{
placement: 1,
instl: 1,
impId: "impId",
expected: 201,
expectedErr: nil,
},
{
placement: 3,
instl: 1,
impId: "impId",
expected: 203,
expectedErr: nil,
},
{
placement: 4,
instl: 1,
impId: "impId",
expected: 202,
expectedErr: nil,
},
{
placement: 4,
instl: 3,
impId: "impId",
expectedErr: &errortypes.BadInput{
Message: "video.size_id can not be resolved in impression with id : impId",
},
},
}

for _, scenario := range testScenarios {
res, err := resolveVideoSizeId(scenario.placement, scenario.instl, scenario.impId)
assert.Equal(t, scenario.expected, res)
assert.Equal(t, scenario.expectedErr, err)
}
}

func TestNoContentResponse(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusNoContent)
Expand Down
159 changes: 159 additions & 0 deletions adapters/rubicon/rubicontest/exemplary/simple-video.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
{
"mockBidRequest": {
"id": "test-request-id",
"device": {
"ip": "123.123.123.123",
"ifa": "zxcjbzxmc-zxcbmz-zxbcz-zxczx"
},
"app": {
"id": "1",
"bundle": "com.wls.testwlsapplication"
},
"imp": [
{
"id": "test-imp-id",
"instl": 1,
"video": {
"placement": 3,
"mimes": [
"video/mp4"
],
"protocols": [
2,
5
],
"w": 1024,
"h": 576
},
"ext": {
"bidder": {
"video": {
},
"accountId": 1001,
"siteId":113932,
"zoneId":535510
}
}
}
]
},
"httpCalls": [
{
"expectedRequest": {
"uri": "uri?tk_xint=pbs-test-tracker",
"body": {
"id": "test-request-id",
"device": {
"ext": {
"rp": {
"pixelratio": 0
}
},
"ip": "123.123.123.123",
"ifa": "zxcjbzxmc-zxcbmz-zxbcz-zxczx"
},
"app": {
"id": "1",
"ext": {
"rp": {
"site_id": 113932
}
},
"publisher": {
"ext": {
"rp": {
"account_id": 1001
}
}
},
"bundle": "com.wls.testwlsapplication"
},
"imp": [
{
"id": "test-imp-id",
"instl": 1,
"video": {
"placement": 3,
"ext": {
"rp": {
"size_id": 203
}
},
"mimes": [
"video/mp4"
],
"protocols": [
2,
5
],
"w": 1024,
"h": 576
},
"ext": {
"rp": {
"track":{
"mint": "",
"mint_version": ""
},
"zone_id": 535510
}
}
}
]
}
},
"mockResponse": {
"status": 200,
"body": {
"id": "test-request-id",
"seatbid": [
{
"bid": [
{
"id": "test_bid_id",
"impid": "test-imp-id",
"price": 0.27543,
"adm": "some-test-ad",
"cid": "test_cid",
"crid": "test_crid",
"dealid": "test_dealid",
"ext": {
"prebid": {
"type": "video"
}
}
}
],
"seat": "adman"
}
],
"cur": "USD"
}
}
}
],
"expectedBidResponses": [
{
"currency": "USD",
"bids": [
{
"bid": {
"id": "test_bid_id",
"impid": "test-imp-id",
"price": 0.27543,
"adm": "some-test-ad",
"cid": "test_cid",
"crid": "test_crid",
"dealid": "test_dealid",
"ext": {
"prebid": {
"type": "video"
}
}
},
"type": "video"
}
]
}
]
}
2 changes: 1 addition & 1 deletion adapters/rubicon/rubicontest/supplemental/required-video-size-id.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
},
"expectedMakeRequestsErrors": [
{
"value": "imp[0].ext.bidder.rubicon.video.size_id must be defined for video impression",
"value": "video.size_id can not be resolved in impression with id : test-imp-1",
"comparison": "literal"
}
]
Expand Down