From a29fc984ee5bf5cdf4002910f79dcc17dd5017d8 Mon Sep 17 00:00:00 2001 From: Veronika Solovei Date: Thu, 20 May 2021 23:16:11 -0700 Subject: [PATCH 1/6] Bugfix for applyCategoryMapping --- exchange/exchange.go | 24 ++++++++- exchange/exchange_test.go | 102 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 2 deletions(-) diff --git a/exchange/exchange.go b/exchange/exchange.go index 0d3d93cf0a7..ef3a96967db 100644 --- a/exchange/exchange.go +++ b/exchange/exchange.go @@ -756,11 +756,31 @@ 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 unique, 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 + + //Find index of bid to remove + dupeBidIndex := -1 + for i, bid := range oldSeatBid.bids { + if bid.bid.ID == dupe.bidID { + dupeBidIndex = i + break + } + } + if dupeBidIndex != -1 { + if dupeBidIndex < len(oldSeatBid.bids)-1 { + oldSeatBid.bids = append(oldSeatBid.bids[:dupeBidIndex], oldSeatBid.bids[dupeBidIndex+1:]...) + } else if dupeBidIndex == len(oldSeatBid.bids)-1 { + oldSeatBid.bids = oldSeatBid.bids[:len(oldSeatBid.bids)-1] + } + } } } delete(res, dupe.bidID) diff --git a/exchange/exchange_test.go b/exchange/exchange_test.go index c64b3935513..01664460c92 100644 --- a/exchange/exchange_test.go +++ b/exchange/exchange_test.go @@ -2547,6 +2547,108 @@ func TestCategoryMappingTwoBiddersOneBidEachNoCategorySamePrice(t *testing.T) { } +func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) { + + 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} + bidApn1_3 := openrtb2.Bid{ID: "bid_idApn1_3", ImpID: "imp_idApn1_3", Price: 10.0000, Cat: cats1, W: 1, H: 1} + bidApn1_4 := openrtb2.Bid{ID: "bid_idApn1_4", ImpID: "imp_idApn1_4", Price: 20.0000, Cat: cats1, W: 1, H: 1} + bidApn1_5 := openrtb2.Bid{ID: "bid_idApn1_5", ImpID: "imp_idApn1_5", Price: 10.0000, Cat: cats1, W: 1, H: 1} + bidApn1_6 := openrtb2.Bid{ID: "bid_idApn1_6", ImpID: "imp_idApn1_6", 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} + bidApn2_3 := openrtb2.Bid{ID: "bid_idApn2_3", ImpID: "imp_idApn2_3", Price: 10.0000, Cat: cats2, W: 1, H: 1} + bidApn2_4 := openrtb2.Bid{ID: "bid_idApn2_4", ImpID: "imp_idApn2_4", Price: 20.0000, Cat: cats2, W: 1, H: 1} + bidApn2_5 := openrtb2.Bid{ID: "bid_idApn2_5", ImpID: "imp_idApn2_5", Price: 10.0000, Cat: cats2, W: 1, H: 1} + bidApn2_6 := openrtb2.Bid{ID: "bid_idApn2_6", ImpID: "imp_idApn2_6", 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_Apn1_3 := pbsOrtbBid{&bidApn1_3, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} + bid1_Apn1_4 := pbsOrtbBid{&bidApn1_4, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} + bid1_Apn1_5 := pbsOrtbBid{&bidApn1_5, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} + bid1_Apn1_6 := pbsOrtbBid{&bidApn1_6, "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, ""} + bid1_Apn2_3 := pbsOrtbBid{&bidApn2_3, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} + bid1_Apn2_4 := pbsOrtbBid{&bidApn2_4, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} + bid1_Apn2_5 := pbsOrtbBid{&bidApn2_5, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} + bid1_Apn2_6 := pbsOrtbBid{&bidApn2_6, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} + + innerBidsApn1 := []*pbsOrtbBid{ + &bid1_Apn1_1, + &bid1_Apn1_2, + &bid1_Apn1_3, + &bid1_Apn1_4, + &bid1_Apn1_5, + &bid1_Apn1_6, + } + + innerBidsApn2 := []*pbsOrtbBid{ + &bid1_Apn2_1, + &bid1_Apn2_2, + &bid1_Apn2_3, + &bid1_Apn2_4, + &bid1_Apn2_5, + &bid1_Apn2_6, + } + + for i := 1; i < 100; i++ { //run it for more iterations to get more cases that depend on random selection + 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) + + 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, rejections, 10, "10 bids should be deduplicated") + + } +} + func TestUpdateRejections(t *testing.T) { rejections := []string{} From 46af302b59613de5a3dd83afb2e36a6fbe2c4c84 Mon Sep 17 00:00:00 2001 From: Veronika Solovei Date: Fri, 21 May 2021 13:32:25 -0700 Subject: [PATCH 2/6] Code refactoring --- exchange/exchange.go | 53 +++++++++++++++++----------- exchange/exchange_test.go | 72 ++++++++++++++++++++++++++++++++------- 2 files changed, 93 insertions(+), 32 deletions(-) diff --git a/exchange/exchange.go b/exchange/exchange.go index ef3a96967db..a863cd71c15 100644 --- a/exchange/exchange.go +++ b/exchange/exchange.go @@ -99,6 +99,16 @@ func (big *bidIDGenerator) New() (string, error) { return rawUuid.String(), err } +type BooleanGenerator interface { + New() bool +} + +type RandomBooleanGenerator struct{} + +func (RandomBooleanGenerator) New() 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, @@ -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, &RandomBooleanGenerator{}) if err != nil { return nil, fmt.Errorf("Error in category mapping : %s", err.Error()) } @@ -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 BooleanGenerator) (map[string]string, map[openrtb_ext.BidderName]*pbsOrtbSeatBid, []string, error) { res := make(map[string]string) type bidDedupe struct { @@ -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.New() { dupeBidPrice = -1 } else { currBidPrice = -1 @@ -760,27 +770,12 @@ func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtReques if len(oldSeatBid.bids) == 1 { seatBidsToRemove = append(seatBidsToRemove, dupe.bidderName) } else { - // This is a very unique, but still possible case where bid needs to be removed from already processed bidder + // 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 - - //Find index of bid to remove - dupeBidIndex := -1 - for i, bid := range oldSeatBid.bids { - if bid.bid.ID == dupe.bidID { - dupeBidIndex = i - break - } - } - if dupeBidIndex != -1 { - if dupeBidIndex < len(oldSeatBid.bids)-1 { - oldSeatBid.bids = append(oldSeatBid.bids[:dupeBidIndex], oldSeatBid.bids[dupeBidIndex+1:]...) - } else if dupeBidIndex == len(oldSeatBid.bids)-1 { - oldSeatBid.bids = oldSeatBid.bids[:len(oldSeatBid.bids)-1] - } - } + removeBidByName(oldSeatBid, dupe.bidID) } } delete(res, dupe.bidID) @@ -818,6 +813,24 @@ func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtReques return res, seatBids, rejections, nil } +func removeBidByName(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) diff --git a/exchange/exchange_test.go b/exchange/exchange_test.go index 01664460c92..379bae67a51 100644 --- a/exchange/exchange_test.go +++ b/exchange/exchange_test.go @@ -1800,6 +1800,14 @@ func (big *mockBidIDGenerator) New() (string, error) { } +type MockRandomBooleanGenerator struct { + returnValue bool +} + +func (m *MockRandomBooleanGenerator) New() bool { + return m.returnValue +} + func newExtRequest() openrtb_ext.ExtRequest { priceGran := openrtb_ext.PriceGranularity{ Precision: 2, @@ -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, &RandomBooleanGenerator{}) assert.Equal(t, nil, err, "Category mapping error should be empty") assert.Equal(t, 1, len(rejections), "There should be 1 bid rejection message") @@ -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, &RandomBooleanGenerator{}) assert.Equal(t, nil, err, "Category mapping error should be empty") assert.Empty(t, rejections, "There should be no bid rejection messages") @@ -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, &RandomBooleanGenerator{}) assert.Equal(t, nil, err, "Category mapping error should be empty") assert.Equal(t, 1, len(rejections), "There should be 1 bid rejection message") @@ -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, &RandomBooleanGenerator{}) assert.Equal(t, nil, err, "Category mapping error should be empty") assert.Empty(t, rejections, "There should be no bid rejection messages") @@ -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, &RandomBooleanGenerator{}) assert.Equal(t, nil, err, "Category mapping error should be empty") assert.Equal(t, 3, len(rejections), "There should be 2 bid rejection messages") @@ -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, &RandomBooleanGenerator{}) assert.Equal(t, nil, err, "Category mapping error should be empty") assert.Equal(t, 2, len(rejections), "There should be 2 bid rejection messages") @@ -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, &RandomBooleanGenerator{}) assert.NoError(t, err, "Category mapping error should be empty") assert.Empty(t, rejections, "There should be 0 bid rejection messages") @@ -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, &RandomBooleanGenerator{}) assert.NoError(t, err, "Category mapping error should be empty") assert.Empty(t, rejections, "There should be 0 bid rejection messages") @@ -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, &RandomBooleanGenerator{}) if len(test.expectedCatDur) > 0 { // Bid deduplication case @@ -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, &RandomBooleanGenerator{}) assert.NoError(t, err, "Category mapping error should be empty") assert.Len(t, rejections, 1, "There should be 1 bid rejection message") @@ -2613,7 +2621,7 @@ func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) &bid1_Apn2_6, } - for i := 1; i < 100; i++ { //run it for more iterations to get more cases that depend on random selection + for i := 1; i < 10; i++ { adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid) seatBidApn1 := pbsOrtbSeatBid{bids: innerBidsApn1, currency: "USD"} @@ -2625,7 +2633,7 @@ func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) adapterBids[bidderNameApn1] = &seatBidApn1 adapterBids[bidderNameApn2] = &seatBidApn2 - _, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData) + _, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &MockRandomBooleanGenerator{true}) assert.NoError(t, err, "Category mapping error should be empty") @@ -2649,6 +2657,46 @@ func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) } } +func TestRemoveBidByName(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} + bidApn1_4 := openrtb2.Bid{ID: "bid_idApn1_4", ImpID: "imp_idApn1_4", Price: 20.0000, Cat: cats1, W: 1, H: 1} + bidApn1_5 := openrtb2.Bid{ID: "bid_idApn1_5", ImpID: "imp_idApn1_5", Price: 10.0000, Cat: cats1, W: 1, H: 1} + bidApn1_6 := openrtb2.Bid{ID: "bid_idApn1_6", ImpID: "imp_idApn1_6", Price: 20.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, ""} + bid1_Apn1_4 := pbsOrtbBid{&bidApn1_4, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} + bid1_Apn1_5 := pbsOrtbBid{&bidApn1_5, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} + bid1_Apn1_6 := pbsOrtbBid{&bidApn1_6, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} + + innerBidsApn1 := []*pbsOrtbBid{ + &bid1_Apn1_1, + &bid1_Apn1_2, + &bid1_Apn1_3, + &bid1_Apn1_4, + &bid1_Apn1_5, + &bid1_Apn1_6, + } + + seatBidApn1 := &pbsOrtbSeatBid{bids: innerBidsApn1, currency: "USD"} + + //Please keep these calls in this order + removeBidByName(seatBidApn1, "bid_idApn1_3") + assert.Len(t, seatBidApn1.bids, 5, "Bids number is incorrect after removing element from the middle") + + removeBidByName(seatBidApn1, "bid_idApn1_6") + assert.Len(t, seatBidApn1.bids, 4, "Bids number is incorrect after removing element from the end") + + removeBidByName(seatBidApn1, "bid_idApn1_1") + assert.Len(t, seatBidApn1.bids, 3, "Bids number is incorrect after removing element from the beginning") + +} + func TestUpdateRejections(t *testing.T) { rejections := []string{} From e7216a4247411e2d09686a0a1f461c9bb819f055 Mon Sep 17 00:00:00 2001 From: Veronika Solovei Date: Mon, 24 May 2021 14:19:08 -0700 Subject: [PATCH 3/6] Unit test simplification --- exchange/exchange_test.go | 68 ++++++++++++--------------------------- 1 file changed, 21 insertions(+), 47 deletions(-) diff --git a/exchange/exchange_test.go b/exchange/exchange_test.go index 379bae67a51..1b4bf6baaf4 100644 --- a/exchange/exchange_test.go +++ b/exchange/exchange_test.go @@ -2577,84 +2577,58 @@ func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) 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} - bidApn1_4 := openrtb2.Bid{ID: "bid_idApn1_4", ImpID: "imp_idApn1_4", Price: 20.0000, Cat: cats1, W: 1, H: 1} - bidApn1_5 := openrtb2.Bid{ID: "bid_idApn1_5", ImpID: "imp_idApn1_5", Price: 10.0000, Cat: cats1, W: 1, H: 1} - bidApn1_6 := openrtb2.Bid{ID: "bid_idApn1_6", ImpID: "imp_idApn1_6", 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} - bidApn2_3 := openrtb2.Bid{ID: "bid_idApn2_3", ImpID: "imp_idApn2_3", Price: 10.0000, Cat: cats2, W: 1, H: 1} - bidApn2_4 := openrtb2.Bid{ID: "bid_idApn2_4", ImpID: "imp_idApn2_4", Price: 20.0000, Cat: cats2, W: 1, H: 1} - bidApn2_5 := openrtb2.Bid{ID: "bid_idApn2_5", ImpID: "imp_idApn2_5", Price: 10.0000, Cat: cats2, W: 1, H: 1} - bidApn2_6 := openrtb2.Bid{ID: "bid_idApn2_6", ImpID: "imp_idApn2_6", 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_Apn1_3 := pbsOrtbBid{&bidApn1_3, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} - bid1_Apn1_4 := pbsOrtbBid{&bidApn1_4, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} - bid1_Apn1_5 := pbsOrtbBid{&bidApn1_5, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} - bid1_Apn1_6 := pbsOrtbBid{&bidApn1_6, "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, ""} - bid1_Apn2_3 := pbsOrtbBid{&bidApn2_3, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} - bid1_Apn2_4 := pbsOrtbBid{&bidApn2_4, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} - bid1_Apn2_5 := pbsOrtbBid{&bidApn2_5, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} - bid1_Apn2_6 := pbsOrtbBid{&bidApn2_6, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} innerBidsApn1 := []*pbsOrtbBid{ &bid1_Apn1_1, &bid1_Apn1_2, - &bid1_Apn1_3, - &bid1_Apn1_4, - &bid1_Apn1_5, - &bid1_Apn1_6, } innerBidsApn2 := []*pbsOrtbBid{ &bid1_Apn2_1, &bid1_Apn2_2, - &bid1_Apn2_3, - &bid1_Apn2_4, - &bid1_Apn2_5, - &bid1_Apn2_6, } - for i := 1; i < 10; i++ { - adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid) + adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid) - seatBidApn1 := pbsOrtbSeatBid{bids: innerBidsApn1, currency: "USD"} - bidderNameApn1 := openrtb_ext.BidderName("appnexus1") + seatBidApn1 := pbsOrtbSeatBid{bids: innerBidsApn1, currency: "USD"} + bidderNameApn1 := openrtb_ext.BidderName("appnexus1") - seatBidApn2 := pbsOrtbSeatBid{bids: innerBidsApn2, currency: "USD"} - bidderNameApn2 := openrtb_ext.BidderName("appnexus2") + seatBidApn2 := pbsOrtbSeatBid{bids: innerBidsApn2, currency: "USD"} + bidderNameApn2 := openrtb_ext.BidderName("appnexus2") - adapterBids[bidderNameApn1] = &seatBidApn1 - adapterBids[bidderNameApn2] = &seatBidApn2 + adapterBids[bidderNameApn1] = &seatBidApn1 + adapterBids[bidderNameApn2] = &seatBidApn2 - _, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &MockRandomBooleanGenerator{true}) + _, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &MockRandomBooleanGenerator{true}) - assert.NoError(t, err, "Category mapping error should be empty") + 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] + //Total number of bids from all bidders in this case should be 2 + bidsFromFirstBidder := adapterBids[bidderNameApn1] + bidsFromSecondBidder := adapterBids[bidderNameApn2] - totalNumberOfbids := 0 + totalNumberOfbids := 0 - if bidsFromFirstBidder.bids != nil { - totalNumberOfbids += len(bidsFromFirstBidder.bids) - } + if bidsFromFirstBidder.bids != nil { + totalNumberOfbids += len(bidsFromFirstBidder.bids) + } - if bidsFromSecondBidder.bids != nil { - totalNumberOfbids += len(bidsFromSecondBidder.bids) - } + if bidsFromSecondBidder.bids != nil { + totalNumberOfbids += len(bidsFromSecondBidder.bids) + } - assert.Equal(t, 2, totalNumberOfbids, "2 bids total should be returned") - assert.Len(t, rejections, 10, "10 bids should be deduplicated") + assert.Equal(t, 2, totalNumberOfbids, "2 bids total should be returned") + assert.Len(t, rejections, 2, "10 bids should be deduplicated") - } } func TestRemoveBidByName(t *testing.T) { From b9f22df01589ebd15cd54cd16e982d9e62225d2c Mon Sep 17 00:00:00 2001 From: Veronika Solovei Date: Tue, 1 Jun 2021 20:11:44 -0700 Subject: [PATCH 4/6] Minor refactoring --- exchange/exchange.go | 10 ++-- exchange/exchange_test.go | 97 ++++++++++++++++++++++++++------------- 2 files changed, 70 insertions(+), 37 deletions(-) diff --git a/exchange/exchange.go b/exchange/exchange.go index a863cd71c15..d47653db9d6 100644 --- a/exchange/exchange.go +++ b/exchange/exchange.go @@ -99,13 +99,13 @@ func (big *bidIDGenerator) New() (string, error) { return rawUuid.String(), err } -type BooleanGenerator interface { +type deduplicateBidBooleanGenerator interface { New() bool } -type RandomBooleanGenerator struct{} +type randomDeduplicateBidBooleanGenerator struct{} -func (RandomBooleanGenerator) New() bool { +func (randomDeduplicateBidBooleanGenerator) New() bool { return rand.Intn(100) < 50 } @@ -215,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, &RandomBooleanGenerator{}) + 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()) } @@ -622,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, booleanGenerator BooleanGenerator) (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 deduplicateBidBooleanGenerator) (map[string]string, map[openrtb_ext.BidderName]*pbsOrtbSeatBid, []string, error) { res := make(map[string]string) type bidDedupe struct { diff --git a/exchange/exchange_test.go b/exchange/exchange_test.go index 1b4bf6baaf4..1b2f45763c4 100644 --- a/exchange/exchange_test.go +++ b/exchange/exchange_test.go @@ -1800,11 +1800,11 @@ func (big *mockBidIDGenerator) New() (string, error) { } -type MockRandomBooleanGenerator struct { +type fakeRandomDeduplicateBidBooleanGenerator struct { returnValue bool } -func (m *MockRandomBooleanGenerator) New() bool { +func (m *fakeRandomDeduplicateBidBooleanGenerator) New() bool { return m.returnValue } @@ -1907,7 +1907,7 @@ func TestCategoryMapping(t *testing.T) { adapterBids[bidderName1] = &seatBid - bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &RandomBooleanGenerator{}) + 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") @@ -1962,7 +1962,7 @@ func TestCategoryMappingNoIncludeBrandCategory(t *testing.T) { adapterBids[bidderName1] = &seatBid - bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &RandomBooleanGenerator{}) + 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") @@ -2014,7 +2014,7 @@ func TestCategoryMappingTranslateCategoriesNil(t *testing.T) { adapterBids[bidderName1] = &seatBid - bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &RandomBooleanGenerator{}) + 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") @@ -2096,7 +2096,7 @@ func TestCategoryMappingTranslateCategoriesFalse(t *testing.T) { adapterBids[bidderName1] = &seatBid - bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &RandomBooleanGenerator{}) + 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") @@ -2166,7 +2166,7 @@ func TestCategoryDedupe(t *testing.T) { adapterBids[bidderName1] = &seatBid - bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &RandomBooleanGenerator{}) + 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") @@ -2246,7 +2246,7 @@ func TestNoCategoryDedupe(t *testing.T) { adapterBids[bidderName1] = &seatBid - bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &RandomBooleanGenerator{}) + 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") @@ -2311,7 +2311,7 @@ func TestCategoryMappingBidderName(t *testing.T) { adapterBids[bidderName1] = &seatBid1 adapterBids[bidderName2] = &seatBid2 - bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &RandomBooleanGenerator{}) + 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") @@ -2365,7 +2365,7 @@ func TestCategoryMappingBidderNameNoCategories(t *testing.T) { adapterBids[bidderName1] = &seatBid1 adapterBids[bidderName2] = &seatBid2 - bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &RandomBooleanGenerator{}) + 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") @@ -2466,7 +2466,7 @@ func TestBidRejectionErrors(t *testing.T) { adapterBids[bidderName] = &seatBid - bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &test.reqExt, adapterBids, categoriesFetcher, targData, &RandomBooleanGenerator{}) + bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &test.reqExt, adapterBids, categoriesFetcher, targData, &randomDeduplicateBidBooleanGenerator{}) if len(test.expectedCatDur) > 0 { // Bid deduplication case @@ -2529,7 +2529,7 @@ func TestCategoryMappingTwoBiddersOneBidEachNoCategorySamePrice(t *testing.T) { adapterBids[bidderNameApn1] = &seatBidApn1 adapterBids[bidderNameApn2] = &seatBidApn2 - bidCategory, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &RandomBooleanGenerator{}) + 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") @@ -2548,14 +2548,14 @@ 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 categoriesFetcher, error := newCategoryFetcher("./test/category-mapping") if error != nil { @@ -2608,7 +2608,7 @@ func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) adapterBids[bidderNameApn1] = &seatBidApn1 adapterBids[bidderNameApn2] = &seatBidApn2 - _, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &MockRandomBooleanGenerator{true}) + _, adapterBids, rejections, err := applyCategoryMapping(nil, &requestExt, adapterBids, categoriesFetcher, targData, &fakeRandomDeduplicateBidBooleanGenerator{true}) assert.NoError(t, err, "Category mapping error should be empty") @@ -2627,7 +2627,16 @@ func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) } assert.Equal(t, 2, totalNumberOfbids, "2 bids total should be returned") - assert.Len(t, rejections, 2, "10 bids should be deduplicated") + + 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") } @@ -2648,27 +2657,51 @@ func TestRemoveBidByName(t *testing.T) { bid1_Apn1_5 := pbsOrtbBid{&bidApn1_5, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} bid1_Apn1_6 := pbsOrtbBid{&bidApn1_6, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} - innerBidsApn1 := []*pbsOrtbBid{ - &bid1_Apn1_1, - &bid1_Apn1_2, - &bid1_Apn1_3, - &bid1_Apn1_4, - &bid1_Apn1_5, - &bid1_Apn1_6, + type aTest struct { + desc string + in string + } + testCases := []aTest{ + { + desc: "incorrect result after removing element from the middle", + in: "bid_idApn1_3", + }, + { + desc: "incorrect result after removing element from the end", + in: "bid_idApn1_6", + }, + { + desc: "incorrect result after removing element from the beginning", + in: "bid_idApn1_1", + }, } + for _, test := range testCases { - seatBidApn1 := &pbsOrtbSeatBid{bids: innerBidsApn1, currency: "USD"} + innerBidsApn1 := []*pbsOrtbBid{ + &bid1_Apn1_1, + &bid1_Apn1_2, + &bid1_Apn1_3, + &bid1_Apn1_4, + &bid1_Apn1_5, + &bid1_Apn1_6, + } - //Please keep these calls in this order - removeBidByName(seatBidApn1, "bid_idApn1_3") - assert.Len(t, seatBidApn1.bids, 5, "Bids number is incorrect after removing element from the middle") + seatBidApn1 := &pbsOrtbSeatBid{bids: innerBidsApn1, currency: "USD"} - removeBidByName(seatBidApn1, "bid_idApn1_6") - assert.Len(t, seatBidApn1.bids, 4, "Bids number is incorrect after removing element from the end") + removeBidByName(seatBidApn1, test.in) + assert.Len(t, seatBidApn1.bids, 5, test.desc) + assert.False(t, checkBidPresentInArray(seatBidApn1.bids, test.in), "Bid should not be present in result") + } - removeBidByName(seatBidApn1, "bid_idApn1_1") - assert.Len(t, seatBidApn1.bids, 3, "Bids number is incorrect after removing element from the beginning") +} +func checkBidPresentInArray(bids []*pbsOrtbBid, bidId string) bool { + for _, bid := range bids { + if bid.bid.ID == bidId { + return true + } + } + return false } func TestUpdateRejections(t *testing.T) { From b14b0d08a704e5511e13a641a28f87de0a09d853 Mon Sep 17 00:00:00 2001 From: Veronika Solovei Date: Tue, 1 Jun 2021 20:15:05 -0700 Subject: [PATCH 5/6] Minor refactoring --- exchange/exchange_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/exchange/exchange_test.go b/exchange/exchange_test.go index 1b2f45763c4..54cab0d0798 100644 --- a/exchange/exchange_test.go +++ b/exchange/exchange_test.go @@ -2557,6 +2557,8 @@ func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) // 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}" + categoriesFetcher, error := newCategoryFetcher("./test/category-mapping") if error != nil { t.Errorf("Failed to create a category Fetcher: %v", error) From 1ff04365306779629e305d73095d92b886782591 Mon Sep 17 00:00:00 2001 From: Veronika Solovei Date: Wed, 2 Jun 2021 18:09:21 -0700 Subject: [PATCH 6/6] Minor refactoring --- exchange/exchange.go | 14 +++++----- exchange/exchange_test.go | 56 +++++++++++++++++---------------------- 2 files changed, 32 insertions(+), 38 deletions(-) diff --git a/exchange/exchange.go b/exchange/exchange.go index d47653db9d6..ba70305f660 100644 --- a/exchange/exchange.go +++ b/exchange/exchange.go @@ -99,13 +99,13 @@ func (big *bidIDGenerator) New() (string, error) { return rawUuid.String(), err } -type deduplicateBidBooleanGenerator interface { - New() bool +type deduplicateChanceGenerator interface { + Generate() bool } type randomDeduplicateBidBooleanGenerator struct{} -func (randomDeduplicateBidBooleanGenerator) New() bool { +func (randomDeduplicateBidBooleanGenerator) Generate() bool { return rand.Intn(100) < 50 } @@ -622,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, booleanGenerator deduplicateBidBooleanGenerator) (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 { @@ -751,7 +751,7 @@ func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtReques currBidPrice = 0 } if dupeBidPrice == currBidPrice { - if booleanGenerator.New() { + if booleanGenerator.Generate() { dupeBidPrice = -1 } else { currBidPrice = -1 @@ -775,7 +775,7 @@ func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtReques // 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 - removeBidByName(oldSeatBid, dupe.bidID) + removeBidById(oldSeatBid, dupe.bidID) } } delete(res, dupe.bidID) @@ -813,7 +813,7 @@ func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtReques return res, seatBids, rejections, nil } -func removeBidByName(seatBid *pbsOrtbSeatBid, bidID string) { +func removeBidById(seatBid *pbsOrtbSeatBid, bidID string) { //Find index of bid to remove dupeBidIndex := -1 for i, bid := range seatBid.bids { diff --git a/exchange/exchange_test.go b/exchange/exchange_test.go index 54cab0d0798..a03c4786b79 100644 --- a/exchange/exchange_test.go +++ b/exchange/exchange_test.go @@ -1804,7 +1804,7 @@ type fakeRandomDeduplicateBidBooleanGenerator struct { returnValue bool } -func (m *fakeRandomDeduplicateBidBooleanGenerator) New() bool { +func (m *fakeRandomDeduplicateBidBooleanGenerator) Generate() bool { return m.returnValue } @@ -2559,6 +2559,9 @@ func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) //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) @@ -2642,39 +2645,42 @@ func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) } -func TestRemoveBidByName(t *testing.T) { +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} - bidApn1_4 := openrtb2.Bid{ID: "bid_idApn1_4", ImpID: "imp_idApn1_4", Price: 20.0000, Cat: cats1, W: 1, H: 1} - bidApn1_5 := openrtb2.Bid{ID: "bid_idApn1_5", ImpID: "imp_idApn1_5", Price: 10.0000, Cat: cats1, W: 1, H: 1} - bidApn1_6 := openrtb2.Bid{ID: "bid_idApn1_6", ImpID: "imp_idApn1_6", Price: 20.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, ""} - bid1_Apn1_4 := pbsOrtbBid{&bidApn1_4, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} - bid1_Apn1_5 := pbsOrtbBid{&bidApn1_5, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} - bid1_Apn1_6 := pbsOrtbBid{&bidApn1_6, "video", nil, &openrtb_ext.ExtBidPrebidVideo{Duration: 30}, nil, 0, false, ""} type aTest struct { - desc string - in string + desc string + inBidName string + outBids []*pbsOrtbBid } testCases := []aTest{ { - desc: "incorrect result after removing element from the middle", - in: "bid_idApn1_3", + 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: "incorrect result after removing element from the end", - in: "bid_idApn1_6", + desc: "remove element from the beginning", + inBidName: "bid_idApn1_1", + outBids: []*pbsOrtbBid{&bid1_Apn1_2, &bid1_Apn1_3}, }, { - desc: "incorrect result after removing element from the beginning", - in: "bid_idApn1_1", + 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 { @@ -2683,29 +2689,17 @@ func TestRemoveBidByName(t *testing.T) { &bid1_Apn1_1, &bid1_Apn1_2, &bid1_Apn1_3, - &bid1_Apn1_4, - &bid1_Apn1_5, - &bid1_Apn1_6, } seatBidApn1 := &pbsOrtbSeatBid{bids: innerBidsApn1, currency: "USD"} - removeBidByName(seatBidApn1, test.in) - assert.Len(t, seatBidApn1.bids, 5, test.desc) - assert.False(t, checkBidPresentInArray(seatBidApn1.bids, test.in), "Bid should not be present in result") + 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") } } -func checkBidPresentInArray(bids []*pbsOrtbBid, bidId string) bool { - for _, bid := range bids { - if bid.bid.ID == bidId { - return true - } - } - return false -} - func TestUpdateRejections(t *testing.T) { rejections := []string{}