CTMBNara Review

Author: t-sormonte

src/main/java/org/prebid/server/bidder/sparteo/SparteoBidder.java

@@ -32,9 +32,7 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.Iterator;
 import java.util.List;
-import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 
@@ -58,21 +56,18 @@ public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest request
         String siteNetworkId = null;
 
         for (Imp imp : request.getImp()) {
-            try {
-                final ExtImpSparteo bidderParams = parseExtImp(imp);
-
-                if (siteNetworkId == null && bidderParams.getNetworkId() != null) {
-                    siteNetworkId = bidderParams.getNetworkId();
+            if (siteNetworkId == null) {
+                try {
+                    siteNetworkId = parseExtImp(imp).getNetworkId();
+                } catch (PreBidException e) {
+                    errors.add(BidderError.badInput(
+                            "ignoring imp id=%s, error processing ext: %s".formatted(
+                                    imp.getId(), e.getMessage())));
                 }
-
-                final ObjectNode modifiedExt = modifyImpExt(imp);
-
-                modifiedImps.add(imp.toBuilder().ext(modifiedExt).build());
-            } catch (PreBidException e) {
-                errors.add(BidderError.badInput(
-                        "ignoring imp id=%s, error processing ext: %s".formatted(
-                                imp.getId(), e.getMessage())));
             }
+
+            final ObjectNode modifiedExt = modifyImpExt(imp);
+            modifiedImps.add(imp.toBuilder().ext(modifiedExt).build());
         }
 
         if (modifiedImps.isEmpty()) {
@@ -99,24 +94,10 @@ private ExtImpSparteo parseExtImp(Imp imp) {
 
     private static ObjectNode modifyImpExt(Imp imp) {
         final ObjectNode modifiedImpExt = imp.getExt().deepCopy();
-        final JsonNode sparteoJsonNode = modifiedImpExt.get("sparteo");
-        final ObjectNode sparteoNode = sparteoJsonNode == null || !sparteoJsonNode.isObject()
-                ? modifiedImpExt.putObject("sparteo")
-                : (ObjectNode) sparteoJsonNode;
-
-        final JsonNode paramsJsonNode = sparteoNode.get("params");
-        final ObjectNode paramsNode = paramsJsonNode == null || !paramsJsonNode.isObject()
-                ? sparteoNode.putObject("params")
-                : (ObjectNode) paramsJsonNode;
-
+        final ObjectNode sparteoNode = modifiedImpExt.putObject("sparteo");
         final JsonNode bidderJsonNode = modifiedImpExt.remove("bidder");
-        if (bidderJsonNode != null && bidderJsonNode.isObject()) {
-            final Iterator<Map.Entry<String, JsonNode>> fields = bidderJsonNode.fields();
-            while (fields.hasNext()) {
-                final Map.Entry<String, JsonNode> field = fields.next();
-                paramsNode.set(field.getKey(), field.getValue());
-            }
-        }
+        sparteoNode.set("params", bidderJsonNode);
+
         return modifiedImpExt;
     }
 
@@ -179,6 +160,26 @@ private List<BidderBid> extractBids(BidResponse bidResponse, List<BidderError> e
                 .toList();
     }
 
+    private BidderBid toBidderBid(Bid bid, String currency, List<BidderError> errors) {
+        try {
+            final BidType bidType = getBidType(bid);
+
+            final Integer mtype = switch (bidType) {
+                case banner -> 1;
+                case video -> 2;
+                case xNative -> 4;
+                default -> null;
+            };
+
+            final Bid bidWithMtype = mtype != null ? bid.toBuilder().mtype(mtype).build() : bid;
+
+            return BidderBid.of(bidWithMtype, bidType, currency);
+        } catch (PreBidException e) {
+            errors.add(BidderError.badServerResponse(e.getMessage()));
+            return null;
+        }
+    }
+
     private BidType getBidType(Bid bid) throws PreBidException {
         final BidType bidType = Optional.ofNullable(bid.getExt())
                 .map(ext -> ext.get("prebid"))
@@ -203,24 +204,4 @@ private ExtBidPrebid parseExtBidPrebid(JsonNode prebidNode) {
             return null;
         }
     }
-
-    private BidderBid toBidderBid(Bid bid, String currency, List<BidderError> errors) {
-        try {
-            final BidType bidType = getBidType(bid);
-
-            final Integer mtype = switch (bidType) {
-                case banner -> 1;
-                case video -> 2;
-                case xNative -> 4;
-                default -> null;
-            };
-
-            final Bid bidWithMtype = mtype != null ? bid.toBuilder().mtype(mtype).build() : bid;
-
-            return BidderBid.of(bidWithMtype, bidType, currency);
-        } catch (PreBidException e) {
-            errors.add(BidderError.badServerResponse(e.getMessage()));
-            return null;
-        }
-    }
 }

src/test/java/org/prebid/server/bidder/sparteo/SparteoBidderTest.java

@@ -63,7 +63,6 @@ public void makeHttpRequestsShouldReturnErrorWhenImpExtIsInvalid() {
         assertThat(result.getErrors()).hasSize(1)
                 .allSatisfy(error -> assertThat(error.getMessage())
                         .startsWith("ignoring imp id=impId, error processing ext: invalid imp.ext"));
-        assertThat(result.getValue()).isEmpty();
     }
 
     @Test
@@ -76,7 +75,6 @@ public void makeHttpRequestsShouldReturnErrorWhenAllImpsAreInvalid() {
         final Result<List<HttpRequest<BidRequest>>> result = sparteoBidder.makeHttpRequests(bidRequest);
 
         // then
-        assertThat(result.getValue()).isEmpty();
         assertThat(result.getErrors()).hasSize(1);
     }
 
@@ -101,7 +99,7 @@ public void makeHttpRequestsShouldReturnPartialResultWhenSomeImpsAreInvalid() {
                 .flatExtracting(BidRequest::getImp)
                 .extracting(Imp::getExt)
                 .extracting((JsonNode ext) -> ext.at("/sparteo/params/key").asText())
-                .containsExactly("value");
+                .containsExactly("", "value");
     }
 
     @Test
@@ -171,32 +169,6 @@ public void makeHttpRequestsShouldUseFirstNetworkIdWhenMultipleImpsDefineIt() {
                 .containsExactly("id1");
     }
 
-    @Test
-    public void makeHttpRequestsShouldMergeBidderAndSparteoParams() {
-        // given
-        final ObjectNode impExt = mapper.createObjectNode();
-        impExt.set("bidder", mapper.createObjectNode().put("paramFromBidder", "value1"));
-        final ObjectNode sparteoNode = impExt.putObject("sparteo");
-        sparteoNode.putObject("params").put("paramFromSparteo", "value2");
-
-        final BidRequest bidRequest = givenBidRequest(givenImp(imp -> imp.ext(impExt)));
-
-        // when
-        final Result<List<HttpRequest<BidRequest>>> result = sparteoBidder.makeHttpRequests(bidRequest);
-
-        // then
-        assertThat(result.getErrors()).isEmpty();
-        assertThat(result.getValue())
-                .extracting(HttpRequest::getPayload)
-                .flatExtracting(BidRequest::getImp)
-                .extracting(Imp::getExt)
-                .extracting(ext -> ext.at("/sparteo/params"))
-                .allSatisfy(params -> {
-                    assertThat(params.at("/paramFromBidder").asText()).isEqualTo("value1");
-                    assertThat(params.at("/paramFromSparteo").asText()).isEqualTo("value2");
-                });
-    }
-
     @Test
     public void makeHttpRequestsShouldOverwriteSparteoParamsWithBidderParamsOnConflict() {
         // given