From fc2d2687adbbc299c8c94736f09d2ce2a5c10285 Mon Sep 17 00:00:00 2001 From: Ronen Stern Date: Wed, 5 Apr 2017 16:51:57 +0300 Subject: [PATCH 1/2] Remove batching mechanism and use AJAX instead of JSONP --- src/adapters/hiromedia.js | 259 ++++++++++----------------- test/spec/adapters/hiromedia_spec.js | 141 ++++++++------- 2 files changed, 166 insertions(+), 234 deletions(-) diff --git a/src/adapters/hiromedia.js b/src/adapters/hiromedia.js index 3d37acc0f45..28c4800d93b 100644 --- a/src/adapters/hiromedia.js +++ b/src/adapters/hiromedia.js @@ -1,18 +1,18 @@ /*jslint white:true, browser:true, single: true*/ -/*global $$PREBID_GLOBAL$$, require, module*/ +/*global require, module*/ /** * Adapter for HIRO Media * * @module HiroMediaAdapter * - * @requires src/adloader + * @requires src/ajax * @requires src/bidfactory * @requires src/bidmanager * @requires src/constants * @requires src/utils */ -var adloader = require('src/adloader'); +var Ajax = require('src/ajax'); var bidfactory = require('src/bidfactory'); var bidmanager = require('src/bidmanager'); var utils = require('src/utils'); @@ -44,7 +44,7 @@ var HiroMediaAdapter = function HiroMediaAdapter() { * Default bid param values * * @memberof module:HiroMediaAdapter~ - * @constant {module:HiroMediaAdapter~bidParams} + * @constant {array.} * @private */ var REQUIRED_BID_PARAMS = ['accountId']; @@ -60,18 +60,6 @@ var HiroMediaAdapter = function HiroMediaAdapter() { bidUrl: 'https://hb-rtb.ktdpublishers.com/bid/get' }; - /** - * Storage for bid objects. - * - * Bids need to be stored between requests and response since the response - * is a global callback. - * - * @memberof module:HiroMediaAdapter~ - * @var {array.} - * @private - */ - var _bidStorage = []; - /** * Call bidmanager.addBidResponse * @@ -81,15 +69,15 @@ var HiroMediaAdapter = function HiroMediaAdapter() { * @memberof module:HiroMediaAdapter~ * @private * - * @param {module:HiroMediaAdapter~bidInfo} bidInfo bid object wrapper to respond for + * @param {object} bid bid object connected to the response * @param {object|boolean} [bidResponse] response object for bid, if not * set the response will be an empty bid response. */ - function addBidResponse(bidInfo, bidResponse) { + function addBidResponse(bid, bidResponse) { - var placementCode = bidInfo.bid.placementCode; + var placementCode = bid.placementCode; var bidStatus = bidResponse ? STATUS.GOOD : STATUS.NO_BID; - var bidObject = bidfactory.createBid(bidStatus, bidInfo.bid); + var bidObject = bidfactory.createBid(bidStatus, bid); bidObject.bidderCode = BIDDER_CODE; @@ -124,40 +112,20 @@ var HiroMediaAdapter = function HiroMediaAdapter() { * @memberof module:HiroMediaAdapter~ * @private * - * @param {object} response [description] + * @param {object} response bid response object + * @param {object} bid bid object connected to response */ - function handleResponse(response) { - - _bidStorage.filter(function (bidInfo) { - return bidInfo.batchKey === response.batchKey; - }).forEach(function (bidInfo) { - - // Sample the bid responses according to `response.chance`, - // if `response.chance` is not provided, sample at 100%. - if (response.chance === undefined || checkChance(response.chance)) { - addBidResponse(bidInfo, response); - } else { - addBidResponse(bidInfo, false); - } + function handleResponse(response, bid) { - }); - - } - /** - * Call {@linkcode module:HiroMediaAdapter~handleResponse} for valid responses - * - * @global - * - * @param {object} [response] the response from the server - */ - $$PREBID_GLOBAL$$.hiromedia_callback = function (response) { - - if (response && response.batchKey) { - handleResponse(response); + // Sample the bid responses according to `response.chance`, + // if `response.chance` is not provided, sample at 100%. + if (response.chance === undefined || checkChance(response.chance)) { + addBidResponse(bid, response); + } else { + addBidResponse(bid, false); } - - }; + } /** * Find browser name and version @@ -250,108 +218,52 @@ var HiroMediaAdapter = function HiroMediaAdapter() { } /** - * Calculate and return a batchKey key for a bid - * - * Bid of similar placement can have similar responses, - * we can calculate a key based on the variant properties - * of a bid which can share the same response + * Build a {@linkcode module:HiroMediaAdapter~bidInfo|bidInfo} object based on a + * bid sent to {@linkcode module:HiroMediaAdapter#callBids|callBids} * * @memberof module:HiroMediaAdapter~ * @private * - * @param {module:HiroMediaAdapter~bidInfo} bidInfo bid information - * @return {string} batch key for bid + * @param {object} bid bid from `Prebid.js` + * @return {module:HiroMediaAdapter~bidInfo} information for bid request */ - function getBatchKey(bidInfo) { - - var bidParams = bidInfo.bidParams; - var batchParams = [ - bidParams.bidUrl, - bidParams.accountId, - bidInfo.selectedSize, - bidInfo.additionalSizes - ]; - - return batchParams.join('-'); - - } - - /** - * Build a set of {@linkcode module:HiroMediaAdapter~bidInfo|bidInfo} objects based on the - * bids sent to {@linkcode module:HiroMediaAdapter#callBids|callBids} - * - * @memberof module:HiroMediaAdapter~ - * @private - * - * @param {object} bids bids sent from `Prebid.js` - * @return {array.} wrapped bids - */ - function processBids(bids) { - - var result = []; - - if (bids) { - - utils.logMessage('hiromedia.processBids, processing ' + bids.length + ' bids'); - - bids.forEach(function (bid) { - - var sizes = utils.parseSizesInput(bid.sizes); - var bidParams = defaultParams(bid.params); - var hasValidBidRequest = utils.hasValidBidRequest(bidParams, REQUIRED_BID_PARAMS, BIDDER_CODE); - var shouldBid = hasValidBidRequest; - var bidInfo = { - bid: bid, - bidParams: bidParams, - shouldBid: shouldBid, - selectedSize: sizes[0], - additionalSizes: sizes.slice(1).join(',') - }; - - if (shouldBid) { - bidInfo.batchKey = getBatchKey(bidInfo); - } - - result.push(bidInfo); - - }); - - } + function processBid(bid) { + + var sizes = utils.parseSizesInput(bid.sizes); + var bidParams = defaultParams(bid.params); + var hasValidBidRequest = utils.hasValidBidRequest(bidParams, REQUIRED_BID_PARAMS, BIDDER_CODE); + var shouldBid = hasValidBidRequest; + var bidInfo = { + bidParams: bidParams, + shouldBid: shouldBid, + selectedSize: sizes[0], + additionalSizes: sizes.slice(1).join(',') + }; - return result; + return bidInfo; } /** - * Send a bid request to the bid server endpoint - * - * Calls `adLoader.loadScript` + * Wrapper around `JSON.parse()` that returns false on error * * @memberof module:HiroMediaAdapter~ * @private * - * @param {string} url base url, can already contain query parameters - * @param {object} requestParams parameters to add to query + * @param {string} text potential JSON string to convert to object + * @return {object|boolean} object parsed from text or `false` in case of and error */ - function sendBidRequest(url,requestParams) { - - if (requestParams) { + function tryJson(text) { - if (url.indexOf('?') !== -1) { - url = url + '&'; - } else { - url = url + '?'; - } - - Object.keys(requestParams).forEach(function (key) { - url = utils.tryAppendQueryString(url, key, requestParams[key]); - }); + var object = false; + try { + object = JSON.parse(text); + } catch (ignore) { + // Ignored } - utils.logMessage('hiromedia.callBids, url:' + url); - - adloader.loadScript(url); + return object; } @@ -366,61 +278,72 @@ var HiroMediaAdapter = function HiroMediaAdapter() { var browser = getBrowser(); var domain = getDomain(); - var bidsRequested = {}; + var bids = params && params.bids; + var ajaxOptions = { + method: 'GET', + withCredentials: true + }; + + // Fixed data, shared by all requests + var fixedRequest = { + adapterVersion: ADAPTER_VERSION, + browser: browser.name, + browserVersion: browser.version, + domain: domain + }; utils.logMessage('hiromedia.callBids'); - if (params) { + if (bids && bids.length) { - // Processed bids are stored in the adapter scope - _bidStorage = processBids(params.bids); + bids.forEach(function (bid) { - } else { + var bidInfo = processBid(bid); - // Ensure we don't run on stale data - _bidStorage = []; + var bidParams = bidInfo.bidParams; + utils.logMessage('hiromedia.callBids, bidInfo ' + bid.placementCode + ' ' + bidInfo.shouldBid); - } + if (bidInfo.shouldBid) { - if (_bidStorage.length) { + var url = bidParams.bidUrl; + var requestParams = Object.assign({}, fixedRequest, bidInfo.bidParams, { + placementCode: bid.placementCode, + selectedSize: bidInfo.selectedSize, + additionalSizes: bidInfo.additionalSizes + }); - // Loop over processed bids and send a request if a request for the bid - // batchKey has not been sent. - _bidStorage.forEach(function (bidInfo) { + Object.keys(requestParams).forEach(function (key){ + if (requestParams[key] === '' || requestParams[key] === undefined) { + delete requestParams[key]; + } + }); - var bid = bidInfo.bid; - var batchKey = bidInfo.batchKey; - var bidParams = bidInfo.bidParams; + utils.logMessage('hiromedia.callBids, bid request ' + url + ' ' + JSON.stringify(bidInfo.bidRequest)); - utils.logMessage('hiromedia.callBids, bidInfo ' + bid.placementCode + ' ' + bidInfo.shouldBid); + Ajax.ajax(url, { - if (bidInfo.shouldBid) { + success: function(responseText) { - var url = bidParams.bidUrl; + var response = tryJson(responseText); + + handleResponse(response, bid); + + }, + + error: function(err, xhr) { - if (!bidsRequested[batchKey]) { + utils.logError('hiromedia.callBids, bid request error', xhr.status, err); - bidsRequested[batchKey] = true; + addBidResponse(bid, false); - sendBidRequest(url,{ - adapterVersion: ADAPTER_VERSION, - callback: '$$PREBID_GLOBAL$$.hiromedia_callback', - batchKey: batchKey, - placementCode: bid.placementCode, - accountId: bidParams.accountId, - browser: browser.name, - browserVersion: browser.version, - domain: domain, - selectedSize: bidInfo.selectedSize, - additionalSizes: bidInfo.additionalSizes - }); + } - } + }, requestParams, ajaxOptions); } else { // No bid - addBidResponse(bidInfo, false); + addBidResponse(bid, false); } @@ -452,10 +375,8 @@ var HiroMediaAdapter = function HiroMediaAdapter() { * @typedef {object} module:HiroMediaAdapter~bidInfo * @private * - * @property {object} bid original bid passed to #callBids * @property {string} selectedSize the first size in the the placement sizes array * @property {string} additionalSizes list of sizes in the placement sizes array besides the first - * @property {string} batchKey key used for batching requests which have the same basic properties * @property {module:HiroMediaAdapter~bidParams} bidParams original params passed for bid in #callBids * @property {boolean} shouldBid flag to determine if the bid is valid for bidding or not */ diff --git a/test/spec/adapters/hiromedia_spec.js b/test/spec/adapters/hiromedia_spec.js index b91bcca38ac..5774ec10e19 100644 --- a/test/spec/adapters/hiromedia_spec.js +++ b/test/spec/adapters/hiromedia_spec.js @@ -2,10 +2,8 @@ /*jshint esversion:6*/ import { expect } from 'chai'; -import querystringify from 'querystringify'; import urlParse from 'url-parse'; -import adloader from 'src/adloader'; import Adapter from 'src/adapters/hiromedia'; import bidmanager from 'src/bidmanager'; import { STATUS } from 'src/constants'; @@ -14,12 +12,11 @@ import * as utils from 'src/utils'; describe('hiromedia adapter', function () { const BIDDER_CODE = 'hiromedia'; - const DEFAULT_CALLBACK_NAME = 'hiromedia_callback'; const DEFAULT_ENDPOINT = 'https://hb-rtb.ktdpublishers.com/bid/get'; let adapter; let sandbox; - let loadScriptStub; + let xhr; let addBidResponseStub; let hasValidBidRequestSpy; let placementId = 0; @@ -32,7 +29,7 @@ describe('hiromedia adapter', function () { sandbox = sinon.sandbox.create(); // Used to spy on bid requests - loadScriptStub = sandbox.stub(adloader, 'loadScript'); + xhr = sandbox.useFakeXMLHttpRequest(); // Used to spy on bid responses addBidResponseStub = sandbox.stub(bidmanager, 'addBidResponse'); @@ -52,7 +49,7 @@ describe('hiromedia adapter', function () { // was made during a test. const assertNoBids = () => { - sinon.assert.notCalled(loadScriptStub); + expect(xhr.requests.length).to.be.equal(0); sinon.assert.notCalled(addBidResponseStub); }; @@ -103,40 +100,39 @@ describe('hiromedia adapter', function () { it('invokes a bid request per placement', () => { const expectedRequests = [{ - batchKey: [DEFAULT_ENDPOINT,'1337','300x250',''].join('-'), placementCode: 'div-gpt-ad-12345-1', selectedSize: '300x250' }, { - batchKey: [DEFAULT_ENDPOINT,'1337','728x90',''].join('-'), placementCode: 'div-gpt-ad-12345-2', selectedSize: '728x90' + }, { + placementCode: 'div-gpt-ad-12345-3', + selectedSize: '300x250' }]; const params = { - bids: [tilePlacement(), leaderPlacement()] + bids: [tilePlacement(), leaderPlacement(), tilePlacement()] }; adapter.callBids(params); - sinon.assert.calledTwice(loadScriptStub); + expect(xhr.requests.length).to.equal(3); sinon.assert.notCalled(addBidResponseStub); - sinon.assert.calledTwice(hasValidBidRequestSpy); + sinon.assert.calledThrice(hasValidBidRequestSpy); expectedRequests.forEach(function(request, index) { expect(hasValidBidRequestSpy.returnValues[index]).to.be.equal(true); // validate request - const bidRequest = loadScriptStub.getCall(index).args[0]; + const bidRequest = xhr.requests[index].url; const defaultBidUrl = urlParse(DEFAULT_ENDPOINT); - const bidUrl = urlParse(bidRequest); - const query = querystringify.parse(bidUrl.query); + const bidUrl = urlParse(bidRequest, true); + const query = bidUrl.query; expect(bidUrl.hostname).to.equal(defaultBidUrl.hostname); expect(bidUrl.pathname).to.equal(defaultBidUrl.pathname); expect(query).to.have.property('adapterVersion').and.to.equal('3'); - expect(query).to.have.property('callback').and.to.equal('$$PREBID_GLOBAL$$.' + DEFAULT_CALLBACK_NAME); - expect(query).to.have.property('batchKey').and.to.equal(request.batchKey); expect(query).to.have.property('placementCode').and.to.equal(request.placementCode); expect(query).to.have.property('accountId').and.to.equal('1337'); expect(query).to.have.property('selectedSize').and.to.equal(request.selectedSize); @@ -161,11 +157,11 @@ describe('hiromedia adapter', function () { }; adapter.callBids(params); - sinon.assert.calledOnce(loadScriptStub); + expect(xhr.requests.length).to.be.equal(1); - const bidRequest = loadScriptStub.getCall(0).args[0]; - const bidUrl = urlParse(bidRequest); - const query = querystringify.parse(bidUrl.query); + const bidRequest = xhr.requests[0].url; + const bidUrl = urlParse(bidRequest, true); + const query = bidUrl.query; expect(query).to.have.property('selectedSize').and.to.equal('300x250'); expect(query).to.have.property('additionalSizes').and.to.equal('300x600,300x900'); @@ -183,9 +179,16 @@ describe('hiromedia adapter', function () { }; adapter.callBids(params); - sinon.assert.notCalled(loadScriptStub); + expect(xhr.requests.length).to.be.equal(0); sinon.assert.calledOnce(hasValidBidRequestSpy); + sinon.assert.calledOnce(addBidResponseStub); + expect(hasValidBidRequestSpy.returnValues[0]).to.be.equal(false); + const placementCode = addBidResponseStub.getCall(0).args[0]; + const bidObject = addBidResponseStub.getCall(0).args[1]; + + expect(placementCode).to.be.equal('div-gpt-ad-12345-1'); + expect(bidObject.getStatusCode()).to.be.equal(STATUS.NO_BID); }); @@ -200,80 +203,89 @@ describe('hiromedia adapter', function () { }; adapter.callBids(params); - sinon.assert.calledOnce(loadScriptStub); + expect(xhr.requests.length).to.be.equal(1); - const bidRequest = loadScriptStub.getCall(0).args[0]; + const bidRequest = xhr.requests[0].url; const defaultBidUrl = urlParse(DEFAULT_ENDPOINT); - const bidUrl = urlParse(bidRequest); - const query = querystringify.parse(bidUrl.query); + const bidUrl = urlParse(bidRequest, true); + const query = bidUrl.query; expect(bidUrl.hostname).to.equal(defaultBidUrl.hostname); expect(bidUrl.pathname).to.equal(defaultBidUrl.pathname); expect(query).to.have.property('someparam').and.to.equal('value'); - }); + }); - it('batches similar bid requests for similar sized placements', () => { + }); - const params = { - bids: [tilePlacement(), tilePlacement()] - }; + describe('response handler', () => { - adapter.callBids(params); - sinon.assert.calledOnce(loadScriptStub); // and only once! + let server; + beforeEach(() => { + server = sandbox.useFakeServer(); }); - }); + const assertSingleFailedBidResponse = () => { - describe('global response handler', () => { + sinon.assert.calledOnce(addBidResponseStub); - const getPbjs = () => window.$$PREBID_GLOBAL$$; - const getResponseHandler = () => window.$$PREBID_GLOBAL$$[DEFAULT_CALLBACK_NAME]; + const placementCode = addBidResponseStub.getCall(0).args[0]; + const bidObject = addBidResponseStub.getCall(0).args[1]; + + expect(placementCode).to.be.equal('div-gpt-ad-12345-1'); + expect(bidObject.getStatusCode()).to.be.equal(STATUS.NO_BID); + + }; + + it('tolerates an empty response', () => { + + server.respondWith(''); + adapter.callBids({bids: [tilePlacement()]}); + server.respond(); + + assertSingleFailedBidResponse(); - it('exists and is a function', () => { - expect(getResponseHandler()).to.exist.and.to.be.a('function'); }); - it('tolerates empty arguments', () => { - expect(getResponseHandler()).to.not.throw(Error); + it('tolerates a response error', () => { + + server.respondWith([500, {}, '']); + adapter.callBids({bids: [tilePlacement()]}); + server.respond(); + + assertSingleFailedBidResponse(); + }); - it('tolerates an empty response', () => { - expect(getResponseHandler().bind(getPbjs(), {})).to.not.throw(Error); + it('tolerates an invalid response', () => { + + server.respondWith('not json'); + adapter.callBids({bids: [tilePlacement()]}); + server.respond(); + + assertSingleFailedBidResponse(); + }); - // This test is coupled with `callBids`, this is done - // to ensure that the response handler is able to - // add bid responses for each placement with the same - // batch key. - // To do this, we have to have the internal state of - // the adapter set up correctly. it('adds a bid reponse for each pending bid', () => { - const expectedResponses = [{ - batchKey: [DEFAULT_ENDPOINT,'1337','300x250',''].join('-'), + const responses = [{ width: '300', height: '250', cpm: 0.4, ad: '' }, { - batchKey: [DEFAULT_ENDPOINT,'1337','728x90',''].join('-'), width: '728', height: '90', cpm: 0.4, ad: '' }]; - - // Instead of the dead stub defined in the top scope, we'll use - // one that mocks a response. - loadScriptStub.restore(); let id = 0; - const activeLoadScriptStub = sandbox.stub(adloader, 'loadScript', (url) => { - const handler = getResponseHandler(); - handler(expectedResponses[id]); + server.respondWith((request) => { + request.respond(200, {}, JSON.stringify(responses[id])); id += 1; }); @@ -282,11 +294,12 @@ describe('hiromedia adapter', function () { }; adapter.callBids(params); + server.respond(); - sinon.assert.calledTwice(activeLoadScriptStub); + expect(server.requests.length).to.be.equal(2); sinon.assert.calledTwice(addBidResponseStub); - expectedResponses.forEach((expectedResponse, i) => { + responses.forEach((expectedResponse, i) => { const placementCode = addBidResponseStub.getCall(i).args[0]; const bidObject = addBidResponseStub.getCall(i).args[1]; @@ -310,7 +323,6 @@ describe('hiromedia adapter', function () { it('adds responses according to the sampling defined in the response', () => { const response = { - batchKey: [DEFAULT_ENDPOINT,'1337','300x250',''].join('-'), cpm: 0.4, chance: 0.25, ad: '' @@ -321,9 +333,7 @@ describe('hiromedia adapter', function () { const randomValues = [0.2, 0.3]; let randomIndex = 0; - loadScriptStub.restore(); - const activeLoadScriptStub = sandbox.stub(adloader, 'loadScript', (url) => { - const handler = getResponseHandler(); + server.respondWith((request) => { const mathRandomStub = sandbox.stub(Math, 'random', function () { @@ -336,7 +346,7 @@ describe('hiromedia adapter', function () { }); - handler(response); + request.respond(200, {}, JSON.stringify(response)); mathRandomStub.restore(); @@ -348,6 +358,7 @@ describe('hiromedia adapter', function () { adapter.callBids(params); adapter.callBids(params); + server.respond(); sinon.assert.calledTwice(addBidResponseStub); From 0941f8dc63355281ad28e2224aea65e1f3a5c2f0 Mon Sep 17 00:00:00 2001 From: Ronen Stern Date: Wed, 3 May 2017 15:30:26 +0300 Subject: [PATCH 2/2] Fix `undefined` value checks --- src/adapters/hiromedia.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/adapters/hiromedia.js b/src/adapters/hiromedia.js index 28c4800d93b..fbabcd59f36 100644 --- a/src/adapters/hiromedia.js +++ b/src/adapters/hiromedia.js @@ -60,6 +60,19 @@ var HiroMediaAdapter = function HiroMediaAdapter() { bidUrl: 'https://hb-rtb.ktdpublishers.com/bid/get' }; + /** + * Returns true if the given value is `undefined` + * + * @memberof module:HiroMediaAdapter~ + * @private + * + * @param {*} value value to check + * @return {boolean} true if the given value is `undefined`, false otherwise + */ + function isUndefined(value) { + return typeof value === 'undefined'; + } + /** * Call bidmanager.addBidResponse * @@ -120,7 +133,7 @@ var HiroMediaAdapter = function HiroMediaAdapter() { // Sample the bid responses according to `response.chance`, // if `response.chance` is not provided, sample at 100%. - if (response.chance === undefined || checkChance(response.chance)) { + if (isUndefined(response.chance) || checkChance(response.chance)) { addBidResponse(bid, response); } else { addBidResponse(bid, false); @@ -313,7 +326,7 @@ var HiroMediaAdapter = function HiroMediaAdapter() { }); Object.keys(requestParams).forEach(function (key){ - if (requestParams[key] === '' || requestParams[key] === undefined) { + if (requestParams[key] === '' || isUndefined(requestParams[key])) { delete requestParams[key]; } });