Yieldlab: Add Digital Service Act (DSA) support #3011

william-harris · 2024-02-23T07:26:18Z

This ports this Go PR

A review and approval from a member of the Yieldlab team are required. (@brushmate, @rey1128, @nkloeber)

rey1128

looks good to me

SerhiiNahornyi · 2024-02-29T12:13:04Z

src/test/java/org/prebid/server/bidder/yieldlab/YieldlabBidderTest.java

+        expectedDsa.put("adrender", 2);
+        expectedDsa.set("transparency", transparencies);
+
+        final var actualDsa = result.getValue().get(0).getBid().getExt().get("dsa");


Code style of repository requires strict types instead of vars

SerhiiNahornyi · 2024-02-29T12:14:19Z

src/test/java/org/prebid/server/bidder/yieldlab/YieldlabBidderTest.java

+        );
+    }
+
+    @ParameterizedTest(name = "{index} - {0}")


We are not using @ParameterizedTest inside repository
Separate test cases == separate tests

SerhiiNahornyi · 2024-02-29T12:15:16Z

src/main/java/org/prebid/server/bidder/yieldlab/model/YieldlabDigitalServicesActResponse.java

+import java.util.List;
+
+@AllArgsConstructor(staticName = "of")
+@Value


@value(staticConstructor = "of")

SerhiiNahornyi · 2024-02-29T12:16:09Z

src/main/java/org/prebid/server/bidder/yieldlab/model/YieldlabDigitalServicesActResponse.java

+    String behalf;
+    String paid;
+    Integer adrender;
+    List<Transparency> transparency;


We prefer to have empty line after each field

SerhiiNahornyi · 2024-02-29T12:16:29Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+        if (dsa == null) {
+            return null;
+        }
+        final var ext = mapper.mapper().createObjectNode();


Please replace each var with strict type

SerhiiNahornyi · 2024-02-29T12:17:24Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+    }
+
+    private static String encodeTransparency(ExtRegsDsaTransparency transparency) {
+        return "%s~%s".formatted(transparency.getDomain(), encodeTransparencyParams(transparency.getDsaParams()));


Nitpick. %s~%s can be class level constant,
_ on line 296 also

SerhiiNahornyi · 2024-02-29T12:18:05Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+    }
+
+    private static boolean hasDsa(BidRequest request) {
+        return request.getRegs() != null


Here you can use Optional approach

I'm not sure I understand what you mean by "Optional approach".

SerhiiNahornyi · 2024-02-29T12:18:36Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+            dsaRequestParams.put("dsadatatopub", dsa.getDataToPub().toString());
+        }
+
+        if (dsa.getTransparency() != null && !dsa.getTransparency().isEmpty()) {


dsa.getTransparency() Can be extracted to separate variable

brushmate · 2024-03-05T07:36:11Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+            .map(Regs::getExt)
+            .map(ExtRegs::getDsa)
+            .map(YieldlabBidder::extractDsaRequestParamsFromDsaRegsExtension)
+            .orElse(Map.of());


Better use the lazy version: .orElseGet(Map::of)

brushmate · 2024-03-05T07:48:43Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+            .collect(Collectors.joining(TRANSPARENCY_TEMPLATE_DELIMITER));
+    }
+
+    private static Predicate<ExtRegsDsaTransparency> transparencyIsValid() {


This should rather be the predicate itself, e.g. accept the transparency object as a parameter and return a boolean. It should be referenced as YieldlabBidder::transparencyIsValid. I would also call the method isTransparencyValid.

brushmate · 2024-03-05T07:49:52Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+
+    private static Predicate<ExtRegsDsaTransparency> transparencyIsValid() {
+        return transparency ->
+            !Objects.isNull(transparency.getDomain())


transparency.getDomain() != null is more expressive IMHO.

brushmate · 2024-03-05T07:51:45Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+            dsaRequestParams.put("dsatransparency", encodeTransparenciesAsString(dsaTransparency));
+        }
+
+        return dsaRequestParams.entrySet().stream()


Is this necessary? All values that might go into the map are checked for nullness already.

brushmate · 2024-03-05T07:53:35Z

src/main/java/org/prebid/server/bidder/yieldlab/model/YieldlabDigitalServicesActResponse.java

+    List<Transparency> transparency;
+
+    @AllArgsConstructor(staticName = "of")
+    @Value


Is staticConstructor missing here as well?

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

AntoxaAntoxic · 2024-03-06T09:31:47Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+        try {
+            dsaNode = mapper.mapper().convertValue(dsa, JsonNode.class);
+        } catch (IllegalArgumentException e) {
+            throw new PreBidException("Failed to serialize DSA object", e);


looks like the PBS Go version has a more detailed error message fmt.Errorf("failed to make JSON for seatbid.bid.ext for adslotID %v. This is most likely a programming issue", bid.ID)

Perhaps it's worth keeping it here as well

Also, according to the PBS Go version the error shouldn't disrupt other bids processing, so the error has to be reported and the ext has to be null in this case. Please double-check

src/test/java/org/prebid/server/bidder/yieldlab/YieldlabBidderTest.java

AntoxaAntoxic · 2024-03-07T14:11:08Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+
+    private static boolean isTransparencyValid(ExtRegsDsaTransparency transparency) {
+        return transparency.getDomain() != null
+                && StringUtils.isNotBlank(transparency.getDomain())


you can use just isNotBlank, it covers not null case as well

AntoxaAntoxic · 2024-03-07T14:16:17Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

@@ -486,7 +492,8 @@ private ObjectNode resolveExtParameter(YieldlabDigitalServicesActResponse dsa) {
        try {
            dsaNode = mapper.mapper().convertValue(dsa, JsonNode.class);
        } catch (IllegalArgumentException e) {
-            throw new PreBidException("Failed to serialize DSA object", e);
+            logger.error("Failed to serialize DSA object for adslot {}", yieldlabResponse.getId(), e);


I believe this error should be returned in the response, not just logged

This would require a big refactoring of the existing implementation as there is no support for returning valid bids along with failed bids. Yieldlab did not write the existing implementation, and we feel it is beyond the scope of this ticket to be refactoring the existing implementation. Our goal is simply to add DSA support since this is a legal requirement. We are fine with not reporting failures.

CTMBNara

Some of the comments not related to the scope, so feel free to ignore them, but please at least provide answers on comments related to difference in logic between PBS-Java and PBS-Go.

CTMBNara · 2024-03-22T14:58:59Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+            .map(Regs::getExt)
+            .map(ExtRegs::getDsa)
+            .map(YieldlabBidder::extractDsaRequestParamsFromDsaRegsExtension)
+            .orElseGet(Map::of);


We are preferring to use Collections.empty* instead of *.of().
Also, as Collections.emptyMap() (and Map.of()) returns already created object, it's better to use .orElse instead of orElseGet.

So, please, change it with:
.orElse(Collections.emptyMap());

CTMBNara · 2024-03-22T15:19:07Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

Comparing the logic with the GO version, I don't see any sorting operations for adSlots in GO. Does PBS-Java need this?
adSlotIds.stream().sorted().collect(...)

It isn't needed, no.

CTMBNara · 2024-03-22T15:35:50Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

Also, i don't see anything similar in GO for ts parameter:
isDebugEnabled(request) ? "200000" : ...

Does PBS-Java need to pass this debug value? Just out of interest I removed the debug part and all tests passed:
final String timestamp = String.valueOf(clock.instant().getEpochSecond());

Same question about
final int weekNumber = isDebugEnabled(bidRequest) ? 35 : ...

and

final String timestamp = isDebugEnabled(bidRequest) ? "200000" : ...

As far as we are concerned, it is not needed no.

CTMBNara · 2024-03-22T15:45:19Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

String.join("ylid:", user.getBuyeruid()) doesn't corresponds to GO logic. It just doesn't do anything.
It's need to be "ylid" + user.getBuyeruid().

As a courtesy I can fix this, but this wasn't written by us and is also out of scope.

Implementing this breaks the tests constructExtImpShouldWorkWithDuplicateKeysTargeting and makeHttpRequestsShouldSendRequestToModifiedUrlWithHeaders, so I will have to decline.

CTMBNara · 2024-03-22T15:54:45Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

uriBuilder.addParameter("lat", resolveNumberParameter(geo.getLat())); uriBuilder.addParameter("lon", resolveNumberParameter(geo.getLon()));

This code doesn't handle nulls as GO does.
Java: null -> null
GO: nil -> "0"

This is out of scope, and I don't intend to change it.

CTMBNara · 2024-03-22T16:09:39Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+        }
+
+        final List<ExtRegsDsaTransparency> dsaTransparency = dsa.getTransparency();
+        if (dsaTransparency != null && !dsaTransparency.isEmpty()) {


Predicate can be simplified: CollectionUtils.isNotEmpty(dsaTransparency)

CTMBNara · 2024-03-22T16:23:06Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+
+        final List<ExtRegsDsaTransparency> dsaTransparency = dsa.getTransparency();
+        if (dsaTransparency != null && !dsaTransparency.isEmpty()) {
+            dsaRequestParams.put("dsatransparency", encodeTransparenciesAsString(dsaTransparency));


encodeTransparencyAsString can return an empty string if all entries are invalid. PBS-GO doesn't add the dsatransparency parameter in this case.

CTMBNara · 2024-03-22T16:37:37Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+    private static String encodeTransparenciesAsString(List<ExtRegsDsaTransparency> transparencies) {
+        return transparencies.stream()
+            .filter(YieldlabBidder::isTransparencyValid)
+            .map(YieldlabBidder::encodeTransparency)
+            .collect(Collectors.joining(TRANSPARENCY_TEMPLATE_DELIMITER));
+    }
+
+    private static boolean isTransparencyValid(ExtRegsDsaTransparency transparency) {
+        return StringUtils.isNotBlank(transparency.getDomain())
+                && transparency.getDsaParams() != null
+                && CollectionUtils.isNotEmpty(transparency.getDsaParams());
+    }
+
+    private static String encodeTransparency(ExtRegsDsaTransparency transparency) {
+        return TRANSPARENCY_TEMPLATE.formatted(transparency.getDomain(),
+            encodeTransparencyParams(transparency.getDsaParams()));
+    }
+
+    private static String encodeTransparencyParams(List<Integer> dsaParams) {
+        return dsaParams.stream().map(Objects::toString).collect(Collectors.joining(
+            TRANSPARENCY_TEMPLATE_PARAMS_DELIMITER));
+    }


There are some differences with GO logic:

transparency0 - invalid, transparency1 - valid
Java: "transparency1"
GO: "~~transparency1"

transparency0 - valid, transparency1 - invalid, transparency2 - valid
Java: "transparency0~~transparency2"
GO: "transparency0~~~~transparency2"

Similar problems with ~ separator:

If transparency1.dsaParams is empty
Java: "transparency0~~transparency2"
GO: "transparency0~~domain1~~~transparency2"

...

And so on :)

To me the Java implementation looks correct, and the GO version incorrect.

CTMBNara · 2024-03-22T16:49:02Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

Out of scope, but....

Can i ask you to extract collectImpExt(imps) from ExtImpYieldlab getMatchedExtImp(...) method, so it will be out of for loop on bids? Just to make it from O(nk) to O(n).... 😅

I'm sorry, but I must decline. Like you said it is out of scope.

CTMBNara · 2024-03-22T17:22:58Z

src/main/java/org/prebid/server/bidder/yieldlab/YieldlabBidder.java

+        final ObjectNode ext = mapper.mapper().createObjectNode();
+        final JsonNode dsaNode;
+        try {
+            dsaNode = mapper.mapper().convertValue(dsa, JsonNode.class);


mapper.mapper().valueToTree(dsa)

Yieldlab: Implementing DSA support

cc0cca6

bretg mentioned this pull request Feb 23, 2024

Port PR from PBS-Go: Yieldlab: Add Digital Service Act (DSA) handling #2984

Closed

SerhiiNahornyi added the do not port label Feb 26, 2024

Code review changes

edf9d9f

william-harris force-pushed the Yieldlab-implementing-dsa-for-yieldlab branch from f381f45 to edf9d9f Compare February 26, 2024 15:54

rey1128 approved these changes Feb 26, 2024

View reviewed changes

SerhiiNahornyi requested changes Feb 29, 2024

View reviewed changes

Addressing code review

3c57c36

brushmate suggested changes Mar 5, 2024

View reviewed changes

Addressing requested changes

35f54c5

william-harris force-pushed the Yieldlab-implementing-dsa-for-yieldlab branch from 07acc54 to 35f54c5 Compare March 5, 2024 08:36

AntoxaAntoxic requested changes Mar 6, 2024

View reviewed changes

Addressing more requested changes

e07cb0b

AntoxaAntoxic reviewed Mar 7, 2024

View reviewed changes

Removing redundant null check

0ffb3a7

CTMBNara requested changes Mar 22, 2024

View reviewed changes

Addressing more requested changes

358acf0

SerhiiNahornyi requested a review from And1sS March 26, 2024 12:36

And1sS approved these changes Mar 26, 2024

View reviewed changes

SerhiiNahornyi approved these changes Mar 26, 2024

View reviewed changes

SerhiiNahornyi merged commit a475313 into prebid:master Mar 26, 2024
5 checks passed

Yieldlab: Add Digital Service Act (DSA) support #3011

Yieldlab: Add Digital Service Act (DSA) support #3011

Conversation

william-harris commented Feb 23, 2024 • edited Loading

rey1128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntoxaAntoxic Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CTMBNara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

william-harris Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

william-harris commented Feb 23, 2024 •

edited

Loading

AntoxaAntoxic Mar 6, 2024 •

edited

Loading

william-harris Mar 25, 2024 •

edited

Loading