Core: fix video cache silent failure

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

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');
 
 /**

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();

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.

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';

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 () {
     <Ad>
       <Wrapper>
         <AdSystem>prebid.org wrapper</AdSystem>
-        <VASTAdTagURI><![CDATA[my-mock-url.com]]></VASTAdTagURI>
-
-        <Creatives></Creatives>
+        <VASTAdTagURI><![CDATA[my-mock-url.com]]></VASTAdTagURI>\n        \n        <Creatives></Creatives>
       </Wrapper>
     </Ad>
   </VAST>`;
@@ -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 () {