PBS Adapter: Support Bidder-Specific Schains #8594

jlquaccia · 2022-06-21T22:02:01Z

Type of change

Bugfix

Description of change

Added support for bidder-specific schains within the PBS adapter module

Other information

#8561

jlquaccia · 2022-06-22T23:50:58Z

added a couple of tests for this issue just now

jlquaccia · 2022-07-06T19:06:29Z

Just a reminder to review whenever one of you have time. Thanks! @robertrmartinez @ChrisHuie or @musikele

dgirardi · 2022-07-12T15:27:10Z

modules/prebidServerBidAdapter/index.js

+    // get reference to pbs config schain bidder names (if any exist)
+    let pbsSchainBidderNamesArr = [];
+    if (request.ext.prebid.schains) {
+      for (let i = 0; i < request.ext.prebid.schains.length; i++) {


request.ext.prebid.schains.flatMap(s => s.bidders)

dgirardi · 2022-07-12T15:39:26Z

modules/prebidServerBidAdapter/index.js

+    let pbjsSchainValuesArr = [];
+
+    // iterate over pbjs bidder specific schains (if any exist)
+    for (let i = 0; i < bidRequests.length; i++) {


I don't think it's correct to update s2sConfig, this should update request. the first may work if you make sure it's not shared across different requests, but why go down that route? conceptually this is part of the request, not cross-request configuration.

I'm not sure this works as intended if two equivalent schain objects happen to be serialized to two different json strings, e.g. { "ver":"1.0", "complete": 1, ...} vs {"complete": 1, "ver": "1.0", ...}" - what should be the equivalence test?

I suspect this is more complicated than it needs to be - would something like this work?

// `key(schain_1)` === `key(schain_2)` IIF schain_1 is equivalent to schain_2 const schains = Object.fromEntries( request.ext.prebid?.schains || []).map(({bidders, schain}) => [key(schain), {bidders: new Set(bidders), schain}] ) request.ext.prebid.schains = Object.values( bidRequests .map((req) => [req.bidder, req.bids[0].schain]) .reduce((chains, [bidder, chain]) => { const chainKey = key(chain); if (!chains.hasOwnProperty(chainKey)) chains[chainKey] = {bidders: new Set(), schain: chain}; chains[chainKey].bidders.add(bidder); return chains; }, schains) ).map(({bidders, schain}) => ({bidders: Array.from(bidders), schain}))

Thanks for the review @dgirardi. Good point, I agree that it would make more sense to update the request itself instead. I went back and modified my PR, I like the route you took with your suggestion above and integrated it into my PR. Let me know if you think any other changes should be made or if the logic seems good.

dgirardi · 2022-07-14T22:38:50Z

modules/prebidServerBidAdapter/index.js

+      bidRequests
+        .map((req) => [req.bidderCode, req.bids[0].schain])
+        .reduce((chains, [bidder, chain]) => {
+          const chainKey = JSON.stringify(chain);


I'm assuming here that as long a bidder is associated only with one chain, it's OK to have duplicates due to different serialization of equivalent schains, e.g. [{bidders: ['A'], schain1}, {bidders: ['B'], schain2}] works for the backend even if schain1 and schain2 are identical.

I refactored things a bit to include the following logic:

if pbjs.setBidderConfig is used to create an schain with bidder A, but bidder B has the same schain object (as configured on s2sConfig.extPrebid.schains) as bidder A`.. now the output in the request will be:

ext.prebid.schains[{bidders: ['bidder A', 'bidder B'], schain: <the schain obj they were both using>}]

dgirardi · 2022-07-14T22:47:26Z

modules/prebidServerBidAdapter/index.js

+              logInfo(`bidder-specific schain for ${bidder} skipped due to existing entry`);
+            } else {
+              chains[chainKey] = {bidders: new Set(), schain: chain};
+              chains[chainKey].bidders.add(bidder);


I believe the bidders.add(bidder) needs to be detented out of if (!chains.hasOwnProperty(chainKey)); otherwise, if multiple bidders have chains that serialize to the same key, you are losing the association for every one of them except the first.

This would also be a good test case, if I understand your intent, you want two separate bid requests with identical chains to be "merged" into a single entry in ext.prebid.schains.

adjusted the logic for this and also added a test case

dgirardi · 2022-07-14T22:55:55Z

modules/prebidServerBidAdapter/index.js

+        .reduce((chains, [bidder, chain]) => {
+          const chainKey = JSON.stringify(chain);
+          if (chainKey && !chains.hasOwnProperty(chainKey)) {
+            if (pbsSchainBidderNamesArr.indexOf(bidder) !== -1) {


the pbsSchainBidderNamesArr check also appears in the wrong place to me, as a consequence of the above. it should guard the bidders.add call.

jlquaccia · 2022-07-15T20:55:43Z

hey @dgirardi, just pushed some updates can you review when you have time?

robertrmartinez

Looks good to me

dgirardi · 2022-07-21T16:40:17Z

modules/prebidServerBidAdapter/index.js

+    ).map(({bidders, schain}) => ({bidders: Array.from(bidders), schain}));
+    // if schains evaluates to an empty array, remove it from the prebid object
+    if (request.ext.prebid.schains.length === 0) delete request.ext.prebid.schains;
+


I don't know if this is in scope for this, but I noticed something while working on a separate issue: on line 918, the PBS adapter sets source.ext.schain with the first schain it finds. Does that make sense? is it just ignored server side?

hmm, looks as though source.ext.schain only gets added if an schain was configured using pbjs.setBidderConfig and the bidder the schain was configured for is the same as what is set under defaultVendor configured in the s2sConfig..

logic appears to be something like that, but I am not 100% sure on what the expected logic is supposed to be here.

let's keep that out of scope, @dgirardi did you already open another issue for this?

* implementation for issue 8561 * removed eslint disable comment * added tests * removed unecessary comments * addressed feedback * reverted a few changes that were used for dev on my local * addressed feedback and refactored code * reverted changes to the prebidServer_example file Co-authored-by: Jason Quaccia <[email protected]>

patmmccann linked an issue Jun 22, 2022 that may be closed by this pull request

pbs adapter: support bidder-specific schains #8561

Closed

patmmccann added the maintenance label Jun 22, 2022

jsnellbaker requested a review from robertrmartinez June 22, 2022 17:21

jsnellbaker assigned robertrmartinez Jun 22, 2022

jlquaccia force-pushed the ISSUE-8561 branch from a51bb24 to 52eba3d Compare June 22, 2022 23:47

ChrisHuie self-requested a review July 6, 2022 10:50

ChrisHuie self-assigned this Jul 6, 2022

ChrisHuie requested a review from musikele July 6, 2022 10:50

ChrisHuie assigned musikele and unassigned ChrisHuie Jul 6, 2022

ChrisHuie requested a review from dgirardi July 12, 2022 14:37

ChrisHuie assigned dgirardi Jul 12, 2022

dgirardi reviewed Jul 12, 2022

View reviewed changes

jlquaccia force-pushed the ISSUE-8561 branch from bd451ec to f210a77 Compare July 14, 2022 22:14

dgirardi reviewed Jul 14, 2022

View reviewed changes

Jason Quaccia added 7 commits July 15, 2022 13:38

implementation for issue 8561

b414ba9

removed eslint disable comment

d582f8d

added tests

631b487

removed unecessary comments

271a409

addressed feedback

7ca83b4

reverted a few changes that were used for dev on my local

b96ecb4

addressed feedback and refactored code

a3ebe09

jlquaccia force-pushed the ISSUE-8561 branch from 4280a4c to a3ebe09 Compare July 15, 2022 20:38

reverted changes to the prebidServer_example file

63dbdcd

robertrmartinez approved these changes Jul 19, 2022

View reviewed changes

dgirardi approved these changes Jul 19, 2022

View reviewed changes

dgirardi reviewed Jul 21, 2022

View reviewed changes

patmmccann merged commit 8d0f10a into prebid:master Jul 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PBS Adapter: Support Bidder-Specific Schains #8594

PBS Adapter: Support Bidder-Specific Schains #8594

jlquaccia commented Jun 21, 2022

jlquaccia commented Jun 22, 2022

jlquaccia commented Jul 6, 2022

dgirardi Jul 12, 2022

dgirardi Jul 12, 2022

jlquaccia Jul 14, 2022

dgirardi Jul 14, 2022

jlquaccia Jul 15, 2022

dgirardi Jul 14, 2022

jlquaccia Jul 15, 2022

dgirardi Jul 14, 2022 •

edited

Loading

jlquaccia commented Jul 15, 2022

robertrmartinez left a comment

dgirardi Jul 21, 2022

jlquaccia Jul 22, 2022

patmmccann Jul 25, 2022

PBS Adapter: Support Bidder-Specific Schains #8594

PBS Adapter: Support Bidder-Specific Schains #8594

Conversation

jlquaccia commented Jun 21, 2022

Type of change

Description of change

Other information

jlquaccia commented Jun 22, 2022

jlquaccia commented Jul 6, 2022

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

dgirardi Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

jlquaccia commented Jul 15, 2022

robertrmartinez 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

dgirardi Jul 14, 2022 •

edited

Loading