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

Make generation of Bid Adm as a first step to remove duplicating call #1214

Merged
merged 4 commits into from
Apr 9, 2021
Merged

Make generation of Bid Adm as a first step to remove duplicating call #1214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
86220f6
Make generation of Bid Adm as a first step to remove duplicating call
DGarbar Apr 6, 2021
4ff977a
Fix test
DGarbar Apr 6, 2021
beebfb6
Make variable final
DGarbar Apr 7, 2021
26046d2
Merge branch 'master' into bid-adm-as-first-step
Apr 9, 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
96 changes: 66 additions & 30 deletions src/main/java/org/prebid/server/auction/BidResponseCreator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.prebid.server.auction.model.MultiBidConfig;
import org.prebid.server.auction.model.TargetingBidInfo;
import org.prebid.server.bidder.BidderCatalog;
import org.prebid.server.bidder.model.BidderBid;
import org.prebid.server.bidder.model.BidderError;
import org.prebid.server.bidder.model.BidderSeatBid;
import org.prebid.server.cache.CacheService;
Expand Down Expand Up @@ -154,6 +155,17 @@ Future<BidResponse> create(List<BidderResponse> bidderResponses,
boolean debugEnabled) {

final long auctionTimestamp = auctionTimestamp(auctionContext);
final Account account = auctionContext.getAccount();

final EventsContext eventsContext = EventsContext.builder()
.enabledForAccount(eventsEnabledForAccount(auctionContext))
.enabledForRequest(eventsEnabledForRequest(auctionContext))
.auctionTimestamp(auctionTimestamp)
.integration(integrationFrom(auctionContext))
.build();

final Map<String, String> bidIdToGeneratedBidId = new HashMap<>();
updateBidAdmInBidderResponses(bidderResponses, account, bidIdToGeneratedBidId, eventsContext);
rpanchyk marked this conversation as resolved.
Show resolved Hide resolved

if (isEmptyBidderResponses(bidderResponses)) {
final BidRequest bidRequest = auctionContext.getBidRequest();
Expand All @@ -178,10 +190,41 @@ Future<BidResponse> create(List<BidderResponse> bidderResponses,
auctionContext,
cacheInfo,
bidderToMultiBids,
auctionTimestamp,
bidIdToGeneratedBidId,
eventsContext,
debugEnabled);
}

private void updateBidAdmInBidderResponses(List<BidderResponse> bidderResponses,
Account account,
Map<String, String> bidIdToGeneratedBidId,
EventsContext eventsContext) {
for (BidderResponse bidderResponse : bidderResponses) {
final String bidder = bidderResponse.getBidder();

for (BidderBid bidderBid : bidderResponse.getSeatBid().getBids()) {
final Bid bid = bidderBid.getBid();
final String generatedBidId = bidIdGenerator.getType() != IdGeneratorType.none
? bidIdGenerator.generateId()
: null;
final String bidId = bid.getId();
bidIdToGeneratedBidId.put(bidId, generatedBidId);

if (bidderBid.getType().equals(BidType.video)) {
final String adm = vastModifier.createBidVastXml(
bidder,
bid.getAdm(),
bid.getNurl(),
generatedBidId == null ? bidId : generatedBidId,
account.getId(),
eventsContext);

bid.setAdm(adm);
}
}
}
}

private static int validateTruncateAttrChars(int truncateAttrChars) {
if (truncateAttrChars < 0 || truncateAttrChars > 255) {
throw new IllegalArgumentException("truncateAttrChars must be between 0 and 255");
Expand All @@ -202,13 +245,14 @@ private Future<BidResponse> cacheBidsAndCreateResponse(List<BidderResponse> bidd
AuctionContext auctionContext,
BidRequestCacheInfo cacheInfo,
Map<String, MultiBidConfig> bidderToMultiBids,
long auctionTimestamp,
Map<String, String> bidIdToGeneratedBidId,
EventsContext eventsContext,
boolean debugEnabled) {
final BidRequest bidRequest = auctionContext.getBidRequest();

final List<Imp> imps = bidRequest.getImp();
final Map<BidderResponse, List<TargetingBidInfo>> bidderResponseToTargetingBidInfos =
toBidderResponseWithTargetingBidInfos(bidderResponses, imps, bidderToMultiBids);
toBidderResponseWithTargetingBidInfos(bidderResponses, imps, bidderToMultiBids, bidIdToGeneratedBidId);

final Set<BidInfo> bidInfos = bidderResponseToTargetingBidInfos.values().stream()
.filter(CollectionUtils::isNotEmpty)
Expand All @@ -228,13 +272,6 @@ private Future<BidResponse> cacheBidsAndCreateResponse(List<BidderResponse> bidd

final Set<BidInfo> bidsToCache = cacheInfo.isShouldCacheWinningBidsOnly() ? winningBidInfos : bidInfos;

final EventsContext eventsContext = EventsContext.builder()
.enabledForAccount(eventsEnabledForAccount(auctionContext))
.enabledForRequest(eventsEnabledForRequest(auctionContext))
.auctionTimestamp(auctionTimestamp)
.integration(integrationFrom(auctionContext))
.build();

return cacheBids(bidsToCache, auctionContext, cacheInfo, eventsContext)
.compose(cacheResult -> videoStoredDataResult(auctionContext)
.map(videoStoredDataResult -> toBidResponse(
Expand All @@ -245,7 +282,6 @@ private Future<BidResponse> cacheBidsAndCreateResponse(List<BidderResponse> bidd
cacheResult,
videoStoredDataResult,
eventsContext,
auctionTimestamp,
debugEnabled)));
}

Expand All @@ -258,12 +294,14 @@ private static ExtRequestTargeting targeting(BidRequest bidRequest) {
private Map<BidderResponse, List<TargetingBidInfo>> toBidderResponseWithTargetingBidInfos(
List<BidderResponse> bidderResponses,
List<Imp> imps,
Map<String, MultiBidConfig> bidderToMultiBids) {
Map<String, MultiBidConfig> bidderToMultiBids,
Map<String, String> bidIdToGeneratedBidId) {

final Map<BidderResponse, List<BidInfo>> bidderResponseToReducedBidInfos = bidderResponses.stream()
.collect(Collectors.toMap(
Function.identity(),
bidderResponse -> toSortedMultiBidInfo(bidderResponse, imps, bidderToMultiBids)));
bidderResponse -> toSortedMultiBidInfo(bidderResponse, imps, bidderToMultiBids,
bidIdToGeneratedBidId)));

final Map<String, Map<String, List<BidInfo>>> impIdToBidderToBidInfos = bidderResponseToReducedBidInfos.values()
.stream()
Expand Down Expand Up @@ -300,8 +338,9 @@ private Map<BidderResponse, List<TargetingBidInfo>> toBidderResponseWithTargetin

private List<BidInfo> toSortedMultiBidInfo(BidderResponse bidderResponse,
List<Imp> imps,
Map<String, MultiBidConfig> bidderToMultiBids) {
final List<BidInfo> bidInfos = toBidInfo(bidderResponse, imps);
Map<String, MultiBidConfig> bidderToMultiBids,
Map<String, String> bidIdToGeneratedBidId) {
final List<BidInfo> bidInfos = toBidInfo(bidderResponse, imps, bidIdToGeneratedBidId);
final Map<String, List<BidInfo>> impIdToBidInfos = bidInfos.stream()
.collect(Collectors.groupingBy(bidInfo -> bidInfo.getCorrespondingImp().getId()));

Expand All @@ -314,20 +353,27 @@ private List<BidInfo> toSortedMultiBidInfo(BidderResponse bidderResponse,
.collect(Collectors.toList());
}

private List<BidInfo> toBidInfo(BidderResponse bidderResponse, List<Imp> imps) {
private List<BidInfo> toBidInfo(BidderResponse bidderResponse,
List<Imp> imps,
Map<String, String> bidIdToGeneratedBidId) {
return Stream.of(bidderResponse)
.map(BidderResponse::getSeatBid)
.filter(Objects::nonNull)
.map(BidderSeatBid::getBids)
.filter(Objects::nonNull)
.flatMap(Collection::stream)
.map(bidderBid -> toBidInfo(bidderBid.getBid(), bidderBid.getType(), imps, bidderResponse.getBidder()))
.map(bidderBid -> toBidInfo(bidderBid.getBid(), bidderBid.getType(), imps, bidderResponse.getBidder(),
bidIdToGeneratedBidId))
.collect(Collectors.toList());
}

private BidInfo toBidInfo(Bid bid, BidType type, List<Imp> imps, String bidder) {
private BidInfo toBidInfo(Bid bid,
BidType type,
List<Imp> imps,
String bidder,
Map<String, String> bidIdToGeneratedBidId) {
return BidInfo.builder()
.generatedBidId(bidIdGenerator.getType() != IdGeneratorType.none ? bidIdGenerator.generateId() : null)
.generatedBidId(bidIdToGeneratedBidId.get(bid.getId()))
.bid(bid)
.bidType(type)
.bidder(bidder)
Expand Down Expand Up @@ -693,7 +739,6 @@ private BidResponse toBidResponse(Map<BidderResponse, List<TargetingBidInfo>> bi
CacheServiceResult cacheResult,
VideoStoredDataResult videoStoredDataResult,
EventsContext eventsContext,
long auctionTimestamp,
boolean debugEnabled) {

final BidRequest bidRequest = auctionContext.getBidRequest();
Expand All @@ -714,6 +759,7 @@ private BidResponse toBidResponse(Map<BidderResponse, List<TargetingBidInfo>> bi
eventsContext))
.collect(Collectors.toList());

final Long auctionTimestamp = eventsContext.getAuctionTimestamp();
final ExtBidResponse extBidResponse = toExtBidResponse(
bidderResponseToTargetingBidInfos.keySet(),
auctionContext,
Expand Down Expand Up @@ -839,16 +885,6 @@ private Bid toBid(TargetingBidInfo targetingBidInfo,
if ((videoCacheId != null && !requestCacheInfo.isReturnCreativeVideoBids())
|| (cacheId != null && !requestCacheInfo.isReturnCreativeBids())) {
bid.setAdm(null);
} else if (bidType.equals(BidType.video)) {
final String adm = vastModifier.createBidVastXml(
bidder,
bid.getAdm(),
bid.getNurl(),
bidInfo.getBidId(),
account.getId(),
eventsContext);

bid.setAdm(adm);
}

final boolean isApp = bidRequest.getApp() != null;
Expand Down
14 changes: 3 additions & 11 deletions src/main/java/org/prebid/server/cache/CacheService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ private Future<CacheServiceResult> doCacheOpenrtb(List<CacheBid> bids,
final String accountId = account.getId();
final List<CachedCreative> cachedCreatives = Stream.concat(
bids.stream().map(cacheBid -> createJsonPutObjectOpenrtb(cacheBid, accountId, eventsContext)),
videoBids.stream().map(cacheBid -> createXmlPutObjectOpenrtb(cacheBid, accountId, eventsContext)))
videoBids.stream().map(this::createXmlPutObjectOpenrtb))
.collect(Collectors.toList());

if (cachedCreatives.isEmpty()) {
Expand Down Expand Up @@ -403,18 +403,10 @@ private CachedCreative createJsonPutObjectOpenrtb(CacheBid cacheBid,
/**
* Makes XML type {@link PutObject} from {@link com.iab.openrtb.response.Bid}. Used for OpenRTB auction request.
*/
private CachedCreative createXmlPutObjectOpenrtb(CacheBid cacheBid,
String accountId,
EventsContext eventsContext) {
private CachedCreative createXmlPutObjectOpenrtb(CacheBid cacheBid) {
final BidInfo bidInfo = cacheBid.getBidInfo();
final com.iab.openrtb.response.Bid bid = bidInfo.getBid();
final String vastXml = vastModifier.createBidVastXml(
bidInfo.getBidder(),
bid.getAdm(),
bid.getNurl(),
bidInfo.getBidId(),
accountId,
eventsContext);
final String vastXml = bid.getAdm();

final PutObject payload = PutObject.builder()
.type("xml")
Expand Down
116 changes: 66 additions & 50 deletions src/test/java/org/prebid/server/auction/BidResponseCreatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.iab.openrtb.response.Response;
import com.iab.openrtb.response.SeatBid;
import io.vertx.core.Future;
import org.apache.commons.collections4.CollectionUtils;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -315,10 +314,12 @@ public void shouldRequestCacheServiceWithWinningBidsOnlyWhenWinningonlyIsTrue()
eq(EventsContext.builder().auctionTimestamp(1000L).build()));
}

@SuppressWarnings("unchecked")
@Test
public void shouldRequestCacheServiceWithVideoBidsToModify() {
// given
final Account account = Account.builder().id("accountId").eventsEnabled(true).build();
final String accountId = "accountId";
final Account account = Account.builder().id(accountId).eventsEnabled(true).build();

final Imp imp1 = givenImp("impId1");
final Imp imp2 = givenImp("impId2");
Expand All @@ -329,35 +330,88 @@ public void shouldRequestCacheServiceWithVideoBidsToModify() {
imp1, imp2),
contextBuilder -> contextBuilder.account(account));

final Bid bid1 = Bid.builder().id("bidId1").impid("impId1").price(BigDecimal.valueOf(5.67)).build();
final String bidId1 = "bidId1";
final Bid bid1 = Bid.builder().id(bidId1).impid("impId1").price(BigDecimal.valueOf(5.67)).nurl(BID_NURL)
.build();
final Bid bid2 = Bid.builder().id("bidId2").impid("impId2").price(BigDecimal.valueOf(7.19)).build();

final String bidder1 = "bidder1";
final List<BidderResponse> bidderResponses = asList(
BidderResponse.of("bidder1", givenSeatBid(BidderBid.of(bid1, video, "USD")), 100),
BidderResponse.of(bidder1, givenSeatBid(BidderBid.of(bid1, video, "USD")), 100),
BidderResponse.of("bidder2", givenSeatBid(BidderBid.of(bid2, banner, "USD")), 100));

final BidRequestCacheInfo cacheInfo = BidRequestCacheInfo.builder()
.doCaching(true)
.shouldCacheVideoBids(true)
.build();

final String modifiedAdm = "modifiedAdm";
given(vastModifier.createBidVastXml(any(), any(), any(), any(), any(), any())).willReturn(modifiedAdm);

// just a stub to get through method call chain
givenCacheServiceResult(singletonMap(bid1, CacheInfo.empty()));

// when
bidResponseCreator.create(bidderResponses, auctionContext, cacheInfo, MULTI_BIDS, false);

// then
final BidInfo bidInfo1 = toBidInfo(bid1, imp1, "bidder1", video);
final BidInfo bidInfo2 = toBidInfo(bid2, imp2, "bidder2", banner);
final EventsContext expectedEventContext = EventsContext.builder()
.enabledForAccount(true)
.enabledForRequest(true)
.auctionTimestamp(1000L)
.build();

verify(vastModifier).createBidVastXml(bidder1, null, BID_NURL, bidId1, accountId, expectedEventContext);

final ArgumentCaptor<List<BidInfo>> bidInfoCaptor = ArgumentCaptor.forClass(List.class);
verify(cacheService).cacheBidsOpenrtb(
argThat(argument -> CollectionUtils.isEqualCollection(argument, asList(bidInfo1, bidInfo2))),
bidInfoCaptor.capture(),
same(auctionContext),
eq(CacheContext.builder().shouldCacheVideoBids(true).build()),
eq(EventsContext.builder()
.enabledForAccount(true)
.enabledForRequest(true)
.auctionTimestamp(1000L)
.build()));
eq(expectedEventContext));

final Bid expectedUpdatedBid1 = bid1.toBuilder().adm(modifiedAdm).build();
final BidInfo bidInfo1 = toBidInfo(expectedUpdatedBid1, imp1, bidder1, video);
final BidInfo bidInfo2 = toBidInfo(bid2, imp2, "bidder2", banner);
assertThat(bidInfoCaptor.getValue()).containsOnly(bidInfo1, bidInfo2);
}

@Test
public void shouldModifyBidAdmWhenBidVideoAndVastModifierReturnValue() {
// given
final AuctionContext auctionContext = givenAuctionContext(givenBidRequest(
identity(),
extBuilder -> extBuilder.targeting(givenTargeting()),
givenImp()));

final String bidId = "bid_id";
final Bid bid = Bid.builder()
.id(bidId)
.price(BigDecimal.ONE)
.adm(BID_ADM)
.nurl(BID_NURL)
.impid(IMP_ID)
.build();

final String bidder = "bidder1";
final List<BidderResponse> bidderResponses = singletonList(
BidderResponse.of(bidder, givenSeatBid(BidderBid.of(bid, video, "USD")), 100));

final String modifiedVast = "modifiedVast";
given(vastModifier.createBidVastXml(anyString(), anyString(), anyString(), anyString(), anyString(), any()))
.willReturn(modifiedVast);

// when
final BidResponse bidResponse =
bidResponseCreator.create(bidderResponses, auctionContext, CACHE_INFO, MULTI_BIDS, false).result();

// then
assertThat(bidResponse.getSeatbid())
.flatExtracting(SeatBid::getBid).hasSize(1)
.extracting(Bid::getAdm)
.containsOnly(modifiedVast);

verify(vastModifier).createBidVastXml(eq(bidder), eq(BID_ADM), eq(BID_NURL), eq(bidId), eq("accountId"), any());
}

@Test
Expand Down Expand Up @@ -865,44 +919,6 @@ public void shouldSetBidAdmToNullIfVideoCacheIdIsPresentAndReturnCreativeVideoBi
verify(cacheService).cacheBidsOpenrtb(anyList(), any(), any(), any());
}

@Test
public void shouldModifyBidAdmWhenBidVideoAndVastModifierReturnValue() {
// given
final AuctionContext auctionContext = givenAuctionContext(givenBidRequest(
identity(),
extBuilder -> extBuilder.targeting(givenTargeting()),
givenImp()));

final String bidId = "bid_id";
final Bid bid = Bid.builder()
.id(bidId)
.price(BigDecimal.ONE)
.adm(BID_ADM)
.nurl(BID_NURL)
.impid(IMP_ID)
.build();

final String bidder = "bidder1";
final List<BidderResponse> bidderResponses = singletonList(
BidderResponse.of(bidder, givenSeatBid(BidderBid.of(bid, video, "USD")), 100));

final String modifiedVast = "modifiedVast";
given(vastModifier.createBidVastXml(anyString(), anyString(), anyString(), anyString(), anyString(), any()))
.willReturn(modifiedVast);

// when
final BidResponse bidResponse =
bidResponseCreator.create(bidderResponses, auctionContext, CACHE_INFO, MULTI_BIDS, false).result();

// then
assertThat(bidResponse.getSeatbid())
.flatExtracting(SeatBid::getBid).hasSize(1)
.extracting(Bid::getAdm)
.containsOnly(modifiedVast);

verify(vastModifier).createBidVastXml(eq(bidder), eq(BID_ADM), eq(BID_NURL), eq(bidId), eq("accountId"), any());
}

@Test
public void shouldSetBidExpWhenCacheIdIsMatched() {
// given
Expand Down
Loading