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.

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

Support deprecated bidders #169

Merged
merged 14 commits into from
Oct 30, 2018
1 change: 1 addition & 0 deletions docs/config-app.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ There are several typical keys:
- `adapters.<BIDDER_NAME>.endpoint` - the url for submitting bids.
- `adapters.<BIDDER_NAME>.usersync-url` - the url for synchronizing UIDs cookie.
- `adapters.<BIDDER_NAME>.pbs-enforces-gdpr` - indicates if pbs server provides gdpr support for bidder or bidder will handle it itself.
- `adapters.<BIDDER_NAME>.deprecated-names` - comma separated deprecated names of bidder.

But feel free to add additional bidder's specific options.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.prebid.server.proto.openrtb.ext.request.ExtRequestTargeting;
import org.prebid.server.validation.RequestValidator;
import org.prebid.server.validation.model.ValidationResult;
import org.springframework.beans.factory.annotation.Value;

import java.util.Collections;
import java.util.List;
Expand Down
72 changes: 48 additions & 24 deletions src/main/java/org/prebid/server/auction/ExchangeService.java
Original file line number Diff line number Diff line change
Expand Up @@ -156,23 +156,48 @@ public Future<BidResponse> holdAuction(BidRequest bidRequest, UidsCookie uidsCoo

final long startTime = clock.millis();

return extractBidderRequests(bidRequest, uidsCookie, aliases, timeout)
// sanity check: discard imps without extension
final List<Imp> imps = imps(bidRequest);

final Map<String, List<String>> deprecatedBiddersErrors = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename pls to errors because it can be global map for error through processing workflow, for ex. i plan to add cache service errors.


// identify valid biddersAndErrors and aliases out of imps
final List<String> bidders = biddersAndErrors(imps, deprecatedBiddersErrors, aliases);

return extractBidderRequests(bidRequest, imps, bidders, uidsCookie, aliases, timeout)
.map(bidderRequests ->
updateRequestMetric(bidderRequests, uidsCookie, aliases, publisherId, metricsContext))
.compose(bidderRequests -> CompositeFuture.join(bidderRequests.stream()
.map(bidderRequest -> requestBids(bidderRequest, startTime,
auctionTimeout(timeout, cacheInfo.doCaching), aliases, bidAdjustments(requestExt),
currencyRates(targeting)))
.collect(Collectors.toList())))
// send all the requests to the bidders and gathers results
// send all the requests to the biddersAndErrors and gathers results
Copy link
Contributor

Choose a reason for hiding this comment

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

Check replacing though all other code, was bidders not biddersAndErrors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checked before commit, missed this one(

.map(CompositeFuture::<BidderResponse>list)
// produce response from bidder results
.map(bidderResponses -> updateMetricsFromResponses(bidderResponses, publisherId))
.compose(result ->
toBidResponse(result, bidRequest, keywordsCreator, cacheInfo, timeout))
toBidResponse(result, bidRequest, keywordsCreator, cacheInfo, timeout, deprecatedBiddersErrors))
.compose(bidResponse -> bidResponsePostProcessor.postProcess(bidRequest, uidsCookie, bidResponse));
}

private List<Imp> imps(BidRequest bidRequest) {
return bidRequest.getImp().stream()
.filter(imp -> imp.getExt() != null)
.collect(Collectors.toList());
}

private List<String> biddersAndErrors(List<Imp> imps, Map<String, List<String>> deprecatedBiddersErrors,
Map<String, String> aliases) {
return imps.stream()
.flatMap(imp -> asStream(imp.getExt().fieldNames())
.filter(bidder -> !Objects.equals(bidder, PREBID_EXT))
.peek(s -> processDeprecatedBidder(s, deprecatedBiddersErrors))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this code will produce multiple message about the same deprecated bidder if it is present in two or more imps.

In general I would choose another approach to implement this. First of all holdAuction and extractBidderRequests method could be left unchanged at all and following line could be added right to toExtBidResponse:

final Map<...> deprecatedBiddersErrors = bidRequest.getImp().stream()
                .filter(imp -> imp.getExt() != null)
                .flatMap(imp -> asStream(imp.getExt.fieldNames())))
                .distinct()
                .filter(bidder -> bidderCatalog.isDeprecatedName(bidder))
                .collect(Collectors.toMap(Function.identity(), bidder -> Collections.singletonList(bidderCatalog.errorForDeprecatedName(bidder))));

As you can see we'll get the same errors map without pushing it through a bunch of methods. This leads to slight duplication but that's ok in this case I think because thereby we've gained cleaner and more focused methods signatures and call chains and that's better for maintenance. Of course if it makes sense common code could be extracted to carefully named methods according to your flavor.

.filter(bidder -> isValidBidder(bidder, aliases)))
.distinct()
.collect(Collectors.toList());
}

/**
* Extracts {@link ExtBidRequest} from bid request.
*/
Expand Down Expand Up @@ -209,6 +234,13 @@ private static String publisherId(BidRequest bidRequest) {
return publisher != null ? publisher.getId() : StringUtils.EMPTY;
}

private void processDeprecatedBidder(String bidder, Map<String, List<String>> deprecatedBiddersErrors) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The method doesn't used anymore.

if (bidderCatalog.isDeprecatedName(bidder)) {
deprecatedBiddersErrors.put(bidder,
Collections.singletonList(bidderCatalog.errorForDeprecatedName(bidder)));
}
}

/**
* Extracts bidAdjustments from {@link ExtBidRequest}.
*/
Expand Down Expand Up @@ -253,28 +285,15 @@ private static Map<String, Map<String, BigDecimal>> currencyRates(ExtRequestTarg
* NOTE: the return list will only contain entries for bidders that both have the extension field in at least one
* {@link Imp}, and are known to {@link BidderCatalog} or aliases from {@link BidRequest}.ext.prebid.aliases.
*/
private Future<List<BidderRequest>> extractBidderRequests(BidRequest bidRequest,
UidsCookie uidsCookie,
private Future<List<BidderRequest>> extractBidderRequests(BidRequest bidRequest, List<Imp> imps,
List<String> bidders, UidsCookie uidsCookie,
Map<String, String> aliases,
Timeout timeout) {
// sanity check: discard imps without extension
final List<Imp> imps = bidRequest.getImp().stream()
.filter(imp -> imp.getExt() != null)
.collect(Collectors.toList());

// identify valid bidders and aliases out of imps
final List<String> bidders = imps.stream()
.flatMap(imp -> asStream(imp.getExt().fieldNames())
.filter(bidder -> !Objects.equals(bidder, PREBID_EXT))
.filter(bidder -> isValidBidder(bidder, aliases)))
.distinct()
.collect(Collectors.toList());

final User user = bidRequest.getUser();
final ExtUser extUser = extUser(user);
final Map<String, String> uidsBody = uidsFromBody(extUser);

// set empty ext.prebid.buyerids attr to avoid leaking of buyerids across bidders
// set empty ext.prebid.buyerids attr to avoid leaking of buyerids across biddersAndErrors
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace biddersAndErrors with bidders.

final ObjectNode userExtNode = !uidsBody.isEmpty() && extUser != null
? removeBuyersidsFromUserExtPrebid(extUser) : null;
final ExtRegs extRegs = extRegs(bidRequest.getRegs());
Expand Down Expand Up @@ -827,14 +846,15 @@ private static MetricName bidderErrorTypeToMetric(BidderError.Type errorType) {
*/
private Future<BidResponse> toBidResponse(List<BidderResponse> bidderResponses, BidRequest bidRequest,
TargetingKeywordsCreator keywordsCreator, BidRequestCacheInfo cacheInfo,
Timeout timeout) {
Timeout timeout, Map<String, List<String>> deprecatedBiddersErrors) {
final Set<Bid> winningBids = newOrEmptySet(keywordsCreator);
final Set<Bid> winningBidsByBidder = newOrEmptySet(keywordsCreator);
populateWinningBids(keywordsCreator, bidderResponses, winningBids, winningBidsByBidder);

return toWinningBidsWithCacheIds(winningBids, bidRequest.getImp(), keywordsCreator, cacheInfo, timeout)
.map(winningBidsWithCacheIds -> toBidResponseWithCacheInfo(bidderResponses, bidRequest,
keywordsCreator, winningBidsWithCacheIds, winningBidsByBidder, cacheInfo));
keywordsCreator, winningBidsWithCacheIds, winningBidsByBidder, cacheInfo,
deprecatedBiddersErrors));
}

/**
Expand Down Expand Up @@ -994,15 +1014,16 @@ private static Map<Bid, CacheIdInfo> toMapBidsWithEmptyCacheIds(Set<Bid> bids) {
private BidResponse toBidResponseWithCacheInfo(List<BidderResponse> bidderResponses, BidRequest bidRequest,
TargetingKeywordsCreator keywordsCreator,
Map<Bid, CacheIdInfo> winningBidsWithCacheIds,
Set<Bid> winningBidsByBidder, BidRequestCacheInfo cacheInfo) {
Set<Bid> winningBidsByBidder, BidRequestCacheInfo cacheInfo,
Map<String, List<String>> deprecatedBiddersErrors) {
final List<SeatBid> seatBids = bidderResponses.stream()
.filter(bidderResponse -> !bidderResponse.getSeatBid().getBids().isEmpty())
.map(bidderResponse ->
toSeatBid(bidderResponse, keywordsCreator, winningBidsWithCacheIds, winningBidsByBidder,
cacheInfo))
.collect(Collectors.toList());

final ExtBidResponse bidResponseExt = toExtBidResponse(bidderResponses, bidRequest);
final ExtBidResponse bidResponseExt = toExtBidResponse(bidderResponses, bidRequest, deprecatedBiddersErrors);

return BidResponse.builder()
.id(bidRequest.getId())
Expand Down Expand Up @@ -1086,7 +1107,8 @@ private Bid toBid(BidderBid bidderBid, String bidder, TargetingKeywordsCreator k
* Creates {@link ExtBidResponse} populated with response time, errors and debug info (if requested) from all
* bidders
*/
private static ExtBidResponse toExtBidResponse(List<BidderResponse> results, BidRequest bidRequest) {
private static ExtBidResponse toExtBidResponse(List<BidderResponse> results, BidRequest bidRequest,
Map<String, List<String>> deprecatedBiddersErrors) {
final Map<String, List<ExtHttpCall>> httpCalls = Objects.equals(bidRequest.getTest(), 1)
? results.stream().collect(
Collectors.toMap(BidderResponse::getBidder, r -> ListUtils.emptyIfNull(r.getSeatBid().getHttpCalls())))
Expand All @@ -1096,6 +1118,8 @@ private static ExtBidResponse toExtBidResponse(List<BidderResponse> results, Bid
final Map<String, List<String>> errors = results.stream()
.collect(Collectors.toMap(BidderResponse::getBidder, r -> messages(r.getSeatBid().getErrors())));

errors.putAll(deprecatedBiddersErrors);
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential problem: there is no guarantee that errors will always be a HashMap since it is produced by a collector. Straightforward way to eliminate this problem - replace error initialization using stream with good old for-loop.


final Map<String, Integer> responseTimeMillis = results.stream()
.collect(Collectors.toMap(BidderResponse::getBidder, BidderResponse::getResponseTime));

Expand Down
12 changes: 12 additions & 0 deletions src/main/java/org/prebid/server/bidder/BidderCatalog.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.prebid.server.bidder;

import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
Expand All @@ -15,8 +16,11 @@ public class BidderCatalog {

private final Map<String, BidderDeps> bidderDepsMap;

private final Map<String, String> deprecatedNameToError = new HashMap<>();

public BidderCatalog(List<BidderDeps> bidderDeps) {
bidderDepsMap = Objects.requireNonNull(bidderDeps).stream()
.peek(deps -> deprecatedNameToError.putAll(deps.getDeprecatedNameToError()))
.collect(Collectors.toMap(BidderDeps::getName, Function.identity()));
}

Expand All @@ -34,6 +38,14 @@ public boolean isValidName(String name) {
return bidderDepsMap.containsKey(name);
}

public boolean isDeprecatedName(String name) {
return deprecatedNameToError.containsKey(name);
}

public String errorForDeprecatedName(String name) {
return deprecatedNameToError.get(name);
}

/**
* Tells if given bidder is enabled and ready for auction.
*/
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/org/prebid/server/bidder/BidderDeps.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import lombok.AllArgsConstructor;
import lombok.Value;

import java.util.Map;

/**
* Gathers all dependencies for bidder.
*/
Expand All @@ -18,6 +20,8 @@ public class BidderDeps {
*/
String name;

Map<String, String> deprecatedNameToError;

/**
* Bidder's meta information is used in {@link org.prebid.server.handler.info.BidderDetailsHandler} handler
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

import java.util.List;

@Configuration
public class AdformConfiguration extends BidderConfiguration {

Expand All @@ -37,6 +39,9 @@ public class AdformConfiguration extends BidderConfiguration {
@Value("${external-url}")
private String externalUrl;

@Value("${adapters.adform.deprecated-names:}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's stick with approach accepted throughout other configurations and avoid default values in @Value annotations, better put it in application.yaml packaged in JAR file.

This applies to all other bidders as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but in this case we don't have default value for property.

Copy link
Contributor

@schernysh schernysh Oct 25, 2018

Choose a reason for hiding this comment

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

Empty list should be the default and it could be specified in configuration file

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it can be like:

adapters:
  rubicon:
    ...
    deprecated-names:

So, set just no value.

private List<String> deprecatedNames;

@Bean
BidderDeps adformBidderDeps(HttpClient httpClient, HttpAdapterConnector httpAdapterConnector) {
return bidderDeps(httpClient, httpAdapterConnector);
Expand All @@ -47,6 +52,11 @@ protected String bidderName() {
return BIDDER_NAME;
}

@Override
protected List<String> deprecatedNames() {
return deprecatedNames;
}

@Override
protected MetaInfo createMetaInfo() {
return new AdformMetaInfo(enabled, pbsEnforcesGdpr);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

import java.util.List;

@Configuration
public class AdtelligentConfiguration extends BidderConfiguration {

Expand All @@ -36,11 +38,19 @@ public class AdtelligentConfiguration extends BidderConfiguration {
@Value("${external-url}")
private String externalUrl;

@Value("${adapters.adtelligent.deprecated-names:}")
private List<String> deprecatedNames;

@Override
protected String bidderName() {
return BIDDER_NAME;
}

@Override
protected List<String> deprecatedNames() {
return deprecatedNames;
}

@Bean
BidderDeps adtelligentBidderDeps(HttpClient httpClient, HttpAdapterConnector httpAdapterConnector) {
return bidderDeps(httpClient, httpAdapterConnector);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

import java.util.List;

@Configuration
public class AppnexusConfiguration extends BidderConfiguration {

Expand All @@ -37,6 +39,9 @@ public class AppnexusConfiguration extends BidderConfiguration {
@Value("${external-url}")
private String externalUrl;

@Value("${adapters.appnexus.deprecated-names:}")
private List<String> deprecatedNames;

@Bean
BidderDeps appnexusBidderDeps(HttpClient httpClient, HttpAdapterConnector httpAdapterConnector) {
return bidderDeps(httpClient, httpAdapterConnector);
Expand All @@ -47,6 +52,11 @@ protected String bidderName() {
return BIDDER_NAME;
}

@Override
protected List<String> deprecatedNames() {
return deprecatedNames;
}

@Override
protected MetaInfo createMetaInfo() {
return new AppnexusMetaInfo(enabled, pbsEnforcesGdpr);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

import java.util.List;

@Configuration
public class BeachfrontConfiguration extends BidderConfiguration {

Expand All @@ -39,6 +41,9 @@ public class BeachfrontConfiguration extends BidderConfiguration {
@Value("${adapters.beachfront.pbs-enforces-gdpr}")
private boolean pbsEnforcesGdpr;

@Value("${adapters.beachfront.deprecated-names:}")
private List<String> deprecatedNames;

@Bean
BidderDeps beachfrontBidderDeps(HttpClient httpClient, HttpAdapterConnector httpAdapterConnector) {
return bidderDeps(httpClient, httpAdapterConnector);
Expand All @@ -49,6 +54,11 @@ protected String bidderName() {
return BIDDER_NAME;
}

@Override
protected List<String> deprecatedNames() {
return deprecatedNames;
}

@Override
protected MetaInfo createMetaInfo() {
return new BeachfrontMetaInfo(enabled, pbsEnforcesGdpr);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,29 @@
import org.prebid.server.bidder.Usersyncer;
import org.prebid.server.vertx.http.HttpClient;

import java.util.List;
import java.util.Map;
import java.util.function.Function;
import java.util.stream.Collectors;

public abstract class BidderConfiguration {

private static final String ERROR_MESSAGE_TEMPLATE_FOR_DISABLED = "%s is not configured properly on this "
+ "Prebid Server deploy. If you believe this should work, contact the company hosting the service "
+ "and tell them to check their configuration.";

private static final String ERROR_MESSAGE_TEMPLATE_FOR_DEPRECATED = "%s has been deprecated and is no "
+ "longer available. Use %s instead.";

protected BidderDeps bidderDeps(HttpClient httpClient, HttpAdapterConnector httpAdapterConnector) {
final String bidderName = bidderName();
final MetaInfo metaInfo = createMetaInfo();
final boolean enabled = metaInfo.info().isEnabled();

final Map<String, String> deprecatedNameToError = deprecatedNames().stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a good reason to construct error message here? Isn't it natural to pass deprecatedNames to BidderDeps and let it create message instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

We did it as well as for disabled bidders - construct error messages in configuration, so just to be in same style.

Copy link
Contributor

Choose a reason for hiding this comment

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

For disabled bidder there is a good reason to place it here - message is shared between disabled bidder and adapter, otherwise it belonged to disabled bidder.

.collect(Collectors.toMap(Function.identity(), deprecatedName -> String.format(
BidderConfiguration.ERROR_MESSAGE_TEMPLATE_FOR_DEPRECATED, deprecatedName, bidderName)));

final Usersyncer usersyncer = createUsersyncer();

final Bidder<?> bidder = enabled ? createBidder(metaInfo)
Expand All @@ -33,11 +45,13 @@ protected BidderDeps bidderDeps(HttpClient httpClient, HttpAdapterConnector http
final BidderRequester bidderRequester = createBidderRequester(httpClient, bidder, adapter, usersyncer,
httpAdapterConnector);

return BidderDeps.of(bidderName, metaInfo, usersyncer, bidder, adapter, bidderRequester);
return BidderDeps.of(bidderName, deprecatedNameToError, metaInfo, usersyncer, bidder, adapter, bidderRequester);
}

protected abstract String bidderName();

protected abstract List<String> deprecatedNames();

protected abstract MetaInfo createMetaInfo();

protected abstract Usersyncer createUsersyncer();
Expand Down
Loading