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

Validating bid response params #1738

Merged
merged 5 commits into from Oct 26, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions modules/appnexusAstBidAdapter.js
Expand Up @@ -182,16 +182,20 @@ function newBid(serverBid, rtbBid) {
const bid = {
requestId: serverBid.uuid,
cpm: rtbBid.cpm,
creative_id: rtbBid.creative_id,
creativeId: rtbBid.creative_id,
dealId: rtbBid.deal_id,
currency: 'USD',
netRevenue: true,
ttl: 300
};

if (rtbBid.rtb.video) {
Object.assign(bid, {
width: rtbBid.rtb.video.player_width,
height: rtbBid.rtb.video.player_height,
vastUrl: rtbBid.rtb.video.asset_url,
descriptionUrl: rtbBid.rtb.video.asset_url
descriptionUrl: rtbBid.rtb.video.asset_url,
ttl: 3600
});
// This supports Outstream Video
if (rtbBid.renderer_url) {
Expand Down
23 changes: 18 additions & 5 deletions src/adapters/bidderFactory.js
Expand Up @@ -116,6 +116,9 @@ import { logWarn, logError, parseQueryStringParameters, delayExecution } from 's
* @property {string} url The URL which makes the sync happen.
*/

// common params for all mediaTypes
const COMMON_BID_RESPONSE_KEYS = ['requestId', 'cpm', 'ttl', 'creativeId', 'netRevenue', 'currency'];

/**
* Register a bidder with prebid, using the given spec.
*
Expand Down Expand Up @@ -303,12 +306,17 @@ export function newBidder(spec) {
onResponse();

function addBidUsingRequestMap(bid) {
const bidRequest = bidRequestMap[bid.requestId];
if (bidRequest) {
const prebidBid = Object.assign(bidfactory.createBid(STATUS.GOOD, bidRequest), bid);
addBidWithCode(bidRequest.placementCode, prebidBid);
// In Prebid 1.0 all the validation logic from bidmanager will move here, as of now we are only validating new params so that adapters dont miss adding them.
if (hasValidKeys(bid)) {
Copy link
Contributor

@dbemiller dbemiller Oct 24, 2017

Choose a reason for hiding this comment

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

Careful here... we've been telling adapters not to add bidderCode, because it breaks aliasing. The bidderFactory should be doing that for them.

But... that happens a few lines after this: Object.assign(bidfactory.createBid(STATUS.GOOD, bidRequest), bid);.

So I don't think should be make sure that bidderCode exists here. If anything, we should make sure that it doesn't exist yet.

const bidRequest = bidRequestMap[bid.requestId];
if (bidRequest) {
const prebidBid = Object.assign(bidfactory.createBid(STATUS.GOOD, bidRequest), bid);
addBidWithCode(bidRequest.placementCode, prebidBid);
} else {
logWarn(`Bidder ${spec.code} made bid for unknown request ID: ${bid.requestId}. Ignoring.`);
}
} else {
logWarn(`Bidder ${spec.code} made bid for unknown request ID: ${bid.requestId}. Ignoring.`);
logError(`Bidder ${spec.code} is missing required params. Check http://prebid.org/dev-docs/bidder-adapter-1.html for list of params.`);
}
}

Expand Down Expand Up @@ -337,6 +345,11 @@ export function newBidder(spec) {
return true;
}

function hasValidKeys(bid) {
let bidKeys = Object.keys(bid);
return COMMON_BID_RESPONSE_KEYS.every(key => bidKeys.includes(key));
}

function newEmptyBid() {
const bid = bidfactory.createBid(STATUS.NO_BID);
bid.code = spec.code;
Expand Down
10 changes: 6 additions & 4 deletions test/spec/modules/appnexusAstBidAdapter_spec.js
Expand Up @@ -296,18 +296,20 @@ describe('AppNexusAdapter', () => {
{
'requestId': '3db3773286ee59',
'cpm': 0.5,
'creative_id': 29681110,
'creativeId': 29681110,
'dealId': undefined,
'width': 300,
'height': 250,
'ad': '<!-- Creative -->',
'mediaType': 'banner'
'mediaType': 'banner',
'currency': 'USD',
'ttl': 300,
'netRevenue': true
}
];
let bidderRequest;

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

it('handles nobid responses', () => {
Expand Down
42 changes: 39 additions & 3 deletions test/spec/unit/core/bidderFactory_spec.js
Expand Up @@ -5,6 +5,7 @@ import * as ajax from 'src/ajax';
import { expect } from 'chai';
import { STATUS } from 'src/constants';
import { userSync } from 'src/userSync'
import * as utils from 'src/utils';

const CODE = 'sampleBidder';
const MOCK_BIDS_REQUEST = {
Expand Down Expand Up @@ -113,7 +114,7 @@ describe('bidders created by newBidder', () => {
expect(spec.buildRequests.firstCall.args[0]).to.deep.equal([MOCK_BIDS_REQUEST.bids[0]]);
});

it("should make no server requests if the spec doesn't return any", () => {
it('should make no server requests if the spec doesn\'t return any', () => {
const bidder = newBidder(spec);

spec.isBidRequestValid.returns(true);
Expand Down Expand Up @@ -262,6 +263,7 @@ describe('bidders created by newBidder', () => {
describe('when the ajax call succeeds', () => {
let ajaxStub;
let userSyncStub;
let logErrorSpy;

beforeEach(() => {
ajaxStub = sinon.stub(ajax, 'ajax', function(url, callbacks) {
Expand All @@ -270,11 +272,13 @@ describe('bidders created by newBidder', () => {
callbacks.success('response body', { getResponseHeader: fakeResponse });
});
userSyncStub = sinon.stub(userSync, 'registerSync')
logErrorSpy = sinon.spy(utils, 'logError');
});

afterEach(() => {
ajaxStub.restore();
userSyncStub.restore();
utils.logError.restore();
});

it('should call spec.interpretResponse() with the response content', () => {
Expand Down Expand Up @@ -324,16 +328,21 @@ describe('bidders created by newBidder', () => {
expect(spec.interpretResponse.calledTwice).to.equal(true);
});

it("should add bids for each placement code into the bidmanager, even if the bidder doesn't bid on all of them", () => {
it('should add bids for each placement code into the bidmanager, even if the bidder doesn\'t bid on all of them', () => {
const bidder = newBidder(spec);

const bid = {
creativeId: 'creative-id',
bidderCode: 'code',
requestId: 'some-id',
ad: 'ad-url.com',
cpm: 0.5,
height: 200,
width: 300,
placementCode: 'mock/placement'
placementCode: 'mock/placement',
currency: 'USD',
netRevenue: true,
ttl: 300
};
spec.isBidRequestValid.returns(true);
spec.buildRequests.returns({
Expand All @@ -352,6 +361,7 @@ describe('bidders created by newBidder', () => {
[bidmanager.addBidResponse.firstCall.args[0], bidmanager.addBidResponse.secondCall.args[0]];
expect(placementsWithBids).to.contain('mock/placement');
expect(placementsWithBids).to.contain('mock/placement2');
expect(logErrorSpy.callCount).to.equal(0);
});

it('should call spec.getUserSyncs() with the response', () => {
Expand Down Expand Up @@ -391,6 +401,32 @@ describe('bidders created by newBidder', () => {
expect(userSyncStub.firstCall.args[1]).to.equal(spec.code);
expect(userSyncStub.firstCall.args[2]).to.equal('usersync.com');
});

it('should logError when required bid response params are missing', () => {
const bidder = newBidder(spec);

const bid = {
requestId: 'some-id',
ad: 'ad-url.com',
cpm: 0.5,
height: 200,
width: 300,
placementCode: 'mock/placement'
};
spec.isBidRequestValid.returns(true);
spec.buildRequests.returns({
method: 'POST',
url: 'test.url.com',
data: {}
});
spec.getUserSyncs.returns([]);

spec.interpretResponse.returns(bid);

bidder.callBids(MOCK_BIDS_REQUEST);

expect(logErrorSpy.calledOnce).to.equal(true);
});
});

describe('when the ajax call fails', () => {
Expand Down