diff --git a/src/main/java/org/prebid/server/bidder/openx/OpenxBidder.java b/src/main/java/org/prebid/server/bidder/openx/OpenxBidder.java index 6483b491a61..7da18272dc5 100644 --- a/src/main/java/org/prebid/server/bidder/openx/OpenxBidder.java +++ b/src/main/java/org/prebid/server/bidder/openx/OpenxBidder.java @@ -16,7 +16,6 @@ import org.prebid.server.bidder.model.BidderError; import org.prebid.server.bidder.model.HttpRequest; import org.prebid.server.bidder.model.Result; -import org.prebid.server.bidder.openx.model.OpenxImpType; import org.prebid.server.bidder.openx.proto.OpenxBidExt; import org.prebid.server.bidder.openx.proto.OpenxRequestExt; import org.prebid.server.bidder.openx.proto.OpenxVideoExt; @@ -43,7 +42,6 @@ import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; -import java.util.stream.Stream; public class OpenxBidder implements Bidder { @@ -68,20 +66,56 @@ public OpenxBidder(String endpointUrl, JacksonMapper mapper) { @Override public Result>> makeHttpRequests(BidRequest bidRequest) { - final Map> differentiatedImps = bidRequest.getImp().stream() - .collect(Collectors.groupingBy(OpenxBidder::resolveImpType)); + final List modifiedImps = new ArrayList<>(); + final List errors = new ArrayList<>(); + final ExtImpOpenx firstValidImpExt = processImps(bidRequest.getImp(), modifiedImps, errors); + + if (modifiedImps.isEmpty()) { + return Result.withErrors(errors); + } - final List processingErrors = new ArrayList<>(); - final List outgoingRequests = makeRequests( - bidRequest, - differentiatedImps.get(OpenxImpType.banner), - differentiatedImps.get(OpenxImpType.video), - differentiatedImps.get(OpenxImpType.xNative), - processingErrors); + final BidRequest modifiedBidRequest = modifyBidRequest(bidRequest, modifiedImps, firstValidImpExt); + return Result.of(Collections.singletonList(makeRequest(modifiedBidRequest)), errors); + } - final List errors = errors(differentiatedImps.get(OpenxImpType.other), processingErrors); + private ExtImpOpenx processImps(List imps, List modifiedImps, List errors) { + ExtImpOpenx firstValidImpExt = null; + for (Imp imp : imps) { + if (!isSupportedImpType(imp)) { + errors.add(unsupportedImpTypeError(imp)); + continue; + } + + final ExtPrebid impExt; + try { + impExt = parseOpenxExt(imp); + } catch (PreBidException e) { + errors.add(invalidImpError(imp, e)); + continue; + } + + modifiedImps.add(makeImp(imp, impExt)); + if (firstValidImpExt == null) { + firstValidImpExt = impExt.getBidder(); + } + } + return firstValidImpExt; + } - return Result.of(createHttpRequests(outgoingRequests), errors); + private static BidderError unsupportedImpTypeError(Imp imp) { + return BidderError.badInput( + "OpenX only supports banner, video and native imps. Ignoring imp id=" + imp.getId()); + } + + private static BidderError invalidImpError(Imp imp, PreBidException e) { + return BidderError.badInput("imp id=%s: %s".formatted(imp.getId(), e.getMessage())); + } + + private BidRequest modifyBidRequest(BidRequest bidRequest, List imps, ExtImpOpenx firstValidImpExt) { + return bidRequest.toBuilder() + .imp(imps) + .ext(makeReqExt(firstValidImpExt)) + .build(); } @Override @@ -94,45 +128,8 @@ public Result> makeBids(BidderCall httpCall, BidRequ } } - private List makeRequests( - BidRequest bidRequest, - List bannerImps, - List videoImps, - List nativeImps, - List errors) { - final List bidRequests = new ArrayList<>(); - // single request for all banner and native imps - final List bannerAndNativeImps = Stream.of(bannerImps, nativeImps) - .filter(Objects::nonNull) - .flatMap(Collection::stream) - .toList(); - final BidRequest bannerAndNativeImpsRequest = createSingleRequest(bannerAndNativeImps, bidRequest, errors); - if (bannerAndNativeImpsRequest != null) { - bidRequests.add(bannerAndNativeImpsRequest); - } - - if (CollectionUtils.isNotEmpty(videoImps)) { - // single request for each video imp - bidRequests.addAll(videoImps.stream() - .map(Collections::singletonList) - .map(imps -> createSingleRequest(imps, bidRequest, errors)) - .filter(Objects::nonNull) - .toList()); - } - return bidRequests; - } - - private static OpenxImpType resolveImpType(Imp imp) { - if (imp.getBanner() != null) { - return OpenxImpType.banner; - } - if (imp.getVideo() != null) { - return OpenxImpType.video; - } - if (imp.getXNative() != null) { - return OpenxImpType.xNative; - } - return OpenxImpType.other; + private static boolean isSupportedImpType(Imp imp) { + return imp.getBanner() != null || imp.getVideo() != null || imp.getXNative() != null; } private static BidType resolveBidType(Imp imp) { @@ -148,53 +145,11 @@ private static BidType resolveBidType(Imp imp) { return BidType.banner; } - private List errors(List notSupportedImps, List processingErrors) { - final List errors = new ArrayList<>(); - // add errors for imps with unsupported media types - if (CollectionUtils.isNotEmpty(notSupportedImps)) { - errors.addAll( - notSupportedImps.stream() - .map(imp -> - "OpenX only supports banner, video and native imps. Ignoring imp id=" + imp.getId()) - .map(BidderError::badInput) - .toList()); - } - - // add errors detected during requests creation - errors.addAll(processingErrors); - - return errors; - } - - private List> createHttpRequests(List bidRequests) { - return bidRequests.stream() - .filter(Objects::nonNull) - .map(singleBidRequest -> BidderUtil.defaultRequest(singleBidRequest, endpointUrl, mapper)) - .toList(); - } - - private BidRequest createSingleRequest(List imps, BidRequest bidRequest, List errors) { - if (CollectionUtils.isEmpty(imps)) { - return null; - } - - List processedImps = null; - try { - processedImps = imps.stream().map(this::makeImp).toList(); - } catch (PreBidException e) { - errors.add(BidderError.badInput(e.getMessage())); - } - - return CollectionUtils.isNotEmpty(processedImps) - ? bidRequest.toBuilder() - .imp(processedImps) - .ext(makeReqExt(imps.getFirst())) - .build() - : null; + private HttpRequest makeRequest(BidRequest bidRequest) { + return BidderUtil.defaultRequest(bidRequest, endpointUrl, mapper); } - private Imp makeImp(Imp imp) { - final ExtPrebid impExt = parseOpenxExt(imp); + private Imp makeImp(Imp imp, ExtPrebid impExt) { final ExtImpOpenx openxImpExt = impExt.getBidder(); final ExtImpPrebid prebidImpExt = impExt.getPrebid(); final Imp.ImpBuilder impBuilder = imp.toBuilder() @@ -202,7 +157,7 @@ private Imp makeImp(Imp imp) { .bidfloor(resolveBidFloor(imp.getBidfloor(), openxImpExt.getCustomFloor())) .ext(makeImpExt(imp.getExt(), MapUtils.isNotEmpty(openxImpExt.getCustomParams()))); - if (resolveImpType(imp) == OpenxImpType.video + if (imp.getVideo() != null && prebidImpExt != null && Objects.equals(prebidImpExt.getIsRewardedInventory(), 1)) { impBuilder.video(imp.getVideo().toBuilder() @@ -218,8 +173,7 @@ private static BigDecimal resolveBidFloor(BigDecimal impBidFloor, BigDecimal cus : impBidFloor; } - private ExtRequest makeReqExt(Imp imp) { - final ExtImpOpenx openxImpExt = parseOpenxExt(imp).getBidder(); + private ExtRequest makeReqExt(ExtImpOpenx openxImpExt) { return mapper.fillExtension( ExtRequest.empty(), OpenxRequestExt.of(openxImpExt.getDelDomain(), openxImpExt.getPlatform(), OPENX_CONFIG)); diff --git a/src/main/java/org/prebid/server/bidder/openx/model/OpenxImpType.java b/src/main/java/org/prebid/server/bidder/openx/model/OpenxImpType.java deleted file mode 100644 index c872e7f97e6..00000000000 --- a/src/main/java/org/prebid/server/bidder/openx/model/OpenxImpType.java +++ /dev/null @@ -1,9 +0,0 @@ -package org.prebid.server.bidder.openx.model; - -public enum OpenxImpType { - - // supported - banner, video, xNative, - // not supported - other -} diff --git a/src/test/java/org/prebid/server/bidder/openx/OpenxBidderTest.java b/src/test/java/org/prebid/server/bidder/openx/OpenxBidderTest.java index f5ba6094c17..74888aa1d68 100644 --- a/src/test/java/org/prebid/server/bidder/openx/OpenxBidderTest.java +++ b/src/test/java/org/prebid/server/bidder/openx/OpenxBidderTest.java @@ -99,6 +99,7 @@ public void makeHttpRequestsShouldReturnResultWithErrorWhenImpExtOmitted() { // given final BidRequest bidRequest = BidRequest.builder() .imp(singletonList(Imp.builder() + .id("impId1") .banner(Banner.builder().build()) .build())) .build(); @@ -109,7 +110,7 @@ public void makeHttpRequestsShouldReturnResultWithErrorWhenImpExtOmitted() { // then assertThat(result.getValue()).isEmpty(); assertThat(result.getErrors()).hasSize(1) - .containsExactly(BidderError.badInput("openx parameters section is missing")); + .containsExactly(BidderError.badInput("imp id=impId1: openx parameters section is missing")); } @Test @@ -117,6 +118,7 @@ public void makeHttpRequestsShouldReturnResultWithErrorWhenImpExtMalformed() { // given final BidRequest bidRequest = BidRequest.builder() .imp(singletonList(Imp.builder() + .id("impId1") .banner(Banner.builder().build()) .ext(mapper.createObjectNode()) .build())) @@ -128,7 +130,7 @@ public void makeHttpRequestsShouldReturnResultWithErrorWhenImpExtMalformed() { // then assertThat(result.getValue()).isEmpty(); assertThat(result.getErrors()).hasSize(1) - .containsExactly(BidderError.badInput("openx parameters section is missing")); + .containsExactly(BidderError.badInput("imp id=impId1: openx parameters section is missing")); } @Test @@ -136,6 +138,7 @@ public void makeHttpRequestsShouldReturnResultWithErrorWhenImpExtOpenxEmpty() { // given final BidRequest bidRequest = BidRequest.builder() .imp(singletonList(Imp.builder() + .id("impId1") .video(Video.builder().build()) .ext(mapper.valueToTree( ExtPrebid.of(null, null))) @@ -148,7 +151,7 @@ public void makeHttpRequestsShouldReturnResultWithErrorWhenImpExtOpenxEmpty() { // then assertThat(result.getValue()).isEmpty(); assertThat(result.getErrors()).hasSize(1) - .containsExactly(BidderError.badInput("openx parameters section is missing")); + .containsExactly(BidderError.badInput("imp id=impId1: openx parameters section is missing")); } @Test @@ -156,6 +159,7 @@ public void makeHttpRequestsShouldReturnResultWithErrorWhenImpExtOpenxMalformed( // given final BidRequest bidRequest = BidRequest.builder() .imp(singletonList(Imp.builder() + .id("impId1") .banner(Banner.builder().build()) .ext(mapper.valueToTree(ExtPrebid.of(null, mapper.createArrayNode()))) .build())) @@ -167,7 +171,7 @@ public void makeHttpRequestsShouldReturnResultWithErrorWhenImpExtOpenxMalformed( // then assertThat(result.getValue()).isEmpty(); assertThat(result.getErrors().getFirst().getMessage()) - .startsWith("Cannot deserialize value of"); + .startsWith("imp id=impId1: Cannot deserialize value of"); } @Test @@ -233,10 +237,9 @@ public void makeHttpRequestsShouldReturnResultWithExpectedFieldsSet() { .containsExactly(BidderError.badInput( "OpenX only supports banner, video and native imps. Ignoring imp id=impId1")); - assertThat(result.getValue()).hasSize(3) + assertThat(result.getValue()).hasSize(1) .extracting(httpRequest -> mapper.readValue(httpRequest.getBody(), BidRequest.class)) .containsExactly( - // check if all banner imps are part of single bidRequest BidRequest.builder() .id("bidRequestId") .imp(asList( @@ -262,46 +265,20 @@ public void makeHttpRequestsShouldReturnResultWithExpectedFieldsSet() { .customParams( givenCustomParams("foo2", "bar2")) .build())) - .build())) - .ext(jacksonMapper.fillExtension( - ExtRequest.empty(), - OpenxRequestExt.of("se-demo-d.openx.net", null, "hb_pbs_1.0.0"))) - .user(User.builder() - .ext(ExtUser.builder().consent("consent").build()) - .build()) - .regs(Regs.builder().coppa(0).ext(ExtRegs.of(1, null, null, null)).build()) - .build(), - // check if each of video imps is a part of separate bidRequest and impId3 is rewarded video - BidRequest.builder() - .id("bidRequestId") - .imp(singletonList( + .build(), Imp.builder() .id("impId3") .video(Video.builder() .ext(mapper.valueToTree(OpenxVideoExt.of(1))) .build()) .tagid("555555") - // check if each of video imps is a part of separate bidRequest .bidfloor(BigDecimal.valueOf(0.1)) .ext(mapper.valueToTree( ExtImpOpenx.builder() .customParams( givenCustomParams("foo3", "bar3")) .build())) - .build())) - - .ext(jacksonMapper.fillExtension( - ExtRequest.empty(), - OpenxRequestExt.of("se-demo-d.openx.net", null, "hb_pbs_1.0.0"))) - .user(User.builder() - .ext(ExtUser.builder().consent("consent").build()) - .build()) - .regs(Regs.builder().coppa(0).ext(ExtRegs.of(1, null, null, null)).build()) - .build(), - // check if each of video imps is a part of separate bidRequest - BidRequest.builder() - .id("bidRequestId") - .imp(singletonList( + .build(), Imp.builder() .id("impId4") .video(Video.builder().build()) @@ -313,7 +290,8 @@ public void makeHttpRequestsShouldReturnResultWithExpectedFieldsSet() { .build())) .build())) .ext(jacksonMapper.fillExtension( - ExtRequest.empty(), OpenxRequestExt.of(null, "PLATFORM", "hb_pbs_1.0.0"))) + ExtRequest.empty(), + OpenxRequestExt.of("se-demo-d.openx.net", null, "hb_pbs_1.0.0"))) .user(User.builder() .ext(ExtUser.builder().consent("consent").build()) .build()) @@ -322,38 +300,29 @@ public void makeHttpRequestsShouldReturnResultWithExpectedFieldsSet() { } @Test - public void makeHttpRequestsShouldReturnResultWithSingleBidRequestForMultipleBannerAndNativeImps() { + public void makeHttpRequestsShouldReturnResultWithSingleBidRequestForMultipleImpsWithDifferentFormat() { // given final BidRequest bidRequest = BidRequest.builder() .id("bidRequestId") .imp(asList( Imp.builder() - .id("impId4") - .banner(Banner.builder().build()) + .id("impId1") + .banner(Banner.builder().w(320).h(200).build()) .ext(mapper.valueToTree( - ExtPrebid.of(null, - ExtImpOpenx.builder() - .customParams(givenCustomParams("foo4", "bar4")) - .delDomain("se-demo-d.openx.net") - .unit("4").build()))).build(), + ExtPrebid.of(null, ExtImpOpenx.builder().unit("1").build()))) + .build(), Imp.builder() - .id("impId5") - .xNative(Native.builder().request("{\"testreq\":1}").build()) + .id("impId2") + .xNative(Native.builder().request("{\"version\":1}").build()) .ext(mapper.valueToTree( - ExtPrebid.of(null, - ExtImpOpenx.builder() - .customParams(givenCustomParams("foo5", "bar5")) - .delDomain("se-demo-d.openx.net") - .unit("5").build()))).build(), + ExtPrebid.of(null, ExtImpOpenx.builder().unit("2").build()))) + .build(), Imp.builder() - .id("impId6") - .xNative(Native.builder().build()) + .id("impId3") + .video(Video.builder().maxduration(10).build()) .ext(mapper.valueToTree( - ExtPrebid.of(null, - ExtImpOpenx.builder() - .customParams(givenCustomParams("foo6", "bar6")) - .delDomain("se-demo-d.openx.net") - .unit("6").build()))).build())) + ExtPrebid.of(null, ExtImpOpenx.builder().unit("3").build()))) + .build())) .user(User.builder().ext(ExtUser.builder().consent("consent").build()).build()) .regs(Regs.builder().coppa(0).ext(ExtRegs.of(1, null, null, null)).build()) .build(); @@ -367,43 +336,29 @@ public void makeHttpRequestsShouldReturnResultWithSingleBidRequestForMultipleBan assertThat(result.getValue()).hasSize(1) .extracting(httpRequest -> mapper.readValue(httpRequest.getBody(), BidRequest.class)) .containsExactly( - // check if all native and banner imps are part of single bidRequest BidRequest.builder() .id("bidRequestId") .imp(asList( Imp.builder() - .id("impId4") - .tagid("4") - .banner(Banner.builder().build()) - .ext(mapper.valueToTree( - ExtImpOpenx.builder() - .customParams( - givenCustomParams("foo4", "bar4")) - .build())) - .build(), + .id("impId1") + .tagid("1") + .banner(Banner.builder().w(320).h(200).build()) + .ext(mapper.valueToTree(ExtImpOpenx.builder().build())).build(), Imp.builder() - .id("impId5") - .tagid("5") - .xNative(Native.builder().request("{\"testreq\":1}").build()) - .ext(mapper.valueToTree( - ExtImpOpenx.builder() - .customParams( - givenCustomParams("foo5", "bar5")) - .build())) + .id("impId2") + .tagid("2") + .xNative(Native.builder().request("{\"version\":1}").build()) + .ext(mapper.valueToTree(ExtImpOpenx.builder().build())) .build(), Imp.builder() - .id("impId6") - .tagid("6") - .xNative(Native.builder().build()) - .ext(mapper.valueToTree( - ExtImpOpenx.builder() - .customParams( - givenCustomParams("foo6", "bar6")) - .build())) + .id("impId3") + .tagid("3") + .video(Video.builder().maxduration(10).build()) + .ext(mapper.valueToTree(ExtImpOpenx.builder().build())) .build())) .ext(jacksonMapper.fillExtension( ExtRequest.empty(), - OpenxRequestExt.of("se-demo-d.openx.net", null, "hb_pbs_1.0.0"))) + OpenxRequestExt.of(null, null, "hb_pbs_1.0.0"))) .user(User.builder() .ext(ExtUser.builder().consent("consent").build()) .build()) @@ -412,67 +367,122 @@ public void makeHttpRequestsShouldReturnResultWithSingleBidRequestForMultipleBan } @Test - public void makeHttpRequestsShouldReturnResultWithSingleBidRequestForMultiFormatImps() { + public void makeHttpRequestsShouldSkipMalformedFirstImpAndDeriveRequestExtFromLaterValidImp() { // given final BidRequest bidRequest = BidRequest.builder() .id("bidRequestId") .imp(asList( Imp.builder() - .id("impId1") - .banner(Banner.builder().w(320).h(200).build()) - .video(Video.builder().maxduration(10).build()) - .ext(mapper.valueToTree( - ExtPrebid.of(null, ExtImpOpenx.builder().unit("1").build()))) + .id("badImp") + .banner(Banner.builder().build()) .build(), Imp.builder() - .id("impId2") - .banner(Banner.builder().w(300).h(150).build()) - .xNative(Native.builder().request("{\"version\":1}").build()) + .id("anotherBadImp") + .banner(Banner.builder().build()) + .build(), + Imp.builder() + .id("goodImp") + .banner(Banner.builder().build()) .ext(mapper.valueToTree( - ExtPrebid.of(null, ExtImpOpenx.builder().unit("2").build()))) + ExtPrebid.of(null, + ExtImpOpenx.builder() + .delDomain("se-demo-d.openx.net") + .platform("PLATFORM") + .unit("555555").build()))) .build())) - .user(User.builder().ext(ExtUser.builder().consent("consent").build()).build()) - .regs(Regs.builder().coppa(0).ext(ExtRegs.of(1, null, null, null)).build()) .build(); // when final Result>> result = target.makeHttpRequests(bidRequest); // then - assertThat(result.getErrors()).isEmpty(); + assertThat(result.getErrors()).hasSize(2) + .containsExactly( + BidderError.badInput("imp id=badImp: openx parameters section is missing"), + BidderError.badInput("imp id=anotherBadImp: openx parameters section is missing")); assertThat(result.getValue()).hasSize(1) .extracting(httpRequest -> mapper.readValue(httpRequest.getBody(), BidRequest.class)) .containsExactly( - // check if all native and banner imps are part of single bidRequest BidRequest.builder() .id("bidRequestId") - .imp(asList( - // verify banner and video media types are preserved in a single imp - Imp.builder() - .id("impId1") - .tagid("1") - .banner(Banner.builder().w(320).h(200).build()) - .video(Video.builder().maxduration(10).build()) - .ext(mapper.valueToTree(ExtImpOpenx.builder().build())).build(), - // verify banner and native media types are preserved in a single imp + .imp(singletonList( Imp.builder() - .id("impId2") - .tagid("2") - .banner(Banner.builder().w(300).h(150).build()) - .xNative(Native.builder().request("{\"version\":1}").build()) + .id("goodImp") + .banner(Banner.builder().build()) + .tagid("555555") .ext(mapper.valueToTree(ExtImpOpenx.builder().build())) .build())) .ext(jacksonMapper.fillExtension( ExtRequest.empty(), - OpenxRequestExt.of(null, null, "hb_pbs_1.0.0"))) - .user(User.builder() - .ext(ExtUser.builder().consent("consent").build()) - .build()) - .regs(Regs.builder().coppa(0).ext(ExtRegs.of(1, null, null, null)).build()) + OpenxRequestExt.of("se-demo-d.openx.net", "PLATFORM", "hb_pbs_1.0.0"))) .build()); } + @Test + public void makeHttpRequestsShouldAttachRewardedVideoExtWhenImpHasBothBannerAndVideo() { + // given + final BidRequest bidRequest = BidRequest.builder() + .id("bidRequestId") + .imp(singletonList(Imp.builder() + .id("impId1") + .banner(Banner.builder().build()) + .video(Video.builder().build()) + .ext(mapper.valueToTree( + ExtPrebid.of( + ExtImpPrebid.builder().isRewardedInventory(1).build(), + ExtImpOpenx.builder().unit("1").build()))) + .build())) + .build(); + + // when + final Result>> result = target.makeHttpRequests(bidRequest); + + // then + assertThat(result.getErrors()).isEmpty(); + assertThat(result.getValue()).hasSize(1) + .extracting(httpRequest -> mapper.readValue(httpRequest.getBody(), BidRequest.class)) + .flatExtracting(BidRequest::getImp) + .containsExactly(Imp.builder() + .id("impId1") + .tagid("1") + .banner(Banner.builder().build()) + .video(Video.builder().ext(mapper.valueToTree(OpenxVideoExt.of(1))).build()) + .ext(mapper.valueToTree(ExtImpOpenx.builder().build())) + .build()); + } + + @Test + public void makeHttpRequestsShouldNotAttachRewardedVideoExtWhenImpHasNoVideo() { + // given + final BidRequest bidRequest = BidRequest.builder() + .id("bidRequestId") + .imp(singletonList(Imp.builder() + .id("impId1") + .banner(Banner.builder().build()) + .ext(mapper.valueToTree( + ExtPrebid.of( + ExtImpPrebid.builder().isRewardedInventory(1).build(), + ExtImpOpenx.builder().unit("1").build()))) + .build())) + .build(); + + // when + final Result>> result = target.makeHttpRequests(bidRequest); + + // then + assertThat(result.getErrors()).isEmpty(); + assertThat(result.getValue()).hasSize(1) + .extracting(httpRequest -> mapper.readValue(httpRequest.getBody(), BidRequest.class)) + .flatExtracting(BidRequest::getImp) + .containsExactly(Imp.builder() + .id("impId1") + .tagid("1") + .banner(Banner.builder().build()) + .ext(mapper.valueToTree(ExtImpOpenx.builder().build())) + .build()); + } + @Test public void makeHttpRequestsShouldPassThroughImpExt() { // given