diff --git a/src/main/java/org/prebid/server/auction/BidResponseCreator.java b/src/main/java/org/prebid/server/auction/BidResponseCreator.java index 7c56ccb0724..16a572afbc9 100644 --- a/src/main/java/org/prebid/server/auction/BidResponseCreator.java +++ b/src/main/java/org/prebid/server/auction/BidResponseCreator.java @@ -75,7 +75,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.EnumMap; import java.util.HashMap; import java.util.HashSet; @@ -102,6 +101,7 @@ public class BidResponseCreator { private final BidderCatalog bidderCatalog; private final EventsService eventsService; private final StoredRequestProcessor storedRequestProcessor; + private final BidResponseReducer bidResponseReducer; private final IdGenerator bidIdGenerator; private final int truncateAttrChars; private final Clock clock; @@ -115,6 +115,7 @@ public BidResponseCreator(CacheService cacheService, BidderCatalog bidderCatalog, EventsService eventsService, StoredRequestProcessor storedRequestProcessor, + BidResponseReducer bidResponseReducer, IdGenerator bidIdGenerator, int truncateAttrChars, Clock clock, @@ -124,6 +125,7 @@ public BidResponseCreator(CacheService cacheService, this.bidderCatalog = Objects.requireNonNull(bidderCatalog); this.eventsService = Objects.requireNonNull(eventsService); this.storedRequestProcessor = Objects.requireNonNull(storedRequestProcessor); + this.bidResponseReducer = Objects.requireNonNull(bidResponseReducer); this.bidIdGenerator = Objects.requireNonNull(bidIdGenerator); this.truncateAttrChars = validateTruncateAttrChars(truncateAttrChars); this.clock = Objects.requireNonNull(clock); @@ -195,7 +197,9 @@ private Future cacheBidsAndCreateResponse(List bidd final BidRequest bidRequest = auctionContext.getBidRequest(); - bidderResponses.forEach(BidResponseCreator::removeRedundantBids); + final List updatedBidderResponses = bidderResponses.stream() + .map(bidResponseReducer::removeRedundantBids) + .collect(Collectors.toList()); ExtRequestTargeting targeting = targeting(bidRequest); final Set winningBids = newOrEmptySet(targeting); @@ -208,12 +212,12 @@ private Future cacheBidsAndCreateResponse(List bidd // determine winning bids only if targeting is present if (targeting != null) { - populateWinningBids(bidderResponses, winningBids, winningBidsByBidder); + populateWinningBids(updatedBidderResponses, winningBids, winningBidsByBidder); } final Set bidsToCache = cacheInfo.isShouldCacheWinningBidsOnly() ? winningBids - : bidderResponses.stream().flatMap(BidResponseCreator::getBids).collect(Collectors.toSet()); + : updatedBidderResponses.stream().flatMap(BidResponseCreator::getBids).collect(Collectors.toSet()); final EventsContext eventsContext = EventsContext.builder() .enabledForAccount(eventsEnabledForAccount(auctionContext)) @@ -223,7 +227,7 @@ private Future cacheBidsAndCreateResponse(List bidd .build(); return toBidsWithCacheIds( - bidderResponses, + updatedBidderResponses, bidsToCache, generatedBidIds, auctionContext, @@ -231,7 +235,7 @@ private Future cacheBidsAndCreateResponse(List bidd eventsContext) .compose(cacheResult -> videoStoredDataResult(auctionContext) .map(videoStoredDataResult -> toBidResponse( - bidderResponses, + updatedBidderResponses, auctionContext, targeting, winningBids, @@ -286,39 +290,6 @@ private ExtBidResponse toExtBidResponse(List bidderResponses, ExtBidResponsePrebid.of(auctionTimestamp)); } - private static void removeRedundantBids(BidderResponse bidderResponse) { - final List responseBidderBids = bidderResponse.getSeatBid().getBids(); - if (responseBidderBids.size() < 2) { // no reason for removing if only one bid was responded - return; - } - - final Map> impIdToBidderBid = responseBidderBids.stream() - .collect(Collectors.groupingBy(bidderBid -> bidderBid.getBid().getImpid())); - - final Set mostValuableBids = impIdToBidderBid.values().stream() - .map(BidResponseCreator::mostValuableBid) - .collect(Collectors.toSet()); - - responseBidderBids.clear(); - responseBidderBids.addAll(mostValuableBids); - } - - private static BidderBid mostValuableBid(List bidderBids) { - if (bidderBids.size() == 1) { - return bidderBids.get(0); - } - - final List dealBidderBids = bidderBids.stream() - .filter(bidderBid -> StringUtils.isNotBlank(bidderBid.getBid().getDealid())) - .collect(Collectors.toList()); - - final List processedBidderBids = dealBidderBids.isEmpty() ? bidderBids : dealBidderBids; - - return processedBidderBids.stream() - .max(Comparator.comparing(bidderBid -> bidderBid.getBid().getPrice(), Comparator.naturalOrder())) - .orElse(bidderBids.get(0)); - } - /** * Returns new {@link HashSet} in case of existing keywordsCreator or empty collection if null. */ diff --git a/src/main/java/org/prebid/server/auction/BidResponseReducer.java b/src/main/java/org/prebid/server/auction/BidResponseReducer.java new file mode 100644 index 00000000000..5d1635abdd2 --- /dev/null +++ b/src/main/java/org/prebid/server/auction/BidResponseReducer.java @@ -0,0 +1,85 @@ +package org.prebid.server.auction; + +import com.iab.openrtb.response.Bid; +import org.apache.commons.collections4.ListUtils; +import org.apache.commons.lang3.StringUtils; +import org.prebid.server.auction.model.BidderResponse; +import org.prebid.server.bidder.model.BidderBid; +import org.prebid.server.bidder.model.BidderSeatBid; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * Class for removing bids from response for the same bidder-imp pair. + */ +public class BidResponseReducer { + + /** + * Removes {@link Bid}s with the same impId taking into account if {@link Bid} has deal. + *

+ * Returns given list of {@link BidderResponse}s if {@link Bid}s have different impIds. + */ + public BidderResponse removeRedundantBids(BidderResponse bidderResponse) { + final List bidderBids = ListUtils.emptyIfNull(bidderResponse.getSeatBid().getBids()); + final Map> impIdToBidderBids = bidderBids.stream() + .collect(Collectors.groupingBy(bidderBid -> bidderBid.getBid().getImpid())); + + final Set updatedBidderBids = impIdToBidderBids.values().stream() + .map(BidResponseReducer::removeRedundantBidsForImp) + .flatMap(Collection::stream) + .collect(Collectors.toSet()); + + if (bidderBids.size() == updatedBidderBids.size()) { + return bidderResponse; + } + + return updateBidderResponse(bidderResponse, updatedBidderBids); + } + + private static List removeRedundantBidsForImp(List bidderBids) { + return bidderBids.size() > 1 ? reduceBidsByImpId(bidderBids) : bidderBids; + } + + private static List reduceBidsByImpId(List bidderBids) { + return bidderBids.stream().anyMatch(bidderBid -> bidderBid.getBid().getDealid() != null) + ? removeRedundantDealsBids(bidderBids) + : removeRedundantForNonDealBids(bidderBids); + } + + private static List removeRedundantDealsBids(List bidderBids) { + final List dealBidderBids = bidderBids.stream() + .filter(bidderBid -> StringUtils.isNotBlank(bidderBid.getBid().getDealid())) + .collect(Collectors.toList()); + + return Collections.singletonList(getHighestPriceBid(bidderBids, dealBidderBids)); + } + + private static List removeRedundantForNonDealBids(List bidderBids) { + return Collections.singletonList(getHighestPriceBid(bidderBids, bidderBids)); + } + + private static BidderBid getHighestPriceBid(List bidderBids, List dealBidderBids) { + return dealBidderBids.stream() + .max(Comparator.comparing(bidderBid -> bidderBid.getBid().getPrice(), Comparator.naturalOrder())) + .orElse(bidderBids.get(0)); + } + + private static BidderResponse updateBidderResponse(BidderResponse bidderResponse, + Set updatedBidderBids) { + + final BidderSeatBid seatBid = bidderResponse.getSeatBid(); + final BidderSeatBid updatedSeatBid = BidderSeatBid.of( + new ArrayList<>(updatedBidderBids), + seatBid.getHttpCalls(), + seatBid.getErrors()); + + return BidderResponse.of(bidderResponse.getBidder(), updatedSeatBid, bidderResponse.getResponseTime()); + } +} diff --git a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java index e02d861c203..c68064be078 100644 --- a/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java +++ b/src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java @@ -11,6 +11,7 @@ import org.prebid.server.auction.AuctionRequestFactory; import org.prebid.server.auction.BidResponseCreator; import org.prebid.server.auction.BidResponsePostProcessor; +import org.prebid.server.auction.BidResponseReducer; import org.prebid.server.auction.ExchangeService; import org.prebid.server.auction.FpdResolver; import org.prebid.server.auction.ImplicitParametersExtractor; @@ -469,6 +470,7 @@ BidResponseCreator bidResponseCreator( BidderCatalog bidderCatalog, EventsService eventsService, StoredRequestProcessor storedRequestProcessor, + BidResponseReducer bidResponseReducer, IdGenerator bidIdGenerator, @Value("${settings.targeting.truncate-attr-chars}") int truncateAttrChars, Clock clock, @@ -479,6 +481,7 @@ BidResponseCreator bidResponseCreator( bidderCatalog, eventsService, storedRequestProcessor, + bidResponseReducer, bidIdGenerator, truncateAttrChars, clock, @@ -541,6 +544,11 @@ StoredRequestProcessor storedRequestProcessor( jsonMerger); } + @Bean + BidResponseReducer bidResponseReducer() { + return new BidResponseReducer(); + } + @Bean StoredResponseProcessor storedResponseProcessor(ApplicationSettings applicationSettings, BidderCatalog bidderCatalog, diff --git a/src/test/java/org/prebid/server/auction/BidResponseCreatorTest.java b/src/test/java/org/prebid/server/auction/BidResponseCreatorTest.java index 4e76a72b36f..b3ccd2076a7 100644 --- a/src/test/java/org/prebid/server/auction/BidResponseCreatorTest.java +++ b/src/test/java/org/prebid/server/auction/BidResponseCreatorTest.java @@ -122,6 +122,8 @@ public class BidResponseCreatorTest extends VertxTest { @Mock private StoredRequestProcessor storedRequestProcessor; @Mock + private BidResponseReducer bidResponseReducer; + @Mock private IdGenerator idGenerator; private Clock clock; @@ -135,6 +137,8 @@ public void setUp() { given(cacheService.getEndpointHost()).willReturn("testHost"); given(cacheService.getEndpointPath()).willReturn("testPath"); given(cacheService.getCachedAssetURLTemplate()).willReturn("uuid="); + given(bidResponseReducer.removeRedundantBids(any())) + .willAnswer(invocationOnMock -> invocationOnMock.getArgument(0)); given(storedRequestProcessor.videoStoredDataResult(any(), anyList(), anyList(), any())) .willReturn(Future.succeededFuture(VideoStoredDataResult.empty())); @@ -147,6 +151,7 @@ public void setUp() { bidderCatalog, eventsService, storedRequestProcessor, + bidResponseReducer, idGenerator, 0, clock, @@ -502,6 +507,7 @@ public void shouldOverrideBidIdWhenGenerateBidIdIsTurnedOn() { bidderCatalog, eventsService, storedRequestProcessor, + bidResponseReducer, idGenerator, 0, clock, @@ -566,6 +572,7 @@ public void shouldUseGeneratedBidIdForEventAndCacheWhenGeneratedIdIsTurnedOn() { bidderCatalog, eventsService, storedRequestProcessor, + bidResponseReducer, idGenerator, 0, clock, @@ -621,34 +628,22 @@ public void shouldSetExpectedResponseSeatBidAndBidFields() { } @Test - public void shouldFilterByDealsAndPriceBidsWhenImpIdsAreEqual() { + public void shouldFilterBidByBidReducerResponse() { // given final AuctionContext auctionContext = givenAuctionContext(givenBidRequest()); - final Bid simpleBidImp1 = Bid.builder().id("bidId1i1").price(BigDecimal.valueOf(5.67)).impid("i1").build(); - final Bid simpleBid1Imp2 = Bid.builder().id("bidId1i2").price(BigDecimal.valueOf(15.67)).impid("i2").build(); - final Bid simpleBid2Imp2 = Bid.builder().id("bidId2i2").price(BigDecimal.valueOf(17.67)).impid("i2").build(); final Bid dealBid1Imp1 = Bid.builder().id("bidId1i1d").dealid("d1").price(BigDecimal.valueOf(4.98)).impid("i1") .build(); final Bid dealBid2Imp1 = Bid.builder().id("bidId2i1d").dealid("d2").price(BigDecimal.valueOf(5.00)).impid("i1") .build(); final BidderSeatBid seatBidWithDeals = givenSeatBid( - BidderBid.of(simpleBidImp1, banner, null), - BidderBid.of(simpleBid1Imp2, banner, null), - BidderBid.of(simpleBid2Imp2, banner, null), // will stay (top price) - BidderBid.of(simpleBid2Imp2, banner, null), // duplicate should be removed - BidderBid.of(dealBid2Imp1, banner, null), // will stay (deal + topPrice) + BidderBid.of(dealBid2Imp1, banner, null), BidderBid.of(dealBid1Imp1, banner, null)); - final Bid simpleBid3Imp2 = Bid.builder().id("bidId3i2").price(BigDecimal.valueOf(7.25)).impid("i2").build(); - final Bid simpleBid4Imp2 = Bid.builder().id("bidId4i2").price(BigDecimal.valueOf(7.26)).impid("i2").build(); - final BidderSeatBid seatBidWithSimpleBids = givenSeatBid( - BidderBid.of(simpleBid3Imp2, banner, null), - BidderBid.of(simpleBid4Imp2, banner, null)); // will stay (top price) + final List bidderResponses = singletonList(BidderResponse.of("bidder1", seatBidWithDeals, 100)); - final List bidderResponses = asList( - BidderResponse.of("bidder1", seatBidWithDeals, 100), - BidderResponse.of("bidder2", seatBidWithSimpleBids, 111)); + given(bidResponseReducer.removeRedundantBids(any())) + .willReturn(BidderResponse.of("bidder1", givenSeatBid(BidderBid.of(dealBid2Imp1, banner, null)), 100)); // when final BidResponse bidResponse = @@ -656,9 +651,9 @@ public void shouldFilterByDealsAndPriceBidsWhenImpIdsAreEqual() { // then assertThat(bidResponse.getSeatbid()) - .flatExtracting(SeatBid::getBid).hasSize(3) + .flatExtracting(SeatBid::getBid).hasSize(1) .extracting(Bid::getId) - .containsOnly("bidId2i2", "bidId2i1d", "bidId4i2"); + .containsOnly("bidId2i1d"); } @Test @@ -1000,6 +995,7 @@ public void shouldTruncateTargetingKeywordsByGlobalConfig() { bidderCatalog, eventsService, storedRequestProcessor, + bidResponseReducer, idGenerator, 20, clock, diff --git a/src/test/java/org/prebid/server/auction/BidResponseReducerTest.java b/src/test/java/org/prebid/server/auction/BidResponseReducerTest.java new file mode 100644 index 00000000000..d20f0dbb716 --- /dev/null +++ b/src/test/java/org/prebid/server/auction/BidResponseReducerTest.java @@ -0,0 +1,116 @@ +package org.prebid.server.auction; + +import com.iab.openrtb.response.Bid; +import org.junit.Test; +import org.prebid.server.auction.model.BidderResponse; +import org.prebid.server.bidder.model.BidderBid; +import org.prebid.server.bidder.model.BidderSeatBid; +import org.prebid.server.proto.openrtb.ext.response.BidType; + +import java.math.BigDecimal; +import java.util.Arrays; + +import static org.assertj.core.api.Java6Assertions.assertThat; + +public class BidResponseReducerTest { + + private final BidResponseReducer bidResponseReducer = new BidResponseReducer(); + + @Test + public void removeRedundantBidsShouldReduceNonDealBidsByPriceDroppingNonDealsBids() { + // given + final BidderResponse bidderResponse = BidderResponse.of( + "bidder1", + givenSeatBid( + givenBidderBid("bidId1", "impId1", "dealId1", 5.0f), // deal + givenBidderBid("bidId2", "impId1", "dealId2", 6.0f), // deal + givenBidderBid("bidId3", "impId1", null, 7.0f) // non deal + ), + 0); + + // when + final BidderResponse resultBidderResponse = bidResponseReducer.removeRedundantBids(bidderResponse); + + // then + assertThat(resultBidderResponse.getSeatBid().getBids()) + .extracting(BidderBid::getBid) + .extracting(Bid::getId) + .containsOnly("bidId2"); + } + + @Test + public void removeRedundantBidsShouldReduceNonDealBidsByPrice() { + // given + final BidderResponse bidderResponse = BidderResponse.of( + "bidder1", + givenSeatBid( + givenBidderBid("bidId1", "impId1", null, 5.0f), // non deal + givenBidderBid("bidId2", "impId1", null, 6.0f), // non deal + givenBidderBid("bidId3", "impId1", null, 7.0f) // non deal + ), + 0); + + // when + final BidderResponse resultBidderResponse = bidResponseReducer.removeRedundantBids(bidderResponse); + + // then + assertThat(resultBidderResponse.getSeatBid().getBids()) + .extracting(BidderBid::getBid) + .extracting(Bid::getId) + .containsOnly("bidId3"); + } + + @Test + public void removeRedundantBidsShouldNotReduceBids() { + // given + final BidderResponse bidderResponse = BidderResponse.of( + "bidder1", + givenSeatBid(givenBidderBid("bidId1", "impId1", null, 5.0f)), + 0); + + // when + final BidderResponse resultBidderResponse = bidResponseReducer.removeRedundantBids(bidderResponse); + + // then + assertThat(resultBidderResponse.getSeatBid().getBids()) + .extracting(BidderBid::getBid) + .extracting(Bid::getId) + .containsOnly("bidId1"); + } + + @Test + public void removeRedundantBidsShouldReduceAllTypesOfBidsForMultipleImps() { + // given + final BidderResponse bidderResponse = BidderResponse.of( + "bidder1", + givenSeatBid( + givenBidderBid("bidId3-1", "impId1", "dealId3", 5.0f), // deal + givenBidderBid("bidId4-1", "impId1", null, 5.0f), // non deal + givenBidderBid("bidId1-2", "impId2", "dealId4", 5.0f), // deal + givenBidderBid("bidId2-2", "impId2", "dealId5", 6.0f), // deal + givenBidderBid("bidId3-2", "impId2", null, 5.0f), // non deal + givenBidderBid("bidId1-3", "impId3", null, 5.0f), // non deal + givenBidderBid("bidId2-3", "impId3", null, 6.0f) // non deal + ), + 0); + + // when + final BidderResponse resultBidderResponse = bidResponseReducer.removeRedundantBids(bidderResponse); + + // then + assertThat(resultBidderResponse.getSeatBid().getBids()) + .extracting(BidderBid::getBid) + .extracting(Bid::getId) + .containsOnly("bidId3-1", "bidId2-2", "bidId2-3"); + } + + private static BidderBid givenBidderBid(String bidId, String impId, String dealId, float price) { + return BidderBid.of( + Bid.builder().id(bidId).impid(impId).dealid(dealId).price(BigDecimal.valueOf(price)).build(), + BidType.banner, "USD"); + } + + private static BidderSeatBid givenSeatBid(BidderBid... bidderBids) { + return BidderSeatBid.of(Arrays.asList(bidderBids), null, null); + } +}