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

Jump to bottom

Bugfix for applyCategoryMapping #1857

Merged
merged 6 commits into from
Jun 3, 2021
Merged

Bugfix for applyCategoryMapping #1857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a29fc98
Bugfix for applyCategoryMapping
May 21, 2021
46af302
Code refactoring
May 21, 2021
e7216a4
Unit test simplification
May 24, 2021
b9f22df
Minor refactoring
Jun 2, 2021
b14b0d0
Minor refactoring
Jun 2, 2021
1ff0436
Minor refactoring
Jun 3, 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
43 changes: 38 additions & 5 deletions exchange/exchange.go
View file
Open in desktop
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
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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) {
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
// 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}"
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

// 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}
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
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")
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

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"}
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

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