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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Dec 20, 2021
Merged

Orbidder: bidfloor currency handling #2093

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5d4ab1d
orbidder adapter: bid floor currency handling implemented and tested
andreasmaurer0210 Nov 24, 2021
794f81d
orbidder adapter: bidfloorcur in Test Cases added
andreasmaurer0210 Nov 24, 2021
000fef8
orbidder adapter: removed unnecessary check for empty Imp slice
andreasmaurer0210 Nov 25, 2021
3aa28cb
orbidder adapter: removed unnecessary if condition
andreasmaurer0210 Dec 14, 2021
9d789f7
orbidder adapter: testcases added
andreasmaurer0210 Dec 15, 2021
cb063f1
orbidder adapter: testcases added
andreasmaurer0210 Dec 15, 2021
87553de
orbidder adapter: further testcase added, assert mock framework in te…
andreasmaurer0210 Dec 17, 2021
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
77 changes: 51 additions & 26 deletions adapters/orbidder/orbidder.go
View file
Open in desktop
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,41 @@ 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

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

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 •
edited
Loading

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 +88,25 @@ 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.BidFloorCur == "" {
imp.BidFloorCur = "EUR"
}

if imp.BidFloor > 0 && strings.ToUpper(imp.BidFloorCur) != "EUR" {
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 +150,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
98 changes: 98 additions & 0 deletions adapters/orbidder/orbidder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ package orbidder

import (
"encoding/json"
"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 Down Expand Up @@ -32,3 +35,98 @@ func TestJsonSamples(t *testing.T) {

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

var testCases = map[string]struct {
imp openrtb2.Imp
setMock func(m *mock.Mock)
expectedImp openrtb2.Imp
}{
"EUR: no bidfloor, no currency": {
imp: openrtb2.Imp{
BidFloor: 0,
BidFloorCur: "",
},
setMock: func(m *mock.Mock) {},
expectedImp: openrtb2.Imp{
BidFloor: 0,
BidFloorCur: "EUR",
},
},
"EUR: bidfloor, no currency": {
imp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "",
},
setMock: func(m *mock.Mock) {},
expectedImp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "EUR",
},
},
"EUR: bidfloor and currency": {
imp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "EUR",
},
setMock: func(m *mock.Mock) {},
expectedImp: openrtb2.Imp{
BidFloor: 1,
BidFloorCur: "EUR",
},
},
"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",
},
},
"USD: no bidfloor": {
imp: openrtb2.Imp{
BidFloor: 0,
BidFloorCur: "USD",
},
setMock: func(m *mock.Mock) {},
expectedImp: openrtb2.Imp{
BidFloor: 0,
BidFloorCur: "EUR",
},
},
}

func TestPreprocessBidFloorCurrency(t *testing.T) {
for name, tc := range testCases {
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.NoError(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)
}
1 change: 1 addition & 0 deletions adapters/orbidder/orbiddertest/exemplary/simple-app-banner.json
View file
Open in desktop
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
1 change: 1 addition & 0 deletions adapters/orbidder/orbiddertest/supplemental/dsp-bad-request-example.json
View file
Open in desktop
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
1 change: 1 addition & 0 deletions adapters/orbidder/orbiddertest/supplemental/dsp-bad-response-example.json
View file
Open in desktop
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
1 change: 1 addition & 0 deletions adapters/orbidder/orbiddertest/supplemental/dsp-internal-server-error-example.json
View file
Open in desktop
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
1 change: 1 addition & 0 deletions adapters/orbidder/orbiddertest/supplemental/dsp-invalid-accountid-example.json
View file
Open in desktop
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
1 change: 1 addition & 0 deletions adapters/orbidder/orbiddertest/supplemental/no-content-response.json
View file
Open in desktop
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
1 change: 1 addition & 0 deletions adapters/orbidder/orbiddertest/supplemental/valid-and-invalid-imps.json
View file
Open in desktop
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