Skip to content
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
48 changes: 48 additions & 0 deletions adapters/targetVideo/params_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package targetVideo

import (
"encoding/json"
"testing"

"github.com/prebid/prebid-server/v3/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)
Copy link
Contributor

Choose a reason for hiding this comment

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

require.NoError(t, err, "Failed to fetch the json-schemas")
Please use this approach and update other tests as well

}

for _, validParam := range validParams {
if err := validator.Validate(openrtb_ext.BidderTargetVideo, json.RawMessage(validParam)); err != nil {
t.Errorf("Schema rejected targetVideo 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.BidderTargetVideo, json.RawMessage(invalidParam)); err == nil {
t.Errorf("Schema allowed unexpected params: %s", invalidParam)
}
}
}

var validParams = []string{
`{"placementId":846}`,
`{"placementId":"846"}`,
}

var invalidParams = []string{
`null`,
`nil`,
`undefined`,
`{"placementId": "%9"}`,
`{"publisherId": "as9""}`,
`{"placementId": true}`,
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 {"placementId": ""} to invalid params to ensure your regex is correct.

}
194 changes: 194 additions & 0 deletions adapters/targetVideo/targetvideo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
package targetVideo

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

"github.com/prebid/openrtb/v20/openrtb2"
"github.com/prebid/prebid-server/v3/adapters"
"github.com/prebid/prebid-server/v3/config"
"github.com/prebid/prebid-server/v3/errortypes"
"github.com/prebid/prebid-server/v3/openrtb_ext"
"github.com/prebid/prebid-server/v3/util/jsonutil"
)

type adapter struct {
endpoint string
}

type impExt struct {
TargetVideo openrtb_ext.ExtImpTargetVideo `json:"targetVideo"`
}

func (a *adapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) {
totalImps := len(request.Imp)
errors := make([]error, 0)
adapterRequests := make([]*adapters.RequestData, 0, totalImps)

// Split multi-imp request into multiple ad server requests. SRA is currently not recommended.
for i := 0; i < totalImps; i++ {
if adapterReq, err := a.makeRequest(*request, request.Imp[i]); err == nil {
adapterRequests = append(adapterRequests, adapterReq)
} else {
errors = append(errors, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick:
Can be rewritten as

adapterReq, err := a.makeRequest(*request, request.Imp[i])
if err != nil {
	errors = append(errors, err)
	continue
}
adapterRequests = append(adapterRequests, adapterReq)

Its more idiomatic way in golang

}

return adapterRequests, errors
}

func (a *adapter) makeRequest(request openrtb2.BidRequest, imp openrtb2.Imp) (*adapters.RequestData, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're missing error path test coverage. After you've made the suggested simplifications to this function, please add additional tests to the supplemental folder to cover each of the unmarshal error paths. You can do this by simply sending a mock bid request with the data under consideration malformed or of a mismatched type.


// For now, this adapter sends one imp per request, but we still
// iterate over all imps in the request to perform the required
// imp.ext transformation.
request.Imp = []openrtb2.Imp{imp}

_, errImp := validateImpAndSetExt(&imp)
if errImp != nil {
return nil, errImp
}

for i := range request.Imp {
if len(request.Imp[i].Ext) == 0 {
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be able to delete this as this should never be true given the logic in validateImpAndSetExt and that your adapter will never be called with an empty imp.ext.

var root map[string]json.RawMessage
if err := json.Unmarshal(request.Imp[i].Ext, &root); err != nil {
// If ext cannot be parsed, skip transformation for this imp
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that you're unmarshalling here as well as in validateImpAndSetExt and that I am suggesting you delete the validation checks in that function, I suggest you delete validateImpAndSetExt entirely and continue with the JSON unmarshaling you have here.


// Try to extract placementId from ext.bidder.targetVideo (or targetvideo)
placementId := ""
if bRaw, ok := root["bidder"]; ok && len(bRaw) > 0 {
var bidder map[string]json.RawMessage
if err := json.Unmarshal(bRaw, &bidder); err == nil {
if placementIdRaw, ok := bidder["placementId"]; ok && len(placementIdRaw) > 0 {

var asStr string
var asInt int64
if err := json.Unmarshal(placementIdRaw, &asStr); err == nil && asStr != "" {
placementId = asStr
} else if err := json.Unmarshal(placementIdRaw, &asInt); err == nil {
placementId = fmt.Sprintf("%d", asInt)
}

}
}
// Remove bidder node as required
delete(root, "bidder")
}

// If we obtained a placementId, set ext.prebid.storedrequest.id = placementId
if placementId != "" {
// Build prebid.storedrequest structure, preserving existing prebid if any
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 JSON test where prebid is set in the mock bid request with fields other than stored request id set in it to ensure they are preserved in the bid request.

var prebid map[string]json.RawMessage
if pr, ok := root["prebid"]; ok && len(pr) > 0 {
_ = json.Unmarshal(pr, &prebid)
}
if prebid == nil {
prebid = make(map[string]json.RawMessage)
}
stored := map[string]string{"id": placementId}
storedRaw, _ := json.Marshal(stored)
prebid["storedrequest"] = storedRaw
prebidRaw, _ := json.Marshal(prebid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of this can be simplified by doing the following:

  1. Change the type of openrtb_ext/imp_targetvideo.go PlacementId from int to jsonutil.StringInt defined in github.com/prebid/prebid-server/v3/util/jsonutil. This will handle converting a string to an int when unmarshaling the placement_id parameter to openrtb_ext.ExtImpTargetVideo. This StringInt type has a custom unmarshaler defined.
  2. Declare the following types at the top of this file:
type impExtBidder struct {
	TargetVideo openrtb_ext.ExtImpTargetVideo `json:"targetVideo"`
}
type impExtPrebid struct {
	Prebid *openrtb_ext.ExtImpPrebid `json:"prebid,omitempty"`
}
  1. Replace all of this code with something like this:
var extBidder adapters.ExtImpBidder
if err := jsonutil.Unmarshal(imp.Ext, &extBidder); err != nil {
	return nil, &errortypes.BadInput{Message: fmt.Sprintf("error parsing imp.ext, err: %s", err)}
}
var extImpTargetVideo openrtb_ext.ExtImpTargetVideo
if err := jsonutil.Unmarshal(extBidder.Bidder, &extImpTargetVideo); err != nil {
	return nil, &errortypes.BadInput{Message: fmt.Sprintf("error parsing imp.ext.bidder, err: %s", err)}
}
var prebid *openrtb_ext.ExtImpPrebid
if extBidder.Prebid == nil {
	prebid = &openrtb_ext.ExtImpPrebid{}
}
if prebid.StoredRequest == nil {
	prebid.StoredRequest = &openrtb_ext.ExtStoredRequest{}
}
prebid.StoredRequest.ID = fmt.Sprintf("%d", extImpTargetVideo.PlacementId)

ext := impExtPrebid{
	Prebid: prebid,
}

extRaw, err := jsonutil.Marshal(ext)
if err != nil {
	return nil, &errortypes.BadInput{Message: fmt.Sprintf("error building imp.ext, err: %s", err)}
}

request.Imp[i].Ext = extRaw

root["prebid"] = prebidRaw
}

// Marshal back the transformed ext
if newExt, err := json.Marshal(root); err == nil {
request.Imp[i].Ext = newExt
}
}

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

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

//fmt.Println("TARGET VIDEO reqJson: ", string(reqJSON))
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Suggested change
//fmt.Println("TARGET VIDEO reqJson: ", string(reqJSON))


return &adapters.RequestData{
Method: "POST",
Uri: a.endpoint,
Body: reqJSON,
Headers: headers,
ImpIDs: openrtb_ext.GetImpIDs(request.Imp),
}, nil
}

func (a *adapter) MakeBids(bidReq *openrtb2.BidRequest, unused *adapters.RequestData, httpRes *adapters.ResponseData) (*adapters.BidderResponse, []error) {
if httpRes.StatusCode == http.StatusNoContent {
return nil, nil
}
if httpRes.StatusCode == http.StatusBadRequest {
return nil, []error{&errortypes.BadInput{Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", httpRes.StatusCode)}}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the following adapter helper functions for status code checks:
adapters/response.go#IsResponseStatusCodeNoContent
adapters/response.go#CheckResponseStatusCodeForErrors


var bidResp openrtb2.BidResponse
if err := jsonutil.Unmarshal(httpRes.Body, &bidResp); err != nil {
return nil, []error{&errortypes.BadServerResponse{Message: fmt.Sprintf("error while decoding response, err: %s", 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 to cover this case.

}

if len(bidResp.SeatBid) == 0 {
return nil, nil
}

if len(bidResp.SeatBid[0].Bid) == 0 {
return nil, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you choose to keep this conditional, please add a supplemental test case to cover.

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can delete these two checks because the for range on line 151 should gracefully handle the empty scenarios.


br := adapters.NewBidderResponse()
errs := []error{}

for _, sb := range bidResp.SeatBid {
for i := range sb.Bid {
bid := sb.Bid[i]
// Ensure imp exists and is video
mediaType := 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.

Nitpick: we don't need mediaType variable here, we can directly assign video. Also remove empty lines.

for _, imp := range bidReq.Imp {
if imp.ID == bid.ImpID {
if imp.Video == nil {
// Not a video impression; skip
errs = append(errs, &errortypes.BadServerResponse{Message: fmt.Sprintf("ignoring bid id=%s for non-video imp id=%s", bid.ID, bid.ImpID)})
mediaType = ""
}
break
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You've only declared video support in your YAML file which means your adapter will only ever be called if the request impressions contain a video object. In that case, this code is unnecessary since you know the request impression will always contain this object. You can just set the mediaType to video.

If you intend to support additional media types in the future, inspecting the media type on the request like you've done here would make sense to add at that time.


br.Bids = append(br.Bids, &adapters.TypedBid{Bid: &bid, BidType: mediaType})
}
}
return br, errs
}

func validateImpAndSetExt(imp *openrtb2.Imp) (int, error) {
if imp.Video == nil {
return 0, &errortypes.BadInput{Message: fmt.Sprintf("Only video impressions are supported by targetvideo. ImpID=%s", imp.ID)}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove this check. In your yaml file you only declared support for the video media type, in which case your adapter will only ever be called if video is not nil.

if len(imp.Ext) == 0 {
return 0, &errortypes.BadInput{Message: fmt.Sprintf("imp.ext is required and must contain bidder params for targetvideo. ImpID=%s", imp.ID)}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove this check. Given that you declared a required bidder param that must not be empty, your adapter will only ever be called if this is not empty.

var ext impExt
if err := jsonutil.Unmarshal(imp.Ext, &ext); err != nil {
return 0, &errortypes.BadInput{Message: fmt.Sprintf("error parsing imp.ext for targetvideo, err: %s", err)}
}

return 0, nil
}

func Builder(bidderName openrtb_ext.BidderName, cfg config.Adapter, server config.Server) (adapters.Bidder, error) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Remove blank line

bidder := &adapter{
endpoint: cfg.Endpoint,
}
return bidder, nil
}
20 changes: 20 additions & 0 deletions adapters/targetVideo/targetvideo_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package targetVideo

import (
"testing"

"github.com/prebid/prebid-server/v3/adapters/adapterstest"
"github.com/prebid/prebid-server/v3/config"
"github.com/prebid/prebid-server/v3/openrtb_ext"
)

func TestJsonSamples(t *testing.T) {
bidder, buildErr := Builder(openrtb_ext.BidderTargetVideo, config.Adapter{
Endpoint: "http://localhost/pbs"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"})

if buildErr != nil {
t.Fatalf("Builder returned unexpected error %v", buildErr)
}

adapterstest.RunJSONBidderTest(t, "targetvideotest", bidder)
}
130 changes: 130 additions & 0 deletions adapters/targetVideo/targetvideotest/exemplary/app-video.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
{
"mockBidRequest": {
"id": "test-request-id-video",
"app": {
"id": "appID",
"publisher": {
"id": "uniq_pub_id"
}
},
"cur": ["EUR"],
"imp": [
{
"id": "test-imp-id-video",
"video": {
"mimes": ["video/mp4"],
"w": 640,
"h": 360
},
"ext": {
"bidder": {
"placementId": "77777"
}
}
}
],
"device": {
"ua": "test-user-agent"
},
"site": {
"domain": "www.publisher.com",
"page": "http://www.publisher.com/some/path",
"ext": {
"amp": 0
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the site object since this is an app request.

},
"httpCalls": [
{
"expectedRequest": {
"headers": {
"Accept": ["application/json"],
"Content-Type": ["application/json;charset=utf-8"]
},
"uri": "http://localhost/pbs",
"body": {
"id": "test-request-id-video",
"app": {
"id": "appID",
"publisher": {
"id": "uniq_pub_id"
}
},
"imp": [
{
"id": "test-imp-id-video",
"video": {
"mimes": ["video/mp4"],
"w": 640,
"h": 360
},
"ext": {
"prebid": {
"storedrequest": {
"id": "77777"
}
}
}
}
],
"device": {
"ua": "test-user-agent"
},
"site": {
"domain": "www.publisher.com",
"page": "http://www.publisher.com/some/path",
"ext": {
"amp": 0
}
},
"cur": ["EUR"]
},
"impIDs":["test-imp-id-video"]
},
"mockResponse": {
"status": 200,
"body": {
"id": "test-request-id-video",
"seatbid": [
{
"seat": "targetVideo",
"bid": [
{
"id": "randomID",
"impid": "test-imp-id-video",
"price": 5.5,
"adid": "12345678",
"adm": "test-imp-id-video",
"cid": "789",
"crid": "12345678",
"h": 360,
"w": 640
}
]
}
],
"cur": "EUR"
}
}
}
],
"expectedBidResponses": [
{
"currency": "EUR",
"bids" : [{
"bid": {
"id": "randomID",
"adid": "12345678",
"impid": "test-imp-id-video",
"price": 5.5,
"adm": "test-imp-id-video",
"crid": "12345678",
"cid": "789",
"h": 360,
"w": 640
},
"type": "video"
}]
}
]
}
Loading
Loading