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

OMS: Video support #4172

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
60 changes: 56 additions & 4 deletions adapters/oms/oms.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ import (
"github.com/prebid/prebid-server/v3/util/jsonutil"
)

type pbsExt struct {
Bidder genericPID `json:"bidder"`
Tid string `json:"tid"`
}

type genericPID struct {
Pid string `json:"pid"`
PublisherID int `json:"publisherId"`
}

Copy link
Contributor

Choose a reason for hiding this comment

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

These new types are not needed here.
You will need to add new field to ExtImpOms, it already exists in the openrtb_ext directory:

type ExtImpOms struct {
	Pid         string `json:"pid"`
	PublisherID int    `json:"publisherId"`
}

And then in MakeRequests follow a general approach to Unmarshal imp.ext:
example

Copy link
Author

Choose a reason for hiding this comment

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

Done

type adapter struct {
endpoint string
}
Expand All @@ -31,9 +41,23 @@ func (a *adapter) MakeRequests(request *openrtb2.BidRequest, requestInfo *adapte
return nil, []error{err}
}

var uri string = a.endpoint
if len(request.Imp[0].Ext) > 0 {
ext := pbsExt{}
err = jsonutil.Unmarshal(request.Imp[0].Ext, &ext)
if err != nil {
return nil, []error{err}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a supplemental test case where the unmarshal fails. You can do this by simply setting your imp.ext to a type other than object.

Copy link
Author

Choose a reason for hiding this comment

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

Done

}

uri = fmt.Sprintf("%s?publisherId=%s", a.endpoint, ext.Bidder.Pid)
if ext.Bidder.Pid == "" && ext.Bidder.PublisherID > 0 {
uri = fmt.Sprintf("%s?publisherId=%d", a.endpoint, ext.Bidder.PublisherID)
}
}

requestData := &adapters.RequestData{
Method: "POST",
Uri: a.endpoint,
Uri: uri,
Body: requestJSON,
ImpIDs: openrtb_ext.GetImpIDs(request.Imp),
}
Expand All @@ -42,7 +66,6 @@ func (a *adapter) MakeRequests(request *openrtb2.BidRequest, requestInfo *adapte
}

func (a *adapter) MakeBids(request *openrtb2.BidRequest, requestData *adapters.RequestData, responseData *adapters.ResponseData) (*adapters.BidderResponse, []error) {

if responseData.StatusCode == http.StatusNoContent {
return nil, nil
}
Expand Down Expand Up @@ -70,14 +93,43 @@ func (a *adapter) MakeBids(request *openrtb2.BidRequest, requestData *adapters.R
if len(response.Cur) == 0 {
bidResponse.Currency = response.Cur
}

for _, seatBid := range response.SeatBid {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion/question: I'm curious about the type of SeatBid if it's an []SomeStruct then this should also use an index - otherwise, seatBid will do a struct copy (shallow) for each loop iteration.

If the type supports it, consider something like this

Suggested change
for _, seatBid := range response.SeatBid {
for sbIndex := range response.SeatBid {
seatBid := &response.SeatBid[sbIndex]

Copy link
Author

Choose a reason for hiding this comment

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

These lines are unchanged.

for i := range seatBid.Bid {
bidType := getBidType(seatBid.Bid[i].MType)

bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{
Bid: &seatBid.Bid[i],
BidType: openrtb_ext.BidTypeBanner,
Bid: &seatBid.Bid[i],
Copy link
Contributor

Choose a reason for hiding this comment

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

observation/curiosity: hmm… so this is a pointer to the copy - see previous comment, is that intended? If you move to seatBid to a pointer, it'll point to that same instance's bid - that's efficient - any worries about contention here? you're not making changes here, I think it's ok

Copy link
Author

Choose a reason for hiding this comment

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

Yes, these lines are unchanged.

BidType: bidType,
BidVideo: getBidVideo(bidType, &seatBid.Bid[i]),
})
}
}

return bidResponse, nil
}

func getBidType(markupType openrtb2.MarkupType) openrtb_ext.BidType {
Copy link
Contributor

Choose a reason for hiding this comment

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

observation: hmm… this seems like it could be general… is there some central place that can do this mapping?

Copy link
Author

Choose a reason for hiding this comment

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

What you mean by central place?

Copy link
Author

Choose a reason for hiding this comment

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

After checking other adapters I made a conclusion that it depends on how any of them want to process bid types (some of them return error if mapping isn't succeeded, some of them return default value). In our case we want to support backward compatibility that's why banner type is returned as default value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@scr-oath are you asking if there is a helper function offered that can be used by any adapter? There isn't. We might be able to offer that at some point but we should keep in mind that not all adapters support the same media types and the default case will vary. Also some adapters will fall back to deriving the media type from the request imp if mtype is not in the response.

switch markupType {
case openrtb2.MarkupVideo:
return openrtb_ext.BidTypeVideo
default:
return openrtb_ext.BidTypeBanner
}
}

func getBidVideo(bidType openrtb_ext.BidType, bid *openrtb2.Bid) *openrtb_ext.ExtBidPrebidVideo {
if bidType != openrtb_ext.BidTypeVideo {
return nil
}

var primaryCategory string
if len(bid.Cat) > 0 {
primaryCategory = bid.Cat[0]
}

return &openrtb_ext.ExtBidPrebidVideo{
Duration: int(bid.Dur),
PrimaryCategory: primaryCategory,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"httpCalls": [
{
"expectedRequest": {
"uri": "http://rt.marphezis.com/pbs",
"uri": "http://rt.marphezis.com/pbs?publisherId=12345",
"headers": {},
"body": {
"id": "test-request-id",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
"httpCalls": [
{
"expectedRequest": {
"uri": "http://rt.marphezis.com/pbs",
"uri": "http://rt.marphezis.com/pbs?publisherId=12345",
"headers": {},
"body": {
"id": "test-request-id-multiple-bids",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
{
"mockBidRequest": {
"id": "test-request-id",
"site": {
"id": "site-id",
"page": "oms.com"
},
"device": {
"os": "android",
"ip": "91.199.242.236",
"ua": "random user agent"
},
"user": {
"ext": {
"eids": [
{
"source": "oms.com",
"uids": [
{
"id": "oms-eid"
}
]
}
]
}
},
"imp": [
{
"id": "test-imp-id",
"banner": {
"format": [
{
"w": 300,
"h": 250
}
]
},
"ext": {
"bidder": {
"publisherId": 12345
}
}
}
]
},
"httpCalls": [
{
"expectedRequest": {
"uri": "http://rt.marphezis.com/pbs?publisherId=12345",
"headers": {},
"body": {
"id": "test-request-id",
"site": {
"id": "site-id",
"page": "oms.com"
},
"device": {
"os": "android",
"ip": "91.199.242.236",
"ua": "random user agent"
},
"user": {
"ext": {
"eids": [
{
"source": "oms.com",
"uids": [
{
"id": "oms-eid"
}
]
}
]
}
},
"imp": [
{
"id": "test-imp-id",
"banner": {
"format": [
{
"w": 300,
"h": 250
}
]
},
"ext": {
"bidder": {
"publisherId": 12345
}
}
}
]
},
"impIDs":["test-imp-id"]
},
"mockResponse": {
"status": 200,
"body": {
"currency": "USD",
"seatbid": [
{
"bid": [
{
"id": "test-slot-id",
"impid": "test-imp-id",
"price": 0.1,
"crid": "creative-123",
"adm": "<iframe id='789abc' name='789abc' src='http://creative-url.oms.com'></iframe>",
"w": 300,
"h": 250,
"ext": {
"prebid": {
"type": "banner"
}
}
}
]
}
]
}
}
}
],
"expectedBidResponses": [
{
"currency": "USD",
"bids": [
{
"bid": {
"id": "test-slot-id",
"impid": "test-imp-id",
"price": 0.1,
"crid": "creative-123",
"adm": "<iframe id='789abc' name='789abc' src='http://creative-url.oms.com'></iframe>",
"w": 300,
"h": 250,
"ext": {
"prebid": {
"type": "banner"
}
}
},
"type": "banner"
}
]
}
]
}

2 changes: 1 addition & 1 deletion adapters/oms/omstest/exemplary/simple-banner-uid.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"httpCalls": [
{
"expectedRequest": {
"uri": "http://rt.marphezis.com/pbs",
"uri": "http://rt.marphezis.com/pbs?publisherId=12345",
"headers": {},
"body": {
"id": "test-request-id",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
"httpCalls": [
{
"expectedRequest": {
"uri": "http://rt.marphezis.com/pbs",
"uri": "http://rt.marphezis.com/pbs?publisherId=12345",
"headers": {},
"body": {
"id": "test-request-id",
Expand Down
Loading
Loading