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

Currency handling fix #1097

Merged
merged 4 commits into from
Nov 13, 2019
Merged

Currency handling fix #1097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
977fadf
Resolves #1067
hhhjort Nov 1, 2019
a8182d8
Added test
hhhjort Nov 4, 2019
08275ca
Addresses PR comments
hhhjort Nov 6, 2019
5f6b6e0
fixes typo
hhhjort Nov 12, 2019
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
7 changes: 6 additions & 1 deletion endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,11 @@ func (deps *endpointDeps) validateRequest(req *openrtb.BidRequest) []error {
return []error{errors.New("request.imp must contain at least one element.")}
}

if len(req.Cur) > 1 {
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
req.Cur = req.Cur[0:1]
errL = append(errL, &errortypes.Warning{Message: fmt.Sprintf("A prebid request can only process one currency. Taking the first currency in the list, %s, as the active currency", req.Cur[0])})
}

var aliases map[string]string
if bidExt, err := deps.parseBidExt(req.Ext); err != nil {
return []error{err}
Expand Down Expand Up @@ -1172,7 +1177,7 @@ func writeError(errs []error, w http.ResponseWriter, labels *pbsmetrics.Labels)
func fatalError(errL []error) bool {
for _, err := range errL {
errCode := errortypes.DecodeError(err)
if errCode != errortypes.BidderTemporarilyDisabledCode || errCode == errortypes.BlacklistedAppCode || errCode == errortypes.BlacklistedAcctCode {
if errCode != errortypes.BidderTemporarilyDisabledCode && errCode != errortypes.WarningCode {
return true
}
}
Expand Down
48 changes: 48 additions & 0 deletions endpoints/openrtb2/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,6 +865,54 @@ func validRequest(t *testing.T, filename string) string {
return string(requestData)
}

func TestCurrencyTrunc(t *testing.T) {
deps := &endpointDeps{
&nobidExchange{},
newParamsValidator(t),
&mockStoredReqFetcher{},
empty_fetcher.EmptyFetcher{},
empty_fetcher.EmptyFetcher{},
&config.Configuration{},
pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList(), config.DisabledMetrics{}),
analyticsConf.NewPBSAnalytics(&config.Analytics{}),
map[string]string{},
false,
[]byte{},
openrtb_ext.BidderMap,
}

ui := uint64(1)
req := openrtb.BidRequest{
ID: "someID",
Imp: []openrtb.Imp{
{
ID: "imp-ID",
Banner: &openrtb.Banner{
W: &ui,
H: &ui,
},
Ext: json.RawMessage("{\"appnexus\": {\"placementId\": 5667}}"),
},
},
Site: &openrtb.Site{
ID: "myID",
},
Cur: []string{"USD", "EUR"},
}

errL := deps.validateRequest(&req)
for _, err := range errL {
if err.Error() != "A prebid request can only process one currency. Taking the first currency in the list, USD, as the active currency" {
t.Errorf(err.Error())
} else {
if errortypes.DecodeError(err) != errortypes.WarningCode {
t.Errorf("The expected warning has the wrong error type/code")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to assert the error code and the error type as well for the multiple currency warning

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That may be a bit overkill. A lot would have to go wrong to get an error with that message, but somehow not have the correct code/type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I was suggesting for those assertions more to safeguard us from situations where somebody touching that code and potentially changing the error type for it, from warning to something else, maybe a fatal error type, then the tests can help catch that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If someone just randomly changes the error type, that is a pretty blatant and silly change. If it something more subtle like the error code changing, then the test will probably target the new code, unless something really subtle is going on. In any case, I had already beefed up the test since it wasn't much work.

}
}
assert.Len(t, req.Cur, 1)
}

// nobidExchange is a well-behaved exchange which always bids "no bid".
type nobidExchange struct {
gotRequest *openrtb.BidRequest
Expand Down
15 changes: 15 additions & 0 deletions errortypes/errortypes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const (
BidderTemporarilyDisabledCode
BlacklistedAcctCode
AcctRequiredCode
WarningCode
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a question I have: Warning seems to be more generic than other code listed here. Should it rather be scoped to the currency warning? Something like OneCurrencySupportedCode or anything like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The most generic error code is not having a code at all. The purpose of the error codes is to aid in automation, that is a client making the requests and receiving the responses can just look at the code rather than parse out the error message and behave accordingly. I would not have added the code at all except we have a piece of code that whitelists which error codes are not fatal codes, and the request can continue. So I felt a generic warning code would cover all the use cases and keep us from generating more codes in the future that will not be used for anything beyond adding to the non-fatal white list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright :)

)

// We should use this code for any Error interface that is not in this package
Expand Down Expand Up @@ -155,6 +156,20 @@ func (err *BidderTemporarilyDisabled) Code() int {
return BidderTemporarilyDisabledCode
}

// Warning is a generic warning type, not a serious error
type Warning struct {
Message string
}

func (err *Warning) Error() string {
return err.Message
}

// Code returns the error code
func (err *Warning) Code() int {
return WarningCode
}

// DecodeError provides the error code for an error, as defined above
func DecodeError(err error) int {
if ce, ok := err.(Coder); ok {
Expand Down