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 bid immutable #1249

Merged
merged 3 commits into from
Apr 29, 2021
+236 −130
Merged

Make bid immutable #1249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e773d2
Make bid immutable
DGarbar Apr 26, 2021
ba3a16c
Fix tests
DGarbar Apr 26, 2021
6629168
Merge branch 'master' into make-bid-immutable
Apr 29, 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
60 changes: 37 additions & 23 deletions src/main/java/com/iab/openrtb/response/Bid.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package com.iab.openrtb.response;

import com.fasterxml.jackson.databind.node.ObjectNode;
import lombok.Data;
import lombok.NoArgsConstructor;
import lombok.experimental.SuperBuilder;
import lombok.Builder;
import lombok.Value;

import java.math.BigDecimal;
import java.util.List;
Expand All @@ -13,22 +12,19 @@
* relates to a specific impression in the bid request via the {@code impid}
* attribute and constitutes an offer to buy that impression for a given
* {@code price}.
* <p>
* IMPORTANT: unlike other data classes this one is mutable (annotated with {@link Data} instead of
* {@link lombok.Value}). Motivation: during the course of processing bids could be altered several times (price
* adjustment, post-processing). Creating new instance of the bid in each of these cases seems to cause unnecessary
* memory pressure. In order to avoid unnecessary allocations this class is made mutable (as an exception) i.e. this
* decision could be seen as a performance optimisation.
*/
@SuperBuilder(toBuilder = true)
@NoArgsConstructor
@Data
@Builder(toBuilder = true)
@Value
public class Bid {

/** Bidder generated bid ID to assist with logging/tracking. (required) */
/**
* Bidder generated bid ID to assist with logging/tracking. (required)
*/
String id;

/** ID of the Imp object in the related bid request. (required) */
/**
* ID of the Imp object in the related bid request. (required)
*/
String impid;

/**
Expand Down Expand Up @@ -72,7 +68,9 @@ public class Bid {
*/
String adm;

/** ID of a preloaded ad to be served if the bid wins. */
/**
* ID of a preloaded ad to be served if the bid wins.
*/
String adid;

/**
Expand Down Expand Up @@ -110,19 +108,29 @@ public class Bid {
*/
String crid;

/** IAB content categories of the creative. Refer to List 5.1. */
/**
* IAB content categories of the creative. Refer to List 5.1.
*/
List<String> cat;

/** Set of attributes describing the creative. Refer to List 5.3. */
/**
* Set of attributes describing the creative. Refer to List 5.3.
*/
List<Integer> attr;

/** API required by the markup if applicable. Refer to List 5.6. */
/**
* API required by the markup if applicable. Refer to List 5.6.
*/
Integer api;

/** Video response protocol of the markup if applicable. Refer to List 5.8. */
/**
* Video response protocol of the markup if applicable. Refer to List 5.8.
*/
Integer protocol;

/** Creative media rating per IQG guidelines. Refer to List 5.19. */
/**
* Creative media rating per IQG guidelines. Refer to List 5.19.
*/
Integer qagmediarating;

/**
Expand All @@ -138,10 +146,14 @@ public class Bid {
*/
String dealid;

/** Width of the creative in device independent pixels (DIPS). */
/**
* Width of the creative in device independent pixels (DIPS).
*/
Integer w;

/** Height of the creative in device independent pixels (DIPS). */
/**
* Height of the creative in device independent pixels (DIPS).
*/
Integer h;

/**
Expand All @@ -162,6 +174,8 @@ public class Bid {
*/
Integer exp;

/** Placeholder for bidder-specific extensions to OpenRTB. */
/**
* Placeholder for bidder-specific extensions to OpenRTB.
*/
ObjectNode ext;
}
140 changes: 91 additions & 49 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 @@ -167,17 +167,18 @@ Future<BidResponse> create(List<BidderResponse> bidderResponses,
.build();

final Map<String, String> bidIdToGeneratedBidId = new HashMap<>();
updateBidAdmInBidderResponses(bidderResponses, account, bidIdToGeneratedBidId, eventsContext);
final List<BidderResponse> modifiedBidderResponses = updateBidAdmInBidderResponses(bidderResponses, account,
bidIdToGeneratedBidId, eventsContext);

if (isEmptyBidderResponses(bidderResponses)) {
if (isEmptyBidderResponses(modifiedBidderResponses)) {
final BidRequest bidRequest = auctionContext.getBidRequest();
return Future.succeededFuture(BidResponse.builder()
.id(bidRequest.getId())
.cur(bidRequest.getCur().get(0))
.nbr(0) // signal "Unknown Error"
.seatbid(Collections.emptyList())
.ext(mapper.mapper().valueToTree(toExtBidResponse(
bidderResponses,
modifiedBidderResponses,
auctionContext,
CacheServiceResult.empty(),
VideoStoredDataResult.empty(),
Expand All @@ -188,7 +189,7 @@ Future<BidResponse> create(List<BidderResponse> bidderResponses,
}

return cacheBidsAndCreateResponse(
bidderResponses,
modifiedBidderResponses,
auctionContext,
cacheInfo,
bidderToMultiBids,
Expand All @@ -197,15 +198,18 @@ Future<BidResponse> create(List<BidderResponse> bidderResponses,
debugEnabled);
}

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

for (BidderBid bidderBid : bidderResponse.getSeatBid().getBids()) {
final Bid bid = bidderBid.getBid();
final List<BidderBid> modifiedBidderBid = new ArrayList<>();
final BidderSeatBid seatBid = bidderResponse.getSeatBid();
for (BidderBid bidderBid : seatBid.getBids()) {
Bid bid = bidderBid.getBid();
final String generatedBidId = bidIdGenerator.getType() != IdGeneratorType.none
? bidIdGenerator.generateId()
: null;
Expand All @@ -221,10 +225,17 @@ private void updateBidAdmInBidderResponses(List<BidderResponse> bidderResponses,
account.getId(),
eventsContext);

bid.setAdm(adm);
bid = bid.toBuilder().adm(adm).build();
}

modifiedBidderBid.add(bidderBid.with(bid));
}

final BidderSeatBid modifiedSeatBid = seatBid.with(modifiedBidderBid);
result.add(bidderResponse.with(modifiedSeatBid));
}

return result;
}

private static int validateTruncateAttrChars(int truncateAttrChars) {
Expand Down Expand Up @@ -267,10 +278,10 @@ private Future<BidResponse> cacheBidsAndCreateResponse(List<BidderResponse> bidd
final Set<BidInfo> winningBidInfos = targeting == null
? null
: bidderResponseToTargetingBidInfos.values().stream()
.flatMap(Collection::stream)
.filter(TargetingBidInfo::isWinningBid)
.map(TargetingBidInfo::getBidInfo)
.collect(Collectors.toSet());
.flatMap(Collection::stream)
.filter(TargetingBidInfo::isWinningBid)
.map(TargetingBidInfo::getBidInfo)
.collect(Collectors.toSet());

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

Expand Down Expand Up @@ -847,16 +858,21 @@ private SeatBid toSeatBid(List<TargetingBidInfo> targetingBidInfos,
.orElseThrow(() -> new IllegalArgumentException("Bidder was not defined for bidInfo"));

final List<Bid> bids = targetingBidInfos.stream()
.map(targetingBidInfo -> injectAdmWithCacheInfo(
targetingBidInfo,
requestCacheInfo,
bidToCacheInfo,
bidRequest,
bidErrors
))
.filter(Objects::nonNull)
.map(targetingBidInfo -> toBid(
targetingBidInfo,
targeting,
bidRequest,
requestCacheInfo,
bidToCacheInfo,
videoStoredDataResult.getImpIdToStoredVideo(),
account,
eventsContext,
bidErrors))
eventsContext))
.filter(Objects::nonNull)
.collect(Collectors.toList());

Expand All @@ -867,52 +883,76 @@ private SeatBid toSeatBid(List<TargetingBidInfo> targetingBidInfos,
.build();
}

/**
* Returns an OpenRTB {@link Bid} with "prebid" and "bidder" extension fields populated.
*/
private Bid toBid(TargetingBidInfo targetingBidInfo,
ExtRequestTargeting targeting,
BidRequest bidRequest,
BidRequestCacheInfo requestCacheInfo,
Map<Bid, CacheInfo> bidsWithCacheIds,
Map<String, Video> impIdToStoredVideo,
Account account,
EventsContext eventsContext,
Map<String, List<ExtBidderError>> bidErrors) {
private TargetingBidInfo injectAdmWithCacheInfo(TargetingBidInfo targetingBidInfo,
BidRequestCacheInfo requestCacheInfo,
Map<Bid, CacheInfo> bidsWithCacheIds,
BidRequest bidRequest,
Map<String, List<ExtBidderError>> bidErrors) {
final BidInfo bidInfo = targetingBidInfo.getBidInfo();
final Bid bid = bidInfo.getBid();
final BidType bidType = bidInfo.getBidType();
final String bidder = bidInfo.getBidder();
final Imp correspondingImp = bidInfo.getCorrespondingImp();

final CacheInfo cacheInfo = bidsWithCacheIds.get(bid);
final boolean isApp = bidRequest.getApp() != null;

final String cacheId = cacheInfo != null ? cacheInfo.getCacheId() : null;
final String videoCacheId = cacheInfo != null ? cacheInfo.getVideoCacheId() : null;

String modifiedBidAdm = bid.getAdm();
if ((videoCacheId != null && !requestCacheInfo.isReturnCreativeVideoBids())
|| (cacheId != null && !requestCacheInfo.isReturnCreativeBids())) {
bid.setAdm(null);
modifiedBidAdm = null;
}

final boolean isApp = bidRequest.getApp() != null;
if (isApp && bidType.equals(BidType.xNative) && bid.getAdm() != null) {
if (isApp && bidType.equals(BidType.xNative) && modifiedBidAdm != null) {
try {
addNativeMarkup(bid, bidRequest.getImp());
modifiedBidAdm = crateNativeMarkup(modifiedBidAdm, correspondingImp);
} catch (PreBidException e) {
bidErrors.computeIfAbsent(bidder, ignored -> new ArrayList<>())
.add(ExtBidderError.of(BidderError.Type.bad_server_response.getCode(), e.getMessage()));
return null;
}
}

final Bid modifiedBid = bid.toBuilder().adm(modifiedBidAdm).build();
final BidInfo modifiedBidInfo = bidInfo.toBuilder()
.bid(modifiedBid)
.cacheInfo(cacheInfo)
.build();
return targetingBidInfo.toBuilder().bidInfo(modifiedBidInfo).build();
}

/**
* Returns an OpenRTB {@link Bid} with "prebid" and "bidder" extension fields populated.
*/
private Bid toBid(TargetingBidInfo targetingBidInfo,
ExtRequestTargeting targeting,
BidRequest bidRequest,
Map<String, Video> impIdToStoredVideo,
Account account,
EventsContext eventsContext) {
final BidInfo bidInfo = targetingBidInfo.getBidInfo();
final BidType bidType = bidInfo.getBidType();
final String bidder = bidInfo.getBidder();
final Bid bid = bidInfo.getBid();

final CacheInfo cacheInfo = bidInfo.getCacheInfo();
final String cacheId = cacheInfo != null ? cacheInfo.getCacheId() : null;
final String videoCacheId = cacheInfo != null ? cacheInfo.getVideoCacheId() : null;

final boolean isApp = bidRequest.getApp() != null;

final Map<String, String> targetingKeywords;
final String bidderCode = targetingBidInfo.getBidderCode();
if (targeting != null && targetingBidInfo.isTargetingEnabled() && targetingBidInfo.isBidderWinningBid()) {
final TargetingKeywordsCreator keywordsCreator = resolveKeywordsCreator(bidType, targeting, isApp,
bidRequest, account);

final boolean isWinningBid = targetingBidInfo.isWinningBid();
targetingKeywords = keywordsCreator.makeFor(bid, bidderCode, isWinningBid, cacheId, bidType.getName(),
videoCacheId);
targetingKeywords = keywordsCreator.makeFor(bid, bidderCode, isWinningBid, cacheId,
bidType.getName(), videoCacheId);
} else {
targetingKeywords = null;
}
Expand All @@ -936,29 +976,29 @@ private Bid toBid(TargetingBidInfo targetingBidInfo,
.video(extBidPrebidVideo)
.build();

bid.setExt(createBidExt(bid.getExt(), extBidPrebid));

final ObjectNode bidExt = createBidExt(bid.getExt(), extBidPrebid);
final Integer ttl = cacheInfo != null ? ObjectUtils.max(cacheInfo.getTtl(), cacheInfo.getVideoTtl()) : null;
bid.setExp(ttl);

return bid;
return bid.toBuilder()
.ext(bidExt)
.exp(ttl)
.build();
}

private void addNativeMarkup(Bid bid, List<Imp> imps) {
private String crateNativeMarkup(String bidAdm, Imp correspondingImp) {
final Response nativeMarkup;
try {
nativeMarkup = mapper.decodeValue(bid.getAdm(), Response.class);
nativeMarkup = mapper.decodeValue(bidAdm, Response.class);
} catch (DecodeException e) {
throw new PreBidException(e.getMessage());
}

final List<Asset> responseAssets = nativeMarkup.getAssets();
if (CollectionUtils.isNotEmpty(responseAssets)) {
final Native nativeImp = imps.stream()
.filter(imp -> imp.getId().equals(bid.getImpid()) && imp.getXNative() != null)
.findFirst()
.map(Imp::getXNative)
.orElseThrow(() -> new PreBidException("Could not find native imp"));
final Native nativeImp = correspondingImp != null ? correspondingImp.getXNative() : null;
if (nativeImp == null) {
throw new PreBidException("Could not find native imp");
}

final Request nativeRequest;
try {
Expand All @@ -968,8 +1008,10 @@ private void addNativeMarkup(Bid bid, List<Imp> imps) {
}

responseAssets.forEach(asset -> setAssetTypes(asset, nativeRequest.getAssets()));
bid.setAdm(mapper.encode(nativeMarkup));
return mapper.encode(nativeMarkup);
}

return bidAdm;
}

private static void setAssetTypes(Asset responseAsset, List<com.iab.openrtb.request.Asset> requestAssets) {
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/org/prebid/server/auction/ExchangeService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1096,15 +1096,16 @@ private BidderBid updateBidderBidWithBidPriceChanges(BidderBid bidderBid,

updateExtWithOrigPriceValues(updatedBidExt, price, bidCurrency);

final Bid.BidBuilder bidBuilder = bid.toBuilder();
if (adjustedPrice.compareTo(price) != 0) {
bid.setPrice(adjustedPrice);
bidBuilder.price(adjustedPrice);
}

if (!updatedBidExt.isEmpty()) {
bid.setExt(updatedBidExt);
bidBuilder.ext(updatedBidExt);
}

return bidderBid;
return bidderBid.with(bidBuilder.build());
}

private static BidAdjustmentMediaType resolveBidAdjustmentMediaType(String bidImpId,
Expand Down
Loading