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.

Sign up for GitHub

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

Jump to bottom

Make response headers available to the specs #1748

Merged
merged 5 commits into from
Oct 24, 2017
Merged

Make response headers available to the specs #1748

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7171fa0
Updated the bidderFactory to make headers accessible to the spec files.
dbemiller Oct 23, 2017
b6e42bd
Merge branch 'master' of https://github.com/prebid/Prebid.js into for…
dbemiller Oct 23, 2017
71b8599
Updated the platform.io adapter to handle the new format.
dbemiller Oct 23, 2017
fc4692b
Merge branch 'master' of https://github.com/prebid/Prebid.js into for…
dbemiller Oct 24, 2017
1ece44f
Updated the jcm adapter.
dbemiller Oct 24, 2017
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
1 change: 1 addition & 0 deletions modules/adbutlerBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export const spec = {
var width;
var height;

serverResponse = serverResponse.body;
if (serverResponse && serverResponse.status === 'SUCCESS' && bidObj) {
CPM = serverResponse.cpm;
minCPM = utils.getBidIdParameter('minCPM', bidObj.params);
Expand Down
1 change: 1 addition & 0 deletions modules/appnexusAstBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export const spec = {
* @return {Bid[]} An array of bids which were nested inside the server.
*/
interpretResponse: function(serverResponse, {bidderRequest}) {
serverResponse = serverResponse.body;
const bids = [];
if (!serverResponse || serverResponse.error) {
let errorMessage = `in response for ${bidderRequest.bidderCode} adapter`;
Expand Down
1 change: 1 addition & 0 deletions modules/beachfrontBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export const spec = {
},

interpretResponse(response, { bidRequest }) {
response = response.body;
if (!response || !response.url || !response.bidPrice) {
utils.logWarn(`No valid bids from ${spec.code} bidder`);
return [];
Expand Down
1 change: 1 addition & 0 deletions modules/pulsepointLiteBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export const spec = {
function bidResponseAvailable(bidRequest, bidResponse) {
const idToImpMap = {};
const idToBidMap = {};
bidResponse = bidResponse.body
// extract the request bids and the response bids, keyed by impr-id
const ortbRequest = parse(bidRequest.data);
ortbRequest.imp.forEach(imp => {
Expand Down
1 change: 1 addition & 0 deletions modules/rubiconBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ export const spec = {
* @return {Bid[]} An array of bids which
*/
interpretResponse: function(responseObj, {bidRequest}) {
responseObj = responseObj.body
let ads = responseObj.ads;

// check overall response
Expand Down
25 changes: 23 additions & 2 deletions src/adapters/bidderFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import { logWarn, logError, parseQueryStringParameters, delayExecution } from 's
* @property {function(BidRequest[], bidderRequest): ServerRequest|ServerRequest[]} buildRequests Build the request to the Server
* which requests Bids for the given array of Requests. Each BidRequest in the argument array is guaranteed to have
* passed the isBidRequestValid() test.
* @property {function(*, BidRequest): Bid[]} interpretResponse Given a successful response from the Server,
* @property {function(ServerResponse, BidRequest): Bid[]} interpretResponse Given a successful response from the Server,
* interpret it and return the Bid objects. This function will be run inside a try/catch.
* If it throws any errors, your bids will be discarded.
* @property {function(SyncOptions, Array): UserSync[]} [getUserSyncs] Given an array of all the responses
Expand Down Expand Up @@ -72,6 +72,15 @@ import { logWarn, logError, parseQueryStringParameters, delayExecution } from 's
* JSON-serialized into the Request body.
*/

/**
* @typedef {object} ServerResponse
*
* @property {*} body The response body. If this is legal JSON, then it will be parsed. Otherwise it'll be a
* string with the body's content.
* @property {{get: function(string): string} headers The response headers.
* Call this like `ServerResponse.headers.get("Content-Type")`
*/

/**
* @typedef {object} Bid
*
Expand Down Expand Up @@ -263,10 +272,16 @@ export function newBidder(spec) {
// If the server responds successfully, use the adapter code to unpack the Bids from it.
// If the adapter code fails, no bids should be added. After all the bids have been added, make
// sure to call the `onResponse` function so that we're one step closer to calling fillNoBids().
function onSuccess(response) {
function onSuccess(response, responseObj) {
try {
response = JSON.parse(response);
} catch (e) { /* response might not be JSON... that's ok. */ }

// Make response headers available for #1742. These are lazy-loaded because most adapters won't need them.
response = {
body: response,
headers: headerParser(responseObj)
};
responses.push(response);

let bids;
Expand Down Expand Up @@ -296,6 +311,12 @@ export function newBidder(spec) {
logWarn(`Bidder ${spec.code} made bid for unknown request ID: ${bid.requestId}. Ignoring.`);
}
}

function headerParser(xmlHttpResponse) {
return {
get: responseObj.getResponseHeader.bind(responseObj)
Copy link
Member

Choose a reason for hiding this comment

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

why do a bind here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paranoia... because I've been burned by this bugs too many times.

The root of the issue is: I don't know how browsers implement XMLHttpRequest.getResponseHeader.

Suppose I just did:

return {
  get: responseObj.getResponseHeader
}

but then they implemented it as something like this:

XmlHttpRequest.prototype.getResponseHeader = function(header) {
  var headers = this.parseResponseHeaders();
  return headers[header];
}

When an adapter calls get, then this evaluates to our object--not the original XmlHttpRequest. Since we don't define parseResponseHeaders, it'd crash on an undefined function error.

I have no idea if this is a real risk... but the tl;dr is that this sucks. Since I can't eject it from the language, defensive binds keep the code safe.

Copy link
Member

Choose a reason for hiding this comment

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

ah ok cool

};
}
}

// If the server responds with an error, there's not much we can do. Log it, and make sure to
Expand Down
24 changes: 13 additions & 11 deletions test/spec/modules/adbutlerBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,19 @@ describe('AdButler adapter', () => {
describe('bid responses', () => {
it('should return complete bid response', () => {
let serverResponse = {
status: 'SUCCESS',
account_id: 167283,
zone_id: 210093,
cpm: 1.5,
width: 300,
height: 250,
place: 0,
ad_code: '<img src="http://image.source.com/img" alt="" title="" border="0" width="300" height="250">',
tracking_pixels: [
'http://tracking.pixel.com/params=info'
]
body: {
status: 'SUCCESS',
account_id: 167283,
zone_id: 210093,
cpm: 1.5,
width: 300,
height: 250,
place: 0,
ad_code: '<img src="http://image.source.com/img" alt="" title="" border="0" width="300" height="250">',
tracking_pixels: [
'http://tracking.pixel.com/params=info'
]
}
},
bids = spec.interpretResponse(serverResponse, {'bidRequest': bidRequests[0]});

Expand Down
8 changes: 4 additions & 4 deletions test/spec/modules/appnexusAstBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ describe('AppNexusAdapter', () => {
];
let bidderRequest;

let result = spec.interpretResponse(response, {bidderRequest});
let result = spec.interpretResponse({ body: response }, {bidderRequest});
expect(Object.keys(result[0])).to.deep.equal(Object.keys(expectedResponse[0]));
});

Expand All @@ -322,7 +322,7 @@ describe('AppNexusAdapter', () => {
};
let bidderRequest;

let result = spec.interpretResponse(response, {bidderRequest});
let result = spec.interpretResponse({ body: response }, {bidderRequest});
expect(result.length).to.equal(0);
});

Expand All @@ -343,7 +343,7 @@ describe('AppNexusAdapter', () => {
};
let bidderRequest;

let result = spec.interpretResponse(response, {bidderRequest});
let result = spec.interpretResponse({ body: response }, {bidderRequest});
expect(result[0]).to.have.property('vastUrl');
expect(result[0]).to.have.property('descriptionUrl');
expect(result[0]).to.have.property('mediaType', 'video');
Expand Down Expand Up @@ -376,7 +376,7 @@ describe('AppNexusAdapter', () => {
};
let bidderRequest;

let result = spec.interpretResponse(response1, {bidderRequest});
let result = spec.interpretResponse({ body: response1 }, {bidderRequest});
expect(result[0].native.title).to.equal('Native Creative');
expect(result[0].native.body).to.equal('Cool description great stuff');
expect(result[0].native.cta).to.equal('Do it');
Expand Down
8 changes: 4 additions & 4 deletions test/spec/modules/beachfrontBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,23 +104,23 @@ describe('BeachfrontAdapter', () => {

describe('spec.interpretResponse', () => {
it('should return no bids if the response is not valid', () => {
const bidResponse = spec.interpretResponse(null, { bidRequest });
const bidResponse = spec.interpretResponse({ body: null }, { bidRequest });
expect(bidResponse.length).to.equal(0);
});

it('should return no bids if the response "url" is missing', () => {
const serverResponse = {
bidPrice: 5.00
};
const bidResponse = spec.interpretResponse(serverResponse, { bidRequest });
const bidResponse = spec.interpretResponse({ body: serverResponse }, { bidRequest });
expect(bidResponse.length).to.equal(0);
});

it('should return no bids if the response "bidPrice" is missing', () => {
const serverResponse = {
url: 'http://reachms.bfmio.com/getmu?aid=bid:19c4a196-fb21-4c81-9a1a-ecc5437a39da'
};
const bidResponse = spec.interpretResponse(serverResponse, { bidRequest });
const bidResponse = spec.interpretResponse({ body: serverResponse }, { bidRequest });
expect(bidResponse.length).to.equal(0);
});

Expand All @@ -130,7 +130,7 @@ describe('BeachfrontAdapter', () => {
url: 'http://reachms.bfmio.com/getmu?aid=bid:19c4a196-fb21-4c81-9a1a-ecc5437a39da',
cmpId: '123abc'
};
const bidResponse = spec.interpretResponse(serverResponse, { bidRequest });
const bidResponse = spec.interpretResponse({ body: serverResponse }, { bidRequest });
expect(bidResponse).to.deep.equal({
requestId: bidRequest.bidId,
bidderCode: spec.code,
Expand Down
6 changes: 3 additions & 3 deletions test/spec/modules/pulsepointLiteBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ describe('PulsePoint Lite Adapter Tests', () => {
}]
}]
};
const bids = spec.interpretResponse(ortbResponse, request);
const bids = spec.interpretResponse({ body: ortbResponse }, request);
expect(bids).to.have.lengthOf(1);
// verify first bid
const bid = bids[0];
Expand All @@ -104,7 +104,7 @@ describe('PulsePoint Lite Adapter Tests', () => {

it('Verify full passback', () => {
const request = spec.buildRequests(slotConfigs);
const bids = spec.interpretResponse(null, request)
const bids = spec.interpretResponse({ body: null }, request)
expect(bids).to.have.lengthOf(0);
});

Expand Down Expand Up @@ -171,7 +171,7 @@ describe('PulsePoint Lite Adapter Tests', () => {
}]
}]
};
const bids = spec.interpretResponse(ortbResponse, request);
const bids = spec.interpretResponse({ body: ortbResponse }, request);
// verify bid
const bid = bids[0];
expect(bid.cpm).to.equal(1.25);
Expand Down
12 changes: 6 additions & 6 deletions test/spec/modules/rubiconBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ describe('the rubicon adapter', () => {
]
};

let bids = spec.interpretResponse(response, {
let bids = spec.interpretResponse({ body: response }, {
bidRequest: bidderRequest.bids[0]
});

Expand Down Expand Up @@ -654,7 +654,7 @@ describe('the rubicon adapter', () => {
}]
};

let bids = spec.interpretResponse(response, {
let bids = spec.interpretResponse({ body: response }, {
bidRequest: bidderRequest.bids[0]
});

Expand All @@ -677,7 +677,7 @@ describe('the rubicon adapter', () => {
'ads': []
};

let bids = spec.interpretResponse(response, {
let bids = spec.interpretResponse({ body: response }, {
bidRequest: bidderRequest.bids[0]
});

Expand All @@ -701,7 +701,7 @@ describe('the rubicon adapter', () => {
}]
};

let bids = spec.interpretResponse(response, {
let bids = spec.interpretResponse({ body: response }, {
bidRequest: bidderRequest.bids[0]
});

Expand All @@ -711,7 +711,7 @@ describe('the rubicon adapter', () => {
it('should handle an error because of malformed json response', () => {
let response = '{test{';

let bids = spec.interpretResponse(response, {
let bids = spec.interpretResponse({ body: response }, {
bidRequest: bidderRequest.bids[0]
});

Expand Down Expand Up @@ -752,7 +752,7 @@ describe('the rubicon adapter', () => {
'account_id': 7780
};

let bids = spec.interpretResponse(response, {
let bids = spec.interpretResponse({ body: response }, {
bidRequest: bidderRequest.bids[0]
});

Expand Down
15 changes: 11 additions & 4 deletions test/spec/unit/core/bidderFactory_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,9 @@ describe('bidders created by newBidder', () => {

beforeEach(() => {
ajaxStub = sinon.stub(ajax, 'ajax', function(url, callbacks) {
callbacks.success('response body');
const fakeResponse = sinon.stub();
fakeResponse.returns('headerContent');
callbacks.success('response body', { getResponseHeader: fakeResponse });
});
userSyncStub = sinon.stub(userSync, 'registerSync')
});
Expand All @@ -275,7 +277,7 @@ describe('bidders created by newBidder', () => {
userSyncStub.restore();
});

it('should call spec.interpretResponse() with the response body content', () => {
it('should call spec.interpretResponse() with the response content', () => {
const bidder = newBidder(spec);

spec.isBidRequestValid.returns(true);
Expand All @@ -289,7 +291,9 @@ describe('bidders created by newBidder', () => {
bidder.callBids(MOCK_BIDS_REQUEST);

expect(spec.interpretResponse.calledOnce).to.equal(true);
expect(spec.interpretResponse.firstCall.args[0]).to.equal('response body');
const response = spec.interpretResponse.firstCall.args[0]
expect(response.body).to.equal('response body')
expect(response.headers.get('some-header')).to.equal('headerContent');
expect(spec.interpretResponse.firstCall.args[1]).to.deep.equal({
method: 'POST',
url: 'test.url.com',
Expand Down Expand Up @@ -364,7 +368,10 @@ describe('bidders created by newBidder', () => {
bidder.callBids(MOCK_BIDS_REQUEST);

expect(spec.getUserSyncs.calledOnce).to.equal(true);
expect(spec.getUserSyncs.firstCall.args[1]).to.deep.equal(['response body']);
expect(spec.getUserSyncs.firstCall.args[1].length).to.equal(1);
expect(spec.getUserSyncs.firstCall.args[1][0].body).to.equal('response body');
expect(spec.getUserSyncs.firstCall.args[1][0].headers).to.have.property('get');
expect(spec.getUserSyncs.firstCall.args[1][0].headers.get).to.be.a('function');
});

it('should register usersync pixels', () => {
Expand Down