Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
142 changes: 91 additions & 51 deletions src/main/java/org/prebid/server/bidder/yahooads/YahooAdsBidder.java

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure you need all the promote* methods, since we already do that for you (see BidRequestOrtb25To26Converter).

Also, is it even possible for gpp, gppSid, and coppa to be in regs.ext? We don't even read them from there in the privacy services.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the review @CTMBNara.

I looked into BidRequestOrtb25To26Converter on this. It promotes gdpr and us_privacy up from regs.ext, but it doesn't touch gpp, gpp_sid, or coppa, so those three don't get promoted for us anywhere in core. Let me know if I've missed something there.

Main reason I kept the promotion was it's a no-op for normal requests (values are already top-level, so it skips), but if any arrive under regs.ext they'd otherwise stay nested and get dropped on the way out. Since these are GPP/COPPA signals, I'd rather not risk silently losing one during the cutover. It only kicks in when the top-level field is empty, and leaves everything else in regs.ext untouched.

Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
package org.prebid.server.bidder.yahooads;

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.node.ArrayNode;
import com.fasterxml.jackson.databind.node.IntNode;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.fasterxml.jackson.databind.node.TextNode;
import com.iab.openrtb.request.App;
import com.iab.openrtb.request.Banner;
import com.iab.openrtb.request.BidRequest;
Expand All @@ -20,8 +18,6 @@
import io.vertx.core.http.HttpMethod;
import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.lang3.StringUtils;
import org.prebid.server.auction.versionconverter.BidRequestOrtbVersionConversionManager;
import org.prebid.server.auction.versionconverter.OrtbVersion;
import org.prebid.server.bidder.Bidder;
import org.prebid.server.bidder.model.BidderBid;
import org.prebid.server.bidder.model.BidderCall;
Expand All @@ -32,9 +28,7 @@
import org.prebid.server.json.DecodeException;
import org.prebid.server.json.JacksonMapper;
import org.prebid.server.proto.openrtb.ext.ExtPrebid;
import org.prebid.server.proto.openrtb.ext.FlexibleExtension;
import org.prebid.server.proto.openrtb.ext.request.ExtRegs;
import org.prebid.server.proto.openrtb.ext.request.ExtRegsDsa;
import org.prebid.server.proto.openrtb.ext.request.yahooads.ExtImpYahooAds;
import org.prebid.server.proto.openrtb.ext.response.BidType;
import org.prebid.server.util.HttpUtil;
Expand All @@ -44,24 +38,24 @@
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.Optional;

public class YahooAdsBidder implements Bidder<BidRequest> {

private static final TypeReference<ExtPrebid<?, ExtImpYahooAds>> YAHOO_ADVERTISING_EXT_TYPE_REFERENCE =
new TypeReference<>() {
};

private static final String GPP_PROPERTY = "gpp";
private static final String GPP_SID_PROPERTY = "gpp_sid";
private static final String COPPA_PROPERTY = "coppa";

private final String endpointUrl;
private final BidRequestOrtbVersionConversionManager conversionManager;
private final JacksonMapper mapper;

public YahooAdsBidder(String endpointUrl,
BidRequestOrtbVersionConversionManager conversionManager,
JacksonMapper mapper) {
this.endpointUrl = HttpUtil.validateUrl(Objects.requireNonNull(endpointUrl));
this.mapper = Objects.requireNonNull(mapper);
this.conversionManager = Objects.requireNonNull(conversionManager);
}

@Override
Expand All @@ -70,15 +64,13 @@ public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest bidRequ
final List<BidderError> errors = new ArrayList<>();

final Regs regs = bidRequest.getRegs();
final BidRequest bidRequestOpenRtb25 = this.conversionManager.convertFromAuctionSupportedVersion(bidRequest,
OrtbVersion.ORTB_2_5);

final List<Imp> impList = bidRequestOpenRtb25.getImp();
final List<Imp> impList = bidRequest.getImp();
for (int i = 0; i < impList.size(); i++) {
try {
final Imp imp = impList.get(i);
final ExtImpYahooAds extImpYahooAds = parseAndValidateImpExt(imp.getExt(), i);
final BidRequest modifiedRequest = modifyRequest(bidRequestOpenRtb25, imp, extImpYahooAds,
final BidRequest modifiedRequest = modifyRequest(bidRequest, imp, extImpYahooAds,
regs);
bidRequests.add(makeHttpRequest(modifiedRequest));
} catch (PreBidException e) {
Expand Down Expand Up @@ -125,7 +117,7 @@ private BidRequest modifyRequest(BidRequest request, Imp imp, ExtImpYahooAds ext
}

if (regs != null) {
requestBuilder.regs(modifyRegs(regs));
requestBuilder.regs(promoteRegsExtToTopLevel(regs));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nitpick (non-blocking): promoteRegsExtToTopLevel(regs) is a pure function of regs, which is captured once before the imp loop and never changes across iterations — but it's recomputed (including a full ext.getProperties() walk) once per imp. For multi-imp requests, consider hoisting it out of the loop in makeHttpRequests and passing the already-promoted Regs into modifyRequest.

}

return requestBuilder
Expand Down Expand Up @@ -170,50 +162,98 @@ private static Banner modifyBanner(Banner banner) {
.build();
}

private Regs modifyRegs(Regs regs) {
final ExtRegs extRegs = resolveExtRegs(regs);
// Promote legacy 2.5 regs.ext gpp/gpp_sid/coppa to their 2.6 top-level slots.
// A field is promoted only when it is absent at top-level and present and well-formed
// in ext; each promoted key is then removed from ext. Anything not promoted (already
// top-level, missing, or malformed) is left untouched, in ext.
private static Regs promoteRegsExtToTopLevel(Regs regs) {
final ExtRegs ext = regs.getExt();
if (ext == null || ext.getProperties().isEmpty()) {
return regs;
}

return Regs.builder().ext(extRegs).build();
}
final String promotedGpp = gppToPromote(regs, ext);
final List<Integer> promotedGppSid = gppSidToPromote(regs, ext);
final Integer promotedCoppa = coppaToPromote(regs, ext);

private ExtRegs resolveExtRegs(Regs regs) {
final Integer gdpr = resolveGdpr(regs);
final String usPrivacy = resolveUsPrivacy(regs);
final String gpp = regs.getGpp();
final List<Integer> gppSid = regs.getGppSid();

final String gpc = Optional.ofNullable(regs.getExt())
.map(ExtRegs::getGpc)
.orElse(null);
final ExtRegsDsa dsa = Optional.ofNullable(regs.getExt())
.map(ExtRegs::getDsa)
.orElse(null);
final ExtRegs extRegs = ExtRegs.of(gdpr, usPrivacy, gpc, dsa);
extRegs.addProperty("gpp", TextNode.valueOf(gpp));
if (!CollectionUtils.isEmpty(gppSid)) {
final ArrayNode gppArrayNode = mapper.mapper().createArrayNode();
gppSid.forEach(gppArrayNode::add);
extRegs.addProperty("gpp_sid", gppArrayNode);
if (promotedGpp == null && promotedGppSid == null && promotedCoppa == null) {
return regs;
}
if (regs.getCoppa() != null) {
extRegs.addProperty("coppa", IntNode.valueOf(regs.getCoppa()));

final Regs.RegsBuilder builder = regs.toBuilder();
if (promotedGpp != null) {
builder.gpp(promotedGpp);
}
if (promotedGppSid != null) {
builder.gppSid(promotedGppSid);
}
if (promotedCoppa != null) {
builder.coppa(promotedCoppa);
}
return builder
.ext(removePromotedKeys(ext, promotedGpp != null, promotedGppSid != null, promotedCoppa != null))
.build();
}

Optional.ofNullable(regs.getExt())
.map(FlexibleExtension::getProperties)
.ifPresent(extRegs::addProperties);
// Value to lift from ext, or null when top-level already has it or ext lacks a valid value.
private static String gppToPromote(Regs regs, ExtRegs ext) {
if (regs.getGpp() != null) {
return null;
}
final JsonNode node = ext.getProperties().get(GPP_PROPERTY);
return node != null && node.isTextual() ? node.asText() : null;
}

return extRegs;
private static List<Integer> gppSidToPromote(Regs regs, ExtRegs ext) {
if (!CollectionUtils.isEmpty(regs.getGppSid())) {
return null;
}
final JsonNode node = ext.getProperties().get(GPP_SID_PROPERTY);
if (node == null || !node.isArray() || node.isEmpty()) {
return null;
}
final List<Integer> sids = new ArrayList<>(node.size());
for (final JsonNode elem : node) {
if (!elem.isIntegralNumber()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue (non-blocking): isIntegralNumber() is true for long/BigInteger nodes, and asInt() on those silently truncates (3000000000-1294967296, 2147483648-2147483648). So an out-of-int-range gpp_sid element passes this "malformed?" guard and then gets shipped to the SSP as a corrupted section ID — which defeats the "leave it in ext untouched" guarantee this method is built around. Same narrowing applies to coppa at line 230.

Suggest elem.canConvertToInt() / node.canConvertToInt() (already the idiom in RubiconBidder) — it returns false for both non-integral and out-of-range values, so oversized input falls through to the safe "leave untouched" path instead of emitting wrong data.

return null;
}
sids.add(elem.asInt());
}
return sids;
}

private static Integer resolveGdpr(Regs regs) {
return regs.getGdpr() != null ? regs.getGdpr()
: (regs.getExt() != null ? regs.getExt().getGdpr() : null);
private static Integer coppaToPromote(Regs regs, ExtRegs ext) {
if (regs.getCoppa() != null) {
return null;
}
final JsonNode node = ext.getProperties().get(COPPA_PROPERTY);
return node != null && node.isIntegralNumber() ? node.asInt() : null;
}

// Rebuild regs.ext keeping the typed fields and every property except the promoted ones.
private static ExtRegs removePromotedKeys(ExtRegs ext,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (design): when a field is already set top-level and a different value also sits in regs.ext, the ext copy is left in place — so the outbound request can carry two conflicting declarations of the same consent signal. This is deliberate and well-tested (makeHttpRequestsShouldKeepExtGppWhenTopLevelGppAlreadySetEvenIfSiblingIsPromoted), but it diverges from core's stricter rule in BidRequestOrtb25To26Converter.resolveRegsExt, which clears the ext copy of gdpr/us_privacy whenever ext had a value at all ("once superseded, always clear").

Consider matching that here: strip the ext key whenever a valid top-level value exists, not only when this call performed the promotion. Then the wire can never carry two values for one signal.

boolean gppPromoted,
boolean gppSidPromoted,
boolean coppaPromoted) {
final ExtRegs result = ExtRegs.of(
ext.getGdpr(), ext.getUsPrivacy(), ext.getGpc(), ext.getDsa());
ext.getProperties().forEach((key, value) -> {
final boolean isPromotedKey = (gppPromoted && GPP_PROPERTY.equals(key))
|| (gppSidPromoted && GPP_SID_PROPERTY.equals(key))
|| (coppaPromoted && COPPA_PROPERTY.equals(key));
if (!isPromotedKey) {
result.addProperty(key, value);
}
});
return isExtEmpty(result) ? null : result;
}

private static String resolveUsPrivacy(Regs regs) {
return regs.getUsPrivacy() != null ? regs.getUsPrivacy()
: (regs.getExt() != null ? regs.getExt().getUsPrivacy() : null);
private static boolean isExtEmpty(ExtRegs ext) {
return ext.getGdpr() == null
&& ext.getUsPrivacy() == null
&& ext.getGpc() == null
&& ext.getDsa() == null
&& ext.getProperties().isEmpty();
}

private HttpRequest<BidRequest> makeHttpRequest(BidRequest outgoingRequest) {
Expand All @@ -228,7 +268,7 @@ private HttpRequest<BidRequest> makeHttpRequest(BidRequest outgoingRequest) {

private static MultiMap makeHeaders(Device device) {
final MultiMap headers = HttpUtil.headers()
.add(HttpUtil.X_OPENRTB_VERSION_HEADER, "2.5");
.add(HttpUtil.X_OPENRTB_VERSION_HEADER, "2.6");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (non-blocking): this hard-coded "2.6" duplicates ortb-version: "2.6" in yahooAds.yaml with no link between them — which is exactly the drift that produced the bug this PR fixes (yaml said 2.6, the code did 2.5). It matches the codebase's dominant convention, so not a new problem, but at minimum consider hoisting it to a named constant next to GPP_PROPERTY (newer bidders like Msft do this). A nice follow-up would be setting this header centrally from BidderInfo.getOrtbVersion() so the ~38 hand-maintained copies across bidders collapse to one source of truth.


final String deviceUa = device != null ? device.getUa() : null;
HttpUtil.addHeaderIfValueIsNotEmpty(headers, HttpUtil.USER_AGENT_HEADER, deviceUa);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.prebid.server.spring.config.bidder;

import org.prebid.server.auction.versionconverter.BidRequestOrtbVersionConversionManager;
import org.prebid.server.bidder.BidderDeps;
import org.prebid.server.bidder.yahooads.YahooAdsBidder;
import org.prebid.server.json.JacksonMapper;
Expand All @@ -26,12 +25,11 @@ BidderConfigurationProperties configurationProperties() {

@Bean
BidderDeps yahooAdsBidderDeps(BidderConfigurationProperties yahooAdsConfigurationProperties,
JacksonMapper mapper,
BidRequestOrtbVersionConversionManager conversionManager) {
JacksonMapper mapper) {

return BidderDepsAssembler.forBidder(BIDDER_NAME)
.withConfig(yahooAdsConfigurationProperties)
.bidderCreator(config -> new YahooAdsBidder(config.getEndpoint(), conversionManager, mapper))
.bidderCreator(config -> new YahooAdsBidder(config.getEndpoint(), mapper))
.assemble();
}
}
Loading