Skip to content

Commit

Permalink
Bugfix for applyCategoryMapping (#1857)
Browse files Browse the repository at this point in the history
  • Loading branch information
@VeronikaSolovei9
VeronikaSolovei9 authored Jun 3, 2021
1 parent 339bcaf commit 5679ef3
Show file tree
Hide file tree
Showing 2 changed files with 201 additions and 15 deletions.
43 changes: 38 additions & 5 deletions exchange/exchange.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,16 @@ func (big *bidIDGenerator) New() (string, error) {
return rawUuid.String(), err
}

type deduplicateChanceGenerator interface {
Generate() bool
}

type randomDeduplicateBidBooleanGenerator struct{}

func (randomDeduplicateBidBooleanGenerator) Generate() bool {
return rand.Intn(100) < 50
}

func NewExchange(adapters map[openrtb_ext.BidderName]adaptedBidder, cache prebid_cache_client.Client, cfg *config.Configuration, metricsEngine metrics.MetricsEngine, infos config.BidderInfos, gDPR gdpr.Permissions, currencyConverter *currency.RateConverter, categoriesFetcher stored_requests.CategoryFetcher) Exchange {
return &exchange{
adapterMap: adapters,
Expand Down Expand Up @@ -205,7 +215,7 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
//If includebrandcategory is present in ext then CE feature is on.
if requestExt.Prebid.Targeting != nil && requestExt.Prebid.Targeting.IncludeBrandCategory != nil {
var rejections []string
bidCategory, adapterBids, rejections, err = applyCategoryMapping(ctx, requestExt, adapterBids, e.categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err = applyCategoryMapping(ctx, requestExt, adapterBids, e.categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})
if err != nil {
return nil, fmt.Errorf("Error in category mapping : %s", err.Error())
}
Expand Down Expand Up @@ -612,7 +622,7 @@ func encodeBidResponseExt(bidResponseExt *openrtb_ext.ExtBidResponse) ([]byte, e
return buffer.Bytes(), err
}

func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtRequest, seatBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, categoriesFetcher stored_requests.CategoryFetcher, targData *targetData) (map[string]string, map[openrtb_ext.BidderName]*pbsOrtbSeatBid, []string, error) {
func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtRequest, seatBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, categoriesFetcher stored_requests.CategoryFetcher, targData *targetData, booleanGenerator deduplicateChanceGenerator) (map[string]string, map[openrtb_ext.BidderName]*pbsOrtbSeatBid, []string, error) {
res := make(map[string]string)

type bidDedupe struct {
Expand Down Expand Up @@ -741,7 +751,7 @@ func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtReques
currBidPrice = 0
}
if dupeBidPrice == currBidPrice {
if rand.Intn(100) < 50 {
if booleanGenerator.Generate() {
dupeBidPrice = -1
} else {
currBidPrice = -1
Expand All @@ -756,11 +766,16 @@ func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtReques
} else {
// An older bid from a different seatBid we've already finished with
oldSeatBid := (seatBids)[dupe.bidderName]
rejections = updateRejections(rejections, dupe.bidID, "Bid was deduplicated")
if len(oldSeatBid.bids) == 1 {
seatBidsToRemove = append(seatBidsToRemove, dupe.bidderName)
rejections = updateRejections(rejections, dupe.bidID, "Bid was deduplicated")
} else {
oldSeatBid.bids = append(oldSeatBid.bids[:dupe.bidIndex], oldSeatBid.bids[dupe.bidIndex+1:]...)
// This is a very rare, but still possible case where bid needs to be removed from already processed bidder
// This happens when current processing bidder has a bid that has same deduplication key as a bid from already processed bidder
// and already processed bid was selected to be removed
// See example of input data in unit test `TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice`
// Need to remove bid by name, not index in this case
removeBidById(oldSeatBid, dupe.bidID)
}
}
delete(res, dupe.bidID)
Expand Down Expand Up @@ -798,6 +813,24 @@ func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtReques
return res, seatBids, rejections, nil
}

func removeBidById(seatBid *pbsOrtbSeatBid, bidID string) {
//Find index of bid to remove
dupeBidIndex := -1
for i, bid := range seatBid.bids {
if bid.bid.ID == bidID {
dupeBidIndex = i
break
}
}
if dupeBidIndex != -1 {
if dupeBidIndex < len(seatBid.bids)-1 {
seatBid.bids = append(seatBid.bids[:dupeBidIndex], seatBid.bids[dupeBidIndex+1:]...)
} else if dupeBidIndex == len(seatBid.bids)-1 {
seatBid.bids = seatBid.bids[:len(seatBid.bids)-1]
}
}
}

func updateRejections(rejections []string, bidID string, reason string) []string {
message := fmt.Sprintf("bid rejected [bid ID: %s] reason: %s", bidID, reason)
return append(rejections, message)
Expand Down
173 changes: 163 additions & 10 deletions exchange/exchange_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1800,6 +1800,14 @@ func (big *mockBidIDGenerator) New() (string, error) {

}

type fakeRandomDeduplicateBidBooleanGenerator struct {
returnValue bool
}

func (m *fakeRandomDeduplicateBidBooleanGenerator) Generate() bool {
return m.returnValue
}

func newExtRequest() openrtb_ext.ExtRequest {
priceGran := openrtb_ext.PriceGranularity{
Precision: 2,
Expand Down Expand Up @@ -1899,7 +1907,7 @@ func TestCategoryMapping(t *testing.T) {

adapterBids[bidderName1] = &seatBid

bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})

assert.Equal(t, nil, err, "Category mapping error should be empty")
assert.Equal(t, 1, len(rejections), "There should be 1 bid rejection message")
Expand Down Expand Up @@ -1954,7 +1962,7 @@ func TestCategoryMappingNoIncludeBrandCategory(t *testing.T) {

adapterBids[bidderName1] = &seatBid

bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})

assert.Equal(t, nil, err, "Category mapping error should be empty")
assert.Empty(t, rejections, "There should be no bid rejection messages")
Expand Down Expand Up @@ -2006,7 +2014,7 @@ func TestCategoryMappingTranslateCategoriesNil(t *testing.T) {

adapterBids[bidderName1] = &seatBid

bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})

assert.Equal(t, nil, err, "Category mapping error should be empty")
assert.Equal(t, 1, len(rejections), "There should be 1 bid rejection message")
Expand Down Expand Up @@ -2088,7 +2096,7 @@ func TestCategoryMappingTranslateCategoriesFalse(t *testing.T) {

adapterBids[bidderName1] = &seatBid

bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})

assert.Equal(t, nil, err, "Category mapping error should be empty")
assert.Empty(t, rejections, "There should be no bid rejection messages")
Expand Down Expand Up @@ -2158,7 +2166,7 @@ func TestCategoryDedupe(t *testing.T) {

adapterBids[bidderName1] = &seatBid

bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})

assert.Equal(t, nil, err, "Category mapping error should be empty")
assert.Equal(t, 3, len(rejections), "There should be 2 bid rejection messages")
Expand Down Expand Up @@ -2238,7 +2246,7 @@ func TestNoCategoryDedupe(t *testing.T) {

adapterBids[bidderName1] = &seatBid

bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})

assert.Equal(t, nil, err, "Category mapping error should be empty")
assert.Equal(t, 2, len(rejections), "There should be 2 bid rejection messages")
Expand Down Expand Up @@ -2303,7 +2311,7 @@ func TestCategoryMappingBidderName(t *testing.T) {
adapterBids[bidderName1] = &seatBid1
adapterBids[bidderName2] = &seatBid2

bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})

assert.NoError(t, err, "Category mapping error should be empty")
assert.Empty(t, rejections, "There should be 0 bid rejection messages")
Expand Down Expand Up @@ -2357,7 +2365,7 @@ func TestCategoryMappingBidderNameNoCategories(t *testing.T) {
adapterBids[bidderName1] = &seatBid1
adapterBids[bidderName2] = &seatBid2

bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})

assert.NoError(t, err, "Category mapping error should be empty")
assert.Empty(t, rejections, "There should be 0 bid rejection messages")
Expand Down Expand Up @@ -2458,7 +2466,7 @@ func TestBidRejectionErrors(t *testing.T) {

adapterBids[bidderName] = &seatBid

bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &test.reqExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &test.reqExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})

if len(test.expectedCatDur) > 0 {
// Bid deduplication case
Expand Down Expand Up @@ -2521,7 +2529,7 @@ func TestCategoryMappingTwoBiddersOneBidEachNoCategorySamePrice(t *testing.T) {
adapterBids[bidderNameApn1] = &seatBidApn1
adapterBids[bidderNameApn2] = &seatBidApn2

bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{})

assert.NoError(t, err, "Category mapping error should be empty")
assert.Len(t, rejections, 1, "There should be 1 bid rejection message")
Expand All @@ -2540,9 +2548,154 @@ func TestCategoryMappingTwoBiddersOneBidEachNoCategorySamePrice(t *testing.T) {
} else {
assert.Nil(t, seatBidApn1.bids, "Appnexus_1 seat bid should not have any bids back")
assert.Len(t, seatBidApn2.bids, 1, "Appnexus_2 seat bid should have only one back")
}
}
}

func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) {
// This test covers a very rare de-duplication case where bid needs to be removed from already processed bidder
// This happens when current processing bidder has a bid that has same de-duplication key as a bid from already processed bidder
// and already processed bid was selected to be removed

//In this test case bids bid_idApn1_1 and bid_idApn1_2 will be removed due to hardcoded "fakeRandomDeduplicateBidBooleanGenerator{true}"

// Also there are should be more than one bids in bidder to test how we remove single element from bids array.
// In case there is just one bid to remove - we remove the entire bidder.

categoriesFetcher, error := newCategoryFetcher("./test/category-mapping")
if error != nil {
t.Errorf("Failed to create a category Fetcher: %v", error)
}

requestExt := newExtRequestTranslateCategories(nil)

targData := &targetData{
priceGranularity: requestExt.Prebid.Targeting.PriceGranularity,
includeWinners: true,
}

requestExt.Prebid.Targeting.DurationRangeSec = []int{30}
requestExt.Prebid.Targeting.IncludeBrandCategory.WithCategory = false

cats1 := []string{"IAB1-3"}
cats2 := []string{"IAB1-4"}

bidApn1_1 := openrtb2.Bid{ID: "bid_idApn1_1", ImpID: "imp_idApn1_1", Price: 10.0000, Cat: cats1, W: 1, H: 1}
bidApn1_2 := openrtb2.Bid{ID: "bid_idApn1_2", ImpID: "imp_idApn1_2", Price: 20.0000, Cat: cats1, W: 1, H: 1}

bidApn2_1 := openrtb2.Bid{ID: "bid_idApn2_1", ImpID: "imp_idApn2_1", Price: 10.0000, Cat: cats2, W: 1, H: 1}
bidApn2_2 := openrtb2.Bid{ID: "bid_idApn2_2", ImpID: "imp_idApn2_2", Price: 20.0000, Cat: cats2, W: 1, H: 1}

bid1_Apn1_1 := pbsOrtbBid{&bidApn1_1, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""}
bid1_Apn1_2 := pbsOrtbBid{&bidApn1_2, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""}

bid1_Apn2_1 := pbsOrtbBid{&bidApn2_1, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""}
bid1_Apn2_2 := pbsOrtbBid{&bidApn2_2, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""}

innerBidsApn1 := []*pbsOrtbBid{
&bid1_Apn1_1,
&bid1_Apn1_2,
}

innerBidsApn2 := []*pbsOrtbBid{
&bid1_Apn2_1,
&bid1_Apn2_2,
}

adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid)

seatBidApn1 := pbsOrtbSeatBid{bids: innerBidsApn1, currency: "USD"}
bidderNameApn1 := openrtb_ext.BidderName("appnexus1")

seatBidApn2 := pbsOrtbSeatBid{bids: innerBidsApn2, currency: "USD"}
bidderNameApn2 := openrtb_ext.BidderName("appnexus2")

adapterBids[bidderNameApn1] = &seatBidApn1
adapterBids[bidderNameApn2] = &seatBidApn2

_, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &fakeRandomDeduplicateBidBooleanGenerator{true})

assert.NoError(t, err, "Category mapping error should be empty")

//Total number of bids from all bidders in this case should be 2
bidsFromFirstBidder := adapterBids[bidderNameApn1]
bidsFromSecondBidder := adapterBids[bidderNameApn2]

totalNumberOfbids := 0

if bidsFromFirstBidder.bids != nil {
totalNumberOfbids += len(bidsFromFirstBidder.bids)
}

if bidsFromSecondBidder.bids != nil {
totalNumberOfbids += len(bidsFromSecondBidder.bids)
}

assert.Equal(t, 2, totalNumberOfbids, "2 bids total should be returned")

assert.Len(t, adapterBids[bidderNameApn1].bids, 0)
assert.Len(t, adapterBids[bidderNameApn2].bids, 2)

assert.Equal(t, "bid_idApn2_1", adapterBids[bidderNameApn2].bids[0].bid.ID, "Incorrect expected bid 1 id")
assert.Equal(t, "bid_idApn2_2", adapterBids[bidderNameApn2].bids[1].bid.ID, "Incorrect expected bid 2 id")

assert.Len(t, rejections, 2, "2 bids should be de-duplicated")
assert.Equal(t, "bid rejected [bid ID: bid_idApn1_1] reason: Bid was deduplicated", rejections[0], "Incorrect rejected bid 1")
assert.Equal(t, "bid rejected [bid ID: bid_idApn1_2] reason: Bid was deduplicated", rejections[1], "Incorrect rejected bid 2")

}

func TestRemoveBidById(t *testing.T) {
cats1 := []string{"IAB1-3"}

bidApn1_1 := openrtb2.Bid{ID: "bid_idApn1_1", ImpID: "imp_idApn1_1", Price: 10.0000, Cat: cats1, W: 1, H: 1}
bidApn1_2 := openrtb2.Bid{ID: "bid_idApn1_2", ImpID: "imp_idApn1_2", Price: 20.0000, Cat: cats1, W: 1, H: 1}
bidApn1_3 := openrtb2.Bid{ID: "bid_idApn1_3", ImpID: "imp_idApn1_3", Price: 10.0000, Cat: cats1, W: 1, H: 1}

bid1_Apn1_1 := pbsOrtbBid{&bidApn1_1, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""}
bid1_Apn1_2 := pbsOrtbBid{&bidApn1_2, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""}
bid1_Apn1_3 := pbsOrtbBid{&bidApn1_3, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""}

type aTest struct {
desc string
inBidName string
outBids []*pbsOrtbBid
}
testCases := []aTest{
{
desc: "remove element from the middle",
inBidName: "bid_idApn1_2",
outBids: []*pbsOrtbBid{&bid1_Apn1_1, &bid1_Apn1_3},
},
{
desc: "remove element from the end",
inBidName: "bid_idApn1_3",
outBids: []*pbsOrtbBid{&bid1_Apn1_1, &bid1_Apn1_2},
},
{
desc: "remove element from the beginning",
inBidName: "bid_idApn1_1",
outBids: []*pbsOrtbBid{&bid1_Apn1_2, &bid1_Apn1_3},
},
{
desc: "remove element that doesn't exist",
inBidName: "bid_idApn",
outBids: []*pbsOrtbBid{&bid1_Apn1_1, &bid1_Apn1_2, &bid1_Apn1_3},
},
}
for _, test := range testCases {

innerBidsApn1 := []*pbsOrtbBid{
&bid1_Apn1_1,
&bid1_Apn1_2,
&bid1_Apn1_3,
}

seatBidApn1 := &pbsOrtbSeatBid{bids: innerBidsApn1, currency: "USD"}

removeBidById(seatBidApn1, test.inBidName)
assert.Len(t, seatBidApn1.bids, len(test.outBids), test.desc)
assert.ElementsMatch(t, seatBidApn1.bids, test.outBids, "Incorrect bids in response")
}

}
Expand Down

0 comments on commit 5679ef3

Please sign in to comment.