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

generate bid.adId uniquely instead of using bidRequest.bidId #3440

Merged
merged 5 commits into from
Feb 6, 2019
Merged

generate bid.adId uniquely instead of using bidRequest.bidId #3440

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a60e67
generate bid.adId uniquely
jsnellbaker Jan 10, 2019
f25a867
remove stray debugger
jsnellbaker Jan 10, 2019
cb0dc8f
Merge branch 'master' into separate_adId_and_bidId
jsnellbaker Jan 23, 2019
8577c5e
update adId reference in currency file
jsnellbaker Jan 23, 2019
a2db0b9
merge branch 'master' into separate_adId_and_bidId + resolve conflicts
jsnellbaker Feb 6, 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
2 changes: 1 addition & 1 deletion modules/currency.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ function wrapFunction(fn, context, params) {
utils.logWarn('Returning NO_BID, getCurrencyConversion threw error: ', e);
params[1] = createBid(STATUS.NO_BID, {
bidder: bid.bidderCode || bid.bidder,
bidId: bid.adId
bidId: bid.requestId
});
}
}
Expand Down
2 changes: 1 addition & 1 deletion modules/prebidServerBidAdapter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ const OPEN_RTB_PROTOCOL = {
bidObject.width = bid.w;
bidObject.height = bid.h;
if (bid.dealid) { bidObject.dealId = bid.dealid; }
bidObject.requestId = bid.id;
bidObject.requestId = bidRequest.bidId;
bidObject.creative_id = bid.crid;
bidObject.creativeId = bid.crid;
if (bid.burl) { bidObject.burl = bid.burl; }
Expand Down
2 changes: 1 addition & 1 deletion src/auction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ function addBidToAuction(auctionInstance, bidResponse) {
function tryAddVideoBid(auctionInstance, bidResponse, bidRequests, afterBidAdded) {
let addBid = true;

const bidRequest = getBidRequest(bidResponse.adId, [bidRequests]);
const bidRequest = getBidRequest(bidResponse.requestId, [bidRequests]);
const videoMediaType =
bidRequest && deepAccess(bidRequest, 'mediaTypes.video');
const context = videoMediaType && deepAccess(videoMediaType, 'context');
Expand Down
4 changes: 2 additions & 2 deletions src/bidfactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ var utils = require('./utils.js');
priceKeyString;
*/
function Bid(statusCode, bidRequest) {
var _bidId = (bidRequest && bidRequest.bidId) || utils.getUniqueIdentifierStr();
var _bidSrc = (bidRequest && bidRequest.src) || 'client';
var _statusCode = statusCode || 0;

this.bidderCode = (bidRequest && bidRequest.bidder) || '';
this.width = 0;
this.height = 0;
this.statusMessage = _getStatus();
this.adId = _bidId;
this.adId = utils.getUniqueIdentifierStr();
this.requestId = bidRequest && bidRequest.bidId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed that the currency file calls bidfactory.createBid() when conversions fail. There might be some (minor) repercussions since the adId will no longer be maintained in the new bid. Might want to at least change this line in the currency module, from bidId: bid.adId to bidId: bid.requestId to maintain the requestId - though that isn't maintained currently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback regarding the currency file. I pushed an update for that reference.

this.mediaType = 'banner';
this.source = _bidSrc;

Expand Down
2 changes: 1 addition & 1 deletion src/native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export const hasNonNativeBidder = adUnit =>
* @return {Boolean} If object is valid
*/
export function nativeBidIsValid(bid, bidRequests) {
const bidRequest = getBidRequest(bid.adId, bidRequests);
const bidRequest = getBidRequest(bid.requestId, bidRequests);
if (!bidRequest) { return false; }

// all native bid responses must define a landing page url
Expand Down
3 changes: 3 additions & 0 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,9 @@ export function flatten(a, b) {
}

export function getBidRequest(id, bidderRequests) {
if (!id) {
return;
}
let bidRequest;
bidderRequests.some(bidderRequest => {
let result = find(bidderRequest.bids, bid => ['bidId', 'adId', 'bid_id'].some(type => bid[type] === id));
Expand Down
2 changes: 1 addition & 1 deletion src/video.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export const hasNonVideoBidder = adUnit =>
* @return {Boolean} If object is valid
*/
export function isValidVideoBid(bid, bidRequests) {
const bidRequest = getBidRequest(bid.adId, bidRequests);
const bidRequest = getBidRequest(bid.requestId, bidRequests);

const videoMediaType =
bidRequest && deepAccess(bidRequest, 'mediaTypes.video');
Expand Down
6 changes: 3 additions & 3 deletions test/spec/auctionmanager_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,13 +768,13 @@ describe('auctionmanager.js', function () {
auctionModule.newAuction.restore();
});

it('should not alter bid adID', function () {
it('should not alter bid requestID', function () {
auction.callBids();

const addedBid2 = auction.getBidsReceived().pop();
assert.equal(addedBid2.adId, bids1[0].requestId);
assert.equal(addedBid2.requestId, bids1[0].requestId);
const addedBid1 = auction.getBidsReceived().pop();
assert.equal(addedBid1.adId, bids[0].requestId);
assert.equal(addedBid1.requestId, bids[0].requestId);
});

it('should not add banner bids that have no width or height', function () {
Expand Down
12 changes: 6 additions & 6 deletions test/spec/modules/prebidServerBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ describe('S2S Adapter', function () {
const response = addBidResponse.firstCall.args[1];
expect(response).to.have.property('statusMessage', 'Bid available');
expect(response).to.have.property('cpm', 0.5);
expect(response).to.have.property('adId', '123');
expect(response).to.have.property('requestId', '123');
expect(response).to.not.have.property('videoCacheKey');
expect(response).to.have.property('cache_id', '7654321');
expect(response).to.have.property('cache_url', 'http://www.test.com/cache?uuid=7654321');
Expand All @@ -861,7 +861,7 @@ describe('S2S Adapter', function () {
const response = addBidResponse.firstCall.args[1];
expect(response).to.have.property('statusMessage', 'Bid available');
expect(response).to.have.property('cpm', 0.5);
expect(response).to.have.property('adId', '123');
expect(response).to.have.property('requestId', '123');
expect(response).to.have.property('videoCacheKey', 'video_cache_id');
expect(response).to.have.property('cache_id', 'video_cache_id');
expect(response).to.have.property('cache_url', 'video_cache_url');
Expand Down Expand Up @@ -913,7 +913,7 @@ describe('S2S Adapter', function () {

expect(addBidResponse.firstCall.args[0]).to.equal('div-gpt-ad-1460505748561-0');

expect(addBidResponse.firstCall.args[1]).to.have.property('adId', '123');
expect(addBidResponse.firstCall.args[1]).to.have.property('requestId', '123');

expect(addBidResponse.firstCall.args[1])
.to.have.property('statusMessage', 'Bid available');
Expand Down Expand Up @@ -992,7 +992,7 @@ describe('S2S Adapter', function () {
expect(response).to.have.property('source', 's2s');

const bid_request_passed = addBidResponse.firstCall.args[1];
expect(bid_request_passed).to.have.property('adId', '123');
expect(bid_request_passed).to.have.property('requestId', '123');
});

it('handles OpenRTB responses and call BIDDER_DONE', function () {
Expand All @@ -1014,7 +1014,7 @@ describe('S2S Adapter', function () {
const response = addBidResponse.firstCall.args[1];
expect(response).to.have.property('statusMessage', 'Bid available');
expect(response).to.have.property('bidderCode', 'appnexus');
expect(response).to.have.property('adId', '123');
expect(response).to.have.property('requestId', '123');
expect(response).to.have.property('cpm', 0.5);
});

Expand All @@ -1034,7 +1034,7 @@ describe('S2S Adapter', function () {
expect(response).to.have.property('vastXml', RESPONSE_OPENRTB_VIDEO.seatbid[0].bid[0].adm);
expect(response).to.have.property('mediaType', 'video');
expect(response).to.have.property('bidderCode', 'appnexus');
expect(response).to.have.property('adId', '123');
expect(response).to.have.property('requestId', '123');
expect(response).to.have.property('cpm', 10);
});

Expand Down
8 changes: 4 additions & 4 deletions test/spec/modules/serverbidServerBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ describe('ServerBid S2S Adapter', function () {
const response = addBidResponse.firstCall.args[1];
expect(response).to.have.property('statusMessage', 'Bid available');
expect(response).to.have.property('cpm', 0.5);
expect(response).to.have.property('adId', '123');
expect(response).to.have.property('requestId', '123');
});

it('registers no-bid response when ad unit not set', function () {
Expand All @@ -261,7 +261,7 @@ describe('ServerBid S2S Adapter', function () {
expect(response).to.have.property('statusMessage', 'Bid returned empty or error response');

const bid_request_passed = addBidResponse.firstCall.args[1];
expect(bid_request_passed).to.have.property('adId', '123');
expect(bid_request_passed).to.have.property('requestId', '123');
});

it('registers no-bid response when ad unit is set', function () {
Expand Down Expand Up @@ -291,8 +291,8 @@ describe('ServerBid S2S Adapter', function () {
expect(addBidResponse.firstCall.args[0]).to.equal('div-gpt-ad-1460505748561-0');
expect(addBidResponse.secondCall.args[0]).to.equal('div-gpt-ad-1460505748561-1');

expect(addBidResponse.firstCall.args[1]).to.have.property('adId', '123');
expect(addBidResponse.secondCall.args[1]).to.have.property('adId', '101111');
expect(addBidResponse.firstCall.args[1]).to.have.property('requestId', '123');
expect(addBidResponse.secondCall.args[1]).to.have.property('requestId', '101111');

expect(addBidResponse.firstCall.args[1])
.to.have.property('statusMessage', 'Bid available');
Expand Down
9 changes: 6 additions & 3 deletions test/spec/native_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ describe('validate native', function () {
}];

let validBid = {
adId: 'test_bid_id',
adId: 'abc123',
requestId: 'test_bid_id',
adUnitCode: '123/prebid_native_adunit',
bidder: 'test_bidder',
native: {
Expand All @@ -126,7 +127,8 @@ describe('validate native', function () {
};

let noIconDimBid = {
adId: 'test_bid_id',
adId: 'abc234',
requestId: 'test_bid_id',
adUnitCode: '123/prebid_native_adunit',
bidder: 'test_bidder',
native: {
Expand All @@ -150,7 +152,8 @@ describe('validate native', function () {
};

let noImgDimBid = {
adId: 'test_bid_id',
adId: 'abc345',
requestId: 'test_bid_id',
adUnitCode: '123/prebid_native_adunit',
bidder: 'test_bidder',
native: {
Expand Down
8 changes: 4 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 @@ -639,7 +639,7 @@ describe('validate bid response: ', function () {
it('should add native bids that do have required assets', function () {
let bidRequest = {
bids: [{
bidId: 1,
bidId: '1',
auctionId: 'first-bid-id',
adUnitCode: 'mock/placement',
params: {
Expand Down Expand Up @@ -676,7 +676,7 @@ describe('validate bid response: ', function () {
it('should not add native bids that do not have required assets', function () {
let bidRequest = {
bids: [{
bidId: 1,
bidId: '1',
auctionId: 'first-bid-id',
adUnitCode: 'mock/placement',
params: {
Expand Down Expand Up @@ -712,7 +712,7 @@ describe('validate bid response: ', function () {
it('should add bid when renderer is present on outstream bids', function () {
let bidRequest = {
bids: [{
bidId: 1,
bidId: '1',
auctionId: 'first-bid-id',
adUnitCode: 'mock/placement',
params: {
Expand Down Expand Up @@ -747,7 +747,7 @@ describe('validate bid response: ', function () {
let bidRequest = {
bids: [{
bidder: CODE,
bidId: 1,
bidId: '1',
auctionId: 'first-bid-id',
adUnitCode: 'mock/placement',
params: {
Expand Down
11 changes: 6 additions & 5 deletions test/spec/video_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ import { isValidVideoBid } from 'src/video';
describe('video.js', function () {
it('validates valid instream bids', function () {
const bid = {
adId: '123abc',
vastUrl: 'http://www.example.com/vastUrl'
adId: '456xyz',
vastUrl: 'http://www.example.com/vastUrl',
requestId: '123abc'
};
const bidRequests = [{
bids: [{
Expand All @@ -21,7 +22,7 @@ describe('video.js', function () {

it('catches invalid instream bids', function () {
const bid = {
adId: '123abc'
requestId: '123abc'
};
const bidRequests = [{
bids: [{
Expand Down Expand Up @@ -51,7 +52,7 @@ describe('video.js', function () {

it('validates valid outstream bids', function () {
const bid = {
adId: '123abc',
requestId: '123abc',
renderer: {
url: 'render.url',
render: () => true,
Expand All @@ -72,7 +73,7 @@ describe('video.js', function () {

it('catches invalid outstream bids', function () {
const bid = {
adId: '123abc'
requestId: '123abc'
};
const bidRequests = [{
bids: [{
Expand Down