Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SupplyChain object support in Prebid #4084

Merged
merged 41 commits into from
Aug 30, 2019
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
b66e75a
moving dctr related code in a function
pm-harshad-mane Jul 16, 2019
edd7614
moving parsedRequest variable out of the loop
pm-harshad-mane Jul 16, 2019
3c9227f
Merge remote-tracking branch 'upstream/master' into small_changes
pm-harshad-mane Jul 16, 2019
f0fcc28
Merge remote-tracking branch 'upstream/master' into small_changes
pm-harshad-mane Aug 9, 2019
e5dd135
added a todo comment
pm-harshad-mane Aug 12, 2019
16bd479
exporting hasOwn function
pm-harshad-mane Aug 12, 2019
7d4cfde
added functionality to pass schain object
pm-harshad-mane Aug 12, 2019
e4cef55
changed logMessage to logError
pm-harshad-mane Aug 12, 2019
4f7ba93
Moved schain related code from adapaterManager.js to schain.js
pm-harshad-mane Aug 12, 2019
f7f9f3f
indentation chnages
pm-harshad-mane Aug 12, 2019
f44628e
logical bug fix
pm-harshad-mane Aug 12, 2019
ec0c32f
added test cases for schain
pm-harshad-mane Aug 12, 2019
9e6ee7d
PubMatic: pass schain object in request
pm-harshad-mane Aug 12, 2019
51d12e6
indentation
pm-harshad-mane Aug 12, 2019
abdcbe6
unit test for PubMatic schain support
pm-harshad-mane Aug 12, 2019
2fe6047
using isInteger from utils
pm-harshad-mane Aug 13, 2019
80b1787
moved schain as a module
pm-harshad-mane Aug 13, 2019
693399c
indentation
pm-harshad-mane Aug 13, 2019
7320a52
removed commented code
pm-harshad-mane Aug 13, 2019
88f495f
added try-catch as the statement code was breaking CI for IE-11
pm-harshad-mane Aug 13, 2019
d09eab4
Revert "added try-catch as the statement code was breaking CI for IE-11"
pm-harshad-mane Aug 13, 2019
e9606bf
added a try-catch for a staement as it was breaking CI sometimes
pm-harshad-mane Aug 14, 2019
355e353
added schain.md for schain module
pm-harshad-mane Aug 14, 2019
1458815
added a few links
pm-harshad-mane Aug 14, 2019
f73d4c1
fixed typos
pm-harshad-mane Aug 14, 2019
b7fa658
simplified the approach in adpater code
pm-harshad-mane Aug 14, 2019
25f877c
trying to restart CI
pm-harshad-mane Aug 14, 2019
b30dcaf
Revert "trying to restart CI"
pm-harshad-mane Aug 14, 2019
e891b9d
adding support in prebidServerBidAdpater as well
pm-harshad-mane Aug 15, 2019
9072e73
bug fix
pm-harshad-mane Aug 15, 2019
02e2e38
minor changes
pm-harshad-mane Aug 15, 2019
9b2a639
modified a comment
pm-harshad-mane Aug 15, 2019
fec36b8
added name to a test case
pm-harshad-mane Aug 20, 2019
4c218f3
Revert "added a try-catch for a staement as it was breaking CI someti…
pm-harshad-mane Aug 23, 2019
31d00d5
moving schain validation logic inside PM adapter
pm-harshad-mane Aug 27, 2019
e344855
Revert "moving schain validation logic inside PM adapter"
pm-harshad-mane Aug 28, 2019
7e4188f
added validation mode: strict, relaxed, off
pm-harshad-mane Aug 28, 2019
fd34738
updated documentation
pm-harshad-mane Aug 28, 2019
01b9c68
Merge remote-tracking branch 'upstream/master' into small_changes
pm-harshad-mane Aug 28, 2019
0c896df
moved a comment
pm-harshad-mane Aug 28, 2019
1f9d0eb
changed value in example
pm-harshad-mane Aug 29, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 52 additions & 39 deletions modules/pubmaticBidAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,38 @@ function _blockedIabCategoriesValidation(payload, blockedIabCategories) {
}
}

function _handleDealCustomTargetings(payload, dctrArr, validBidRequests) {
var dctr = '';
var dctrLen;
// set dctr value in site.ext, if present in validBidRequests[0], else ignore
if (dctrArr.length > 0) {
if (validBidRequests[0].params.hasOwnProperty('dctr')) {
dctr = validBidRequests[0].params.dctr;
if (utils.isStr(dctr) && dctr.length > 0) {
var arr = dctr.split('|');
dctr = '';
arr.forEach(val => {
dctr += (val.length > 0) ? (val.trim() + '|') : '';
});
dctrLen = dctr.length;
if (dctr.substring(dctrLen, dctrLen - 1) === '|') {
dctr = dctr.substring(0, dctrLen - 1);
}
payload.site.ext = {
key_val: dctr.trim()
}
} else {
utils.logWarn(LOG_WARN_PREFIX + 'Ignoring param : dctr with value : ' + dctr + ', expects string-value, found empty or non-string value');
}
if (dctrArr.length > 1) {
utils.logWarn(LOG_WARN_PREFIX + 'dctr value found in more than 1 adunits. Value from 1st adunit will be picked. Ignoring values from subsequent adunits');
}
} else {
utils.logWarn(LOG_WARN_PREFIX + 'dctr value not found in 1st adunit, ignoring values from subsequent adunits');
}
}
}

pm-harshad-mane marked this conversation as resolved.
Show resolved Hide resolved
export const spec = {
code: BIDDER_CODE,
supportedMediaTypes: [BANNER, VIDEO, NATIVE],
Expand Down Expand Up @@ -779,11 +811,10 @@ export const spec = {
var conf = _initConf(refererInfo);
var payload = _createOrtbTemplate(conf);
var bidCurrency = '';
var dctr = '';
var dctrLen;
var dctrArr = [];
var bid;
var blockedIabCategories = [];
let schainConfig;
validBidRequests.forEach(originalBid => {
bid = utils.deepClone(originalBid);
bid.params.adSlot = bid.params.adSlot || '';
Expand Down Expand Up @@ -818,6 +849,7 @@ export const spec = {
if (impObj) {
payload.imp.push(impObj);
}
schainConfig = schainConfig || bid.schain;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems a bit strange to me.

According to the implementation of the copySchainObjectInAdunits(..) function, it seems like you would always have one of the following two cases:

  • The Supply Chain object has been injected into all bids
  • The Supply Chain object has been injected into none of the bids

However, this specific line of code suggests that there could be scenarios where some bids of a Supply Chain, and some do not. Furthermore, if a Bid is missing a Supply Chain, then it's somehow acceptable to copy the Supply Chain from the previous Bid...?

This is also making me question whether or not the Supply Chain should actually be injected into individual bids. It seems like "Supply Chain" is a property of the request as a whole.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming back to this: it seems like the Supply Chain should be injected into the bidderRequest rather than the individual validBidRequests. This would eliminate the need for this strange line of code.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(To be fair: I'm not actually super familiar w/ the Prebid.js API since I haven't worked on this codebase before. So, bidderRequest might not be serving the purpose that I think it serves. However, my main point is that it seems like the Supply Chain should be a property of the request/transaction as a whole, rather than a property of the individual Bids)

Copy link
Contributor Author

@pm-harshad-mane pm-harshad-mane Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree that we should not duplicate the schain data in bids.
I am not sure how we can pass schain data in bidderRequest object through an optional module, this can be done easily from adapterManager.
Thus I followed the approach used by userId module.

});

if (payload.imp.length == 0) {
Expand All @@ -835,6 +867,19 @@ export const spec = {
payload.ext.wrapper.wp = 'pbjs';
payload.user.gender = (conf.gender ? conf.gender.trim() : UNDEFINED);
payload.user.geo = {};
payload.user.geo.lat = _parseSlotParam('lat', conf.lat);
payload.user.geo.lon = _parseSlotParam('lon', conf.lon);
payload.user.yob = _parseSlotParam('yob', conf.yob);
payload.device.geo = payload.user.geo;
payload.site.page = conf.kadpageurl.trim() || payload.site.page.trim();
payload.site.domain = _getDomainFromURL(payload.site.page);
if (schainConfig) {
payload.source = {
ext: {
schain: schainConfig
}
};
}

// Attaching GDPR Consent Params
if (bidderRequest && bidderRequest.gdprConsent) {
Expand All @@ -849,43 +894,10 @@ export const spec = {
};
}

payload.user.geo.lat = _parseSlotParam('lat', conf.lat);
payload.user.geo.lon = _parseSlotParam('lon', conf.lon);
payload.user.yob = _parseSlotParam('yob', conf.yob);
payload.device.geo = payload.user.geo;
payload.site.page = conf.kadpageurl.trim() || payload.site.page.trim();
payload.site.domain = _getDomainFromURL(payload.site.page);

// set dctr value in site.ext, if present in validBidRequests[0], else ignore
if (dctrArr.length > 0) {
if (validBidRequests[0].params.hasOwnProperty('dctr')) {
dctr = validBidRequests[0].params.dctr;
if (utils.isStr(dctr) && dctr.length > 0) {
var arr = dctr.split('|');
dctr = '';
arr.forEach(val => {
dctr += (val.length > 0) ? (val.trim() + '|') : '';
});
dctrLen = dctr.length;
if (dctr.substring(dctrLen, dctrLen - 1) === '|') {
dctr = dctr.substring(0, dctrLen - 1);
}
payload.site.ext = {
key_val: dctr.trim()
}
} else {
utils.logWarn(LOG_WARN_PREFIX + 'Ignoring param : dctr with value : ' + dctr + ', expects string-value, found empty or non-string value');
}
if (dctrArr.length > 1) {
utils.logWarn(LOG_WARN_PREFIX + 'dctr value found in more than 1 adunits. Value from 1st adunit will be picked. Ignoring values from subsequent adunits');
}
} else {
utils.logWarn(LOG_WARN_PREFIX + 'dctr value not found in 1st adunit, ignoring values from subsequent adunits');
}
}

_handleDealCustomTargetings(payload, dctrArr, validBidRequests);
_handleEids(payload, validBidRequests);
_blockedIabCategoriesValidation(payload, blockedIabCategories);

return {
method: 'POST',
url: ENDPOINT,
Expand All @@ -902,6 +914,8 @@ export const spec = {
interpretResponse: (response, request) => {
const bidResponses = [];
var respCur = DEFAULT_CURRENCY;
let parsedRequest = JSON.parse(request.data);
let parsedReferrer = parsedRequest.site && parsedRequest.site.ref ? parsedRequest.site.ref : '';
try {
if (response.body && response.body.seatbid && utils.isArray(response.body.seatbid)) {
// Supporting multiple bid responses for same adSize
Expand All @@ -910,7 +924,6 @@ export const spec = {
seatbidder.bid &&
utils.isArray(seatbidder.bid) &&
seatbidder.bid.forEach(bid => {
let parsedRequest = JSON.parse(request.data);
let newBid = {
requestId: bid.impid,
cpm: (parseFloat(bid.price) || 0).toFixed(2),
Expand All @@ -921,7 +934,7 @@ export const spec = {
currency: respCur,
netRevenue: NET_REVENUE,
ttl: 300,
referrer: parsedRequest.site && parsedRequest.site.ref ? parsedRequest.site.ref : '',
referrer: parsedReferrer,
ad: bid.adm
};
if (parsedRequest.imp && parsedRequest.imp.length > 0) {
Expand Down
127 changes: 127 additions & 0 deletions modules/schain.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
import {config} from '../src/config';
import {getGlobal} from '../src/prebidGlobal';
import { isNumber, isStr, isArray, isPlainObject, hasOwn, logError, isInteger } from '../src/utils';

// validate the supply chanin object
pm-harshad-mane marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/InteractiveAdvertisingBureau/openrtb/blob/master/supplychainobject.md

export function isSchainObjectValid(schainObject) {
const schainErrorPrefix = 'Invalid schain object found: ';
const shouldBeAString = ' should be a string';
const shouldBeAnInteger = ' should be an Integer';
const shouldBeAnObject = ' should be an object';
const shouldBeAnArray = ' should be an Array';

if (!isPlainObject(schainObject)) {
logError(schainErrorPrefix + `schain` + shouldBeAnObject);
return false;
}

// complete: Integer
if (!isNumber(schainObject.complete) || !isInteger(schainObject.complete)) {
logError(schainErrorPrefix + `schain.complete` + shouldBeAnInteger);
return false;
}

// ver: String
if (!isStr(schainObject.ver)) {
logError(schainErrorPrefix + `schain.ver` + shouldBeAString);
return false;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern: the schema ultimately depends upon the version of the Supply Chain spec that is being used. Ex: they recently released a "final" version of the previous draft that renamed a number of fields.

To accommodate this, a lot of the validation logic will probably need to be dependent upon the version. Ex:

export function isSchainObjectValid(schainObject) {
  // Move the version-checking logic to the very beginning
  if ( !(isPlainObject(schainObject) && isStr(schainObject.ver) )  ) {
      logError(schainErrorPrefix + `schain.ver` + shouldBeAString);
      return false;
  }

  // Now that we've verified there is a version, we need to evaluate the appropriate version of the schema.
  switch(schainObject.ver) {
    case "1.0":
      return isSchainObjectValid_1_0(schainObject);

    case "2.0":
      return isSchainObjectValid_2_0(schainObject);

    default:
      logError("Unsupported schain version: " + schainObject.ver);
      return false;
  }
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this a bit more, though... I'm concerned about the fact that "Prebid.js" and "The Supply Chain Spec" have completely different release cycles. That is: there could be a new release of the "Supply Chain Spec" at any time. With the current arrangement, it seems like nobody would be able to upgrade to this new version of the "Supply Chain Spec" until after there has been a Prebid.js update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had asked @pm-harshad-mane to put the validation logic in. Considering we're facing a deadline from some DSPs to complete the schain work, I'm wondering if the better option is just to remove the validation logic and let bidders validate the schain objects themselves if they wish to.


// ext: Object [optional]
if (hasOwn(schainObject, 'ext')) {
if (!isPlainObject(schainObject.ext)) {
logError(schainErrorPrefix + `schain.ext` + shouldBeAnObject);
return false;
}
}

// nodes: Array of objects
if (!isArray(schainObject.nodes)) {
logError(schainErrorPrefix + `schain.nodes` + shouldBeAnArray);
return false;
}

// now validate each node
let isEachNodeIsValid = true;
schainObject.nodes.forEach(node => {
// asi: String
if (!isStr(node.asi)) {
isEachNodeIsValid = isEachNodeIsValid && false;
logError(schainErrorPrefix + `schain.nodes[].asi` + shouldBeAString);
}

// sid: String
if (!isStr(node.sid)) {
isEachNodeIsValid = isEachNodeIsValid && false;
logError(schainErrorPrefix + `schain.nodes[].sid` + shouldBeAString);
}

// hp: Integer
if (!isNumber(node.hp) || !isInteger(node.hp)) {
isEachNodeIsValid = isEachNodeIsValid && false;
logError(schainErrorPrefix + `schain.nodes[].hp` + shouldBeAnInteger);
}

// rid: String [Optional]
if (hasOwn(node, 'rid')) {
if (!isStr(node.rid)) {
isEachNodeIsValid = isEachNodeIsValid && false;
logError(schainErrorPrefix + `schain.nodes[].rid` + shouldBeAString);
}
}

// name: String [Optional]
if (hasOwn(node, 'name')) {
if (!isStr(node.name)) {
isEachNodeIsValid = isEachNodeIsValid && false;
logError(schainErrorPrefix + `schain.nodes[].name` + shouldBeAString);
}
}

// domain: String [Optional]
if (hasOwn(node, 'domain')) {
if (!isStr(node.domain)) {
isEachNodeIsValid = isEachNodeIsValid && false;
logError(schainErrorPrefix + `schain.nodes[].domain` + shouldBeAString);
}
}

// ext: Object [Optional]
if (hasOwn(node, 'ext')) {
if (!isPlainObject(node.ext)) {
isEachNodeIsValid = isEachNodeIsValid && false;
logError(schainErrorPrefix + `schain.nodes[].ext` + shouldBeAnObject);
}
}
});

if (!isEachNodeIsValid) {
return false;
}

return true;
}

export function copySchainObjectInAdunits(adUnits, schainObject) {
// copy schain object in all adUnits as adUnit.bid.schain
adUnits.forEach(adUnit => {
adUnit.bids.forEach(bid => {
bid.schain = schainObject;
});
});
}

export function init(config) {
getGlobal().requestBids.before(function(fn, reqBidsConfigObj) {
let schainObject = config.getConfig('schain');
if (isSchainObjectValid(schainObject)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this confirms my suspicion from my earlier comment. The Supply Chain is only being made available to downstream components if Prebid.js itself has been updated to recognize the version.

Therefore, it would probably be worthwhile to provide a way to "relax" the validation. Really, it seems like there would be 3 cases:

  • "strict": If the schain fails validation, then log an error. Do not forward schain to the adapters.
  • "relaxed": If the schain fails validation, then log an error. BUT: forward the schain to the adapters anyway
  • "off": Do not even bother doing validation.

Additionally... are we really expecting the schain to be a dynamic value? Or, is it a value that the end-users configure once and then never change? If it's a case of "configure once and then never change", then there really isn't a need to re-validate it with each request. Once end-users have verified the correctness of the initial Prebid integration, they could then disable this validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, the value will be dynamic.

copySchainObjectInAdunits(reqBidsConfigObj.adUnits || getGlobal().adUnits, schainObject);
// calling fn allows prebid to continue processing
return fn.call(this, reqBidsConfigObj);
}
}, 40);
}

init(config)
42 changes: 42 additions & 0 deletions modules/schain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# schain module

Aggregators who manage Prebid wrappers on behalf of multiple publishers need to declare their intermediary status in the Supply Chain Object.
As the spec prohibits us from adding upstream intermediaries, Prebid requests in this case need to come with the schain information.
In this use case, it's seems cumbersome to have every bidder in the wrapper separately configured the same schain information.

Refer:
- https://iabtechlab.com/sellers-json/
- https://github.com/InteractiveAdvertisingBureau/openrtb/blob/master/supplychainobject.md

## Sample code for passing the schain object
```
pbjs.setConfig( {
"schain": {
"ver":"1.0",
"complete": 1,
"nodes": [
{
"asi":"indirectseller.com",
"sid":"00001",
"hp":1
},

{
"asi":"indirectseller-2.com",
"sid":"00002",
"hp":2
},
]
}
});
```

## Workflow
The schain module is not enabled by default as it may not be neccessary for all publishers.
If required, schain module can be included as following
```
$ gulp build --modules=schain,pubmaticBidAdapter,openxBidAdapter,rubiconBidAdapter,sovrnBidAdapter
```
The schian module will validate the schain object passed using pbjs.setConfig API.
pm-harshad-mane marked this conversation as resolved.
Show resolved Hide resolved
If the schain object is valid then it will be passed on to bidders/adapters in ```validBidRequests[].schain```
You may refer pubmaticBidAdapter implementaion for the same.
5 changes: 4 additions & 1 deletion modules/widespaceBidAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,10 @@ export const spec = {
function storeData(data, name, stringify = true) {
const value = stringify ? JSON.stringify(data) : data;
if (LOCAL_STORAGE_AVAILABLE) {
localStorage.setItem(name, value);
try {
pm-harshad-mane marked this conversation as resolved.
Show resolved Hide resolved
// adding try-catch as this code was breaking CI sometimes for IE-11
localStorage.setItem(name, value);
} catch (e) {}
return true;
} else if (COOKIE_ENABLED) {
const theDate = new Date();
Expand Down
2 changes: 1 addition & 1 deletion src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ export function _map(object, callback) {
return output;
}

var hasOwn = function (objectToCheck, propertyToCheckFor) {
export function hasOwn(objectToCheck, propertyToCheckFor) {
if (objectToCheck.hasOwnProperty) {
return objectToCheck.hasOwnProperty(propertyToCheckFor);
} else {
Expand Down
23 changes: 22 additions & 1 deletion test/spec/modules/pubmaticBidAdapter_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,27 @@ describe('PubMatic adapter', function () {
let bannerVideoAndNativeBidRequests;
let bannerBidResponse;
let videoBidResponse;
let schainConfig;

beforeEach(function () {
schainConfig = {
'ver': '1.0',
'complete': 1,
'nodes': [
{
'asi': 'indirectseller.com',
'sid': '00001',
'hp': 1
},

{
'asi': 'indirectseller-2.com',
'sid': '00002',
'hp': 2
}
]
};

bidRequests = [
{
bidder: 'pubmatic',
Expand Down Expand Up @@ -55,7 +74,8 @@ describe('PubMatic adapter', function () {
bidId: '23acc48ad47af5',
requestId: '0fb4905b-9456-4152-86be-c6f6d259ba99',
bidderRequestId: '1c56ad30b9b8ca8',
transactionId: '92489f71-1bf2-49a0-adf9-000cea934729'
transactionId: '92489f71-1bf2-49a0-adf9-000cea934729',
schain: schainConfig
}
];

Expand Down Expand Up @@ -728,6 +748,7 @@ describe('PubMatic adapter', function () {
expect(data.imp[0].banner.h).to.equal(250); // height
expect(data.imp[0].ext.pmZoneId).to.equal(bidRequests[0].params.pmzoneid.split(',').slice(0, 50).map(id => id.trim()).join()); // pmzoneid
expect(data.imp[0].bidfloorcur).to.equal(bidRequests[0].params.currency);
expect(data.source.ext.schain).to.deep.equal(bidRequests[0].schain);
});

it('Request params check: without adSlot', function () {
Expand Down
Loading