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

New Adapter: jixie #1698

Merged
merged 14 commits into from
Feb 25, 2021
Merged
Show file tree
Hide file tree
Changes from 7 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
163 changes: 163 additions & 0 deletions adapters/jixie/jixie.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
package jixie

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

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

type JixieAdapter struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please name this adapter. There is no need to export it with a upper case letter. There is no need to repeat the package name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in commit 0x3afae48. thank you

endpoint string
testing bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you are using the testing field only for JSON tests. This is not really needed as the JSON tests won't be hitting your server and so you can remove this field and also the if block on L43

}

// Builder builds a new instance of the Jixie adapter for the given bidder with the given config.
func Builder(bidderName openrtb_ext.BidderName, config config.Adapter) (adapters.Bidder, error) {
bidder := &JixieAdapter{
endpoint: config.Endpoint,
}
return bidder, nil
}

// Adding header fields to request header
func addHeaderIfNonEmpty(headers http.Header, headerName string, headerValue string) {
if len(headerValue) > 0 {
headers.Add(headerName, headerValue)
}
}

func buildEndpoint(endpoint string, testing bool, timeout int64) string {

if timeout == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to set the timeout value outside this function, somewhere in MakeRequests because arguments are pass by value in Go and so only the timeout copy local to this method will be updated to the new value and won't be reflected in req.TMax as you want it to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tks; tried to fix based on all your comments in 0x1deb9f1 (14 Feb). Sorry I didn't know there is an update slot here.

timeout = 1000
}

if testing {
return endpoint + "?hbofftest=1"
}
return endpoint
}

func (a *JixieAdapter) MakeRequests(request *openrtb.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) {
var errs []error
if len(request.Imp) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this check because Prebid Server discards bidRequests with zero-length []Imp arrays upstream and in consequence, makes the true branch of this if statement unreachable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in 0xaa2d1d5. tks

return nil, []error{&errortypes.BadInput{
Message: fmt.Sprintf("No Imps in Bid Request"),
}}
}

for _, imp := range request.Imp {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we don't want to do anything with the unmarshalled info? Is the purpose of this for loop is just to check if imps don't have unmarshaling errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tks for this great note. Now do more checking; + 2 test json files. (in 0xaa2d1d5)


var bidderExt adapters.ExtImpBidder
if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil {
return nil, []error{&errortypes.BadInput{
Message: err.Error()},
}
}
var jxExt openrtb_ext.ExtImpJixie
if err := json.Unmarshal(bidderExt.Bidder, &jxExt); err != nil {
return nil, []error{&errortypes.BadInput{
Message: fmt.Sprintf("ignoring imp id=%s, invalid ImpExt", imp.ID)},
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to return an error here? It might make more sense to append the error and continue processing the rest of the imps. Same for the bidder ext above.

}
}
}

data, err := json.Marshal(request)
if err != nil {
return nil, []error{&errortypes.BadInput{
Message: fmt.Sprintf("Error in packaging request to JSON"),
}}
}

headers := http.Header{}
headers.Add("Content-Type", "application/json;charset=utf-8")
headers.Add("Accept", "application/json")
if request.Device != nil {
addHeaderIfNonEmpty(headers, "User-Agent", request.Device.UA)
addHeaderIfNonEmpty(headers, "X-Forwarded-For", request.Device.IP)
addHeaderIfNonEmpty(headers, "Accept-Language", request.Device.Language)
if request.Device.DNT != nil {
addHeaderIfNonEmpty(headers, "DNT", strconv.Itoa(int(*request.Device.DNT)))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider a test that sets the "Accept-Language" and "DNT" the headers:

72     "httpCalls": [{
73       "expectedRequest": {
74         "uri": "https://hb.jixie.io/v2/hbsvrpost?hbofftest=1",
75         "headers": {
76           "Accept": [
77             "application/json"
78           ],
   +         "Accept-Language": [
   +            "anyAcceptLanguageValue"
   +          ],
   +         "DNT": [
   +            "anyDntValue"
   +          ],
79           "Content-Type": [
80             "application/json;charset=utf-8"
81           ],
82           "X-Forwarded-For": [
83             "111.222.333.444"
84           ],
85           "Referer": [
86             "http://www.publisher.com/today/site?param1=a&param2=b"
87           ],
88           "User-Agent": [
89             "Mozilla/5.0 (Linux; Android 8.0.0; SM-G960F Build/R16NW) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.84 Mobile Safari/537.36"
90           ]
91         },
92         "body": {
adapters/jixie/jixietest/exemplary/banner-and-video-site.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

decided not to add the lang and dnt to the headers after all. Changed jixie.go 0xaa2d1d5


if request.Site != nil {
addHeaderIfNonEmpty(headers, "Referer", request.Site.Page)
}

theurl := buildEndpoint(a.endpoint, a.testing, request.TMax)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is testing a left-over from a debug phase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in earlier commit. tks

return []*adapters.RequestData{{
Method: "POST",
Uri: theurl,
Body: data,
Headers: headers,
}}, errs
}

func ContainsAny(raw string, keys []string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's probably no need to export this function. Consider renaming to lowercase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-> containsAny

lowerCased := strings.ToLower(raw)
for i := 0; i < len(keys); i++ {
if strings.Contains(lowerCased, keys[i]) {
return true
}
}
return false

}

func getBidType(bidAdm string) openrtb_ext.BidType {
if bidAdm != "" && ContainsAny(bidAdm, []string{"<?xml", "<vast"}) {
return openrtb_ext.BidTypeVideo
}
return openrtb_ext.BidTypeBanner
}

// MakeBids make the bids for the bid response.
func (a *JixieAdapter) MakeBids(internalRequest *openrtb.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {

if response.StatusCode == http.StatusNoContent {
// no bid response
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 a bad request is received. Would it be useful to add that scenario in our context?

124   func (a *JixieAdapter) MakeBids(internalRequest *openrtb.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
125  
126       if response.StatusCode == http.StatusNoContent {
127           // no bid response
128           return nil, nil
129       }
    +
    +     if response.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),
    +     	}}
    +     }
130  
131       if response.StatusCode != http.StatusOK {
132           return nil, []error{&errortypes.BadServerResponse{
133               Message: fmt.Sprintf("Invalid Status Returned: %d. Run with request.debug = 1 for more info", response.StatusCode),
134           }}
135       }
adapters/jixie/jixie.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added. tks. in 0xaa2d1d5


if response.StatusCode != http.StatusOK {
return nil, []error{&errortypes.BadServerResponse{
Message: fmt.Sprintf("Invalid Status Returned: %d. Run with request.debug = 1 for more info", response.StatusCode),
}}
}

var bidResp openrtb.BidResponse

if err := json.Unmarshal(response.Body, &bidResp); err != nil {
return nil, []error{&errortypes.BadServerResponse{
Message: fmt.Sprintf("Unable to unpackage bid response. Error: %s", err.Error()),
}}
}

var bids []*adapters.TypedBid

for _, sb := range bidResp.SeatBid {
for i := range sb.Bid {

sb.Bid[i].ImpID = sb.Bid[i].ID

bids = append(bids, &adapters.TypedBid{
Bid: &sb.Bid[i],
BidType: getBidType(sb.Bid[i].AdM),
})
}
}
adsResp := adapters.NewBidderResponseWithBidsCapacity(len(bids))
adsResp.Bids = bids

return adsResp, nil

}
26 changes: 26 additions & 0 deletions adapters/jixie/jixie_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package jixie

import (
"github.com/prebid/prebid-server/adapters"
"github.com/prebid/prebid-server/adapters/adapterstest"
"github.com/prebid/prebid-server/config"
"github.com/prebid/prebid-server/openrtb_ext"
"testing"
)

func TestJsonSamples(t *testing.T) {
bidder, buildErr := Builder(openrtb_ext.BidderJixie, config.Adapter{
Endpoint: "https://hb.jixie.io/v2/hbsvrpost"})

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

setTesting(bidder)
adapterstest.RunJSONBidderTest(t, "jixietest", bidder)
}

func setTesting(bidder adapters.Bidder) {
bidderJixie, _ := bidder.(*JixieAdapter)
bidderJixie.testing = true
}
Loading