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

Orbidder: bidfloor currency handling #2093

66 changes: 40 additions & 26 deletions adapters/orbidder/orbidder.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ package orbidder
import (
"encoding/json"
"fmt"
"net/http"

"github.com/mxmCherry/openrtb/v15/openrtb2"
"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"
"net/http"
"strings"
)

type OrbidderAdapter struct {
Expand All @@ -18,30 +18,11 @@ type OrbidderAdapter struct {

// MakeRequests makes the HTTP requests which should be made to fetch bids from orbidder.
func (rcv *OrbidderAdapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) {
var errs []error
var validImps []openrtb2.Imp

// check if imps exists, if not return error and do send request to orbidder.
if len(request.Imp) == 0 {
return nil, []error{&errortypes.BadInput{
Message: "No impressions in request",
}}
}

// validate imps
for _, imp := range request.Imp {
if err := preprocess(&imp); err != nil {
errs = append(errs, err)
continue
}
validImps = append(validImps, imp)
}

validImps, errs := getValidImpressions(request, reqInfo)
if len(validImps) == 0 {
return nil, errs
}

//set imp array to only valid imps
request.Imp = validImps

requestBodyJSON, err := json.Marshal(request)
Expand All @@ -55,14 +36,34 @@ func (rcv *OrbidderAdapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *
headers.Add("Accept", "application/json")

return []*adapters.RequestData{{
Method: "POST",
Method: http.MethodPost,
Uri: rcv.endpoint,
Body: requestBodyJSON,
Headers: headers,
}}, errs
}

func preprocess(imp *openrtb2.Imp) error {
// getValidImpressions validate imps and check for bid floor currency. Convert to EUR if necessary
func getValidImpressions(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]openrtb2.Imp, []error) {
var errs []error
var validImps []openrtb2.Imp

for _, imp := range request.Imp {
if err := preprocessBidFloorCurrency(&imp, reqInfo); err != nil {
errs = append(errs, err)
continue
}

if err := preprocessExtensions(&imp); err != nil {
errs = append(errs, err)
continue
}
validImps = append(validImps, imp)
}
return validImps, errs
}

func preprocessExtensions(imp *openrtb2.Imp) error {
var bidderExt adapters.ExtImpBidder
if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you just testing that unmarshalling imp.ext does not produce an error? Seems that you are throwing away the result.

Copy link
Contributor Author

@andreasmaurer0210 andreasmaurer0210 Dec 15, 2021

Choose a reason for hiding this comment

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

Yes, you are right. We only checking for errors in unmarshalling imp.ext. In the past we had some problems with invalid Extensions send to our adapter, so we find this check useful. We added some testcases for the preprocessing.

return &errortypes.BadInput{
Expand All @@ -80,8 +81,21 @@ func preprocess(imp *openrtb2.Imp) error {
return nil
}

func preprocessBidFloorCurrency(imp *openrtb2.Imp, reqInfo *adapters.ExtraRequestInfo) error {
// we expect every currency related data to be EUR
if imp.BidFloor > 0 && strings.ToUpper(imp.BidFloorCur) != "EUR" && imp.BidFloorCur != "" {
if convertedValue, err := reqInfo.ConvertCurrency(imp.BidFloor, imp.BidFloorCur, "EUR"); err != nil {
return err
} else {
imp.BidFloor = convertedValue
}
}
imp.BidFloorCur = "EUR"
return nil
}

// MakeBids unpacks server response into Bids.
func (rcv OrbidderAdapter) MakeBids(internalRequest *openrtb2.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
func (rcv OrbidderAdapter) MakeBids(_ *openrtb2.BidRequest, _ *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
if response.StatusCode == http.StatusNoContent {
return nil, nil
}
Expand Down Expand Up @@ -125,7 +139,7 @@ func (rcv OrbidderAdapter) MakeBids(internalRequest *openrtb2.BidRequest, extern
}

// Builder builds a new instance of the Orbidder adapter for the given bidder with the given config.
func Builder(bidderName openrtb_ext.BidderName, config config.Adapter) (adapters.Bidder, error) {
func Builder(_ openrtb_ext.BidderName, config config.Adapter) (adapters.Bidder, error) {
bidder := &OrbidderAdapter{
endpoint: config.Endpoint,
}
Expand Down
147 changes: 147 additions & 0 deletions adapters/orbidder/orbidder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ package orbidder

import (
"encoding/json"
"errors"
"github.com/mxmCherry/openrtb/v15/openrtb2"
"github.com/prebid/prebid-server/adapters"
"github.com/stretchr/testify/mock"
"testing"

"github.com/prebid/prebid-server/adapters/adapterstest"
Expand All @@ -22,6 +26,16 @@ func TestUnmarshalOrbidderExtImp(t *testing.T) {
}, impExt)
}

func TestPreprocessExtensions(t *testing.T) {
for name, tc := range testCasesExtension {
t.Run(name, func(t *testing.T) {
imp := tc.imp
err := preprocessExtensions(&imp)
tc.assertError(t, err)
})
}
}

func TestJsonSamples(t *testing.T) {
bidder, buildErr := Builder(openrtb_ext.BidderOrbidder, config.Adapter{
Endpoint: "https://orbidder-test"})
Expand All @@ -32,3 +46,136 @@ func TestJsonSamples(t *testing.T) {

adapterstest.RunJSONBidderTest(t, "orbiddertest", bidder)
}

var testCasesCurrency = map[string]struct {
imp openrtb2.Imp
setMock func(m *mock.Mock)
expectedImp openrtb2.Imp
assertError func(t assert.TestingT, err error, msgAndArgs ...interface{}) bool
}{
"EUR: no bidfloor, no currency": {
imp: openrtb2.Imp{
BidFloor: 0,
BidFloorCur: "",
},
setMock: func(m *mock.Mock) {},
expectedImp: openrtb2.Imp{
BidFloor: 0,
BidFloorCur: "EUR",
},
assertError: assert.NoError,
},
"EUR: bidfloor, no currency": {
imp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "",
},
setMock: func(m *mock.Mock) {},
expectedImp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "EUR",
},
assertError: assert.NoError,
},
"EUR: bidfloor and currency": {
imp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "EUR",
},
setMock: func(m *mock.Mock) {},
expectedImp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "EUR",
},
assertError: assert.NoError,
},
"USD: bidfloor with currency": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add another similar test case where the mock converter returns an error to make sure the error from the converter is properly propagated?

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.

imp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "USD",
},
setMock: func(m *mock.Mock) {
m.On("GetRate", "USD", "EUR").Return(2.5, nil)
},
expectedImp: openrtb2.Imp{
BidFloor: 2.5,
BidFloorCur: "EUR",
},
assertError: assert.NoError,
},
"USD: no bidfloor": {
imp: openrtb2.Imp{
BidFloor: 0,
BidFloorCur: "USD",
},
setMock: func(m *mock.Mock) {},
expectedImp: openrtb2.Imp{
BidFloor: 0,
BidFloorCur: "EUR",
},
assertError: assert.NoError,
},
"ABC: invalid currency code": {
imp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "ABC",
},
setMock: func(m *mock.Mock) {
m.On("GetRate", "ABC", "EUR").Return(0.0, errors.New("currency conversion error"))
},
expectedImp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "ABC",
},
assertError: assert.Error,
},
}

var testCasesExtension = map[string]struct {
imp openrtb2.Imp
assertError func(t assert.TestingT, err error, msgAndArgs ...interface{}) bool
}{
"Valid Orbidder Extension": {
imp: openrtb2.Imp{
Ext: json.RawMessage(`{"bidder":{"accountId":"orbidder-test", "placementId":"center-banner", "bidfloor": 0.1}}`),
},
assertError: assert.NoError,
},
"Invalid Orbidder Extension": {
imp: openrtb2.Imp{
Ext: json.RawMessage(`{"there's'":{"something":"strange", "in the":"neighbourhood", "who you gonna call?": 0.1}}`),
},
assertError: assert.Error,
},
}

func TestPreprocessBidFloorCurrency(t *testing.T) {
for name, tc := range testCasesCurrency {
t.Run(name, func(t *testing.T) {
imp := tc.imp
mockConversions := &mockCurrencyConversion{}
tc.setMock(&mockConversions.Mock)
extraRequestInfo := adapters.ExtraRequestInfo{
CurrencyConversions: mockConversions,
}
err := preprocessBidFloorCurrency(&imp, &extraRequestInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to call mockCurrencyConversion.AssertExpectations(t) to make sure that the calls that you're expecting to the currency converter implementation are called with the args you have specified in setMock in those test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing this out to us. We added a call of AsserrExpectations in our test method. Always happy to learn new stuff :)

assert.True(t, mockConversions.AssertExpectations(t))
tc.assertError(t, err)
assert.Equal(t, tc.expectedImp, imp)
})
}
}

type mockCurrencyConversion struct {
mock.Mock
}

func (m *mockCurrencyConversion) GetRate(from string, to string) (float64, error) {
args := m.Called(from, to)
return args.Get(0).(float64), args.Error(1)
}

func (m *mockCurrencyConversion) GetRates() *map[string]map[string]float64 {
args := m.Called()
return args.Get(0).(*map[string]map[string]float64)
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
}
]
},
"bidfloorcur": "EUR",
"ext": {
"bidder": {
"accountId": "orbidder-test",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
}
]
},
"bidfloorcur": "EUR",
"ext": {
"bidder": {
"accountId": "orbidder-test",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
}
]
},
"bidfloorcur": "EUR",
"ext": {
"bidder": {
"accountId": "orbidder-test",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
}
]
},
"bidfloorcur": "EUR",
"ext": {
"bidder": {
"accountId": "orbidder-test",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
}
]
},
"bidfloorcur": "EUR",
"ext": {
"bidder": {
"accountId": "orbidder-test",
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
}
]
},
"bidfloorcur": "EUR",
"ext": {
"bidder": {
"accountId": "orbidder-test",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
}
]
},
"bidfloorcur": "EUR",
"ext": {
"bidder": {
"accountId": "orbidder-test",
Expand Down