From 39c7a9f21b28c81170984e0455aacc40ad433403 Mon Sep 17 00:00:00 2001 From: osulzhenko <125548596+osulzhenko@users.noreply.github.com> Date: Wed, 21 Aug 2024 09:11:38 +0300 Subject: [PATCH] Core: Remove all imp.ext.prebid.imp (#3378) --- .../prebid/server/auction/ImpAdjuster.java | 11 ++-- .../functional/tests/ImpRequestSpec.groovy | 28 +++++++++ .../server/auction/ImpAdjusterTest.java | 57 ++++++------------- 3 files changed, 51 insertions(+), 45 deletions(-) diff --git a/src/main/java/org/prebid/server/auction/ImpAdjuster.java b/src/main/java/org/prebid/server/auction/ImpAdjuster.java index 32ecff524c5..86e581b06e8 100644 --- a/src/main/java/org/prebid/server/auction/ImpAdjuster.java +++ b/src/main/java/org/prebid/server/auction/ImpAdjuster.java @@ -41,18 +41,17 @@ public Imp adjust(Imp originalImp, String bidder, BidderAliases bidderAliases, L final JsonNode bidderNode = getBidderNode(bidder, bidderAliases, impExtPrebidImp); if (bidderNode == null || bidderNode.isEmpty()) { + removeImpExtPrebidImp(originalImp.getExt()); return originalImp; } - // remove circular references according to the requirements removeExtPrebidBidder(bidderNode); try { final JsonNode originalImpNode = jacksonMapper.mapper().valueToTree(originalImp); final JsonNode mergedImpNode = jsonMerger.merge(bidderNode, originalImpNode); - // clean up merged imp.ext.prebid.imp - removeImpExtPrebidImp(mergedImpNode); + removeImpExtPrebidImp(mergedImpNode.get(IMP_EXT)); final Imp resultImp = jacksonMapper.mapper().convertValue(mergedImpNode, Imp.class); @@ -61,6 +60,7 @@ public Imp adjust(Imp originalImp, String bidder, BidderAliases bidderAliases, L } catch (Exception e) { debugMessages.add("imp.ext.prebid.imp.%s can not be merged into original imp [id=%s], reason: %s" .formatted(bidder, originalImp.getId(), e.getMessage())); + removeImpExtPrebidImp(originalImp.getExt()); return originalImp; } } @@ -90,9 +90,8 @@ private static void removeExtPrebidBidder(JsonNode bidderNode) { .ifPresent(ext -> ext.remove(EXT_PREBID_BIDDER)); } - private static void removeImpExtPrebidImp(JsonNode mergedImpNode) { - Optional.ofNullable(mergedImpNode.get(IMP_EXT)) - .map(extNode -> extNode.get(EXT_PREBID)) + private static void removeImpExtPrebidImp(JsonNode impExt) { + Optional.ofNullable(impExt.get(EXT_PREBID)) .map(ObjectNode.class::cast) .ifPresent(prebid -> prebid.remove(EXT_PREBID_IMP)); } diff --git a/src/test/groovy/org/prebid/server/functional/tests/ImpRequestSpec.groovy b/src/test/groovy/org/prebid/server/functional/tests/ImpRequestSpec.groovy index bf34515eb04..9071eea8194 100644 --- a/src/test/groovy/org/prebid/server/functional/tests/ImpRequestSpec.groovy +++ b/src/test/groovy/org/prebid/server/functional/tests/ImpRequestSpec.groovy @@ -13,6 +13,7 @@ import static org.prebid.server.functional.model.bidder.BidderName.ALIAS_CAMEL_C import static org.prebid.server.functional.model.bidder.BidderName.EMPTY import static org.prebid.server.functional.model.bidder.BidderName.GENERIC import static org.prebid.server.functional.model.bidder.BidderName.GENERIC_CAMEL_CASE +import static org.prebid.server.functional.model.bidder.BidderName.RUBICON import static org.prebid.server.functional.model.bidder.BidderName.UNKNOWN import static org.prebid.server.functional.model.bidder.BidderName.WILDCARD import static org.prebid.server.functional.model.response.auction.ErrorType.PREBID @@ -156,6 +157,33 @@ class ImpRequestSpec extends BaseSpec { bidderName << [WILDCARD, UNKNOWN] } + def "PBS shouldn't update imp fields and without warning when imp.ext.prebid.imp contain not applicable bidder"() { + given: "Default basic BidRequest" + def impPmp = Pmp.defaultPmp + def bidRequest = BidRequest.defaultBidRequest.tap { + imp.first.tap { + pmp = impPmp + ext.prebid.imp = [(RUBICON): new Imp(pmp: Pmp.defaultPmp)] + } + } + + when: "Requesting PBS auction" + def response = defaultPbsServiceWithAlias.sendAuctionRequest(bidRequest) + + then: "Bid response should not contain warning" + assert !response?.ext?.warnings + + and: "BidderRequest should contain pmp from original imp" + def bidderRequest = bidder.getBidderRequest(bidRequest.id) + assert bidderRequest.imp.pmp == [impPmp] + + and: "PBS should remove imp.ext.prebid.imp from bidderRequest" + assert !bidderRequest?.imp?.first?.ext?.prebid?.imp + + and: "PBS should remove imp.ext.prebid.bidder from bidderRequest" + assert !bidderRequest?.imp?.first?.ext?.prebid?.bidder + } + def "PBS should validate imp and add proper warning when imp.ext.prebid.imp contain invalid ortb data"() { given: "BidRequest with invalid config for ext.prebid.imp" def impPmp = Pmp.defaultPmp diff --git a/src/test/java/org/prebid/server/auction/ImpAdjusterTest.java b/src/test/java/org/prebid/server/auction/ImpAdjusterTest.java index 4f8e069499c..717ca62dedd 100644 --- a/src/test/java/org/prebid/server/auction/ImpAdjusterTest.java +++ b/src/test/java/org/prebid/server/auction/ImpAdjusterTest.java @@ -78,24 +78,7 @@ public void adjustShouldReturnOriginalImpWhenImpExtPrebidImpIsAbsent() { } @Test - public void adjustShouldReturnOriginalImpWhenImpExtPrebidImpDoesNotHaveRequestedBidder() { - // given - final Imp givenImp = Imp.builder() - .ext(mapper.createObjectNode().set("prebid", mapper.createObjectNode() - .set("imp", mapper.createObjectNode().set("anotherBidder", mapper.createObjectNode())))) - .build(); - final List debugMessages = new ArrayList<>(); - - // when - final Imp result = target.adjust(givenImp, "someBidder", bidderAliases, debugMessages); - - // then - assertThat(result).isSameAs(givenImp); - assertThat(debugMessages).isEmpty(); - } - - @Test - public void adjustShouldReturnOriginalImpWhenImpExtPrebidImpHasEmptyBidder() { + public void adjustShouldRemoveExpImpFromOriginalImpWhenImpExtPrebidImpHasEmptyBidder() { // given final Imp givenImp = Imp.builder() .ext(mapper.createObjectNode().set("prebid", mapper.createObjectNode() @@ -107,24 +90,10 @@ public void adjustShouldReturnOriginalImpWhenImpExtPrebidImpHasEmptyBidder() { final Imp result = target.adjust(givenImp, "someBidder", bidderAliases, debugMessages); // then - assertThat(result).isSameAs(givenImp); - assertThat(debugMessages).isEmpty(); - } - - @Test - public void adjustShouldReturnOriginalImpWhenMergedImpNodeIsEmpty() { - // given - final Imp givenImp = Imp.builder() - .ext(mapper.createObjectNode().set("prebid", mapper.createObjectNode() - .set("imp", mapper.createObjectNode().set("someBidder", mapper.createObjectNode())))) - .build(); - final List debugMessages = new ArrayList<>(); - - // when - final Imp result = target.adjust(givenImp, "someBidder", bidderAliases, debugMessages); + final Imp expectedImp = givenImp.toBuilder() + .ext(mapper.createObjectNode().set("prebid", mapper.createObjectNode())).build(); - // then - assertThat(result).isSameAs(givenImp); + assertThat(result).isEqualTo(expectedImp); assertThat(debugMessages).isEmpty(); } @@ -214,7 +183,7 @@ public void resolveImpShouldMergeBidderSpecificImpIntoOriginalImpCaseAliasBidder } @Test - public void resolveImpShouldReturnOriginalImpWhenResultingImpValidationFailed() throws ValidationException { + public void resolveImpShouldReturnImpWithoutExpImpWhenResultingImpValidationFailed() throws ValidationException { // given doThrow(new ValidationException("imp validation failed")).when(impValidator).validateImp(any()); @@ -231,14 +200,19 @@ public void resolveImpShouldReturnOriginalImpWhenResultingImpValidationFailed() final Imp result = target.adjust(givenImp, "someBidder", bidderAliases, debugMessages); // then - assertThat(result).isSameAs(givenImp); + final Imp expectedImp = givenImp.toBuilder() + .ext(mapper.createObjectNode().put("originAttr", "originValue") + .set("prebid", mapper.createObjectNode().put("prebidOriginAttr", "prebidOriginValue"))) + .build(); + + assertThat(result).isEqualTo(expectedImp); assertThat(debugMessages).containsOnly( "imp.ext.prebid.imp.someBidder can not be merged into original imp [id=impId], " + "reason: imp validation failed"); } @Test - public void resolveImpShouldReturnOriginalImpWhenMergingFailed() { + public void resolveImpShouldReturnImpWithoutExpWhenMergingFailed() { // given final ObjectNode invalidBidderImp = mapper.createObjectNode() .put("bidfloor", "2.0") @@ -251,7 +225,12 @@ public void resolveImpShouldReturnOriginalImpWhenMergingFailed() { final Imp result = target.adjust(givenImp, "someBidder", bidderAliases, debugMessages); // then - assertThat(result).isSameAs(givenImp); + final Imp expectedImp = givenImp.toBuilder() + .ext(mapper.createObjectNode().put("originAttr", "originValue") + .set("prebid", mapper.createObjectNode().put("prebidOriginAttr", "prebidOriginValue"))) + .build(); + + assertThat(result).isEqualTo(expectedImp); assertThat(debugMessages).hasSize(1).first() .satisfies(message -> assertThat(message).startsWith( "imp.ext.prebid.imp.someBidder can not be merged into original imp [id=impId],"