From f8f3a519d0f23e546b8db6faa23bac5e265ca9d5 Mon Sep 17 00:00:00 2001 From: hamousavi Date: Wed, 7 Feb 2018 18:00:16 -0500 Subject: [PATCH 1/7] whitelist of variables --- src/service/url-replacements-impl.js | 4 +- src/service/variable-source.js | 48 ++- test/functional/test-variable-source.js | 386 +++++++++++++----- test/functional/url-expander/test-expander.js | 37 ++ 4 files changed, 375 insertions(+), 100 deletions(-) diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index abba66686812..69885c9671eb 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -70,9 +70,7 @@ function encodeValue(val) { export class GlobalVariableSource extends VariableSource { constructor(ampdoc) { - super(); - /** @const {!./ampdoc-impl.AmpDoc} */ - this.ampdoc = ampdoc; + super(ampdoc); /** * @private diff --git a/src/service/variable-source.js b/src/service/variable-source.js index a0519686920d..eda3f0e01724 100644 --- a/src/service/variable-source.js +++ b/src/service/variable-source.js @@ -109,7 +109,10 @@ export function getNavigationData(win, attribute) { * and override initialize() to add more supported variables. */ export class VariableSource { - constructor() { + constructor(ampdoc) { + /** @const {!./ampdoc-impl.AmpDoc} */ + this.ampdoc = ampdoc; + /** @private {!RegExp|undefined} */ this.replacementExpr_ = undefined; @@ -121,6 +124,9 @@ export class VariableSource { /** @private {boolean} */ this.initialized_ = false; + + /** @const @private {?Array} */ + this.ampVariableSubstitutionWhitelist_ = this.createWhitelist_(); } /** @@ -164,6 +170,11 @@ export class VariableSource { */ set(varName, syncResolver) { dev().assert(varName.indexOf('RETURN') == -1); + if (this.ampVariableSubstitutionWhitelist_ && + !this.ampVariableSubstitutionWhitelist_.includes(varName)) { + return this; + } + this.replacements_[varName] = this.replacements_[varName] || {sync: undefined, async: undefined}; this.replacements_[varName].sync = syncResolver; @@ -184,6 +195,10 @@ export class VariableSource { */ setAsync(varName, asyncResolver) { dev().assert(varName.indexOf('RETURN') == -1); + if (this.ampVariableSubstitutionWhitelist_ && + !this.ampVariableSubstitutionWhitelist_.includes(varName)) { + return this; + } this.replacements_[varName] = this.replacements_[varName] || {sync: undefined, async: undefined}; this.replacements_[varName].async = asyncResolver; @@ -246,6 +261,11 @@ export class VariableSource { * @private */ buildExpr_(keys, isV2) { + // If a whitelist is provided, the keys must all belong to the whitelist. + if (this.ampVariableSubstitutionWhitelist_) { + keys = keys.filter(key => + this.ampVariableSubstitutionWhitelist_.includes(key)); + } // The keys must be sorted to ensure that the longest keys are considered // first. This avoids a problem where a RANDOM conflicts with RANDOM_ONE. keys.sort((s1, s2) => s2.length - s1.length); @@ -264,4 +284,30 @@ export class VariableSource { } return new RegExp(regexStr, 'g'); } + + /** + * @return {?Array} the whitelist of allowed AMP variables + * (if provided in a meta tag). + * @private + */ + createWhitelist_() { + if(!this.ampdoc){ + return null; + } + + const head = this.ampdoc.getRootNode().head; + if (head) { + // A meta[name="amp-variable-substitution-whitelist"] tag, if present, + // contains, in its content attribute, a whitelist of variable + // substitution. + const meta = + head.querySelector('meta[name="amp-variable-substitution-whitelist"]'); + + if (meta) { + return meta.getAttribute('content').split(',') + .map(variable => variable.trim()); + } + } + return null; + } } diff --git a/test/functional/test-variable-source.js b/test/functional/test-variable-source.js index 0ee3ab5364dd..eda3f0e01724 100644 --- a/test/functional/test-variable-source.js +++ b/test/functional/test-variable-source.js @@ -1,5 +1,5 @@ /** - * Copyright 2015 The AMP HTML Authors. All Rights Reserved. + * Copyright 2016 The AMP HTML Authors. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -13,107 +13,301 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +import {dev} from '../log'; +import {isFiniteNumber} from '../types'; +import {loadPromise} from '../event-helper'; -import { - VariableSource, - getTimingDataAsync, -} from '../../src/service/variable-source'; - - -describe('VariableSource', () => { - let varSource; - beforeEach(() => { - varSource = new VariableSource(); - }); - - it('Works without any variables', () => { - expect(varSource.getExpr()).to.be.ok; - expect(varSource.get('')).to.be.undefined; - }); - - it('Works with sync variables', () => { - varSource.set('Foo', () => 'bar'); - expect(varSource.getExpr()).to.be.ok; - expect(varSource.get('Foo')['sync']()).to.equal('bar'); - expect(varSource.get('foo')).to.be.undefined; - expect(varSource.get('AFoo')).to.be.undefined; - }); - - it('Works with async variables', () => { - varSource.setAsync('Foo', () => Promise.resolve('bar')); - expect(varSource.getExpr()).to.be.ok; - - return varSource.get('Foo')['async']().then(value => { - expect(value).to.equal('bar'); - }); - }); +/** @typedef {string|number|boolean|undefined|null} */ +let ResolverReturnDef; - it('Works with both sync and async variables', () => { - varSource.setBoth('Foo', () => 'bar', () => Promise.resolve('bar')); - expect(varSource.getExpr()).to.be.ok; +/** @typedef {function(...*):ResolverReturnDef} */ +export let SyncResolverDef; - expect(varSource.get('Foo')['sync']()).to.equal('bar'); - return varSource.get('Foo')['async']().then(value => { - expect(value).to.equal('bar'); - }); - }); +/** @typedef {function(...*):!Promise} */ +let AsyncResolverDef; - it('Works with multiple variables', () => { - varSource.setBoth('Foo', () => 'bar', () => Promise.resolve('bar')); - varSource.set('Baz', () => 'Foo'); - expect(varSource.getExpr()).to.be.ok; +/** @typedef {{sync: SyncResolverDef, async: AsyncResolverDef}} */ +let ReplacementDef; - expect(varSource.get('Foo')['sync']()).to.equal('bar'); - expect(varSource.get('Baz')['sync']()).to.equal('Foo'); - return varSource.get('Foo')['async']().then(value => { - expect(value).to.equal('bar'); - }); - }); - - it('Works with sync variable that is set multiple times', () => { - varSource.set('Foo', () => 'bar').set('Foo', () => 'baz'); - expect(varSource.getExpr()).to.be.ok; - expect(varSource.get('Foo')['sync']()).to.equal('baz'); - }); - - it('Works with async variable that is set multiple times', () => { - varSource.setAsync('Foo', () => Promise.resolve('bar')) - .setAsync('Foo', () => Promise.resolve('baz')); - return varSource.get('Foo')['async']().then(value => { - expect(value).to.equal('baz'); - }); - }); - - describes.fakeWin('getTimingData', {}, env => { - let win; - - beforeEach(() => { - win = env.win; - win.performance = { - timing: { - navigationStart: 1, - loadEventStart: 0, - }, - }; - }); - it('should resolve immediate when data is ready', () => { - win.performance.timing.loadEventStart = 12; - return getTimingDataAsync(win, 'navigationStart', 'loadEventStart') - .then(value => { - expect(value).to.equal(11); - }); +/** + * Returns navigation timing information based on the start and end events. + * The data for the timing events is retrieved from performance.timing API. + * If start and end events are both given, the result is the difference between + * the two. If only start event is given, the result is the timing value at + * start event. + * @param {!Window} win + * @param {string} startEvent + * @param {string=} endEvent + * @return {!Promise} + */ +export function getTimingDataAsync(win, startEvent, endEvent) { + const metric = getTimingDataSync(win, startEvent, endEvent); + if (metric === '') { + // Metric is not yet available. Retry after a delay. + return loadPromise(win).then(() => { + return getTimingDataSync(win, startEvent, endEvent); }); + } + return Promise.resolve(metric); +} + +/** + * Returns navigation timing information based on the start and end events. + * The data for the timing events is retrieved from performance.timing API. + * If start and end events are both given, the result is the difference between + * the two. If only start event is given, the result is the timing value at + * start event. Enforces synchronous evaluation. + * @param {!Window} win + * @param {string} startEvent + * @param {string=} endEvent + * @return {ResolverReturnDef} undefined if API is not available, empty string + * if it is not yet available, or value as string + */ +export function getTimingDataSync(win, startEvent, endEvent) { + const timingInfo = win['performance'] && win['performance']['timing']; + if (!timingInfo || timingInfo['navigationStart'] == 0) { + // Navigation timing API is not supported. + return; + } + + const metric = (endEvent === undefined) + ? timingInfo[startEvent] + : timingInfo[endEvent] - timingInfo[startEvent]; + + if (!isFiniteNumber(metric)) { + // The metric is not supported. + return; + } else if (metric < 0) { + return ''; + } else { + return metric; + } +} + +/** + * Returns navigation information from the current browsing context. + * @param {!Window} win + * @param {string} attribute + * @return {ResolverReturnDef} + * @private + */ +export function getNavigationData(win, attribute) { + const navigationInfo = win['performance'] && + win['performance']['navigation']; + if (!navigationInfo || navigationInfo[attribute] === undefined) { + // PerformanceNavigation interface is not supported or attribute is not + // implemented. + return; + } + return navigationInfo[attribute]; +} + + +/** + * A class to provide variable substitution related features. Extend this class + * and override initialize() to add more supported variables. + */ +export class VariableSource { + constructor(ampdoc) { + /** @const {!./ampdoc-impl.AmpDoc} */ + this.ampdoc = ampdoc; + + /** @private {!RegExp|undefined} */ + this.replacementExpr_ = undefined; - it('should wait for load event', () => { - win.readyState = 'other'; - const p = getTimingDataAsync(win, 'navigationStart', 'loadEventStart'); - expect(win.eventListeners.count('load')).to.equal(1); - win.performance.timing.loadEventStart = 12; - win.eventListeners.fire({type: 'load'}); - return p.then(value => { - expect(value).to.equal(11); + /** @private {!RegExp|undefined} */ + this.replacementExprV2_ = undefined; + + /** @private @const {!Object} */ + this.replacements_ = Object.create(null); + + /** @private {boolean} */ + this.initialized_ = false; + + /** @const @private {?Array} */ + this.ampVariableSubstitutionWhitelist_ = this.createWhitelist_(); + } + + /** + * Lazily initialize the default replacements. + * @private + */ + initialize_() { + this.initialize(); + this.initialized_ = true; + } + + /** + * Override this method to set all the variables supported by derived class. + */ + initialize() { + // Needs to be implemented by derived classes. + } + + /** + * Method exists to assist stubbing in tests. + * @param {string} name + * @return {!ReplacementDef} + */ + get(name) { + if (!this.initialized_) { + this.initialize_(); + } + + return this.replacements_[name]; + } + + /** + * Sets a synchronous value resolver for the variable with the specified name. + * The value resolver may optionally take an extra parameter. + * Can be called in conjunction with setAsync to allow for additional + * asynchronous resolver where expand will use async and expandSync the sync + * version. + * @param {string} varName + * @param {!SyncResolverDef} syncResolver + * @return {!VariableSource} + */ + set(varName, syncResolver) { + dev().assert(varName.indexOf('RETURN') == -1); + if (this.ampVariableSubstitutionWhitelist_ && + !this.ampVariableSubstitutionWhitelist_.includes(varName)) { + return this; + } + + this.replacements_[varName] = + this.replacements_[varName] || {sync: undefined, async: undefined}; + this.replacements_[varName].sync = syncResolver; + this.replacementExpr_ = undefined; + this.replacementExprV2_ = undefined; + return this; + } + + /** + * Sets an async value resolver for the variable with the specified name. + * The value resolver may optionally take an extra parameter. + * Can be called in conjuction with setAsync to allow for additional + * asynchronous resolver where expand will use async and expandSync the sync + * version. + * @param {string} varName + * @param {!AsyncResolverDef} asyncResolver + * @return {!VariableSource} + */ + setAsync(varName, asyncResolver) { + dev().assert(varName.indexOf('RETURN') == -1); + if (this.ampVariableSubstitutionWhitelist_ && + !this.ampVariableSubstitutionWhitelist_.includes(varName)) { + return this; + } + this.replacements_[varName] = + this.replacements_[varName] || {sync: undefined, async: undefined}; + this.replacements_[varName].async = asyncResolver; + this.replacementExpr_ = undefined; + this.replacementExprV2_ = undefined; + return this; + } + + /** + * Helper method to set both sync and async resolvers. + * @param {string} varName + * @param {!SyncResolverDef} syncResolver + * @param {!AsyncResolverDef} asyncResolver + * @return {!VariableSource} + */ + setBoth(varName, syncResolver, asyncResolver) { + return this.set(varName, syncResolver).setAsync(varName, asyncResolver); + } + + /** + * Returns a Regular expression that can be used to detect all the variables + * in a template. + * @param {!Object=} opt_bindings + * @param {boolean=} isV2 flag to ignore capture of args + */ + getExpr(opt_bindings, isV2) { + if (!this.initialized_) { + this.initialize_(); + } + + const additionalKeys = opt_bindings ? Object.keys(opt_bindings) : null; + if (additionalKeys && additionalKeys.length > 0) { + const allKeys = Object.keys(this.replacements_); + additionalKeys.forEach(key => { + if (this.replacements_[key] === undefined) { + allKeys.push(key); + } }); - }); - }); -}); + return this.buildExpr_(allKeys, isV2); + } + if (!this.replacementExpr_ && !isV2) { + this.replacementExpr_ = this.buildExpr_( + Object.keys(this.replacements_)); + } + // sometimes the v1 expand will be called before the v2 + // so we need to cache both versions + if (!this.replacementExprV2_ && isV2) { + this.replacementExprV2_ = this.buildExpr_( + Object.keys(this.replacements_), isV2); + } + + return isV2 ? this.replacementExprV2_ : + this.replacementExpr_; + } + + /** + * @param {!Array} keys + * @param {boolean=} isV2 flag to ignore capture of args + * @return {!RegExp} + * @private + */ + buildExpr_(keys, isV2) { + // If a whitelist is provided, the keys must all belong to the whitelist. + if (this.ampVariableSubstitutionWhitelist_) { + keys = keys.filter(key => + this.ampVariableSubstitutionWhitelist_.includes(key)); + } + // The keys must be sorted to ensure that the longest keys are considered + // first. This avoids a problem where a RANDOM conflicts with RANDOM_ONE. + keys.sort((s1, s2) => s2.length - s1.length); + const all = keys.join('|'); + // Match the given replacement patterns, as well as optionally + // arguments to the replacement behind it in parentheses. + // Example string that match + // FOO_BAR + // FOO_BAR(arg1) + // FOO_BAR(arg1,arg2) + // FOO_BAR(arg1, arg2) + let regexStr = '\\$?(' + all + ')'; + // ignore the capturing of arguments in new parser + if (!isV2) { + regexStr += '(?:\\(((?:\\s*[0-9a-zA-Z-_.]*\\s*(?=,|\\)),?)*)\\s*\\))?'; + } + return new RegExp(regexStr, 'g'); + } + + /** + * @return {?Array} the whitelist of allowed AMP variables + * (if provided in a meta tag). + * @private + */ + createWhitelist_() { + if(!this.ampdoc){ + return null; + } + + const head = this.ampdoc.getRootNode().head; + if (head) { + // A meta[name="amp-variable-substitution-whitelist"] tag, if present, + // contains, in its content attribute, a whitelist of variable + // substitution. + const meta = + head.querySelector('meta[name="amp-variable-substitution-whitelist"]'); + + if (meta) { + return meta.getAttribute('content').split(',') + .map(variable => variable.trim()); + } + } + return null; + } +} diff --git a/test/functional/url-expander/test-expander.js b/test/functional/url-expander/test-expander.js index 3b699f3db91c..048689380a7e 100644 --- a/test/functional/url-expander/test-expander.js +++ b/test/functional/url-expander/test-expander.js @@ -14,8 +14,10 @@ * limitations under the License. */ +import {AmpDocSingle} from '../../../src/service/ampdoc-impl'; import {Expander} from '../../../src/service/url-expander/expander'; import {GlobalVariableSource} from '../../../src/service/url-replacements-impl'; +import {createElementWithAttributes} from '../../../src/dom'; describes.realWin('Expander', { amp: { @@ -86,6 +88,41 @@ describes.realWin('Expander', { }); }); + describe('#whitelist', () => { + let variableSource; + let expander; + + const mockBindings = { + RANDOM: () => 0.1234, + ABC: () => 'three', + ABCD: () => 'four', + }; + beforeEach(() => { + const fakeMeta = { + getAttribute: () => 'ABC,ABCD,CANONICAL', + }; + sandbox.stub(env.win.document.head, + 'querySelector').callsFake(() => fakeMeta); + + variableSource = new GlobalVariableSource(env.ampdoc); + expander = new Expander(variableSource); + }); + + it('should not replace unwhitelisted RANDOM', () => { + const url = 'http://www.google.com/?test=RANDOM'; + const expected = 'http://www.google.com/?test=RANDOM'; + return expect(expander.expand(url, mockBindings)) + .to.eventually.equal(expected); + }); + + it('should replace whitelisted ABCD', () => { + const url = 'http://www.google.com/?test=ABCD'; + const expected = 'http://www.google.com/?test=four'; + return expect(expander.expand(url, mockBindings)) + .to.eventually.equal(expected); + }); + }); + describe('#expand', () => { function mockClientIdFn(str) { if (str === '__ga') { From 713a39763d40edcb9c4198fbf7c6a9f02f679f29 Mon Sep 17 00:00:00 2001 From: hamousavi Date: Wed, 7 Feb 2018 18:05:38 -0500 Subject: [PATCH 2/7] revert variable-source test --- src/service/variable-source.js | 386 ++++++++------------------------- 1 file changed, 96 insertions(+), 290 deletions(-) diff --git a/src/service/variable-source.js b/src/service/variable-source.js index eda3f0e01724..5587492cee1c 100644 --- a/src/service/variable-source.js +++ b/src/service/variable-source.js @@ -1,5 +1,5 @@ /** - * Copyright 2016 The AMP HTML Authors. All Rights Reserved. + * Copyright 2015 The AMP HTML Authors. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -13,301 +13,107 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import {dev} from '../log'; -import {isFiniteNumber} from '../types'; -import {loadPromise} from '../event-helper'; -/** @typedef {string|number|boolean|undefined|null} */ -let ResolverReturnDef; - -/** @typedef {function(...*):ResolverReturnDef} */ -export let SyncResolverDef; - -/** @typedef {function(...*):!Promise} */ -let AsyncResolverDef; - -/** @typedef {{sync: SyncResolverDef, async: AsyncResolverDef}} */ -let ReplacementDef; - - -/** - * Returns navigation timing information based on the start and end events. - * The data for the timing events is retrieved from performance.timing API. - * If start and end events are both given, the result is the difference between - * the two. If only start event is given, the result is the timing value at - * start event. - * @param {!Window} win - * @param {string} startEvent - * @param {string=} endEvent - * @return {!Promise} - */ -export function getTimingDataAsync(win, startEvent, endEvent) { - const metric = getTimingDataSync(win, startEvent, endEvent); - if (metric === '') { - // Metric is not yet available. Retry after a delay. - return loadPromise(win).then(() => { - return getTimingDataSync(win, startEvent, endEvent); +import { + VariableSource, + getTimingDataAsync, +} from '../../src/service/variable-source'; + + +describe('VariableSource', () => { + let varSource; + beforeEach(() => { + varSource = new VariableSource(); + }); + + it('Works without any variables', () => { + expect(varSource.getExpr()).to.be.ok; + expect(varSource.get('')).to.be.undefined; + }); + + it('Works with sync variables', () => { + varSource.set('Foo', () => 'bar'); + expect(varSource.getExpr()).to.be.ok; + expect(varSource.get('Foo')['sync']()).to.equal('bar'); + expect(varSource.get('foo')).to.be.undefined; + expect(varSource.get('AFoo')).to.be.undefined; + }); + + it('Works with async variables', () => { + varSource.setAsync('Foo', () => Promise.resolve('bar')); + expect(varSource.getExpr()).to.be.ok; + + return varSource.get('Foo')['async']().then(value => { + expect(value).to.equal('bar'); }); - } - return Promise.resolve(metric); -} - -/** - * Returns navigation timing information based on the start and end events. - * The data for the timing events is retrieved from performance.timing API. - * If start and end events are both given, the result is the difference between - * the two. If only start event is given, the result is the timing value at - * start event. Enforces synchronous evaluation. - * @param {!Window} win - * @param {string} startEvent - * @param {string=} endEvent - * @return {ResolverReturnDef} undefined if API is not available, empty string - * if it is not yet available, or value as string - */ -export function getTimingDataSync(win, startEvent, endEvent) { - const timingInfo = win['performance'] && win['performance']['timing']; - if (!timingInfo || timingInfo['navigationStart'] == 0) { - // Navigation timing API is not supported. - return; - } - - const metric = (endEvent === undefined) - ? timingInfo[startEvent] - : timingInfo[endEvent] - timingInfo[startEvent]; - - if (!isFiniteNumber(metric)) { - // The metric is not supported. - return; - } else if (metric < 0) { - return ''; - } else { - return metric; - } -} - -/** - * Returns navigation information from the current browsing context. - * @param {!Window} win - * @param {string} attribute - * @return {ResolverReturnDef} - * @private - */ -export function getNavigationData(win, attribute) { - const navigationInfo = win['performance'] && - win['performance']['navigation']; - if (!navigationInfo || navigationInfo[attribute] === undefined) { - // PerformanceNavigation interface is not supported or attribute is not - // implemented. - return; - } - return navigationInfo[attribute]; -} - - -/** - * A class to provide variable substitution related features. Extend this class - * and override initialize() to add more supported variables. - */ -export class VariableSource { - constructor(ampdoc) { - /** @const {!./ampdoc-impl.AmpDoc} */ - this.ampdoc = ampdoc; - - /** @private {!RegExp|undefined} */ - this.replacementExpr_ = undefined; + }); - /** @private {!RegExp|undefined} */ - this.replacementExprV2_ = undefined; + it('Works with both sync and async variables', () => { + varSource.setBoth('Foo', () => 'bar', () => Promise.resolve('bar')); + expect(varSource.getExpr()).to.be.ok; - /** @private @const {!Object} */ - this.replacements_ = Object.create(null); - - /** @private {boolean} */ - this.initialized_ = false; - - /** @const @private {?Array} */ - this.ampVariableSubstitutionWhitelist_ = this.createWhitelist_(); - } - - /** - * Lazily initialize the default replacements. - * @private - */ - initialize_() { - this.initialize(); - this.initialized_ = true; - } - - /** - * Override this method to set all the variables supported by derived class. - */ - initialize() { - // Needs to be implemented by derived classes. - } - - /** - * Method exists to assist stubbing in tests. - * @param {string} name - * @return {!ReplacementDef} - */ - get(name) { - if (!this.initialized_) { - this.initialize_(); - } - - return this.replacements_[name]; - } - - /** - * Sets a synchronous value resolver for the variable with the specified name. - * The value resolver may optionally take an extra parameter. - * Can be called in conjunction with setAsync to allow for additional - * asynchronous resolver where expand will use async and expandSync the sync - * version. - * @param {string} varName - * @param {!SyncResolverDef} syncResolver - * @return {!VariableSource} - */ - set(varName, syncResolver) { - dev().assert(varName.indexOf('RETURN') == -1); - if (this.ampVariableSubstitutionWhitelist_ && - !this.ampVariableSubstitutionWhitelist_.includes(varName)) { - return this; - } - - this.replacements_[varName] = - this.replacements_[varName] || {sync: undefined, async: undefined}; - this.replacements_[varName].sync = syncResolver; - this.replacementExpr_ = undefined; - this.replacementExprV2_ = undefined; - return this; - } + expect(varSource.get('Foo')['sync']()).to.equal('bar'); + return varSource.get('Foo')['async']().then(value => { + expect(value).to.equal('bar'); + }); + }); - /** - * Sets an async value resolver for the variable with the specified name. - * The value resolver may optionally take an extra parameter. - * Can be called in conjuction with setAsync to allow for additional - * asynchronous resolver where expand will use async and expandSync the sync - * version. - * @param {string} varName - * @param {!AsyncResolverDef} asyncResolver - * @return {!VariableSource} - */ - setAsync(varName, asyncResolver) { - dev().assert(varName.indexOf('RETURN') == -1); - if (this.ampVariableSubstitutionWhitelist_ && - !this.ampVariableSubstitutionWhitelist_.includes(varName)) { - return this; - } - this.replacements_[varName] = - this.replacements_[varName] || {sync: undefined, async: undefined}; - this.replacements_[varName].async = asyncResolver; - this.replacementExpr_ = undefined; - this.replacementExprV2_ = undefined; - return this; - } + it('Works with multiple variables', () => { + varSource.setBoth('Foo', () => 'bar', () => Promise.resolve('bar')); + varSource.set('Baz', () => 'Foo'); + expect(varSource.getExpr()).to.be.ok; - /** - * Helper method to set both sync and async resolvers. - * @param {string} varName - * @param {!SyncResolverDef} syncResolver - * @param {!AsyncResolverDef} asyncResolver - * @return {!VariableSource} - */ - setBoth(varName, syncResolver, asyncResolver) { - return this.set(varName, syncResolver).setAsync(varName, asyncResolver); - } + expect(varSource.get('Foo')['sync']()).to.equal('bar'); + expect(varSource.get('Baz')['sync']()).to.equal('Foo'); + return varSource.get('Foo')['async']().then(value => { + expect(value).to.equal('bar'); + }); + }); + + it('Works with sync variable that is set multiple times', () => { + varSource.set('Foo', () => 'bar').set('Foo', () => 'baz'); + expect(varSource.getExpr()).to.be.ok; + expect(varSource.get('Foo')['sync']()).to.equal('baz'); + }); + + it('Works with async variable that is set multiple times', () => { + varSource.setAsync('Foo', () => Promise.resolve('bar')) + .setAsync('Foo', () => Promise.resolve('baz')); + return varSource.get('Foo')['async']().then(value => { + expect(value).to.equal('baz'); + }); + }); + + describes.fakeWin('getTimingData', {}, env => { + let win; + + beforeEach(() => { + win = env.win; + win.performance = { + timing: { + navigationStart: 1, + loadEventStart: 0, + }, + }; + }); - /** - * Returns a Regular expression that can be used to detect all the variables - * in a template. - * @param {!Object=} opt_bindings - * @param {boolean=} isV2 flag to ignore capture of args - */ - getExpr(opt_bindings, isV2) { - if (!this.initialized_) { - this.initialize_(); - } + it('should resolve immediate when data is ready', () => { + win.performance.timing.loadEventStart = 12; + return getTimingDataAsync(win, 'navigationStart', 'loadEventStart') + .then(value => { + expect(value).to.equal(11); + }); + }); - const additionalKeys = opt_bindings ? Object.keys(opt_bindings) : null; - if (additionalKeys && additionalKeys.length > 0) { - const allKeys = Object.keys(this.replacements_); - additionalKeys.forEach(key => { - if (this.replacements_[key] === undefined) { - allKeys.push(key); - } + it('should wait for load event', () => { + win.readyState = 'other'; + const p = getTimingDataAsync(win, 'navigationStart', 'loadEventStart'); + expect(win.eventListeners.count('load')).to.equal(1); + win.performance.timing.loadEventStart = 12; + win.eventListeners.fire({type: 'load'}); + return p.then(value => { + expect(value).to.equal(11); }); - return this.buildExpr_(allKeys, isV2); - } - if (!this.replacementExpr_ && !isV2) { - this.replacementExpr_ = this.buildExpr_( - Object.keys(this.replacements_)); - } - // sometimes the v1 expand will be called before the v2 - // so we need to cache both versions - if (!this.replacementExprV2_ && isV2) { - this.replacementExprV2_ = this.buildExpr_( - Object.keys(this.replacements_), isV2); - } - - return isV2 ? this.replacementExprV2_ : - this.replacementExpr_; - } - - /** - * @param {!Array} keys - * @param {boolean=} isV2 flag to ignore capture of args - * @return {!RegExp} - * @private - */ - buildExpr_(keys, isV2) { - // If a whitelist is provided, the keys must all belong to the whitelist. - if (this.ampVariableSubstitutionWhitelist_) { - keys = keys.filter(key => - this.ampVariableSubstitutionWhitelist_.includes(key)); - } - // The keys must be sorted to ensure that the longest keys are considered - // first. This avoids a problem where a RANDOM conflicts with RANDOM_ONE. - keys.sort((s1, s2) => s2.length - s1.length); - const all = keys.join('|'); - // Match the given replacement patterns, as well as optionally - // arguments to the replacement behind it in parentheses. - // Example string that match - // FOO_BAR - // FOO_BAR(arg1) - // FOO_BAR(arg1,arg2) - // FOO_BAR(arg1, arg2) - let regexStr = '\\$?(' + all + ')'; - // ignore the capturing of arguments in new parser - if (!isV2) { - regexStr += '(?:\\(((?:\\s*[0-9a-zA-Z-_.]*\\s*(?=,|\\)),?)*)\\s*\\))?'; - } - return new RegExp(regexStr, 'g'); - } - - /** - * @return {?Array} the whitelist of allowed AMP variables - * (if provided in a meta tag). - * @private - */ - createWhitelist_() { - if(!this.ampdoc){ - return null; - } - - const head = this.ampdoc.getRootNode().head; - if (head) { - // A meta[name="amp-variable-substitution-whitelist"] tag, if present, - // contains, in its content attribute, a whitelist of variable - // substitution. - const meta = - head.querySelector('meta[name="amp-variable-substitution-whitelist"]'); - - if (meta) { - return meta.getAttribute('content').split(',') - .map(variable => variable.trim()); - } - } - return null; - } -} + }); + }); +}); \ No newline at end of file From cd9d0c99a7e68ad0b8fcea9545fc475dcec87c4f Mon Sep 17 00:00:00 2001 From: hamousavi Date: Wed, 7 Feb 2018 18:09:05 -0500 Subject: [PATCH 3/7] reset --- src/service/variable-source.js | 386 ++++++++++++++++++------ test/functional/test-variable-source.js | 386 ++++++------------------ 2 files changed, 386 insertions(+), 386 deletions(-) diff --git a/src/service/variable-source.js b/src/service/variable-source.js index 5587492cee1c..eda3f0e01724 100644 --- a/src/service/variable-source.js +++ b/src/service/variable-source.js @@ -1,5 +1,5 @@ /** - * Copyright 2015 The AMP HTML Authors. All Rights Reserved. + * Copyright 2016 The AMP HTML Authors. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -13,107 +13,301 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +import {dev} from '../log'; +import {isFiniteNumber} from '../types'; +import {loadPromise} from '../event-helper'; -import { - VariableSource, - getTimingDataAsync, -} from '../../src/service/variable-source'; - - -describe('VariableSource', () => { - let varSource; - beforeEach(() => { - varSource = new VariableSource(); - }); - - it('Works without any variables', () => { - expect(varSource.getExpr()).to.be.ok; - expect(varSource.get('')).to.be.undefined; - }); - - it('Works with sync variables', () => { - varSource.set('Foo', () => 'bar'); - expect(varSource.getExpr()).to.be.ok; - expect(varSource.get('Foo')['sync']()).to.equal('bar'); - expect(varSource.get('foo')).to.be.undefined; - expect(varSource.get('AFoo')).to.be.undefined; - }); - - it('Works with async variables', () => { - varSource.setAsync('Foo', () => Promise.resolve('bar')); - expect(varSource.getExpr()).to.be.ok; - - return varSource.get('Foo')['async']().then(value => { - expect(value).to.equal('bar'); - }); - }); +/** @typedef {string|number|boolean|undefined|null} */ +let ResolverReturnDef; - it('Works with both sync and async variables', () => { - varSource.setBoth('Foo', () => 'bar', () => Promise.resolve('bar')); - expect(varSource.getExpr()).to.be.ok; +/** @typedef {function(...*):ResolverReturnDef} */ +export let SyncResolverDef; - expect(varSource.get('Foo')['sync']()).to.equal('bar'); - return varSource.get('Foo')['async']().then(value => { - expect(value).to.equal('bar'); - }); - }); +/** @typedef {function(...*):!Promise} */ +let AsyncResolverDef; - it('Works with multiple variables', () => { - varSource.setBoth('Foo', () => 'bar', () => Promise.resolve('bar')); - varSource.set('Baz', () => 'Foo'); - expect(varSource.getExpr()).to.be.ok; +/** @typedef {{sync: SyncResolverDef, async: AsyncResolverDef}} */ +let ReplacementDef; - expect(varSource.get('Foo')['sync']()).to.equal('bar'); - expect(varSource.get('Baz')['sync']()).to.equal('Foo'); - return varSource.get('Foo')['async']().then(value => { - expect(value).to.equal('bar'); - }); - }); - - it('Works with sync variable that is set multiple times', () => { - varSource.set('Foo', () => 'bar').set('Foo', () => 'baz'); - expect(varSource.getExpr()).to.be.ok; - expect(varSource.get('Foo')['sync']()).to.equal('baz'); - }); - - it('Works with async variable that is set multiple times', () => { - varSource.setAsync('Foo', () => Promise.resolve('bar')) - .setAsync('Foo', () => Promise.resolve('baz')); - return varSource.get('Foo')['async']().then(value => { - expect(value).to.equal('baz'); - }); - }); - - describes.fakeWin('getTimingData', {}, env => { - let win; - - beforeEach(() => { - win = env.win; - win.performance = { - timing: { - navigationStart: 1, - loadEventStart: 0, - }, - }; - }); - it('should resolve immediate when data is ready', () => { - win.performance.timing.loadEventStart = 12; - return getTimingDataAsync(win, 'navigationStart', 'loadEventStart') - .then(value => { - expect(value).to.equal(11); - }); +/** + * Returns navigation timing information based on the start and end events. + * The data for the timing events is retrieved from performance.timing API. + * If start and end events are both given, the result is the difference between + * the two. If only start event is given, the result is the timing value at + * start event. + * @param {!Window} win + * @param {string} startEvent + * @param {string=} endEvent + * @return {!Promise} + */ +export function getTimingDataAsync(win, startEvent, endEvent) { + const metric = getTimingDataSync(win, startEvent, endEvent); + if (metric === '') { + // Metric is not yet available. Retry after a delay. + return loadPromise(win).then(() => { + return getTimingDataSync(win, startEvent, endEvent); }); + } + return Promise.resolve(metric); +} + +/** + * Returns navigation timing information based on the start and end events. + * The data for the timing events is retrieved from performance.timing API. + * If start and end events are both given, the result is the difference between + * the two. If only start event is given, the result is the timing value at + * start event. Enforces synchronous evaluation. + * @param {!Window} win + * @param {string} startEvent + * @param {string=} endEvent + * @return {ResolverReturnDef} undefined if API is not available, empty string + * if it is not yet available, or value as string + */ +export function getTimingDataSync(win, startEvent, endEvent) { + const timingInfo = win['performance'] && win['performance']['timing']; + if (!timingInfo || timingInfo['navigationStart'] == 0) { + // Navigation timing API is not supported. + return; + } + + const metric = (endEvent === undefined) + ? timingInfo[startEvent] + : timingInfo[endEvent] - timingInfo[startEvent]; + + if (!isFiniteNumber(metric)) { + // The metric is not supported. + return; + } else if (metric < 0) { + return ''; + } else { + return metric; + } +} + +/** + * Returns navigation information from the current browsing context. + * @param {!Window} win + * @param {string} attribute + * @return {ResolverReturnDef} + * @private + */ +export function getNavigationData(win, attribute) { + const navigationInfo = win['performance'] && + win['performance']['navigation']; + if (!navigationInfo || navigationInfo[attribute] === undefined) { + // PerformanceNavigation interface is not supported or attribute is not + // implemented. + return; + } + return navigationInfo[attribute]; +} + + +/** + * A class to provide variable substitution related features. Extend this class + * and override initialize() to add more supported variables. + */ +export class VariableSource { + constructor(ampdoc) { + /** @const {!./ampdoc-impl.AmpDoc} */ + this.ampdoc = ampdoc; + + /** @private {!RegExp|undefined} */ + this.replacementExpr_ = undefined; - it('should wait for load event', () => { - win.readyState = 'other'; - const p = getTimingDataAsync(win, 'navigationStart', 'loadEventStart'); - expect(win.eventListeners.count('load')).to.equal(1); - win.performance.timing.loadEventStart = 12; - win.eventListeners.fire({type: 'load'}); - return p.then(value => { - expect(value).to.equal(11); + /** @private {!RegExp|undefined} */ + this.replacementExprV2_ = undefined; + + /** @private @const {!Object} */ + this.replacements_ = Object.create(null); + + /** @private {boolean} */ + this.initialized_ = false; + + /** @const @private {?Array} */ + this.ampVariableSubstitutionWhitelist_ = this.createWhitelist_(); + } + + /** + * Lazily initialize the default replacements. + * @private + */ + initialize_() { + this.initialize(); + this.initialized_ = true; + } + + /** + * Override this method to set all the variables supported by derived class. + */ + initialize() { + // Needs to be implemented by derived classes. + } + + /** + * Method exists to assist stubbing in tests. + * @param {string} name + * @return {!ReplacementDef} + */ + get(name) { + if (!this.initialized_) { + this.initialize_(); + } + + return this.replacements_[name]; + } + + /** + * Sets a synchronous value resolver for the variable with the specified name. + * The value resolver may optionally take an extra parameter. + * Can be called in conjunction with setAsync to allow for additional + * asynchronous resolver where expand will use async and expandSync the sync + * version. + * @param {string} varName + * @param {!SyncResolverDef} syncResolver + * @return {!VariableSource} + */ + set(varName, syncResolver) { + dev().assert(varName.indexOf('RETURN') == -1); + if (this.ampVariableSubstitutionWhitelist_ && + !this.ampVariableSubstitutionWhitelist_.includes(varName)) { + return this; + } + + this.replacements_[varName] = + this.replacements_[varName] || {sync: undefined, async: undefined}; + this.replacements_[varName].sync = syncResolver; + this.replacementExpr_ = undefined; + this.replacementExprV2_ = undefined; + return this; + } + + /** + * Sets an async value resolver for the variable with the specified name. + * The value resolver may optionally take an extra parameter. + * Can be called in conjuction with setAsync to allow for additional + * asynchronous resolver where expand will use async and expandSync the sync + * version. + * @param {string} varName + * @param {!AsyncResolverDef} asyncResolver + * @return {!VariableSource} + */ + setAsync(varName, asyncResolver) { + dev().assert(varName.indexOf('RETURN') == -1); + if (this.ampVariableSubstitutionWhitelist_ && + !this.ampVariableSubstitutionWhitelist_.includes(varName)) { + return this; + } + this.replacements_[varName] = + this.replacements_[varName] || {sync: undefined, async: undefined}; + this.replacements_[varName].async = asyncResolver; + this.replacementExpr_ = undefined; + this.replacementExprV2_ = undefined; + return this; + } + + /** + * Helper method to set both sync and async resolvers. + * @param {string} varName + * @param {!SyncResolverDef} syncResolver + * @param {!AsyncResolverDef} asyncResolver + * @return {!VariableSource} + */ + setBoth(varName, syncResolver, asyncResolver) { + return this.set(varName, syncResolver).setAsync(varName, asyncResolver); + } + + /** + * Returns a Regular expression that can be used to detect all the variables + * in a template. + * @param {!Object=} opt_bindings + * @param {boolean=} isV2 flag to ignore capture of args + */ + getExpr(opt_bindings, isV2) { + if (!this.initialized_) { + this.initialize_(); + } + + const additionalKeys = opt_bindings ? Object.keys(opt_bindings) : null; + if (additionalKeys && additionalKeys.length > 0) { + const allKeys = Object.keys(this.replacements_); + additionalKeys.forEach(key => { + if (this.replacements_[key] === undefined) { + allKeys.push(key); + } }); - }); - }); -}); \ No newline at end of file + return this.buildExpr_(allKeys, isV2); + } + if (!this.replacementExpr_ && !isV2) { + this.replacementExpr_ = this.buildExpr_( + Object.keys(this.replacements_)); + } + // sometimes the v1 expand will be called before the v2 + // so we need to cache both versions + if (!this.replacementExprV2_ && isV2) { + this.replacementExprV2_ = this.buildExpr_( + Object.keys(this.replacements_), isV2); + } + + return isV2 ? this.replacementExprV2_ : + this.replacementExpr_; + } + + /** + * @param {!Array} keys + * @param {boolean=} isV2 flag to ignore capture of args + * @return {!RegExp} + * @private + */ + buildExpr_(keys, isV2) { + // If a whitelist is provided, the keys must all belong to the whitelist. + if (this.ampVariableSubstitutionWhitelist_) { + keys = keys.filter(key => + this.ampVariableSubstitutionWhitelist_.includes(key)); + } + // The keys must be sorted to ensure that the longest keys are considered + // first. This avoids a problem where a RANDOM conflicts with RANDOM_ONE. + keys.sort((s1, s2) => s2.length - s1.length); + const all = keys.join('|'); + // Match the given replacement patterns, as well as optionally + // arguments to the replacement behind it in parentheses. + // Example string that match + // FOO_BAR + // FOO_BAR(arg1) + // FOO_BAR(arg1,arg2) + // FOO_BAR(arg1, arg2) + let regexStr = '\\$?(' + all + ')'; + // ignore the capturing of arguments in new parser + if (!isV2) { + regexStr += '(?:\\(((?:\\s*[0-9a-zA-Z-_.]*\\s*(?=,|\\)),?)*)\\s*\\))?'; + } + return new RegExp(regexStr, 'g'); + } + + /** + * @return {?Array} the whitelist of allowed AMP variables + * (if provided in a meta tag). + * @private + */ + createWhitelist_() { + if(!this.ampdoc){ + return null; + } + + const head = this.ampdoc.getRootNode().head; + if (head) { + // A meta[name="amp-variable-substitution-whitelist"] tag, if present, + // contains, in its content attribute, a whitelist of variable + // substitution. + const meta = + head.querySelector('meta[name="amp-variable-substitution-whitelist"]'); + + if (meta) { + return meta.getAttribute('content').split(',') + .map(variable => variable.trim()); + } + } + return null; + } +} diff --git a/test/functional/test-variable-source.js b/test/functional/test-variable-source.js index eda3f0e01724..5587492cee1c 100644 --- a/test/functional/test-variable-source.js +++ b/test/functional/test-variable-source.js @@ -1,5 +1,5 @@ /** - * Copyright 2016 The AMP HTML Authors. All Rights Reserved. + * Copyright 2015 The AMP HTML Authors. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -13,301 +13,107 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import {dev} from '../log'; -import {isFiniteNumber} from '../types'; -import {loadPromise} from '../event-helper'; -/** @typedef {string|number|boolean|undefined|null} */ -let ResolverReturnDef; - -/** @typedef {function(...*):ResolverReturnDef} */ -export let SyncResolverDef; - -/** @typedef {function(...*):!Promise} */ -let AsyncResolverDef; - -/** @typedef {{sync: SyncResolverDef, async: AsyncResolverDef}} */ -let ReplacementDef; - - -/** - * Returns navigation timing information based on the start and end events. - * The data for the timing events is retrieved from performance.timing API. - * If start and end events are both given, the result is the difference between - * the two. If only start event is given, the result is the timing value at - * start event. - * @param {!Window} win - * @param {string} startEvent - * @param {string=} endEvent - * @return {!Promise} - */ -export function getTimingDataAsync(win, startEvent, endEvent) { - const metric = getTimingDataSync(win, startEvent, endEvent); - if (metric === '') { - // Metric is not yet available. Retry after a delay. - return loadPromise(win).then(() => { - return getTimingDataSync(win, startEvent, endEvent); +import { + VariableSource, + getTimingDataAsync, +} from '../../src/service/variable-source'; + + +describe('VariableSource', () => { + let varSource; + beforeEach(() => { + varSource = new VariableSource(); + }); + + it('Works without any variables', () => { + expect(varSource.getExpr()).to.be.ok; + expect(varSource.get('')).to.be.undefined; + }); + + it('Works with sync variables', () => { + varSource.set('Foo', () => 'bar'); + expect(varSource.getExpr()).to.be.ok; + expect(varSource.get('Foo')['sync']()).to.equal('bar'); + expect(varSource.get('foo')).to.be.undefined; + expect(varSource.get('AFoo')).to.be.undefined; + }); + + it('Works with async variables', () => { + varSource.setAsync('Foo', () => Promise.resolve('bar')); + expect(varSource.getExpr()).to.be.ok; + + return varSource.get('Foo')['async']().then(value => { + expect(value).to.equal('bar'); }); - } - return Promise.resolve(metric); -} - -/** - * Returns navigation timing information based on the start and end events. - * The data for the timing events is retrieved from performance.timing API. - * If start and end events are both given, the result is the difference between - * the two. If only start event is given, the result is the timing value at - * start event. Enforces synchronous evaluation. - * @param {!Window} win - * @param {string} startEvent - * @param {string=} endEvent - * @return {ResolverReturnDef} undefined if API is not available, empty string - * if it is not yet available, or value as string - */ -export function getTimingDataSync(win, startEvent, endEvent) { - const timingInfo = win['performance'] && win['performance']['timing']; - if (!timingInfo || timingInfo['navigationStart'] == 0) { - // Navigation timing API is not supported. - return; - } - - const metric = (endEvent === undefined) - ? timingInfo[startEvent] - : timingInfo[endEvent] - timingInfo[startEvent]; - - if (!isFiniteNumber(metric)) { - // The metric is not supported. - return; - } else if (metric < 0) { - return ''; - } else { - return metric; - } -} - -/** - * Returns navigation information from the current browsing context. - * @param {!Window} win - * @param {string} attribute - * @return {ResolverReturnDef} - * @private - */ -export function getNavigationData(win, attribute) { - const navigationInfo = win['performance'] && - win['performance']['navigation']; - if (!navigationInfo || navigationInfo[attribute] === undefined) { - // PerformanceNavigation interface is not supported or attribute is not - // implemented. - return; - } - return navigationInfo[attribute]; -} - - -/** - * A class to provide variable substitution related features. Extend this class - * and override initialize() to add more supported variables. - */ -export class VariableSource { - constructor(ampdoc) { - /** @const {!./ampdoc-impl.AmpDoc} */ - this.ampdoc = ampdoc; - - /** @private {!RegExp|undefined} */ - this.replacementExpr_ = undefined; + }); - /** @private {!RegExp|undefined} */ - this.replacementExprV2_ = undefined; + it('Works with both sync and async variables', () => { + varSource.setBoth('Foo', () => 'bar', () => Promise.resolve('bar')); + expect(varSource.getExpr()).to.be.ok; - /** @private @const {!Object} */ - this.replacements_ = Object.create(null); - - /** @private {boolean} */ - this.initialized_ = false; - - /** @const @private {?Array} */ - this.ampVariableSubstitutionWhitelist_ = this.createWhitelist_(); - } - - /** - * Lazily initialize the default replacements. - * @private - */ - initialize_() { - this.initialize(); - this.initialized_ = true; - } - - /** - * Override this method to set all the variables supported by derived class. - */ - initialize() { - // Needs to be implemented by derived classes. - } - - /** - * Method exists to assist stubbing in tests. - * @param {string} name - * @return {!ReplacementDef} - */ - get(name) { - if (!this.initialized_) { - this.initialize_(); - } - - return this.replacements_[name]; - } - - /** - * Sets a synchronous value resolver for the variable with the specified name. - * The value resolver may optionally take an extra parameter. - * Can be called in conjunction with setAsync to allow for additional - * asynchronous resolver where expand will use async and expandSync the sync - * version. - * @param {string} varName - * @param {!SyncResolverDef} syncResolver - * @return {!VariableSource} - */ - set(varName, syncResolver) { - dev().assert(varName.indexOf('RETURN') == -1); - if (this.ampVariableSubstitutionWhitelist_ && - !this.ampVariableSubstitutionWhitelist_.includes(varName)) { - return this; - } - - this.replacements_[varName] = - this.replacements_[varName] || {sync: undefined, async: undefined}; - this.replacements_[varName].sync = syncResolver; - this.replacementExpr_ = undefined; - this.replacementExprV2_ = undefined; - return this; - } + expect(varSource.get('Foo')['sync']()).to.equal('bar'); + return varSource.get('Foo')['async']().then(value => { + expect(value).to.equal('bar'); + }); + }); - /** - * Sets an async value resolver for the variable with the specified name. - * The value resolver may optionally take an extra parameter. - * Can be called in conjuction with setAsync to allow for additional - * asynchronous resolver where expand will use async and expandSync the sync - * version. - * @param {string} varName - * @param {!AsyncResolverDef} asyncResolver - * @return {!VariableSource} - */ - setAsync(varName, asyncResolver) { - dev().assert(varName.indexOf('RETURN') == -1); - if (this.ampVariableSubstitutionWhitelist_ && - !this.ampVariableSubstitutionWhitelist_.includes(varName)) { - return this; - } - this.replacements_[varName] = - this.replacements_[varName] || {sync: undefined, async: undefined}; - this.replacements_[varName].async = asyncResolver; - this.replacementExpr_ = undefined; - this.replacementExprV2_ = undefined; - return this; - } + it('Works with multiple variables', () => { + varSource.setBoth('Foo', () => 'bar', () => Promise.resolve('bar')); + varSource.set('Baz', () => 'Foo'); + expect(varSource.getExpr()).to.be.ok; - /** - * Helper method to set both sync and async resolvers. - * @param {string} varName - * @param {!SyncResolverDef} syncResolver - * @param {!AsyncResolverDef} asyncResolver - * @return {!VariableSource} - */ - setBoth(varName, syncResolver, asyncResolver) { - return this.set(varName, syncResolver).setAsync(varName, asyncResolver); - } + expect(varSource.get('Foo')['sync']()).to.equal('bar'); + expect(varSource.get('Baz')['sync']()).to.equal('Foo'); + return varSource.get('Foo')['async']().then(value => { + expect(value).to.equal('bar'); + }); + }); + + it('Works with sync variable that is set multiple times', () => { + varSource.set('Foo', () => 'bar').set('Foo', () => 'baz'); + expect(varSource.getExpr()).to.be.ok; + expect(varSource.get('Foo')['sync']()).to.equal('baz'); + }); + + it('Works with async variable that is set multiple times', () => { + varSource.setAsync('Foo', () => Promise.resolve('bar')) + .setAsync('Foo', () => Promise.resolve('baz')); + return varSource.get('Foo')['async']().then(value => { + expect(value).to.equal('baz'); + }); + }); + + describes.fakeWin('getTimingData', {}, env => { + let win; + + beforeEach(() => { + win = env.win; + win.performance = { + timing: { + navigationStart: 1, + loadEventStart: 0, + }, + }; + }); - /** - * Returns a Regular expression that can be used to detect all the variables - * in a template. - * @param {!Object=} opt_bindings - * @param {boolean=} isV2 flag to ignore capture of args - */ - getExpr(opt_bindings, isV2) { - if (!this.initialized_) { - this.initialize_(); - } + it('should resolve immediate when data is ready', () => { + win.performance.timing.loadEventStart = 12; + return getTimingDataAsync(win, 'navigationStart', 'loadEventStart') + .then(value => { + expect(value).to.equal(11); + }); + }); - const additionalKeys = opt_bindings ? Object.keys(opt_bindings) : null; - if (additionalKeys && additionalKeys.length > 0) { - const allKeys = Object.keys(this.replacements_); - additionalKeys.forEach(key => { - if (this.replacements_[key] === undefined) { - allKeys.push(key); - } + it('should wait for load event', () => { + win.readyState = 'other'; + const p = getTimingDataAsync(win, 'navigationStart', 'loadEventStart'); + expect(win.eventListeners.count('load')).to.equal(1); + win.performance.timing.loadEventStart = 12; + win.eventListeners.fire({type: 'load'}); + return p.then(value => { + expect(value).to.equal(11); }); - return this.buildExpr_(allKeys, isV2); - } - if (!this.replacementExpr_ && !isV2) { - this.replacementExpr_ = this.buildExpr_( - Object.keys(this.replacements_)); - } - // sometimes the v1 expand will be called before the v2 - // so we need to cache both versions - if (!this.replacementExprV2_ && isV2) { - this.replacementExprV2_ = this.buildExpr_( - Object.keys(this.replacements_), isV2); - } - - return isV2 ? this.replacementExprV2_ : - this.replacementExpr_; - } - - /** - * @param {!Array} keys - * @param {boolean=} isV2 flag to ignore capture of args - * @return {!RegExp} - * @private - */ - buildExpr_(keys, isV2) { - // If a whitelist is provided, the keys must all belong to the whitelist. - if (this.ampVariableSubstitutionWhitelist_) { - keys = keys.filter(key => - this.ampVariableSubstitutionWhitelist_.includes(key)); - } - // The keys must be sorted to ensure that the longest keys are considered - // first. This avoids a problem where a RANDOM conflicts with RANDOM_ONE. - keys.sort((s1, s2) => s2.length - s1.length); - const all = keys.join('|'); - // Match the given replacement patterns, as well as optionally - // arguments to the replacement behind it in parentheses. - // Example string that match - // FOO_BAR - // FOO_BAR(arg1) - // FOO_BAR(arg1,arg2) - // FOO_BAR(arg1, arg2) - let regexStr = '\\$?(' + all + ')'; - // ignore the capturing of arguments in new parser - if (!isV2) { - regexStr += '(?:\\(((?:\\s*[0-9a-zA-Z-_.]*\\s*(?=,|\\)),?)*)\\s*\\))?'; - } - return new RegExp(regexStr, 'g'); - } - - /** - * @return {?Array} the whitelist of allowed AMP variables - * (if provided in a meta tag). - * @private - */ - createWhitelist_() { - if(!this.ampdoc){ - return null; - } - - const head = this.ampdoc.getRootNode().head; - if (head) { - // A meta[name="amp-variable-substitution-whitelist"] tag, if present, - // contains, in its content attribute, a whitelist of variable - // substitution. - const meta = - head.querySelector('meta[name="amp-variable-substitution-whitelist"]'); - - if (meta) { - return meta.getAttribute('content').split(',') - .map(variable => variable.trim()); - } - } - return null; - } -} + }); + }); +}); \ No newline at end of file From fc13f6b7ec0b02ec18e59d2a88fea56d48620e1d Mon Sep 17 00:00:00 2001 From: hamousavi Date: Wed, 7 Feb 2018 18:10:52 -0500 Subject: [PATCH 4/7] add tests for variable source --- test/functional/test-variable-source.js | 33 +++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/functional/test-variable-source.js b/test/functional/test-variable-source.js index 5587492cee1c..e13dd807d03b 100644 --- a/test/functional/test-variable-source.js +++ b/test/functional/test-variable-source.js @@ -84,6 +84,39 @@ describe('VariableSource', () => { }); }); + describes.fakeWin('#whitelist', { + amp: { + ampdoc: 'single', + }, + }, env => { + let variableSource; + beforeEach(() => { + const fakeMeta = { + getAttribute: () => 'ABC,ABCD,CANONICAL', + }; + sandbox.stub(env.win.document.head, + 'querySelector').callsFake(() => fakeMeta); + variableSource = new VariableSource(env.ampdoc); + }); + + it('Works with whitelisted variables', () => { + variableSource.setAsync('ABCD', () => Promise.resolve('abcd')); + expect(variableSource.getExpr()).to.be.ok; + + return variableSource.get('ABCD')['async']().then(value => { + expect(value).to.equal('abcd'); + }); + }); + + it('Should not work with unwhitelisted variables', () => { + variableSource.setAsync('RANDOM', () => Promise.resolve('0.1234')); + expect(variableSource.getExpr()).to.be.ok; + + expect(variableSource.get('RANDOM')).to.be.undefined; + }); + + }); + describes.fakeWin('getTimingData', {}, env => { let win; From a7b42acecde54ddcdbbdb300683315fa5366c817 Mon Sep 17 00:00:00 2001 From: hamousavi Date: Wed, 14 Feb 2018 18:16:50 -0500 Subject: [PATCH 5/7] more code review changes --- src/service/url-replacements-impl.js | 4 +- src/service/variable-source.js | 78 ++++++++++++++++--------- test/functional/test-variable-source.js | 14 +++-- 3 files changed, 60 insertions(+), 36 deletions(-) diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index 69885c9671eb..e779a766bd46 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -68,7 +68,9 @@ function encodeValue(val) { * Class to provide variables that pertain to top level AMP window. */ export class GlobalVariableSource extends VariableSource { - + /** + * @param {?./ampdoc-impl.AmpDoc} ampdoc + */ constructor(ampdoc) { super(ampdoc); diff --git a/src/service/variable-source.js b/src/service/variable-source.js index eda3f0e01724..1588574ce57a 100644 --- a/src/service/variable-source.js +++ b/src/service/variable-source.js @@ -109,8 +109,11 @@ export function getNavigationData(win, attribute) { * and override initialize() to add more supported variables. */ export class VariableSource { + /** + * @param {!./ampdoc-impl.AmpDoc} ampdoc + */ constructor(ampdoc) { - /** @const {!./ampdoc-impl.AmpDoc} */ + /** @protected @const {?./ampdoc-impl.AmpDoc} */ this.ampdoc = ampdoc; /** @private {!RegExp|undefined} */ @@ -125,8 +128,42 @@ export class VariableSource { /** @private {boolean} */ this.initialized_ = false; + // The whitelist of variables allowed for variable substitution. /** @const @private {?Array} */ - this.ampVariableSubstitutionWhitelist_ = this.createWhitelist_(); + this.whitelist_ = this.getVariableWhitelist_(); + } + + /** + * @return {?Array} The whitelist of allowed AMP variables. (if provided in + a meta tag). + * @private + */ + getVariableWhitelist_() { + if (this.whitelist_) { + return this.whitelist_; + } + + if (!this.ampdoc) { + return null; + } + + const head = this.ampdoc.getRootNode().head; + if (!head) { + return null; + } + + // A meta[name="amp-variable-substitution-whitelist"] tag, if present, + // contains, in its content attribute, a whitelist of variable + // substitution. + const meta = + head.querySelector('meta[name="amp-variable-substitution-whitelist"]'); + if (!meta) { + return null; + } + + this.whitelist_ = meta.getAttribute('content').split(',') + .map(variable => variable.trim()); + return this.whitelist_; } /** @@ -170,8 +207,7 @@ export class VariableSource { */ set(varName, syncResolver) { dev().assert(varName.indexOf('RETURN') == -1); - if (this.ampVariableSubstitutionWhitelist_ && - !this.ampVariableSubstitutionWhitelist_.includes(varName)) { + if (this.isWhitelisted_(varName)) { return this; } @@ -195,8 +231,7 @@ export class VariableSource { */ setAsync(varName, asyncResolver) { dev().assert(varName.indexOf('RETURN') == -1); - if (this.ampVariableSubstitutionWhitelist_ && - !this.ampVariableSubstitutionWhitelist_.includes(varName)) { + if (this.isWhitelisted_(varName)) { return this; } this.replacements_[varName] = @@ -262,9 +297,9 @@ export class VariableSource { */ buildExpr_(keys, isV2) { // If a whitelist is provided, the keys must all belong to the whitelist. - if (this.ampVariableSubstitutionWhitelist_) { + if (this.getVariableWhitelist_()) { keys = keys.filter(key => - this.ampVariableSubstitutionWhitelist_.includes(key)); + this.getVariableWhitelist_().includes(key)); } // The keys must be sorted to ensure that the longest keys are considered // first. This avoids a problem where a RANDOM conflicts with RANDOM_ONE. @@ -286,28 +321,13 @@ export class VariableSource { } /** - * @return {?Array} the whitelist of allowed AMP variables - * (if provided in a meta tag). + * @param {string} varName + * @return {boolean} If a whitelist is provided and + it contains the variable name returns true. * @private */ - createWhitelist_() { - if(!this.ampdoc){ - return null; - } - - const head = this.ampdoc.getRootNode().head; - if (head) { - // A meta[name="amp-variable-substitution-whitelist"] tag, if present, - // contains, in its content attribute, a whitelist of variable - // substitution. - const meta = - head.querySelector('meta[name="amp-variable-substitution-whitelist"]'); - - if (meta) { - return meta.getAttribute('content').split(',') - .map(variable => variable.trim()); - } - } - return null; + isWhitelisted_(varName) { + return this.getVariableWhitelist_() && + !this.getVariableWhitelist_().includes(varName); } } diff --git a/test/functional/test-variable-source.js b/test/functional/test-variable-source.js index e13dd807d03b..bcd00fbc94cb 100644 --- a/test/functional/test-variable-source.js +++ b/test/functional/test-variable-source.js @@ -19,6 +19,8 @@ import { getTimingDataAsync, } from '../../src/service/variable-source'; +import {createElementWithAttributes} from '../../src/dom'; + describe('VariableSource', () => { let varSource; @@ -84,18 +86,18 @@ describe('VariableSource', () => { }); }); - describes.fakeWin('#whitelist', { + describes.realWin('Whitelist of variable substitutions', { amp: { ampdoc: 'single', }, }, env => { let variableSource; beforeEach(() => { - const fakeMeta = { - getAttribute: () => 'ABC,ABCD,CANONICAL', - }; - sandbox.stub(env.win.document.head, - 'querySelector').callsFake(() => fakeMeta); + env.win.document.head.appendChild( + createElementWithAttributes(env.win.document, 'meta', { + name: 'amp-variable-substitution-whitelist', + content: 'ABC,ABCD,CANONICAL', + })); variableSource = new VariableSource(env.ampdoc); }); From 222c8486356fb39e766613bbdd5003ed24757054 Mon Sep 17 00:00:00 2001 From: hamousavi Date: Thu, 15 Feb 2018 16:43:57 -0500 Subject: [PATCH 6/7] even more code review --- src/service/url-replacements-impl.js | 2 +- src/service/variable-source.js | 33 ++++++++++--------- test/functional/test-variable-source.js | 8 +++-- test/functional/url-expander/test-expander.js | 12 +++---- 4 files changed, 31 insertions(+), 24 deletions(-) diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index e779a766bd46..d5c597915f04 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -69,7 +69,7 @@ function encodeValue(val) { */ export class GlobalVariableSource extends VariableSource { /** - * @param {?./ampdoc-impl.AmpDoc} ampdoc + * @param {!./ampdoc-impl.AmpDoc} ampdoc */ constructor(ampdoc) { super(ampdoc); diff --git a/src/service/variable-source.js b/src/service/variable-source.js index 1588574ce57a..2888ba698e1b 100644 --- a/src/service/variable-source.js +++ b/src/service/variable-source.js @@ -113,7 +113,7 @@ export class VariableSource { * @param {!./ampdoc-impl.AmpDoc} ampdoc */ constructor(ampdoc) { - /** @protected @const {?./ampdoc-impl.AmpDoc} */ + /** @protected @const {!./ampdoc-impl.AmpDoc} */ this.ampdoc = ampdoc; /** @private {!RegExp|undefined} */ @@ -128,14 +128,16 @@ export class VariableSource { /** @private {boolean} */ this.initialized_ = false; - // The whitelist of variables allowed for variable substitution. - /** @const @private {?Array} */ + /** + * The whitelist of variables allowed for variable substitution. + * @private @const {?Array} + */ this.whitelist_ = this.getVariableWhitelist_(); } /** * @return {?Array} The whitelist of allowed AMP variables. (if provided in - a meta tag). + * a meta tag). * @private */ getVariableWhitelist_() { @@ -153,8 +155,7 @@ export class VariableSource { } // A meta[name="amp-variable-substitution-whitelist"] tag, if present, - // contains, in its content attribute, a whitelist of variable - // substitution. + // contains, in its content attribute, a whitelist of variable substitution. const meta = head.querySelector('meta[name="amp-variable-substitution-whitelist"]'); if (!meta) { @@ -207,7 +208,7 @@ export class VariableSource { */ set(varName, syncResolver) { dev().assert(varName.indexOf('RETURN') == -1); - if (this.isWhitelisted_(varName)) { + if (!this.isWhitelisted_(varName)) { return this; } @@ -231,7 +232,7 @@ export class VariableSource { */ setAsync(varName, asyncResolver) { dev().assert(varName.indexOf('RETURN') == -1); - if (this.isWhitelisted_(varName)) { + if (!this.isWhitelisted_(varName)) { return this; } this.replacements_[varName] = @@ -296,10 +297,11 @@ export class VariableSource { * @private */ buildExpr_(keys, isV2) { - // If a whitelist is provided, the keys must all belong to the whitelist. + // If a whitelist is present, the keys must belong to the whitelist. + // We filter the keys one last time to ensure no unwhitelisted key is + // allowed. if (this.getVariableWhitelist_()) { - keys = keys.filter(key => - this.getVariableWhitelist_().includes(key)); + keys = keys.filter(key => this.getVariableWhitelist_().includes(key)); } // The keys must be sorted to ensure that the longest keys are considered // first. This avoids a problem where a RANDOM conflicts with RANDOM_ONE. @@ -321,13 +323,14 @@ export class VariableSource { } /** + * Returns `true` if a variable whitelist is *not* present or the present + * whitelist contains the given variable name. * @param {string} varName - * @return {boolean} If a whitelist is provided and - it contains the variable name returns true. + * @return {boolean} * @private */ isWhitelisted_(varName) { - return this.getVariableWhitelist_() && - !this.getVariableWhitelist_().includes(varName); + return !this.getVariableWhitelist_() || + this.getVariableWhitelist_().includes(varName); } } diff --git a/test/functional/test-variable-source.js b/test/functional/test-variable-source.js index bcd00fbc94cb..97ee9c56aae4 100644 --- a/test/functional/test-variable-source.js +++ b/test/functional/test-variable-source.js @@ -22,10 +22,14 @@ import { import {createElementWithAttributes} from '../../src/dom'; -describe('VariableSource', () => { +describes.fakeWin('VariableSource', { + amp: { + ampdoc: 'single', + }, +}, env => { let varSource; beforeEach(() => { - varSource = new VariableSource(); + varSource = new VariableSource(env.ampdoc); }); it('Works without any variables', () => { diff --git a/test/functional/url-expander/test-expander.js b/test/functional/url-expander/test-expander.js index 048689380a7e..a265a5b84247 100644 --- a/test/functional/url-expander/test-expander.js +++ b/test/functional/url-expander/test-expander.js @@ -88,7 +88,7 @@ describes.realWin('Expander', { }); }); - describe('#whitelist', () => { + describe('Whitelist of variables', () => { let variableSource; let expander; @@ -98,11 +98,11 @@ describes.realWin('Expander', { ABCD: () => 'four', }; beforeEach(() => { - const fakeMeta = { - getAttribute: () => 'ABC,ABCD,CANONICAL', - }; - sandbox.stub(env.win.document.head, - 'querySelector').callsFake(() => fakeMeta); + env.win.document.head.appendChild( + createElementWithAttributes(env.win.document, 'meta', { + name: 'amp-variable-substitution-whitelist', + content: 'ABC,ABCD,CANONICAL', + })); variableSource = new GlobalVariableSource(env.ampdoc); expander = new Expander(variableSource); From 0bb598dc16c7df4d0b460dfc47c4036c9374c36d Mon Sep 17 00:00:00 2001 From: hamousavi Date: Fri, 16 Feb 2018 16:35:43 -0500 Subject: [PATCH 7/7] change amp-doc to nullable --- src/service/variable-source.js | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/service/variable-source.js b/src/service/variable-source.js index 2888ba698e1b..4efc31f90d25 100644 --- a/src/service/variable-source.js +++ b/src/service/variable-source.js @@ -110,10 +110,10 @@ export function getNavigationData(win, attribute) { */ export class VariableSource { /** - * @param {!./ampdoc-impl.AmpDoc} ampdoc + * @param {?./ampdoc-impl.AmpDoc} ampdoc */ constructor(ampdoc) { - /** @protected @const {!./ampdoc-impl.AmpDoc} */ + /** @protected @const {?./ampdoc-impl.AmpDoc} */ this.ampdoc = ampdoc; /** @private {!RegExp|undefined} */ @@ -132,7 +132,7 @@ export class VariableSource { * The whitelist of variables allowed for variable substitution. * @private @const {?Array} */ - this.whitelist_ = this.getVariableWhitelist_(); + this.variableWhitelist_ = this.getVariableWhitelist_(); } /** @@ -141,10 +141,13 @@ export class VariableSource { * @private */ getVariableWhitelist_() { - if (this.whitelist_) { - return this.whitelist_; + if (this.variableWhitelist_) { + return this.variableWhitelist_; } + // TODO(hamousavi): Remove this conditional and change the type annotation + // for this.ampdoc to non-nullable. This is a temporary measures because + // tests currently do not respect the non-nullability measure. if (!this.ampdoc) { return null; } @@ -162,9 +165,9 @@ export class VariableSource { return null; } - this.whitelist_ = meta.getAttribute('content').split(',') + this.variableWhitelist_ = meta.getAttribute('content').split(',') .map(variable => variable.trim()); - return this.whitelist_; + return this.variableWhitelist_; } /**