From 9e04626ed303ca5a4ca799aa6d1d9b8353257f8c Mon Sep 17 00:00:00 2001 From: Anton Babak <76536883+AntoxaAntoxic@users.noreply.github.com> Date: Tue, 28 Jan 2025 14:57:13 +0100 Subject: [PATCH] Fix Bidder Aliases Validation (#3696) --- .../server/validation/ImpValidator.java | 45 +++++--- .../server/functional/tests/AliasSpec.groovy | 107 +++++++++++------- .../server/validation/ImpValidatorTest.java | 37 ++++++ 3 files changed, 133 insertions(+), 56 deletions(-) diff --git a/src/main/java/org/prebid/server/validation/ImpValidator.java b/src/main/java/org/prebid/server/validation/ImpValidator.java index d5d9ecba2f1..986e63eca68 100644 --- a/src/main/java/org/prebid/server/validation/ImpValidator.java +++ b/src/main/java/org/prebid/server/validation/ImpValidator.java @@ -28,6 +28,7 @@ import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.exception.ExceptionUtils; +import org.prebid.server.auction.BidderAliases; import org.prebid.server.bidder.BidderCatalog; import org.prebid.server.json.JacksonMapper; import org.prebid.server.proto.openrtb.ext.request.ExtImpPrebid; @@ -365,9 +366,10 @@ private void validateImpExt(ObjectNode ext, Map aliases, int imp validateImpExtPrebid(ext != null ? ext.get(PREBID_EXT) : null, aliases, impIndex, warnings); } - private void validateImpExtPrebid(JsonNode extPrebidNode, Map aliases, int impIndex, - List warnings) - throws ValidationException { + private void validateImpExtPrebid(JsonNode extPrebidNode, + Map requestAliases, + int impIndex, + List warnings) throws ValidationException { if (extPrebidNode == null) { throw new ValidationException( @@ -387,15 +389,21 @@ private void validateImpExtPrebid(JsonNode extPrebidNode, Map al } final ExtImpPrebid extPrebid = parseExtImpPrebid((ObjectNode) extPrebidNode, impIndex); - validateImpExtPrebidBidder(extPrebidBidderNode, extPrebid.getStoredAuctionResponse(), - aliases, impIndex, warnings); - validateImpExtPrebidStoredResponses(extPrebid, aliases, impIndex, warnings); + final BidderAliases aliases = BidderAliases.of(requestAliases, null, bidderCatalog); + validateImpExtPrebidBidder( + extPrebidBidderNode, + extPrebid.getStoredAuctionResponse(), + aliases, + impIndex, + warnings); + + validateImpExtPrebidStoredResponses(extPrebid, aliases, impIndex, warnings); validateImpExtPrebidImp(extPrebidNode.get(IMP_EXT), aliases, impIndex, warnings); } private void validateImpExtPrebidImp(JsonNode imp, - Map aliases, + BidderAliases aliases, int impIndex, List warnings) { if (imp == null) { @@ -406,7 +414,7 @@ private void validateImpExtPrebidImp(JsonNode imp, while (bidders.hasNext()) { final Map.Entry bidder = bidders.next(); final String bidderName = bidder.getKey(); - final String resolvedBidderName = aliases.getOrDefault(bidderName, bidderName); + final String resolvedBidderName = aliases.resolveBidder(bidderName); if (!bidderCatalog.isValidName(resolvedBidderName) && !bidderCatalog.isDeprecatedName(resolvedBidderName)) { bidders.remove(); warnings.add("WARNING: request.imp[%d].ext.prebid.imp.%s was dropped with the reason: invalid bidder" @@ -417,7 +425,7 @@ private void validateImpExtPrebidImp(JsonNode imp, private void validateImpExtPrebidBidder(JsonNode extPrebidBidder, ExtStoredAuctionResponse storedAuctionResponse, - Map aliases, + BidderAliases aliases, int impIndex, List warnings) throws ValidationException { if (extPrebidBidder == null) { @@ -433,7 +441,7 @@ private void validateImpExtPrebidBidder(JsonNode extPrebidBidder, final Map.Entry bidderExtension = bidderExtensions.next(); final String bidder = bidderExtension.getKey(); try { - validateImpBidderExtName(impIndex, bidderExtension, aliases.getOrDefault(bidder, bidder)); + validateImpBidderExtName(impIndex, bidderExtension, aliases.resolveBidder(bidder)); } catch (ValidationException ex) { bidderExtensions.remove(); warnings.add("WARNING: request.imp[%d].ext.prebid.bidder.%s was dropped with a reason: %s" @@ -447,7 +455,7 @@ private void validateImpExtPrebidBidder(JsonNode extPrebidBidder, } private void validateImpExtPrebidStoredResponses(ExtImpPrebid extPrebid, - Map aliases, + BidderAliases aliases, int impIndex, List warnings) throws ValidationException { final ExtStoredAuctionResponse extStoredAuctionResponse = extPrebid.getStoredAuctionResponse(); @@ -479,8 +487,11 @@ private void validateImpExtPrebidStoredResponses(ExtImpPrebid extPrebid, } } - private void validateStoredBidResponse(ExtStoredBidResponse extStoredBidResponse, ObjectNode bidderNode, - Map aliases, int impIndex) throws ValidationException { + private void validateStoredBidResponse(ExtStoredBidResponse extStoredBidResponse, + ObjectNode bidderNode, + BidderAliases aliases, + int impIndex) throws ValidationException { + final String bidder = extStoredBidResponse.getBidder(); final String id = extStoredBidResponse.getId(); if (StringUtils.isEmpty(bidder)) { @@ -493,7 +504,7 @@ private void validateStoredBidResponse(ExtStoredBidResponse extStoredBidResponse "Id was not defined for request.imp[%d].ext.prebid.storedbidresponse.id".formatted(impIndex)); } - final String resolvedBidder = aliases.getOrDefault(bidder, bidder); + final String resolvedBidder = aliases.resolveBidder(bidder); if (!bidderCatalog.isValidName(resolvedBidder)) { throw new ValidationException( @@ -518,8 +529,10 @@ private ExtImpPrebid parseExtImpPrebid(ObjectNode extImpPrebid, int impIndex) th } } - private void validateImpBidderExtName(int impIndex, Map.Entry bidderExtension, String bidderName) - throws ValidationException { + private void validateImpBidderExtName(int impIndex, + Map.Entry bidderExtension, + String bidderName) throws ValidationException { + if (bidderCatalog.isValidName(bidderName)) { final Set messages = bidderParamValidator.validate(bidderName, bidderExtension.getValue()); if (!messages.isEmpty()) { diff --git a/src/test/groovy/org/prebid/server/functional/tests/AliasSpec.groovy b/src/test/groovy/org/prebid/server/functional/tests/AliasSpec.groovy index d4b8f84c826..1ead8c32708 100644 --- a/src/test/groovy/org/prebid/server/functional/tests/AliasSpec.groovy +++ b/src/test/groovy/org/prebid/server/functional/tests/AliasSpec.groovy @@ -1,24 +1,41 @@ package org.prebid.server.functional.tests +import org.prebid.server.functional.model.bidder.AppNexus import org.prebid.server.functional.model.bidder.Generic import org.prebid.server.functional.model.bidder.Openx import org.prebid.server.functional.model.request.auction.BidRequest import org.prebid.server.functional.service.PrebidServerException -import org.prebid.server.functional.testcontainers.Dependencies +import org.prebid.server.functional.service.PrebidServerService import org.prebid.server.functional.util.PBSUtils import static io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST import static org.prebid.server.functional.model.bidder.BidderName.ALIAS +import static org.prebid.server.functional.model.bidder.BidderName.APPNEXUS import static org.prebid.server.functional.model.bidder.BidderName.BOGUS import static org.prebid.server.functional.model.bidder.BidderName.GENERIC import static org.prebid.server.functional.model.bidder.BidderName.GENER_X import static org.prebid.server.functional.model.bidder.BidderName.OPENX import static org.prebid.server.functional.model.bidder.CompressionType.GZIP +import static org.prebid.server.functional.model.response.auction.ErrorType.PREBID import static org.prebid.server.functional.testcontainers.Dependencies.networkServiceContainer import static org.prebid.server.functional.util.HttpUtil.CONTENT_ENCODING_HEADER class AliasSpec extends BaseSpec { + private static final Map ADDITIONAL_BIDDERS_CONFIG = ["adapters.${OPENX.value}.enabled" : "true", + "adapters.${OPENX.value}.endpoint" : "$networkServiceContainer.rootUri/${OPENX.value}/auction".toString(), + "adapters.${APPNEXUS.value}.enabled" : "true", + "adapters.${APPNEXUS.value}.endpoint": "$networkServiceContainer.rootUri/${APPNEXUS.value}/auction".toString()] + private static PrebidServerService pbsServiceWithAdditionalBidders + + def setupSpec() { + pbsServiceWithAdditionalBidders = pbsServiceFactory.getService(ADDITIONAL_BIDDERS_CONFIG + GENERIC_ALIAS_CONFIG) + } + + def cleanupSpec() { + pbsServiceFactory.removeContainer(ADDITIONAL_BIDDERS_CONFIG + GENERIC_ALIAS_CONFIG) + } + def "PBS should be able to take alias from request"() { given: "Default bid request with alias" def bidRequest = BidRequest.defaultBidRequest.tap { @@ -133,56 +150,19 @@ class AliasSpec extends BaseSpec { } def "PBS aliased bidder config should be independently from parent"() { - given: "Pbs config" - def prebidServerService = pbsServiceFactory.getService(GENERIC_ALIAS_CONFIG) - - and: "Default bid request with alias" + given: "Default bid request with alias" def bidRequest = BidRequest.defaultBidRequest.tap { imp[0].ext.prebid.bidder.alias = new Generic() } when: "PBS processes auction request" - prebidServerService.sendAuctionRequest(bidRequest) + pbsServiceWithAdditionalBidders.sendAuctionRequest(bidRequest) then: "Bidder request should contain request per-alies" def bidderRequests = bidder.getBidderRequests(bidRequest.id) assert bidderRequests.size() == 2 } - def "PBS should ignore alias logic when hardcoded alias endpoints are present"() { - given: "PBs server with aliases config" - def pbsConfig = ["adapters.generic.aliases.alias.enabled" : "true", - "adapters.generic.aliases.alias.endpoint": "$networkServiceContainer.rootUri/alias/auction".toString(), - "adapters.openx.enabled" : "true", - "adapters.openx.endpoint" : "$networkServiceContainer.rootUri/openx/auction".toString()] - def pbsService = pbsServiceFactory.getService(pbsConfig) - - and: "Default bid request with openx and alias bidder" - def bidRequest = BidRequest.defaultBidRequest.tap { - imp[0].ext.prebid.bidder.alias = new Generic() - imp[0].ext.prebid.bidder.generic = new Generic() - imp[0].ext.prebid.bidder.openx = new Openx() - ext.prebid.aliases = [(ALIAS.value): OPENX] - } - - when: "PBS processes auction request" - def bidResponse = pbsService.sendAuctionRequest(bidRequest) - - then: "PBS should call only generic bidder" - def responseDebug = bidResponse.ext.debug - assert responseDebug.httpcalls[GENERIC.value] - - and: "PBS shouldn't call only opexn,alias bidder" - assert !responseDebug.httpcalls[OPENX.value] - assert !responseDebug.httpcalls[ALIAS.value] - - and: "PBS should call only generic bidder" - assert bidder.getBidderRequest(bidRequest.id) - - cleanup: "Stop and remove pbs container" - pbsServiceFactory.removeContainer(pbsConfig) - } - def "PBS should ignore aliases for requests with a base adapter"() { given: "PBs server with aliases config" def pbsConfig = ["adapters.openx.enabled" : "true", @@ -209,6 +189,53 @@ class AliasSpec extends BaseSpec { pbsServiceFactory.removeContainer(pbsConfig) } + def "PBS should validate request as alias request and without any warnings when required properties in place"() { + given: "Default bid request with openx and alias bidder" + def bidRequest = BidRequest.defaultBidRequest.tap { + imp[0].ext.prebid.bidder.appNexus = AppNexus.default + imp[0].ext.prebid.bidder.generic = null + ext.prebid.aliases = [(APPNEXUS.value): OPENX] + } + + when: "PBS processes auction request" + def bidResponse = pbsServiceWithAdditionalBidders.sendAuctionRequest(bidRequest) + + then: "PBS contain http call for specific bidder" + def responseDebug = bidResponse.ext.debug + assert responseDebug.httpcalls.size() == 1 + assert responseDebug.httpcalls[APPNEXUS.value]*.uri == ["$networkServiceContainer.rootUri/${APPNEXUS.value}/auction"] + + and: "PBS should not contain any warnings" + assert !bidResponse.ext.warnings + } + + def "PBS should validate request as alias request and emit proper warnings when validation fails for request"() { + given: "Default bid request with openx and alias bidder" + def bidRequest = BidRequest.defaultBidRequest.tap { + imp[0].ext.prebid.bidder.appNexus = new AppNexus() + imp[0].ext.prebid.bidder.generic = null + ext.prebid.aliases = [(APPNEXUS.value): OPENX] + } + + when: "PBS processes auction request" + def bidResponse = pbsServiceWithAdditionalBidders.sendAuctionRequest(bidRequest) + + then: "PBS shouldn't contain http calls" + assert !bidResponse.ext.debug.httpcalls + + and: "Bid response should contain warning" + assert bidResponse.ext?.warnings[PREBID]?.code == [999, 999] + assert bidResponse.ext?.warnings[PREBID]*.message == + ["WARNING: request.imp[0].ext.prebid.bidder.${APPNEXUS.value} was dropped with a reason: " + + "request.imp[0].ext.prebid.bidder.${APPNEXUS.value} failed validation.\n" + + "\$.placement_id: is missing but it is required\n" + + "\$.member: is missing but it is required\n" + + "\$.placementId: is missing but it is required\n" + + "\$.inv_code: is missing but it is required\n" + + "\$.invCode: is missing but it is required", + "WARNING: request.imp[0].ext must contain at least one valid bidder"] + } + def "PBS should invoke as aliases when alias is unknown and core bidder is specified"() { given: "Default bid request with generic and alias bidder" def bidRequest = BidRequest.defaultBidRequest.tap { diff --git a/src/test/java/org/prebid/server/validation/ImpValidatorTest.java b/src/test/java/org/prebid/server/validation/ImpValidatorTest.java index 652150b6732..fa109dc8f58 100644 --- a/src/test/java/org/prebid/server/validation/ImpValidatorTest.java +++ b/src/test/java/org/prebid/server/validation/ImpValidatorTest.java @@ -49,6 +49,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.BDDMockito.given; import static org.mockito.Mock.Strictness.LENIENT; +import static org.mockito.Mockito.verify; @ExtendWith(MockitoExtension.class) public class ImpValidatorTest extends VertxTest { @@ -1516,6 +1517,42 @@ public void validateImpsShouldReturnWarningMessageAndDropBidderWhenBidderExtIsIn .containsOnly(mapper.createObjectNode()); } + @Test + public void validateImpsShouldReturnWarningMessageAndDropBidderWhenBidderExtIsInvalidAndBidderIsHardcodedAlias() + throws ValidationException { + + // given + final List givenImps = singletonList(validImpBuilder() + .ext(mapper.valueToTree(singletonMap("prebid", singletonMap("bidder", singletonMap("someAlias", 0))))) + .build()); + given(bidderParamValidator.validate(any(), any())) + .willReturn(new LinkedHashSet<>(asList("errorMessage1", "errorMessage2"))); + given(bidderCatalog.isValidName("someAlias")).willReturn(true); + + final List debugMessages = new ArrayList<>(); + + // when + target.validateImps(givenImps, Map.of("someAlias", "rubicon"), debugMessages); + + // then + assertThat(debugMessages) + .containsExactly( + """ + WARNING: request.imp[0].ext.prebid.bidder.someAlias was dropped with a reason: \ + request.imp[0].ext.prebid.bidder.someAlias failed validation. + errorMessage1 + errorMessage2""", + "WARNING: request.imp[0].ext must contain at least one valid bidder"); + + assertThat(givenImps) + .extracting(Imp::getExt) + .extracting(impExt -> impExt.get("prebid")) + .extracting(prebid -> prebid.get("bidder")) + .containsOnly(mapper.createObjectNode()); + + verify(bidderParamValidator).validate(eq("someAlias"), any()); + } + @Test public void validateImpsShouldReturnWarningMessageAndDropBidderWhenImpExtPrebidImpBidderIsUnknown() throws ValidationException {