Fix setting Rubicon size id using video placement (#1103)

prebid · Aug 10, 2021 · c70d454 · c70d454
1 parent 09efbb4
commit c70d454
Show file tree

Hide file tree

Showing 7 changed files with 66 additions and 54 deletions.
diff --git a/src/main/java/org/prebid/server/auction/BidderAliases.java b/src/main/java/org/prebid/server/auction/BidderAliases.java
@@ -1,9 +1,9 @@
 package org.prebid.server.auction;
 
+import org.apache.commons.collections4.MapUtils;
 import org.apache.commons.lang3.ObjectUtils;
 import org.prebid.server.bidder.BidderCatalog;
 
-import java.util.Collections;
 import java.util.Map;
 import java.util.Objects;
 
@@ -21,8 +21,8 @@ public class BidderAliases {
     private BidderAliases(
             Map<String, String> aliasToBidder, Map<String, Integer> aliasToVendorId, BidderCatalog bidderCatalog) {
 
-        this.aliasToBidder = ObjectUtils.firstNonNull(aliasToBidder, Collections.emptyMap());
-        this.aliasToVendorId = ObjectUtils.firstNonNull(aliasToVendorId, Collections.emptyMap());
+        this.aliasToBidder = MapUtils.emptyIfNull(aliasToBidder);
+        this.aliasToVendorId = MapUtils.emptyIfNull(aliasToVendorId);
         this.bidderCatalog = Objects.requireNonNull(bidderCatalog);
     }
 
@@ -43,7 +43,7 @@ public boolean isAliasDefined(String alias) {
     public String resolveBidder(String aliasOrBidder) {
         return aliasToBidder.containsKey(aliasOrBidder)
                 ? aliasToBidder.get(aliasOrBidder)
-                : ObjectUtils.firstNonNull(resolveBidderViaCatalog(aliasOrBidder), aliasOrBidder);
+                : ObjectUtils.defaultIfNull(resolveBidderViaCatalog(aliasOrBidder), aliasOrBidder);
     }
 
     public Integer resolveAliasVendorId(String alias) {

diff --git a/src/main/java/org/prebid/server/auction/ExchangeService.java b/src/main/java/org/prebid/server/auction/ExchangeService.java
@@ -405,7 +405,7 @@ private Map<String, User> prepareUsers(List<String> bidders,
 
         final Map<String, User> bidderToUser = new HashMap<>();
         for (String bidder : bidders) {
-            final ExtBidderConfigFpd fpdConfig = ObjectUtils.firstNonNull(biddersToConfigs.get(bidder),
+            final ExtBidderConfigFpd fpdConfig = ObjectUtils.defaultIfNull(biddersToConfigs.get(bidder),
                     biddersToConfigs.get(ALL_BIDDERS_CONFIG));
 
             final boolean useFirstPartyData = firstPartyDataBidders == null || firstPartyDataBidders.contains(bidder);
@@ -578,7 +578,7 @@ private BidderRequest createBidderRequest(BidderPrivacyResult bidderPrivacyResul
         final List<String> firstPartyDataBidders = firstPartyDataBidders(bidRequest.getExt());
         final boolean useFirstPartyData = firstPartyDataBidders == null || firstPartyDataBidders.contains(bidder);
 
-        final ExtBidderConfigFpd fpdConfig = ObjectUtils.firstNonNull(biddersToConfigs.get(bidder),
+        final ExtBidderConfigFpd fpdConfig = ObjectUtils.defaultIfNull(biddersToConfigs.get(bidder),
                 biddersToConfigs.get(ALL_BIDDERS_CONFIG));
 
         final Site bidRequestSite = bidRequest.getSite();

diff --git a/src/main/java/org/prebid/server/bidder/rubicon/RubiconBidder.java b/src/main/java/org/prebid/server/bidder/rubicon/RubiconBidder.java
@@ -386,7 +386,7 @@ private Imp makeImp(Imp imp, ExtImpPrebid extPrebid, ExtImpRubicon extRubicon, S
         if (isVideo(imp)) {
             builder
                     .banner(null)
-                    .video(makeVideo(imp, site, extRubicon.getVideo(), extPrebid));
+                    .video(makeVideo(imp, extRubicon.getVideo(), extPrebid, referer(site)));
         } else {
             builder
                     .banner(makeBanner(imp, overriddenSizes(extRubicon)))
@@ -656,39 +656,45 @@ private static boolean isFullyPopulatedVideo(Video video) {
                 && video.getLinearity() != null && video.getApi() != null;
     }
 
-    private Video makeVideo(Imp imp, Site site, RubiconVideoParams rubiconVideoParams, ExtImpPrebid prebidImpExt) {
-        final Video video = imp.getVideo();
-        final String impId = imp.getId();
-        final String referer = site != null ? site.getPage() : null;
-        final String videoType = prebidImpExt != null && prebidImpExt.getIsRewardedInventory() != null
-                && prebidImpExt.getIsRewardedInventory() == 1 ? "rewarded" : null;
+    private static String referer(Site site) {
+        return site != null ? site.getPage() : null;
+    }
 
-        if (rubiconVideoParams == null && videoType == null) {
-            return video;
-        }
+    private Video makeVideo(Imp imp, RubiconVideoParams rubiconVideoParams, ExtImpPrebid prebidImpExt, String referer) {
+        final Video video = imp.getVideo();
 
         final Integer skip = rubiconVideoParams != null ? rubiconVideoParams.getSkip() : null;
         final Integer skipDelay = rubiconVideoParams != null ? rubiconVideoParams.getSkipdelay() : null;
         final Integer sizeId = rubiconVideoParams != null ? rubiconVideoParams.getSizeId() : null;
+
         final Integer resolvedSizeId = sizeId == null || sizeId == 0
-                ? resolveVideoSizeId(video.getPlacement(), imp.getInstl()) : sizeId;
-        validateVideoSizeId(resolvedSizeId, referer, impId);
+                ? resolveVideoSizeId(video.getPlacement(), imp.getInstl())
+                : sizeId;
+        validateVideoSizeId(resolvedSizeId, referer, imp.getId());
+
+        final String videoType = prebidImpExt != null && prebidImpExt.getIsRewardedInventory() != null
+                && prebidImpExt.getIsRewardedInventory() == 1 ? "rewarded" : null;
+
+        // optimization for empty ext params
+        if (skip == null && skipDelay == null && resolvedSizeId == null && videoType == null) {
+            return video;
+        }
 
         return video.toBuilder()
                 .ext(mapper.mapper().valueToTree(
                         RubiconVideoExt.of(skip, skipDelay, RubiconVideoExtRp.of(resolvedSizeId), videoType)))
                 .build();
     }
 
-    private void validateVideoSizeId(Integer resolvedSizeId, String referer, String impId) {
+    private static void validateVideoSizeId(Integer resolvedSizeId, String referer, String impId) {
         // log only 1% of cases to monitor how often video impressions does not have size id
         if (resolvedSizeId == null) {
             MISSING_VIDEO_SIZE_LOGGER.warn(String.format("RP adapter: video request with no size_id. Referrer URL = %s,"
                     + " impId = %s", referer, impId), 0.01d);
         }
     }
 
-    private Integer resolveVideoSizeId(Integer placement, Integer instl) {
+    private static Integer resolveVideoSizeId(Integer placement, Integer instl) {
         if (placement != null) {
             if (placement == 1) {
                 return 201;

diff --git a/src/main/java/org/prebid/server/bidder/triplelift/TripleliftBidder.java b/src/main/java/org/prebid/server/bidder/triplelift/TripleliftBidder.java
@@ -89,7 +89,7 @@ private Imp modifyImp(Imp imp) throws PreBidException {
         final ExtImpTriplelift impExt = parseImpExt(imp);
         return imp.toBuilder()
                 .tagid(impExt.getInventoryCode())
-                .bidfloor(ObjectUtils.firstNonNull(impExt.getFloor(), imp.getBidfloor()))
+                .bidfloor(ObjectUtils.defaultIfNull(impExt.getFloor(), imp.getBidfloor()))
                 .build();
     }
 

diff --git a/src/main/java/org/prebid/server/json/JsonMerger.java b/src/main/java/org/prebid/server/json/JsonMerger.java
@@ -48,7 +48,7 @@ public <T> T merge(T originalObject, String storedData, String id, Class<T> clas
 
     public <T> T merge(T originalObject, T mergingObject, Class<T> classToCast) {
         if (!ObjectUtils.allNotNull(originalObject, mergingObject)) {
-            return ObjectUtils.firstNonNull(originalObject, mergingObject);
+            return ObjectUtils.defaultIfNull(originalObject, mergingObject);
         }
 
         final JsonNode originJsonNode = mapper.mapper().valueToTree(originalObject);

diff --git a/src/main/java/org/prebid/server/settings/model/Account.java b/src/main/java/org/prebid/server/settings/model/Account.java
@@ -38,20 +38,20 @@ public class Account {
 
     public Account merge(Account another) {
         return Account.builder()
-                .id(ObjectUtils.firstNonNull(id, another.id))
-                .priceGranularity(ObjectUtils.firstNonNull(priceGranularity, another.priceGranularity))
-                .bannerCacheTtl(ObjectUtils.firstNonNull(bannerCacheTtl, another.bannerCacheTtl))
-                .videoCacheTtl(ObjectUtils.firstNonNull(videoCacheTtl, another.videoCacheTtl))
-                .eventsEnabled(ObjectUtils.firstNonNull(eventsEnabled, another.eventsEnabled))
-                .enforceCcpa(ObjectUtils.firstNonNull(enforceCcpa, another.enforceCcpa))
-                .gdpr(ObjectUtils.firstNonNull(gdpr, another.gdpr))
-                .analyticsSamplingFactor(ObjectUtils.firstNonNull(
+                .id(ObjectUtils.defaultIfNull(id, another.id))
+                .priceGranularity(ObjectUtils.defaultIfNull(priceGranularity, another.priceGranularity))
+                .bannerCacheTtl(ObjectUtils.defaultIfNull(bannerCacheTtl, another.bannerCacheTtl))
+                .videoCacheTtl(ObjectUtils.defaultIfNull(videoCacheTtl, another.videoCacheTtl))
+                .eventsEnabled(ObjectUtils.defaultIfNull(eventsEnabled, another.eventsEnabled))
+                .enforceCcpa(ObjectUtils.defaultIfNull(enforceCcpa, another.enforceCcpa))
+                .gdpr(ObjectUtils.defaultIfNull(gdpr, another.gdpr))
+                .analyticsSamplingFactor(ObjectUtils.defaultIfNull(
                         analyticsSamplingFactor, another.analyticsSamplingFactor))
-                .truncateTargetAttr(ObjectUtils.firstNonNull(truncateTargetAttr, another.truncateTargetAttr))
-                .defaultIntegration(ObjectUtils.firstNonNull(defaultIntegration, another.defaultIntegration))
-                .analyticsConfig(ObjectUtils.firstNonNull(analyticsConfig, another.analyticsConfig))
-                .bidValidations(ObjectUtils.firstNonNull(bidValidations, another.bidValidations))
-                .status(ObjectUtils.firstNonNull(status, another.status))
+                .truncateTargetAttr(ObjectUtils.defaultIfNull(truncateTargetAttr, another.truncateTargetAttr))
+                .defaultIntegration(ObjectUtils.defaultIfNull(defaultIntegration, another.defaultIntegration))
+                .analyticsConfig(ObjectUtils.defaultIfNull(analyticsConfig, another.analyticsConfig))
+                .bidValidations(ObjectUtils.defaultIfNull(bidValidations, another.bidValidations))
+                .status(ObjectUtils.defaultIfNull(status, another.status))
                 .build();
     }
 

diff --git a/src/test/java/org/prebid/server/bidder/rubicon/RubiconBidderTest.java b/src/test/java/org/prebid/server/bidder/rubicon/RubiconBidderTest.java
@@ -421,32 +421,34 @@ public void makeHttpRequestsShouldCreateVideoRequestIfImpHasBannerAndVideoButAll
     }
 
     @Test
-    public void shouldSetSizeIdTo201IfplacementIs1IfSizeIdIsNotPresent() {
+    public void shouldSetSizeIdTo201IfPlacementIs1AndSizeIdIsNotPresent() {
         // given
         final BidRequest bidRequest = givenBidRequest(
                 builder -> builder.instl(1).video(Video.builder().placement(1).build()),
-                builder -> builder.video(RubiconVideoParams.builder().skip(5).skipdelay(10).sizeId(null).build()));
+                builder -> builder.video(RubiconVideoParams.builder().sizeId(null).build()));
 
         // when
         final Result<List<HttpRequest<BidRequest>>> result = rubiconBidder.makeHttpRequests(bidRequest);
 
         // then
         assertThat(result.getErrors()).isEmpty();
         assertThat(result.getValue()).hasSize(1).doesNotContainNull()
-            .extracting(httpRequest -> mapper.readValue(httpRequest.getBody(), BidRequest.class))
-            .flatExtracting(BidRequest::getImp)
-            .extracting(Imp::getVideo).doesNotContainNull()
-            .extracting(Video::getExt).doesNotContainNull()
-            .extracting(ext -> mapper.treeToValue(ext, RubiconVideoExt.class))
-            .containsOnly(RubiconVideoExt.of(5, 10, RubiconVideoExtRp.of(201), null));
+                .extracting(httpRequest -> mapper.readValue(httpRequest.getBody(), BidRequest.class))
+                .flatExtracting(BidRequest::getImp)
+                .extracting(Imp::getVideo).doesNotContainNull()
+                .extracting(Video::getExt).doesNotContainNull()
+                .extracting(ext -> mapper.treeToValue(ext, RubiconVideoExt.class))
+                .extracting(RubiconVideoExt::getRp)
+                .extracting(RubiconVideoExtRp::getSizeId)
+                .containsOnly(201);
     }
 
     @Test
-    public void shouldSetSizeIdTo203IfplacementIs3IfSizeIdIsNotPresent() {
+    public void shouldSetSizeIdTo203IfPlacementIs3AndSizeIdIsNotPresent() {
         // given
         final BidRequest bidRequest = givenBidRequest(
                 builder -> builder.instl(1).video(Video.builder().placement(3).build()),
-                builder -> builder.video(RubiconVideoParams.builder().skip(5).skipdelay(10).sizeId(null).build()));
+                builder -> builder.video(RubiconVideoParams.builder().sizeId(null).build()));
 
         // when
         final Result<List<HttpRequest<BidRequest>>> result = rubiconBidder.makeHttpRequests(bidRequest);
@@ -459,28 +461,32 @@ public void shouldSetSizeIdTo203IfplacementIs3IfSizeIdIsNotPresent() {
                 .extracting(Imp::getVideo).doesNotContainNull()
                 .extracting(Video::getExt).doesNotContainNull()
                 .extracting(ext -> mapper.treeToValue(ext, RubiconVideoExt.class))
-                .containsOnly(RubiconVideoExt.of(5, 10, RubiconVideoExtRp.of(203), null));
+                .extracting(RubiconVideoExt::getRp)
+                .extracting(RubiconVideoExtRp::getSizeId)
+                .containsOnly(203);
     }
 
     @Test
-    public void shouldCalculateSizeIdUsingInstlIfPlacementAndSizeIdIsNotPresent() {
+    public void shouldSetSizeIdTo202UsingInstlFlagIfPlacementAndSizeIdAreNotPresent() {
         // given
         final BidRequest bidRequest = givenBidRequest(
                 builder -> builder.instl(1).video(Video.builder().placement(null).build()),
-                builder -> builder.video(RubiconVideoParams.builder().skip(5).skipdelay(10).sizeId(null).build()));
+                builder -> builder.video(RubiconVideoParams.builder().sizeId(null).build()));
 
         // when
         final Result<List<HttpRequest<BidRequest>>> result = rubiconBidder.makeHttpRequests(bidRequest);
 
         // then
         assertThat(result.getErrors()).isEmpty();
         assertThat(result.getValue()).hasSize(1).doesNotContainNull()
-            .extracting(httpRequest -> mapper.readValue(httpRequest.getBody(), BidRequest.class))
-            .flatExtracting(BidRequest::getImp)
-            .extracting(Imp::getVideo).doesNotContainNull()
-            .extracting(Video::getExt).doesNotContainNull()
-            .extracting(ext -> mapper.treeToValue(ext, RubiconVideoExt.class))
-            .containsOnly(RubiconVideoExt.of(5, 10, RubiconVideoExtRp.of(202), null));
+                .extracting(httpRequest -> mapper.readValue(httpRequest.getBody(), BidRequest.class))
+                .flatExtracting(BidRequest::getImp)
+                .extracting(Imp::getVideo).doesNotContainNull()
+                .extracting(Video::getExt).doesNotContainNull()
+                .extracting(ext -> mapper.treeToValue(ext, RubiconVideoExt.class))
+                .extracting(RubiconVideoExt::getRp)
+                .extracting(RubiconVideoExtRp::getSizeId)
+                .containsOnly(202);
     }
 
     @Test