-
Notifications
You must be signed in to change notification settings - Fork 753
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
sovrn: Add video support #2232
sovrn: Add video support #2232
Changes from all commits
dd18694
f619601
563787f
cd70e4a
446c8d8
572725e
4d20c54
c61126a
058319c
0bd1577
cd0de9b
5d707f9
12005e8
1ecb04e
1dc8299
d284e95
20e2b1f
c576243
2a721ba
48f2f32
bf1cd4f
a3b0275
06a29b2
deaf51a
16dcca8
43cc6dc
4595b2d
a1f4cb6
8647533
b91d1c4
2721ae6
2477b94
2ca2b68
8278529
048b418
1ee0493
64de231
38ab1e8
79898d5
45306cf
d8cad4d
8b2c31a
e0a95db
2ffe1ae
56289aa
107fed4
64abea6
6295412
b109f7e
9ee163a
cdf6506
21315af
eaffe1b
2ba8ed5
773db5a
746cb3d
44cedfe
f01da54
0f909dd
d206910
bc09bb3
34cb1e3
c3cbbd7
77c738f
4cf6a48
dee970d
043b71b
471c305
981f980
4fe7708
f8043c2
408ba00
ce28f3e
7d3c80f
b1a5d48
ad6ac9b
edd2ca2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -21,28 +21,6 @@ type SovrnAdapter struct { | |||
} | ||||
|
||||
func (s *SovrnAdapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||||
errs := make([]error, 0, len(request.Imp)) | ||||
|
||||
for i := 0; i < len(request.Imp); i++ { | ||||
_, err := preprocess(&request.Imp[i]) | ||||
if err != nil { | ||||
errs = append(errs, err) | ||||
request.Imp = append(request.Imp[:i], request.Imp[i+1:]...) | ||||
i-- | ||||
} | ||||
} | ||||
|
||||
// If all the requests were malformed, don't bother making a server call with no impressions. | ||||
if len(request.Imp) == 0 { | ||||
return nil, errs | ||||
} | ||||
|
||||
reqJSON, err := json.Marshal(request) | ||||
if err != nil { | ||||
errs = append(errs, err) | ||||
return nil, errs | ||||
} | ||||
|
||||
headers := http.Header{} | ||||
headers.Add("Content-Type", "application/json") | ||||
if request.Device != nil { | ||||
|
@@ -61,6 +39,70 @@ func (s *SovrnAdapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapt | |||
} | ||||
} | ||||
|
||||
errs := make([]error, 0, len(request.Imp)) | ||||
var err error | ||||
validImps := make([]openrtb2.Imp, 0, len(request.Imp)) | ||||
|
||||
for _, imp := range request.Imp { | ||||
var bidderExt adapters.ExtImpBidder | ||||
if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil { | ||||
errs = append(errs, &errortypes.BadInput{ | ||||
Message: err.Error(), | ||||
}) | ||||
continue | ||||
} | ||||
|
||||
var sovrnExt openrtb_ext.ExtImpSovrn | ||||
if err := json.Unmarshal(bidderExt.Bidder, &sovrnExt); err != nil { | ||||
errs = append(errs, &errortypes.BadInput{ | ||||
Message: err.Error(), | ||||
}) | ||||
continue | ||||
} | ||||
|
||||
tagId := getTagId(sovrnExt) | ||||
if tagId == "" { | ||||
errs = append(errs, &errortypes.BadInput{ | ||||
Message: "Missing required parameter 'tagid'", | ||||
}) | ||||
continue | ||||
} | ||||
|
||||
imp.TagID = tagId | ||||
|
||||
if imp.BidFloor == 0 && sovrnExt.BidFloor > 0 { | ||||
imp.BidFloor = sovrnExt.BidFloor | ||||
} | ||||
|
||||
// Validate video params if appropriate | ||||
video := imp.Video | ||||
if video != nil { | ||||
if video.MIMEs == nil || | ||||
video.MinDuration == 0 || | ||||
video.MaxDuration == 0 || | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please review this logic again. For video endpoint there is a possibility to have only maxDuration when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll need to review this with product. I'll let you know how that conversation goes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cpabst Were you able to check about this with your team? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. He has been very busy. 😄 |
||||
video.Protocols == nil { | ||||
errs = append(errs, &errortypes.BadInput{ | ||||
Message: "Missing required video parameter", | ||||
}) | ||||
continue | ||||
} | ||||
} | ||||
|
||||
validImps = append(validImps, imp) | ||||
} | ||||
|
||||
if len(validImps) == 0 { | ||||
return nil, errs | ||||
} | ||||
|
||||
request.Imp = validImps | ||||
|
||||
reqJSON, err := json.Marshal(request) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point, this request can have both valid and invalid imps because on L92 you're just reassigning the valid imps to the same place in the imps array of the request. You'd actually have to remove the invalid ones as you find them in the for loop so that you're only left with the valid ones or a much easier way would be to just create a new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Done. |
||||
if err != nil { | ||||
errs = append(errs, err) | ||||
return nil, errs | ||||
} | ||||
|
||||
return []*adapters.RequestData{{ | ||||
Method: "POST", | ||||
Uri: s.URI, | ||||
|
@@ -75,78 +117,77 @@ func addHeaderIfNonEmpty(headers http.Header, headerName string, headerValue str | |||
} | ||||
} | ||||
|
||||
func (s *SovrnAdapter) MakeBids(internalRequest *openrtb2.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) { | ||||
if response.StatusCode == http.StatusNoContent { | ||||
func (s *SovrnAdapter) MakeBids(request *openrtb2.BidRequest, bidderRequest *adapters.RequestData, bidderResponse *adapters.ResponseData) (*adapters.BidderResponse, []error) { | ||||
if bidderResponse.StatusCode == http.StatusNoContent { | ||||
return nil, nil | ||||
} | ||||
|
||||
if response.StatusCode == http.StatusBadRequest { | ||||
if bidderResponse.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), | ||||
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", bidderResponse.StatusCode), | ||||
}} | ||||
} | ||||
|
||||
if response.StatusCode != http.StatusOK { | ||||
if bidderResponse.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), | ||||
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", bidderResponse.StatusCode), | ||||
}} | ||||
} | ||||
|
||||
var bidResp openrtb2.BidResponse | ||||
if err := json.Unmarshal(response.Body, &bidResp); err != nil { | ||||
var bidResponse openrtb2.BidResponse | ||||
if err := json.Unmarshal(bidderResponse.Body, &bidResponse); err != nil { | ||||
return nil, []error{&errortypes.BadServerResponse{ | ||||
Message: err.Error(), | ||||
}} | ||||
} | ||||
|
||||
bidResponse := adapters.NewBidderResponseWithBidsCapacity(5) | ||||
response := adapters.NewBidderResponseWithBidsCapacity(5) | ||||
errs := make([]error, 0) | ||||
|
||||
for _, sb := range bidResp.SeatBid { | ||||
for i := 0; i < len(sb.Bid); i++ { | ||||
bid := sb.Bid[i] | ||||
for _, sb := range bidResponse.SeatBid { | ||||
for _, bid := range sb.Bid { | ||||
adm, err := url.QueryUnescape(bid.AdM) | ||||
if err == nil { | ||||
bid.AdM = adm | ||||
bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{ | ||||
|
||||
bidType := openrtb_ext.BidTypeBanner | ||||
|
||||
impIdx, impIdErr := getImpIdx(bid.ImpID, request) | ||||
if impIdErr != nil { | ||||
errs = append(errs, impIdErr) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a test case for this error condition There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||
continue | ||||
} else if request.Imp[impIdx].Video != nil { | ||||
bidType = openrtb_ext.BidTypeVideo | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned by @VeronikaSolovei9, this can return the wrong bid type in case where the imp is a multi-format imp. But also, since you're only looking at the first imp in the request, this might also return the wrong bid types if not all imps in the request are of same type. For example, say the first imp in the request is of type video but the second one is of type banner, then you'll be returning wrong bid type for the bid corresponding to the second imp. We highly recommend using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm solving this presently with using the Bid.impid value to set bid type. We will look into using the recommended value in the future. |
||||
} | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case you return bid type in bid.ext we recommend to use this approach: prebid-server/adapters/algorix/algorix.go Line 190 in c93a036
It will also help to determine bid type correctly in case if req.imp is multi typed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not support multi-typed imps yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may not support it, but request may have it. In your code you take imp type from request and match it with response: |
||||
|
||||
response.Bids = append(response.Bids, &adapters.TypedBid{ | ||||
Bid: &bid, | ||||
BidType: openrtb_ext.BidTypeBanner, | ||||
BidType: bidType, | ||||
}) | ||||
} | ||||
} | ||||
} | ||||
|
||||
return bidResponse, nil | ||||
return response, errs | ||||
} | ||||
|
||||
func preprocess(imp *openrtb2.Imp) (string, error) { | ||||
var bidderExt adapters.ExtImpBidder | ||||
if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil { | ||||
return "", &errortypes.BadInput{ | ||||
Message: err.Error(), | ||||
} | ||||
func getTagId(sovrnExt openrtb_ext.ExtImpSovrn) string { | ||||
if len(sovrnExt.Tagid) > 0 { | ||||
return sovrnExt.Tagid | ||||
} else { | ||||
return sovrnExt.TagId | ||||
} | ||||
} | ||||
|
||||
var sovrnExt openrtb_ext.ExtImpSovrn | ||||
if err := json.Unmarshal(bidderExt.Bidder, &sovrnExt); err != nil { | ||||
return "", &errortypes.BadInput{ | ||||
Message: err.Error(), | ||||
func getImpIdx(impId string, request *openrtb2.BidRequest) (int, error) { | ||||
for idx, imp := range request.Imp { | ||||
if imp.ID == impId { | ||||
return idx, nil | ||||
} | ||||
} | ||||
|
||||
imp.TagID = getTagid(sovrnExt) | ||||
|
||||
if imp.BidFloor == 0 && sovrnExt.BidFloor > 0 { | ||||
imp.BidFloor = sovrnExt.BidFloor | ||||
} | ||||
|
||||
return imp.TagID, nil | ||||
} | ||||
|
||||
func getTagid(sovrnExt openrtb_ext.ExtImpSovrn) string { | ||||
if len(sovrnExt.Tagid) > 0 { | ||||
return sovrnExt.Tagid | ||||
} else { | ||||
return sovrnExt.TagId | ||||
return -1, &errortypes.BadInput{ | ||||
Message: fmt.Sprintf("Imp ID %s in bid didn't match with any imp in the original request", impId), | ||||
} | ||||
} | ||||
|
||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add coverage for this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Fixed a bunch of other tests as well.