✨ Implement new URL macro PAGE_VIEW_ID_64 (#23451)

* Added PAGE_VIEW_64 macro * Added PAGE_VIEW_64 macro doc in var substitution * Fix lint * Fix types in doc * Fix tests * Added PAGE_VIEW_64 macro * Added PAGE_VIEW_64 macro doc in var substitution * Fix lint * Fix types in doc * Fix tests * Reuse CID code for random string generation * Add integration test * Fix tests * Fix unit tests * Remove amp-analytics intergration tests
ampproject · Aug 13, 2019 · 6d2f4c3 · 6d2f4c3
1 parent e1579c9
commit 6d2f4c3
Show file tree

Hide file tree

Showing 8 changed files with 83 additions and 23 deletions.
diff --git a/spec/amp-var-substitutions.md b/spec/amp-var-substitutions.md
@@ -265,6 +265,7 @@ The tables below list the available URL variables grouped by type of usage. Furt
 | [Client ID](#client-id) | `CLIENT_ID` | `${clientId}` |
 | [Extra URL Parameters](#extra-url-parameters) | N/A | `${extraUrlParams}` |
 | [Page View ID](#page-view-id) | `PAGE_VIEW_ID` | `${pageViewId}` |
+| [Page View ID 64](#page-view-id-64) | `PAGE_VIEW_ID_64` | `${pageViewId64}` |
 | [Query Parameter](#query-parameter) | `QUERY_PARAM` | `${queryParam}` |
 | [Random](#random) | `RANDOM` | `${random}` |
 | [Request Count](#request-count) | N/A | `${requestCount}` |
@@ -901,6 +902,18 @@ Provides a string that is intended to be random and likely to be unique per URL,
 * **amp-analytics variable**: `${pageViewId}`
   * Example value: `978`
 
+#### Page View ID 64
+
+Provides a string that is intended to be random with a high entropy and likely to be unique per URL, user and day.
+
+* **platform variable**: `PAGE_VIEW_ID_64`
+  *  Example: <br>
+  ```html
+  <amp-pixel src="https://foo.com/pixel?pid=PAGE_VIEW_ID_64"></amp-pixel>
+  ```
+* **amp-analytics variable**: `${pageViewId64}`
+  * Example value: `U6XEpUs3yaeQyR2DKATQH1pTZ6kg140fvuLbtl5nynb`
+
 #### Query Parameter
 
 Pulls a value from the provided query string parameter.

diff --git a/src/service/cid-impl.js b/src/service/cid-impl.js
@@ -22,19 +22,20 @@
  * For details, see https://goo.gl/Mwaacs
  */
 
-import {CacheCidApi} from './cache-cid-api';
 import {GoogleCidApi, TokenStatus} from './cid-api';
+import {dev, rethrowAsync, user, userAssert} from '../log';
+import {getCookie, setCookie} from '../cookies';
+import {getServiceForDoc, registerServiceBuilderForDoc} from '../service';
+import {getSourceOrigin, isProxyOrigin, parseUrlDeprecated} from '../url';
+import {parseJson, tryParseJson} from '../json';
+
+import {CacheCidApi} from './cache-cid-api';
 import {Services} from '../services';
 import {ViewerCidApi} from './viewer-cid-api';
 import {base64UrlEncodeFromBytes} from '../utils/base64';
-import {dev, rethrowAsync, user, userAssert} from '../log';
 import {dict} from '../utils/object';
-import {getCookie, setCookie} from '../cookies';
 import {getCryptoRandomBytesArray} from '../utils/bytes';
-import {getServiceForDoc, registerServiceBuilderForDoc} from '../service';
-import {getSourceOrigin, isProxyOrigin, parseUrlDeprecated} from '../url';
 import {isIframed} from '../dom';
-import {parseJson, tryParseJson} from '../json';
 import {tryResolve} from '../utils/promise';
 
 const ONE_DAY_MILLIS = 24 * 3600 * 1000;
@@ -415,7 +416,7 @@ function getOrCreateCookie(cid, getCidStruct, persistenceConsent) {
     return /** @type {!Promise<?string>} */ (Promise.resolve(existingCookie));
   }
 
-  const newCookiePromise = getNewCidForCookie(win)
+  const newCookiePromise = getRandomString64(win)
     // Create new cookie, always prefixed with "amp-", so that we can see from
     // the value whether we created it.
     .then(randomStr => 'amp-' + randomStr);
@@ -648,7 +649,7 @@ function getEntropy(win) {
  * @param {!Window} win
  * @return {!Promise<string>} The cid
  */
-function getNewCidForCookie(win) {
+export function getRandomString64(win) {
   const entropy = getEntropy(win);
   if (typeof entropy == 'string') {
     return Services.cryptoFor(win).sha384Base64(entropy);

diff --git a/src/service/document-info-impl.js b/src/service/document-info-impl.js
@@ -20,6 +20,8 @@ import {
   parseQueryString,
   parseUrlDeprecated,
 } from '../url';
+
+import {getRandomString64} from './cid-impl';
 import {isArray} from '../types';
 import {map} from '../utils/object';
 import {registerServiceBuilderForDoc} from '../service';
@@ -32,6 +34,7 @@ const filteredLinkRels = ['prefetch', 'preload', 'preconnect', 'dns-prefetch'];
  *     - sourceUrl: the source url of an amp document.
  *     - canonicalUrl: The doc's canonical.
  *     - pageViewId: Id for this page view. Low entropy but should be unique
+ *     - pageViewId64: Id for this page view. High entropy but should be unique
  *       for concurrent page views of a user().
  *     - linkRels: A map object of link tag's rel (key) and corresponding
  *       hrefs (value). rel could be 'canonical', 'icon', etc.
@@ -45,6 +48,7 @@ const filteredLinkRels = ['prefetch', 'preload', 'preconnect', 'dns-prefetch'];
  *   sourceUrl: string,
  *   canonicalUrl: string,
  *   pageViewId: string,
+ *   pageViewId64: !Promise<string>,
  *   linkRels: !Object<string, string|!Array<string>>,
  *   metaTags: !Object<string, string|!Array<string>>,
  *   replaceParams: ?Object<string, string|!Array<string>>
@@ -88,6 +92,7 @@ export class DocInfo {
         : sourceUrl;
     }
     const pageViewId = getPageViewId(ampdoc.win);
+    const pageViewId64 = getRandomString64(ampdoc.win);
     const linkRels = getLinkRels(ampdoc.win.document);
     const metaTags = getMetaTags(ampdoc.win.document);
     const replaceParams = getReplaceParams(ampdoc);
@@ -99,6 +104,7 @@ export class DocInfo {
       },
       canonicalUrl,
       pageViewId,
+      pageViewId64,
       linkRels,
       metaTags,
       replaceParams,

diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js
@@ -23,9 +23,6 @@ import {
   getTimingDataAsync,
   getTimingDataSync,
 } from './variable-source';
-import {Expander} from './url-expander/expander';
-import {Services} from '../services';
-import {WindowInterface} from '../window-interface';
 import {
   addMissingParamsToUrl,
   addParamsToUrl,
@@ -37,12 +34,16 @@ import {
   removeFragment,
 } from '../url';
 import {dev, devAssert, user, userAssert} from '../log';
-import {getTrackImpressionPromise} from '../impression.js';
-import {hasOwn} from '../utils/object';
 import {
   installServiceInEmbedScope,
   registerServiceBuilderForDoc,
 } from '../service';
+
+import {Expander} from './url-expander/expander';
+import {Services} from '../services';
+import {WindowInterface} from '../window-interface';
+import {getTrackImpressionPromise} from '../impression.js';
+import {hasOwn} from '../utils/object';
 import {internalRuntimeVersion} from '../internal-version';
 import {tryResolve} from '../utils/promise';
 
@@ -236,6 +237,11 @@ export class GlobalVariableSource extends VariableSource {
     // all the page views a single user is making at a time.
     this.set('PAGE_VIEW_ID', () => this.getDocInfo_().pageViewId);
 
+    // Returns a random string that will be the constant for the duration of
+    // single page view. It should have sufficient entropy to be unique for
+    // all the page views a single user is making at a time.
+    this.setAsync('PAGE_VIEW_ID_64', () => this.getDocInfo_().pageViewId64);
+
     this.setBoth(
       'QUERY_PARAM',
       (param, defaultValue = '') => {
@@ -1165,6 +1171,7 @@ export class UrlReplacements {
       'CLIENT_ID': true,
       'QUERY_PARAM': true,
       'PAGE_VIEW_ID': true,
+      'PAGE_VIEW_ID_64': true,
       'NAV_TIMING': true,
     };
     const additionalUrlParameters =

diff --git a/test/integration/test-amp-analytics.js b/test/integration/test-amp-analytics.js
@@ -15,6 +15,7 @@
  */
 
 import {BrowserController, RequestBank} from '../../testing/test-helper';
+
 import {parseQueryString} from '../../src/url';
 
 describe('amp-analytics', function() {

diff --git a/test/unit/test-document-info.js b/test/unit/test-document-info.js
@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+import * as CID from '../../src/service/cid-impl';
+
 import {Services} from '../../src/services';
 import {createIframePromise} from '../../testing/iframe';
 import {installDocService} from '../../src/service/ampdoc-impl';
@@ -27,6 +29,7 @@ describe
 
     beforeEach(() => {
       sandbox = sinon.sandbox;
+      sandbox.stub(CID, 'getRandomString64').returns('abcdef');
     });
 
     afterEach(() => {
@@ -145,6 +148,17 @@ describe
       });
     });
 
+    it('should provide the pageViewId64', () => {
+      return getWin({'canonical': ['https://twitter.com/']}).then(win => {
+        expect(Services.documentInfoForDoc(win.document).pageViewId64).to.equal(
+          'abcdef'
+        );
+        expect(Services.documentInfoForDoc(win.document).pageViewId64).to.equal(
+          'abcdef'
+        );
+      });
+    });
+
     it('should provide the relative canonicalUrl as absolute', () => {
       return getWin({'canonical': ['./foo.html']}).then(win => {
         expect(Services.documentInfoForDoc(win.document).canonicalUrl).to.equal(

diff --git a/test/unit/test-url-replacements.js b/test/unit/test-url-replacements.js
@@ -15,18 +15,11 @@
  */
 
 import * as trackPromise from '../../src/impression';
-import {Observable} from '../../src/observable';
-import {Services} from '../../src/services';
-import {cidServiceForDocForTesting} from '../../src/service/cid-impl';
-import {createIframePromise} from '../../testing/iframe';
+
 import {
   extractClientIdFromGaCookie,
   installUrlReplacementsServiceForDoc,
 } from '../../src/service/url-replacements-impl';
-import {installActivityServiceForTesting} from '../../extensions/amp-analytics/0.1/activity-impl';
-import {installCryptoService} from '../../src/service/crypto-impl';
-import {installDocService} from '../../src/service/ampdoc-impl';
-import {installDocumentInfoServiceForDoc} from '../../src/service/document-info-impl';
 import {
   markElementScheduledForTesting,
   resetScheduledElementForTesting,
@@ -35,6 +28,15 @@ import {
   mockWindowInterface,
   stubServiceForDoc,
 } from '../../testing/test-helper';
+
+import {Observable} from '../../src/observable';
+import {Services} from '../../src/services';
+import {cidServiceForDocForTesting} from '../../src/service/cid-impl';
+import {createIframePromise} from '../../testing/iframe';
+import {installActivityServiceForTesting} from '../../extensions/amp-analytics/0.1/activity-impl';
+import {installCryptoService} from '../../src/service/crypto-impl';
+import {installDocService} from '../../src/service/ampdoc-impl';
+import {installDocumentInfoServiceForDoc} from '../../src/service/document-info-impl';
 import {parseUrlDeprecated} from '../../src/url';
 import {registerServiceBuilder} from '../../src/service';
 import {setCookie} from '../../src/cookies';
@@ -177,6 +179,14 @@ describes.sandboxed('UrlReplacements', {}, () => {
       Math: {
         random: () => 0.1234,
       },
+      crypto: {
+        getRandomValues: array => {
+          array[0] = 1;
+          array[1] = 2;
+          array[2] = 3;
+          array[15] = 15;
+        },
+      },
       services: {
         'viewport': {obj: {}},
         'cid': {
@@ -214,7 +224,7 @@ describes.sandboxed('UrlReplacements', {}, () => {
       // Restrict the number of replacement params to globalVaraibleSource
       // Please consider adding the logic to amp-analytics instead.
       // Please contact @lannka or @zhouyx if the test fail.
-      expect(variables.length).to.equal(71);
+      expect(variables.length).to.equal(72);
     });
   });
 
@@ -595,6 +605,12 @@ describes.sandboxed('UrlReplacements', {}, () => {
     });
   });
 
+  it('should replace PAGE_VIEW_ID_64', () => {
+    return expandUrlAsync('?pid=PAGE_VIEW_ID_64').then(res => {
+      expect(res).to.match(/pid=([a-zA-Z0-9_-]+){10,}/);
+    });
+  });
+
   it('should replace CLIENT_ID', () => {
     setCookie(window, 'url-abc', 'cid-for-abc');
     // Make sure cookie does not exist

diff --git a/test/unit/test-viewer.js b/test/unit/test-viewer.js
@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+import {parseUrlDeprecated, removeFragment} from '../../src/url';
+
 import {Services} from '../../src/services';
 import {Viewer} from '../../src/service/viewer-impl';
 import {dev} from '../../src/log';
@@ -22,7 +24,6 @@ import {installDocumentInfoServiceForDoc} from '../../src/service/document-info-
 import {installGlobalDocumentStateService} from '../../src/service/document-state';
 import {installPlatformService} from '../../src/service/platform-impl';
 import {installTimerService} from '../../src/service/timer-impl';
-import {parseUrlDeprecated, removeFragment} from '../../src/url';
 
 describes.sandboxed('Viewer', {}, () => {
   let windowMock;
@@ -56,6 +57,7 @@ describes.sandboxed('Viewer', {}, () => {
     const WindowApi = function() {};
     windowApi = new WindowApi();
     windowApi.Math = window.Math;
+    windowApi.crypto = window.crypto;
     windowApi.setTimeout = window.setTimeout;
     windowApi.clearTimeout = window.clearTimeout;
     windowApi.Promise = window.Promise;