Rubicon: simplify logic for truncating segtaxes (#3796)

CTMBNara · web-flow · commit 65fd3e1d8bcf · 2025-03-05T11:05:50.000+01:00
diff --git a/src/main/java/org/prebid/server/bidder/rubicon/RubiconBidder.java b/src/main/java/org/prebid/server/bidder/rubicon/RubiconBidder.java
@@ -29,7 +29,6 @@
 import io.vertx.core.MultiMap;
 import io.vertx.core.http.HttpMethod;
 import org.apache.commons.collections4.CollectionUtils;
-import org.apache.commons.collections4.MapUtils;
 import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.math.NumberUtils;
@@ -106,19 +105,16 @@
 import org.prebid.server.proto.openrtb.ext.response.ExtBidPrebidMeta;
 import org.prebid.server.util.BidderUtil;
 import org.prebid.server.util.HttpUtil;
-import org.prebid.server.util.ListUtil;
 import org.prebid.server.util.ObjectUtil;
 import org.prebid.server.version.PrebidVersionProvider;
 
 import java.math.BigDecimal;
 import java.net.URISyntaxException;
-import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Base64;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.Deque;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -130,6 +126,7 @@
 import java.util.Set;
 import java.util.function.Function;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
 
 public class RubiconBidder implements Bidder<BidRequest> {
@@ -814,10 +811,6 @@ private static String getTextValueFromNodeByPath(JsonNode node, String path) {
         return nodeByPath != null && nodeByPath.isTextual() ? nodeByPath.textValue() : null;
     }
 
-    private static String getTextValueFromNode(JsonNode node) {
-        return node != null && node.isTextual() ? node.textValue() : null;
-    }
-
     private void populateFirstPartyDataAttributes(ObjectNode sourceNode, ObjectNode targetNode) {
         if (sourceNode == null || sourceNode.isNull()) {
             return;
@@ -1259,97 +1252,40 @@ private void mergeFirstPartyDataFromUser(ExtUser userExt, ObjectNode result) {
         }
     }
 
-    private static void enrichWithIabAndSegtaxAttribute(ObjectNode target, List<Data> data, Set<Integer> segtaxValues) {
-        final Map<Integer, Deque<Segment>> segments = CollectionUtils.emptyIfNull(data).stream()
+    private static void enrichWithIabAndSegtaxAttribute(ObjectNode target, List<Data> data, Set<Integer> iabTaxes) {
+        CollectionUtils.emptyIfNull(data).stream()
                 .filter(Objects::nonNull)
-                .map(RubiconBidder::getValidSegments)
-                .filter(Objects::nonNull)
-                .collect(Collectors.toMap(
-                        Map.Entry::getKey,
-                        Map.Entry::getValue,
-                        (first, second) -> {
-                            first.addAll(second);
-                            return first;
-                        }));
-
-        if (MapUtils.isEmpty(segments)) {
-            return;
-        }
-
-        final Map<Integer, List<String>> relevantSegments = pickRelevantSegments(segments);
-        final Map<String, List<String>> resultSegments = groupBySegtaxValues(relevantSegments, segtaxValues);
-
-        resultSegments.forEach((segtaxValue, segmentIds) -> {
-            final ArrayNode array = target.putArray(segtaxValue);
-            segmentIds.forEach(array::add);
-        });
+                .flatMap(dataEntry -> extractTaxToSegmentId(dataEntry, iabTaxes))
+                .limit(MAX_NUMBER_OF_SEGMENTS)
+                .forEach(entry -> getArrayNodeOrCreate(target, entry.getKey()).add(entry.getValue()));
     }
 
-    private static Map<String, List<String>> groupBySegtaxValues(Map<Integer, List<String>> segments,
-                                                                 Set<Integer> segtaxValues) {
-
-        return segments.entrySet().stream()
-                .collect(Collectors.toMap(
-                        entry -> resolveSegmentName(entry.getKey(), segtaxValues),
-                        Map.Entry::getValue,
-                        ListUtil::union));
-    }
-
-    private static String resolveSegmentName(Integer taxonomyId, Set<Integer> segtaxValues) {
-        return segtaxValues.contains(taxonomyId) ? SEGTAX_IAB : SEGTAX_TAX + taxonomyId;
-    }
-
-    private static Map.Entry<Integer, Deque<Segment>> getValidSegments(Data data) {
+    private static Stream<Map.Entry<String, String>> extractTaxToSegmentId(Data data, Set<Integer> iabTaxes) {
         final ObjectNode ext = data.getExt();
         final JsonNode taxonomyId = ext != null ? ext.get(SEGTAX) : null;
         if (taxonomyId == null || !taxonomyId.isInt()) {
-            return null;
+            return Stream.empty();
         }
 
-        final Deque<Segment> segments = getValidOnlySegments(data.getSegment());
-        return CollectionUtils.isNotEmpty(segments) ? Map.entry(taxonomyId.intValue(), segments) : null;
+        final String taxKey = resolveTaxName(taxonomyId.intValue(), iabTaxes);
+        return CollectionUtils.emptyIfNull(data.getSegment()).stream()
+                .filter(Objects::nonNull)
+                .map(Segment::getId)
+                .filter(StringUtils::isNotBlank)
+                .map(id -> Map.entry(taxKey, id));
     }
 
-    private static Deque<Segment> getValidOnlySegments(List<Segment> segments) {
-        return CollectionUtils.isNotEmpty(segments)
-                ? segments.stream()
-                .filter(segment -> StringUtils.isNotBlank(segment.getId()))
-                .collect(Collectors.toCollection(ArrayDeque::new))
-                : null;
+    private static String resolveTaxName(Integer taxonomyId, Set<Integer> iabTaxes) {
+        return iabTaxes.contains(taxonomyId) ? SEGTAX_IAB : SEGTAX_TAX + taxonomyId;
     }
 
-    private static Map<Integer, List<String>> pickRelevantSegments(final Map<Integer, Deque<Segment>> segments) {
-        final Map<Integer, List<String>> result = new HashMap<>();
-        final List<Integer> segmentsKeys = new ArrayList<>(segments.keySet());
-
-        int i = 0;
-        int consumedSegmentsCount = 0;
-
-        while (consumedSegmentsCount < MAX_NUMBER_OF_SEGMENTS && !segmentsKeys.isEmpty()) {
-            final int segmentsIndex = i % segmentsKeys.size();
-            final Integer segmentKey = segmentsKeys.get(segmentsIndex);
-            final Deque<Segment> currentSegments = segments.get(segmentKey);
-
-            final Segment lastSegment = currentSegments.pollLast();
-            result.computeIfAbsent(segmentKey, key -> new ArrayList<>()).add(lastSegment.getId());
-            consumedSegmentsCount++;
-
-            if (currentSegments.isEmpty()) {
-                segmentsKeys.remove(segmentKey);
-                i--;
-            }
-            i++;
+    private static ArrayNode getArrayNodeOrCreate(ObjectNode parent, String field) {
+        final JsonNode node = parent.get(field);
+        if (node == null || !node.isArray()) {
+            return parent.putArray(field);
         }
 
-        return result;
-    }
-
-    private static Segment getAndRemoveLastSegment(List<Segment> list) {
-        final int lastElementIndex = list.size() - 1;
-        final Segment lastSegment = list.get(lastElementIndex);
-        list.remove(lastElementIndex);
-
-        return lastSegment;
+        return (ArrayNode) node;
     }
 
     private void processWarnings(List<BidderError> errors, List<String> priceFloorsWarnings) {
diff --git a/src/test/java/org/prebid/server/bidder/rubicon/RubiconBidderTest.java b/src/test/java/org/prebid/server/bidder/rubicon/RubiconBidderTest.java
@@ -1119,34 +1119,6 @@ public void makeHttpRequestsShouldFillUserExtRpWithIabAttributeIfSegtaxEqualsFou
                                 .build()));
     }
 
-    @Test
-    public void makeHttpRequestsShouldFillUserExtRpWithSegtaxValuesWithSegtaxesFromEachData() {
-        // given
-        final List<Data> dataWithSegments = asList(givenTestDataWithSegmentEntries(3),
-                givenDataWithSegmentEntry(3, "Included_SegmentId_1"),
-                givenDataWithSegmentEntry(3, "Included_SegmentId_2"));
-
-        final BidRequest bidRequest = givenBidRequest(
-                builder -> builder.user(User.builder().data(dataWithSegments).build()),
-                builder -> builder.video(Video.builder().build()),
-                identity());
-
-        // when
-        final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);
-
-        // then
-        assertThat(result.getErrors()).isEmpty();
-        assertThat(result.getValue()).hasSize(1).doesNotContainNull()
-                .extracting(httpRequest -> mapper.readValue(httpRequest.getBody(), BidRequest.class))
-                .extracting(BidRequest::getUser).doesNotContainNull()
-                .extracting(User::getExt)
-                .extracting(userExt -> userExt.getProperty("rp"))
-                .extracting(rp -> rp.get("target"))
-                .extracting(target -> target.get("tax3"))
-                .flatExtracting(tax3 -> mapper.convertValue(tax3, List.class))
-                .contains("Included_SegmentId_1", "Included_SegmentId_2");
-    }
-
     @Test
     public void makeHttpRequestsShouldRemoveUserDataObject() {
         // given
@@ -1170,7 +1142,7 @@ public void makeHttpRequestsShouldRemoveUserDataObject() {
     }
 
     @Test
-    public void makeHttpRequestsShouldFillSiteExtRpWithSegtaxValuesWithNoMoreThanHundredEntriesWithEvenDistribution()
+    public void makeHttpRequestsShouldFillSiteExtRpWithSegtaxValuesWithNoMoreThanHundredEntriesFromDifferentSources()
             throws IOException {
 
         // given
@@ -1199,16 +1171,16 @@ public void makeHttpRequestsShouldFillSiteExtRpWithSegtaxValuesWithNoMoreThanHun
         final BidRequest capturedBidRequest = mapper.readValue(result.getValue().get(0).getBody(), BidRequest.class);
         final JsonNode targetNode = capturedBidRequest.getSite().getExt().getProperty("rp").get("target");
 
-        assertThat(targetNode.elements()).toIterable().hasSize(4);
+        assertThat(targetNode.elements()).toIterable().hasSize(3);
 
         final List<String> expectedIabValues = Streams.concat(
                         IntStream.range(1, 6).mapToObj(i -> "firstSegmentId_" + i),
                         IntStream.range(1, 5).mapToObj(i -> "secondSegmentId_" + i),
                         IntStream.range(1, 2).mapToObj(i -> "fifthSegmentId_" + i),
-                        IntStream.range(59, 101).mapToObj(i -> "sixthSegmentId_" + i))
+                        IntStream.range(1, 86).mapToObj(i -> "sixthSegmentId_" + i))
                 .toList();
 
-        assertThat(targetNode.get("iab").elements()).toIterable().hasSize(52)
+        assertThat(targetNode.get("iab").elements()).toIterable().hasSize(95)
                 .extracting(JsonNode::asText)
                 .containsExactlyInAnyOrderElementsOf(expectedIabValues);
 
@@ -1222,11 +1194,7 @@ public void makeHttpRequestsShouldFillSiteExtRpWithSegtaxValuesWithNoMoreThanHun
                 .extracting(JsonNode::asText)
                 .containsExactlyInAnyOrderElementsOf(expectedTax4Values);
 
-        final List<String> expectedTax7Values = IntStream.range(58, 101).mapToObj(i -> "seventhSegmentId_" + i)
-                .toList();
-        assertThat(targetNode.get("tax7").elements()).toIterable().hasSize(43)
-                .extracting(JsonNode::asText)
-                .containsExactlyInAnyOrderElementsOf(expectedTax7Values);
+        assertThat(targetNode.get("tax7")).isNull();
     }
 
     @Test
@@ -1262,7 +1230,7 @@ public void makeHttpRequestsShouldFillSiteExtRpWithSegtaxValuesWithNoMoreThanHun
     }
 
     @Test
-    public void makeHttpRequestsShouldFillUserExtRpWithSegtaxValuesWithNoMoreThanHundredEntriesWithEvenDistribution()
+    public void makeHttpRequestsShouldFillUserExtRpWithSegtaxValuesWithNoMoreThanHundredEntriesFromDifferentSources()
             throws IOException {
 
         // given
@@ -1289,7 +1257,7 @@ public void makeHttpRequestsShouldFillUserExtRpWithSegtaxValuesWithNoMoreThanHun
         final BidRequest capturedBidRequest = mapper.readValue(result.getValue().get(0).getBody(), BidRequest.class);
         final JsonNode targetNode = capturedBidRequest.getUser().getExt().getProperty("rp").get("target");
 
-        assertThat(targetNode.elements()).toIterable().hasSize(7);
+        assertThat(targetNode.elements()).toIterable().hasSize(6);
 
         final List<String> expectedIabValues = IntStream.range(1, 3).mapToObj(i -> "fourthSegmentId_" + i).toList();
         assertThat(targetNode.get("iab").elements()).toIterable()
@@ -1321,18 +1289,13 @@ public void makeHttpRequestsShouldFillUserExtRpWithSegtaxValuesWithNoMoreThanHun
                 .extracting(JsonNode::asText)
                 .containsExactlyInAnyOrderElementsOf(expectedTax5Values);
 
-        final List<String> expectedTax6Values = IntStream.range(59, 101).mapToObj(i -> "sixthSegmentId_" + i).toList();
+        final List<String> expectedTax6Values = IntStream.range(1, 86).mapToObj(i -> "sixthSegmentId_" + i).toList();
         assertThat(targetNode.get("tax6").elements()).toIterable()
-                .hasSize(42)
+                .hasSize(85)
                 .extracting(JsonNode::asText)
                 .containsExactlyInAnyOrderElementsOf(expectedTax6Values);
 
-        final List<String> expectedTax7Values = IntStream.range(58, 101).mapToObj(i -> "seventhSegmentId_" + i)
-                .toList();
-        assertThat(targetNode.get("tax7").elements()).toIterable()
-                .hasSize(43)
-                .extracting(JsonNode::asText)
-                .containsExactlyInAnyOrderElementsOf(expectedTax7Values);
+        assertThat(targetNode.get("tax7")).isNull();
 
     }
 
@@ -2118,9 +2081,9 @@ public void makeHttpRequestsShouldNotCreateUserExtTpIdWithAdServerEidSourceIfExt
         final BidRequest bidRequest = givenBidRequest(builder -> builder.user(User.builder()
                         .ext(ExtUser.builder()
                                 .eids(singletonList(Eid.builder()
-                                                .source("adserver.org")
-                                                .uids(singletonList(Uid.builder().id("id").build()))
-                                                .build()))
+                                        .source("adserver.org")
+                                        .uids(singletonList(Uid.builder().id("id").build()))
+                                        .build()))
                                 .build())
                         .build()),
                 builder -> builder.video(Video.builder().build()), identity());
@@ -2724,7 +2687,7 @@ public void makeHttpRequestsShouldMergeImpExtRubiconAndDataKeywordsToRubiconImpE
     }
 
     @Test
-    public void makeHttpRequestsShouldCopyDataSearchToRubiconImpExtRpTargetSearch() throws IOException {
+    public void makeHttpRequestsShouldCopyDataSearchToRubiconImpExtRpTargetSearch() {
         // given
         final BidRequest bidRequest = givenBidRequest(
                 identity(),
@@ -2970,8 +2933,7 @@ public void makeHttpRequestsShouldProcessMetricsAndFillViewabilityVendor() {
     }
 
     @Test
-    public void makeHttpRequestsShouldReturnOnlyLineItemRequestsWithExpectedFieldsWhenImpPmpDealsArePresent()
-            throws IOException {
+    public void makeHttpRequestsShouldReturnOnlyLineItemRequestsWithExpectedFieldsWhenImpPmpDealsArePresent() {
         // given
         final List<Deal> dealsList = asList(
                 Deal.builder().ext(mapper.valueToTree(ExtDeal.of(ExtDealLine.of(null, "123",
@@ -4074,16 +4036,6 @@ private static RubiconBid givenRubiconBid(UnaryOperator<RubiconBid.RubiconBidBui
         return bidCustomizer.apply(RubiconBid.builder()).build();
     }
 
-    private static Data givenTestDataWithSegmentEntries(Integer segtax) {
-        final List<Segment> segments = IntStream.range(0, 1000)
-                .mapToObj(index -> Segment.builder().id("segmentId_" + index).build())
-                .toList();
-        return Data.builder()
-                .segment(segments)
-                .ext(mapper.createObjectNode().put("segtax", segtax))
-                .build();
-    }
-
     private static ObjectNode givenImpExtRpTarget() {
         return mapper.createObjectNode()
                 .put("pbs_login", USERNAME)
diff --git a/src/test/resources/org/prebid/server/it/openrtb2/magnite/test-magnite-bid-request.json b/src/test/resources/org/prebid/server/it/openrtb2/magnite/test-magnite-bid-request.json
@@ -55,8 +55,8 @@
         "site_id": 3001,
         "target": {
           "iab": [
-            "segmentId2",
             "segmentId1",
+            "segmentId2",
             "segmentId3"
           ]
         }
diff --git a/src/test/resources/org/prebid/server/it/openrtb2/rubicon/test-rubicon-bid-request.json b/src/test/resources/org/prebid/server/it/openrtb2/rubicon/test-rubicon-bid-request.json
@@ -55,8 +55,8 @@
         "site_id": 3001,
         "target": {
           "iab": [
-            "segmentId2",
             "segmentId1",
+            "segmentId2",
             "segmentId3"
           ]
         }

Original file line number	Diff line number	Diff line change
`@@ -55,8 +55,8 @@`
`55`	`55`	`"site_id": 3001,`
`56`	`56`	`"target": {`
`57`	`57`	`"iab": [`
`58`		`- "segmentId2",`
`59`	`58`	`"segmentId1",`
	`59`	`+ "segmentId2",`
`60`	`60`	`"segmentId3"`
`61`	`61`	`]`
`62`	`62`	`}`