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 1 commit
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
24 changes: 22 additions & 2 deletions exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

//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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean if we determine a dedupe is required but the bid index can't be found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should not be the case. But if this happens then there is nothing to delete from list and we just skip it.

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]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the else case necessary? I think append might gracefully handle nil slices or slices with no elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

else if is the case when we need to remove last element from list. First if is the case when we need to remove element from the middle of array.

}
}
}
}
delete(res, dupe.bidID)
Expand Down
102 changes: 102 additions & 0 deletions exchange/exchange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2547,6 +2547,108 @@ func TestCategoryMappingTwoBiddersOneBidEachNoCategorySamePrice(t *testing.T) {

}

func TestCategoryMappingTwoBiddersManyBidsEachNoCategorySamePrice(t *testing.T) {
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

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}
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{}

Expand Down