From ddc6edd069ebbef5775522493bd28e7d6a951f74 Mon Sep 17 00:00:00 2001 From: Demetrio Girardi Date: Tue, 18 Jun 2024 22:43:55 -0700 Subject: [PATCH] Core: fix video cache silent failure (#11781) * move some video cache logic to videoCache.js * move more video cache logic to videoCache.js * Fix silent failure on video cache (https://github.com/prebid/Prebid.js/issues/11778) --- modules/ceeIdSystem.js | 2 +- src/auction.js | 68 ++----------------- src/videoCache.js | 72 ++++++++++++++++++++ test/mocks/videoCacheStub.js | 2 +- test/spec/auctionmanager_spec.js | 2 +- test/spec/videoCache_spec.js | 109 ++++++++++++++++--------------- 6 files changed, 136 insertions(+), 119 deletions(-) diff --git a/modules/ceeIdSystem.js b/modules/ceeIdSystem.js index d1534ddada27..30240e3f75fb 100644 --- a/modules/ceeIdSystem.js +++ b/modules/ceeIdSystem.js @@ -43,7 +43,7 @@ export const ceeIdSubmodule = { * performs action to obtain id and return a value * @function * @returns {(IdResponse|undefined)} - */ + */ getId(config) { const { params = {} } = config; const { tokenName, value } = params diff --git a/src/auction.js b/src/auction.js index 881dee9f2de4..b422ffa73335 100644 --- a/src/auction.js +++ b/src/auction.js @@ -82,7 +82,7 @@ import { } from './utils.js'; import {getPriceBucketString} from './cpmBucketManager.js'; import {getNativeTargeting, isNativeResponse, setNativeResponseProperties} from './native.js'; -import {getCacheUrl, store} from './videoCache.js'; +import {batchAndStore} from './videoCache.js'; import {Renderer} from './Renderer.js'; import {config} from './config.js'; import {userSync} from './userSync.js'; @@ -94,7 +94,7 @@ import {auctionManager} from './auctionManager.js'; import {bidderSettings} from './bidderSettings.js'; import * as events from './events.js'; import adapterManager from './adapterManager.js'; -import { EVENTS, GRANULARITY_OPTIONS, JSON_MAPPING, REJECTION_REASON, S2S, TARGETING_KEYS } from './constants.js'; +import {EVENTS, GRANULARITY_OPTIONS, JSON_MAPPING, REJECTION_REASON, S2S, TARGETING_KEYS} from './constants.js'; import {defer, GreedyPromise} from './utils/promise.js'; import {useMetrics} from './utils/perfMetrics.js'; import {adjustCpm} from './utils/cpm.js'; @@ -580,68 +580,10 @@ function tryAddVideoBid(auctionInstance, bidResponse, afterBidAdded, {index = au } } -const _storeInCache = (batch) => { - store(batch.map(entry => entry.bidResponse), function (error, cacheIds) { - cacheIds.forEach((cacheId, i) => { - const { auctionInstance, bidResponse, afterBidAdded } = batch[i]; - if (error) { - logWarn(`Failed to save to the video cache: ${error}. Video bid must be discarded.`); - } else { - if (cacheId.uuid === '') { - logWarn(`Supplied video cache key was already in use by Prebid Cache; caching attempt was rejected. Video bid must be discarded.`); - } else { - bidResponse.videoCacheKey = cacheId.uuid; - if (!bidResponse.vastUrl) { - bidResponse.vastUrl = getCacheUrl(bidResponse.videoCacheKey); - } - addBidToAuction(auctionInstance, bidResponse); - afterBidAdded(); - } - } - }); - }); -}; - -const storeInCache = FEATURES.VIDEO ? _storeInCache : () => {}; - -let batchSize, batchTimeout; -config.getConfig('cache', (cacheConfig) => { - batchSize = typeof cacheConfig.cache.batchSize === 'number' && cacheConfig.cache.batchSize > 0 - ? cacheConfig.cache.batchSize - : 1; - batchTimeout = typeof cacheConfig.cache.batchTimeout === 'number' && cacheConfig.cache.batchTimeout > 0 - ? cacheConfig.cache.batchTimeout - : 0; -}); - -export const batchingCache = (timeout = setTimeout, cache = storeInCache) => { - let batches = [[]]; - let debouncing = false; - const noTimeout = cb => cb(); - - return function(auctionInstance, bidResponse, afterBidAdded) { - const batchFunc = batchTimeout > 0 ? timeout : noTimeout; - if (batches[batches.length - 1].length >= batchSize) { - batches.push([]); - } - - batches[batches.length - 1].push({auctionInstance, bidResponse, afterBidAdded}); - - if (!debouncing) { - debouncing = true; - batchFunc(() => { - batches.forEach(cache); - batches = [[]]; - debouncing = false; - }, batchTimeout); - } - } -}; - -const batchAndStore = batchingCache(); - export const callPrebidCache = hook('async', function(auctionInstance, bidResponse, afterBidAdded, videoMediaType) { - batchAndStore(auctionInstance, bidResponse, afterBidAdded); + if (FEATURES.VIDEO) { + batchAndStore(auctionInstance, bidResponse, afterBidAdded); + } }, 'callPrebidCache'); /** diff --git a/src/videoCache.js b/src/videoCache.js index 6cba77de3087..cf39c1c94526 100644 --- a/src/videoCache.js +++ b/src/videoCache.js @@ -12,6 +12,8 @@ import {ajaxBuilder} from './ajax.js'; import {config} from './config.js'; import {auctionManager} from './auctionManager.js'; +import {logError, logWarn} from './utils.js'; +import {addBidToAuction} from './auction.js'; /** * Might be useful to be configurable in the future @@ -159,3 +161,73 @@ export function store(bids, done, getAjax = ajaxBuilder) { export function getCacheUrl(id) { return `${config.getConfig('cache.url')}?uuid=${id}`; } + +export const _internal = { + store +} + +export function storeBatch(batch) { + const bids = batch.map(entry => entry.bidResponse) + function err(msg) { + logError(`Failed to save to the video cache: ${msg}. Video bids will be discarded:`, bids) + } + _internal.store(bids, function (error, cacheIds) { + if (error) { + err(error) + } else if (batch.length !== cacheIds.length) { + logError(`expected ${batch.length} cache IDs, got ${cacheIds.length} instead`) + } else { + cacheIds.forEach((cacheId, i) => { + const {auctionInstance, bidResponse, afterBidAdded} = batch[i]; + if (cacheId.uuid === '') { + logWarn(`Supplied video cache key was already in use by Prebid Cache; caching attempt was rejected. Video bid must be discarded.`); + } else { + bidResponse.videoCacheKey = cacheId.uuid; + if (!bidResponse.vastUrl) { + bidResponse.vastUrl = getCacheUrl(bidResponse.videoCacheKey); + } + addBidToAuction(auctionInstance, bidResponse); + afterBidAdded(); + } + }); + } + }); +}; + +let batchSize, batchTimeout; +if (FEATURES.VIDEO) { + config.getConfig('cache', (cacheConfig) => { + batchSize = typeof cacheConfig.cache.batchSize === 'number' && cacheConfig.cache.batchSize > 0 + ? cacheConfig.cache.batchSize + : 1; + batchTimeout = typeof cacheConfig.cache.batchTimeout === 'number' && cacheConfig.cache.batchTimeout > 0 + ? cacheConfig.cache.batchTimeout + : 0; + }); +} + +export const batchingCache = (timeout = setTimeout, cache = storeBatch) => { + let batches = [[]]; + let debouncing = false; + const noTimeout = cb => cb(); + + return function (auctionInstance, bidResponse, afterBidAdded) { + const batchFunc = batchTimeout > 0 ? timeout : noTimeout; + if (batches[batches.length - 1].length >= batchSize) { + batches.push([]); + } + + batches[batches.length - 1].push({auctionInstance, bidResponse, afterBidAdded}); + + if (!debouncing) { + debouncing = true; + batchFunc(() => { + batches.forEach(cache); + batches = [[]]; + debouncing = false; + }, batchTimeout); + } + }; +}; + +export const batchAndStore = batchingCache(); diff --git a/test/mocks/videoCacheStub.js b/test/mocks/videoCacheStub.js index 7ce899cae35f..acae5cd6a328 100644 --- a/test/mocks/videoCacheStub.js +++ b/test/mocks/videoCacheStub.js @@ -1,4 +1,4 @@ -import * as videoCache from 'src/videoCache.js'; +import {_internal as videoCache} from 'src/videoCache.js'; /** * Function which can be called from unit tests to stub out the video cache. diff --git a/test/spec/auctionmanager_spec.js b/test/spec/auctionmanager_spec.js index e5cdb66e75f5..26f641a10e7d 100644 --- a/test/spec/auctionmanager_spec.js +++ b/test/spec/auctionmanager_spec.js @@ -12,7 +12,7 @@ import * as auctionModule from 'src/auction.js'; import { registerBidder } from 'src/adapters/bidderFactory.js'; import { createBid } from 'src/bidfactory.js'; import { config } from 'src/config.js'; -import * as store from 'src/videoCache.js'; +import {_internal as store} from 'src/videoCache.js'; import * as ajaxLib from 'src/ajax.js'; import {find} from 'src/polyfill.js'; import { server } from 'test/mocks/xhr.js'; diff --git a/test/spec/videoCache_spec.js b/test/spec/videoCache_spec.js index fc6e71779cb5..7d07da9de901 100644 --- a/test/spec/videoCache_spec.js +++ b/test/spec/videoCache_spec.js @@ -1,39 +1,13 @@ import chai from 'chai'; -import {getCacheUrl, store} from 'src/videoCache.js'; +import {batchingCache, getCacheUrl, store, _internal, storeBatch} from 'src/videoCache.js'; import {config} from 'src/config.js'; import {server} from 'test/mocks/xhr.js'; import {auctionManager} from '../../src/auctionManager.js'; import {AuctionIndex} from '../../src/auctionIndex.js'; -import {batchingCache} from '../../src/auction.js'; +import * as utils from 'src/utils.js'; const should = chai.should(); -function getMockBid(bidder, auctionId, bidderRequestId) { - return { - 'bidder': bidder, - 'params': { - 'placementId': '10433394', - 'member': 123, - 'keywords': { - 'foo': ['bar', 'baz'], - 'fizz': ['buzz'] - } - }, - 'bid_id': '12345abc', - 'adUnitCode': 'div-gpt-ad-1460505748561-0', - 'mediaTypes': { - 'banner': { - 'sizes': [[300, 250]] - } - }, - 'transactionId': '4ef956ad-fd83-406d-bd35-e4bb786ab86c', - 'sizes': [300, 250], - 'bidId': '123', - 'bidderRequestId': bidderRequestId, - 'auctionId': auctionId - }; -} - describe('The video cache', function () { function assertError(callbackSpy) { callbackSpy.calledOnce.should.equal(true); @@ -126,9 +100,7 @@ describe('The video cache', function () { prebid.org wrapper - - - + \n \n `; @@ -335,34 +307,36 @@ describe('The video cache', function () { JSON.parse(request.requestBody).should.deep.equal(payload); }); - it('should wait the duration of the batchTimeout and pass the correct batchSize if batched requests are enabled in the config', () => { - const mockAfterBidAdded = function() {}; - let callback = null; - let mockTimeout = sinon.stub().callsFake((cb) => { callback = cb }); + if (FEATURES.VIDEO) { + it('should wait the duration of the batchTimeout and pass the correct batchSize if batched requests are enabled in the config', () => { + const mockAfterBidAdded = function() {}; + let callback = null; + let mockTimeout = sinon.stub().callsFake((cb) => { callback = cb }); - config.setConfig({ - cache: { - url: 'https://prebid.adnxs.com/pbc/v1/cache', - batchSize: 3, - batchTimeout: 20 - } - }); + config.setConfig({ + cache: { + url: 'https://prebid.adnxs.com/pbc/v1/cache', + batchSize: 3, + batchTimeout: 20 + } + }); - let stubCache = sinon.stub(); - const batchAndStore = batchingCache(mockTimeout, stubCache); - for (let i = 0; i < 3; i++) { - batchAndStore({}, {}, mockAfterBidAdded); - } + let stubCache = sinon.stub(); + const batchAndStore = batchingCache(mockTimeout, stubCache); + for (let i = 0; i < 3; i++) { + batchAndStore({}, {}, mockAfterBidAdded); + } - sinon.assert.calledOnce(mockTimeout); - sinon.assert.calledWith(mockTimeout, sinon.match.any, 20); + sinon.assert.calledOnce(mockTimeout); + sinon.assert.calledWith(mockTimeout, sinon.match.any, 20); - const expectedBatch = [{ afterBidAdded: mockAfterBidAdded, auctionInstance: { }, bidResponse: { } }, { afterBidAdded: mockAfterBidAdded, auctionInstance: { }, bidResponse: { } }, { afterBidAdded: mockAfterBidAdded, auctionInstance: { }, bidResponse: { } }]; + const expectedBatch = [{ afterBidAdded: mockAfterBidAdded, auctionInstance: { }, bidResponse: { } }, { afterBidAdded: mockAfterBidAdded, auctionInstance: { }, bidResponse: { } }, { afterBidAdded: mockAfterBidAdded, auctionInstance: { }, bidResponse: { } }]; - callback(); + callback(); - sinon.assert.calledWith(stubCache, expectedBatch); - }); + sinon.assert.calledWith(stubCache, expectedBatch); + }); + } function assertRequestMade(bid, expectedValue) { store([bid], function () { }); @@ -393,6 +367,35 @@ describe('The video cache', function () { return callback; } }); + + describe('storeBatch', () => { + let sandbox; + let err, cacheIds + beforeEach(() => { + err = null; + cacheIds = []; + sandbox = sinon.createSandbox(); + sandbox.stub(utils, 'logError'); + sandbox.stub(_internal, 'store').callsFake((_, cb) => cb(err, cacheIds)); + }); + afterEach(() => { + sandbox.restore(); + }) + it('should log an error when store replies with an error', () => { + err = new Error('err'); + storeBatch([]); + sinon.assert.called(utils.logError); + }); + it('should not process returned uuids if they do not match the batch size', () => { + const el = {auctionInstance: {}, bidResponse: {}, afterBidAdded: sinon.stub()} + const batch = [el, el]; + cacheIds = [{uuid: 'mock-id'}] + storeBatch(batch); + expect(el.bidResponse.videoCacheKey).to.not.exist; + sinon.assert.notCalled(batch[0].afterBidAdded); + sinon.assert.called(utils.logError); + }) + }) }); describe('The getCache function', function () {