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

Implementation of setBidderConfig and bidder-specific data #4334

Merged
merged 7 commits into from
Nov 13, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
18 changes: 15 additions & 3 deletions src/adapterManager.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/** @module adaptermanger */

import { flatten, getBidderCodes, getDefinedParams, shuffle, timestamp, getBidderRequest } from './utils';
import { flatten, getBidderCodes, getDefinedParams, shuffle, timestamp, getBidderRequest, bind } from './utils';
import { getLabels, resolveStatus } from './sizeMapping';
import { processNativeAdUnitParams, nativeAdapters } from './native';
import { newBidder } from './adapters/bidderFactory';
Expand Down Expand Up @@ -337,9 +337,21 @@ adapterManager.callBids = (adUnits, bidRequests, addBidResponse, doneCb, request
request: requestCallbacks.request.bind(null, bidRequest.bidderCode),
done: requestCallbacks.done
} : undefined);
adapter.callBids(bidRequest, addBidResponse.bind(bidRequest), doneCb.bind(bidRequest), ajax, onTimelyResponse);
config.runWithBidder(
bidRequest.bidderCode,
bind.call(
adapter.callBids,
adapter,
bidRequest,
addBidResponse.bind(bidRequest),
doneCb.bind(bidRequest),
ajax,
onTimelyResponse,
config.callbackWithBidder(bidRequest.bidderCode)
)
);
});
}
};

function doingS2STesting() {
return _s2sConfig && _s2sConfig.enabled && _s2sConfig.testing && s2sTestingModule;
Expand Down
9 changes: 4 additions & 5 deletions src/adapters/bidderFactory.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,7 @@ export function newBidder(spec) {
getSpec: function() {
return Object.freeze(spec);
},
registerSyncs,
snapwich marked this conversation as resolved.
Show resolved Hide resolved
callBids: function(bidderRequest, addBidResponse, done, ajax, onTimelyResponse) {
callBids: function(bidderRequest, addBidResponse, done, ajax, onTimelyResponse, configEnabledCallback) {
if (!Array.isArray(bidderRequest.bids)) {
return;
}
Expand Down Expand Up @@ -216,7 +215,7 @@ export function newBidder(spec) {
// Callbacks don't compose as nicely as Promises. We should call done() once _all_ the
// Server requests have returned and been processed. Since `ajax` accepts a single callback,
// we need to rig up a function which only executes after all the requests have been responded.
const onResponse = delayExecution(afterAllResponses, requests.length)
const onResponse = delayExecution(configEnabledCallback(afterAllResponses), requests.length)
requests.forEach(processRequest);

function formatGetParameters(data) {
Expand All @@ -233,7 +232,7 @@ export function newBidder(spec) {
ajax(
`${request.url}${formatGetParameters(request.data)}`,
{
success: onSuccess,
success: configEnabledCallback(onSuccess),
error: onFailure
},
undefined,
Expand All @@ -247,7 +246,7 @@ export function newBidder(spec) {
ajax(
request.url,
{
success: onSuccess,
success: configEnabledCallback(onSuccess),
error: onFailure
},
typeof request.data === 'string' ? request.data : JSON.stringify(request.data),
Expand Down
114 changes: 106 additions & 8 deletions src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ export function newConfig() {
let listeners = [];
let defaults;
let config;
let bidderConfig;
let currBidder = null;

function resetConfig() {
defaults = {};
Expand Down Expand Up @@ -86,7 +88,7 @@ export function newConfig() {
if (validatePriceGranularity(val)) {
if (typeof val === 'string') {
this._priceGranularity = (hasGranularity(val)) ? val : GRANULARITY_OPTIONS.MEDIUM;
} else if (typeof val === 'object') {
} else if (utils.isPlainObject(val)) {
this._customPriceBucket = val;
this._priceGranularity = GRANULARITY_OPTIONS.CUSTOM;
utils.logMessage('Using custom price granularity');
Expand All @@ -111,7 +113,7 @@ export function newConfig() {
if (validatePriceGranularity(val[item])) {
if (typeof val === 'string') {
aggregate[item] = (hasGranularity(val[item])) ? val[item] : this._priceGranularity;
} else if (typeof val === 'object') {
} else if (utils.isPlainObject(val)) {
aggregate[item] = val[item];
utils.logMessage(`Using custom price granularity for ${item}`);
}
Expand Down Expand Up @@ -182,6 +184,7 @@ export function newConfig() {
}

config = newConfig;
bidderConfig = {};

function hasGranularity(val) {
return find(Object.keys(GRANULARITY_OPTIONS), option => val === GRANULARITY_OPTIONS[option]);
Expand All @@ -196,7 +199,7 @@ export function newConfig() {
if (!hasGranularity(val)) {
utils.logWarn('Prebid Warning: setPriceGranularity was called with invalid setting, using `medium` as default.');
}
} else if (typeof val === 'object') {
} else if (utils.isPlainObject(val)) {
if (!isValidPriceConfig(val)) {
utils.logError('Invalid custom price value passed to `setPriceGranularity()`');
return false;
Expand All @@ -206,6 +209,39 @@ export function newConfig() {
}
}

/**
* Returns base config with bidder overrides (if there is currently a bidder)
* @private
*/
function _getConfig() {
if (currBidder && bidderConfig && utils.isPlainObject(bidderConfig[currBidder])) {
let currBidderConfig = bidderConfig[currBidder];

const configTopicSet = Object.keys(config).concat(Object.keys(currBidderConfig)).reduce((set, key) => {
set.add(key);
return set;
}, new Set());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please import set from core-js. We faced an issue in IE11 with direct usage

Copy link
Contributor

Choose a reason for hiding this comment

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

@jaiminpanchal27 I made sure to only use functions supported in IE. If I import from core-js, can I then use the full Set API, e.g. passing a list into the constructor, using ... to get the values?

Also, is this being done elsewhere? I don't see Set being imported from core-js in other files

Copy link
Contributor

Choose a reason for hiding this comment

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

I found an example in the code and pushed my changes. Please re-review

const uniqTopics = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you use uniqTopics here ? Set stores unique values.

Copy link
Contributor

Choose a reason for hiding this comment

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

I needed to convert the Set to an array. As you mentioned in your other comment, if core-js supports the full Set API, this step in the code won't be needed

configTopicSet.forEach(key => uniqTopics.push(key));

return uniqTopics.reduce((memo, topic) => {
if (!currBidderConfig[topic]) {
memo[topic] = config[topic];
} else if (!config[topic]) {
memo[topic] = currBidderConfig[topic];
} else {
if (utils.isPlainObject(currBidderConfig[topic])) {
memo[topic] = Object.assign({}, config[topic], currBidderConfig[topic]);
} else {
memo[topic] = currBidderConfig[topic];
}
}
return memo;
}, {});
}
return Object.assign({}, config);
}

/*
* Returns configuration object if called without parameters,
* or single configuration property if given a string matching a configuration
Expand All @@ -217,18 +253,25 @@ export function newConfig() {
function getConfig(...args) {
if (args.length <= 1 && typeof args[0] !== 'function') {
const option = args[0];
return option ? utils.deepAccess(config, option) : config;
return option ? utils.deepAccess(_getConfig(), option) : _getConfig();
}

return subscribe(...args);
}

/**
* Internal API for modules (such as prebid-server) that might need access to all bidder config
*/
function getBidderConfig() {
return bidderConfig;
}

/*
* Sets configuration given an object containing key-value pairs and calls
* listeners that were added by the `subscribe` function
*/
function setConfig(options) {
if (typeof options !== 'object') {
if (!utils.isPlainObject(options)) {
utils.logError('setConfig options must be an object');
return;
}
Expand All @@ -239,7 +282,7 @@ export function newConfig() {
topics.forEach(topic => {
let option = options[topic];

if (typeof defaults[topic] === 'object' && typeof option === 'object') {
if (utils.isPlainObject(defaults[topic]) && utils.isPlainObject(option)) {
option = Object.assign({}, defaults[topic], option);
}

Expand All @@ -254,7 +297,7 @@ export function newConfig() {
* @param {object} options
*/
function setDefaults(options) {
if (typeof defaults !== 'object') {
if (!utils.isPlainObject(defaults)) {
utils.logError('defaults must be an object');
return;
}
Expand Down Expand Up @@ -327,13 +370,68 @@ export function newConfig() {
.forEach(listener => listener.callback(options));
}

function setBidderConfig(config) {
try {
check(config);
config.bidders.forEach(bidder => {
if (!bidderConfig[bidder]) {
bidderConfig[bidder] = {};
}
Object.keys(config.config).forEach(topic => {
let option = config.config[topic];
if (utils.isPlainObject(option)) {
bidderConfig[bidder][topic] = Object.assign({}, bidderConfig[bidder][topic] || {}, option);
} else {
bidderConfig[bidder][topic] = option;
}
});
});
} catch (e) {
utils.logError(e);
}
function check(obj) {
if (!utils.isPlainObject(obj)) {
throw 'setBidderConfig bidder options must be an object';
}
if (!(Array.isArray(obj.bidders) && obj.bidders.length)) {
throw 'setBidderConfig bidder options must contain a bidders list with at least 1 bidder';
}
if (!utils.isPlainObject(obj.config)) {
throw 'setBidderConfig bidder options must contain a config object';
}
}
}

/**
* Internal functions for core to execute some synchronous code while having an active bidder set.
*/
function runWithBidder(bidder, fn) {
currBidder = bidder;
try {
return fn();
} finally {
currBidder = null;
Copy link
Member

Choose a reason for hiding this comment

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

nice bit of code here!

}
}
function callbackWithBidder(bidder) {
return function(cb) {
return function(...args) {
return runWithBidder(bidder, utils.bind.call(cb, this, ...args))
}
}
}

resetConfig();

return {
getConfig,
setConfig,
setDefaults,
resetConfig
resetConfig,
runWithBidder,
callbackWithBidder,
setBidderConfig,
getBidderConfig
Copy link
Member

Choose a reason for hiding this comment

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

we just have to make sure no adapter accesses this config method correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes. they'd have to import it directly so it'd be pretty obvious. maybe could add a linter check that bid adapters can't import this method.

};
}

Expand Down
1 change: 1 addition & 0 deletions src/prebid.js
Original file line number Diff line number Diff line change
Expand Up @@ -780,6 +780,7 @@ $$PREBID_GLOBAL$$.getConfig = config.getConfig;
* ```
*/
$$PREBID_GLOBAL$$.setConfig = config.setConfig;
$$PREBID_GLOBAL$$.setBidderConfig = config.setBidderConfig;
Copy link
Member

Choose a reason for hiding this comment

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

what was the thought process of making this a separate method vs using another argument on setConfig?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No major reason I think. I thought being a little more explicit was nice, but I could be happy with just using another argument as well.


$$PREBID_GLOBAL$$.que.push(() => listenMessagesFromCreative());

Expand Down
2 changes: 1 addition & 1 deletion src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,7 @@ export function delayExecution(func, numRequiredCalls) {
return function () {
numCalls++;
if (numCalls === numRequiredCalls) {
func.apply(null, arguments);
func.apply(this, arguments);
}
}
}
Expand Down
Loading