Skip to content

Commit

Permalink
Review feedback.
Browse files Browse the repository at this point in the history
  • Loading branch information
@avimehta
avimehta committed Dec 2, 2016
1 parent 304536b commit e51b599
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 39 deletions.
4 changes: 2 additions & 2 deletions extensions/amp-analytics/0.1/amp-analytics.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,8 @@ export class AmpAnalytics extends AMP.BaseElement {
}

return Promise.all(requestPromises)
.then(() => this.addParamsToUrl_(request, params))
.then(request => {
.then(() => {
request = this.addParamsToUrl_(request, params);
this.config_['vars']['requestCount']++;
const expansionOptions = this.expansionOptions_(event, trigger);
return this.variableService_.expandTemplate(request, expansionOptions);
Expand Down
19 changes: 18 additions & 1 deletion extensions/amp-analytics/0.1/test/test-variables.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,25 @@
import {ExpansionOptions, variableServiceFor} from '../variables';
import {adopt} from '../../../../src/runtime';
import {cryptoFor} from '../../../../src/crypto';
import * as sinon from 'sinon';

adopt(window);

describe('amp-analytics.VariableService', function() {
let variables;
let variables, sandbox;

beforeEach(() => {
sandbox = sinon.sandbox.create();
return cryptoFor(window).then(() => {
variables = variableServiceFor(window);
});
});

afterEach(() => {
sandbox.restore();
});


it('correctly encodes scalars and arrays', () => {
expect(variables.encodeVars('abc %&')).to.equal('abc%20%25%26');
const array = ['abc %&', 'a b'];
Expand Down Expand Up @@ -79,9 +87,18 @@ describe('amp-analytics.VariableService', function() {
});
});

it('default filterdoesn\'t work when experiment is off' , () =>
variables.expandTemplate('${bar|default:baz}',
new ExpansionOptions({'foo': ' Hello world! '}))
.then(actual => expect(actual).to.equal('')));

describe('filter:', () => {
const vars = new ExpansionOptions({'foo': ' Hello world! '});

beforeEach(() => {
sandbox.stub(variables, 'isFilterExperimentOn_', () => true);
});

function check(input, output) {
return variables.expandTemplate(input, vars).then(actual =>
expect(actual).to.equal(output));
Expand Down
86 changes: 50 additions & 36 deletions extensions/amp-analytics/0.1/variables.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/

import {isExperimentOn} from '../../../src/experiments';
import {cryptoFor} from '../../../src/crypto';
import {dev, user} from '../../../src/log';
import {fromClass} from '../../../src/service';
Expand All @@ -25,17 +26,20 @@ const TAG = 'Analytics.Variables';
/** @const {RegExp} */
const VARIABLE_ARGS_REGEXP = /^(?:([^ ]*)(\([^)]*\))|.+)$/;

/** @typedef {{name: string, argList: string}} */
let FunctionNameArgsDef;

/**
* @struct
* @const
*/
class FilterDef {
class Filter {
/**
* @param {function(...?):(?|!Promise<?>)} filter
* @param {function(...?):(string|!Promise<string>)} filter
* @param {boolean=} opt_allowNull
*/
constructor(filter, opt_allowNull) {
/** @type {!function(...?):(?|!Promise<?>)} */
/** @type {!function(...?):(string|!Promise<string>)} */
this.filter = filter;

/** @type{boolean} */
Expand All @@ -59,7 +63,7 @@ export class ExpansionOptions {
constructor(vars, opt_iterations, opt_noEncode) {
/** @const {!Object<string, string|Array<string>>} */
this.vars = vars;
/** @const {!number} */
/** @const {number} */
this.iterations = opt_iterations === undefined ? 2 : opt_iterations;
/** @const {boolean} */
this.noEncode = !!opt_noEncode;
Expand All @@ -69,18 +73,18 @@ export class ExpansionOptions {


/**
* @param {!string} str
* @param {Number} s
* @param {Number} l
* @param {string} str
* @param {string} s
* @param {string=} opt_l
* @return {string}
*/
function substrFilter(str, s, l) {
function substrFilter(str, s, opt_l) {
const start = Number(s);
let length = str.length;
user().assert(isFiniteNumber(start),
'Start index ' + start + 'in substr filter should be a number');
if (l) {
length = Number(l);
if (opt_l) {
length = Number(opt_l);
user().assert(isFiniteNumber(length),
'Length ' + length + ' in substr filter should be a number');
}
Expand All @@ -89,15 +93,19 @@ function substrFilter(str, s, l) {
}

/**
* @param {*} value
* @param {*} defaultValue
* @return {*}
* @param {string} value
* @param {string} defaultValue
* @return {string}
*/
function defaultFilter(value, defaultValue) {
return value || user().assertString(defaultValue);
}


/**
* Provides support for processing of advanced variable syntax like nested
* expansions filters etc.
*/
export class VariableService {
/**
* @param {!Window} window
Expand All @@ -107,29 +115,29 @@ export class VariableService {
/** @private {!Window} */
this.win_ = window;

/** @private {!Object<string, FilterDef>} */
/** @private {!Object<string, Filter>} */
this.filters_ = map();

this.register_('default', new FilterDef(defaultFilter, true));
this.register_('substr', new FilterDef(substrFilter));
this.register_('trim', new FilterDef(
this.register_('default', new Filter(defaultFilter, /* allowNulls */ true));
this.register_('substr', new Filter(substrFilter));
this.register_('trim', new Filter(
value => user().assertString(value).trim()));
this.register_('json', new FilterDef(value => JSON.stringify(value)));
this.register_('toLowerCase', new FilterDef(value =>
user().assertString(value).toLowerCase()));
this.register_('toUpperCase', new FilterDef(value =>
user().assertString(value).toUpperCase()));
this.register_('not', new FilterDef(value => String(!value)));
this.register_('base64', new FilterDef(
this.register_('json', new Filter(value => JSON.stringify(value)));
this.register_('toLowerCase', new Filter(
value => user().assertString(value).toLowerCase()));
this.register_('toUpperCase', new Filter(
value => user().assertString(value).toUpperCase()));
this.register_('not', new Filter(value => String(!value)));
this.register_('base64', new Filter(
value => btoa(user().assertString(value))));
this.register_('hash', new FilterDef(this.hashFilter_.bind(this)));
this.register_('if', new FilterDef(
this.register_('hash', new Filter(this.hashFilter_.bind(this)));
this.register_('if', new Filter(
(value, thenValue, elseValue) => value ? thenValue : elseValue, true));
}

/**
* @param {string} name
* @param {FilterDef} filter
* @param {Filter} filter
*/
register_(name, filter) {
dev().assert(!this.filters_[name], 'Filter "' + name
Expand All @@ -139,7 +147,7 @@ export class VariableService {

/**
* @param {string} filterStr
* @return {Object<string, FilterDef|Array<string>>}
* @return {Object<string, Filter|Array<string>>}
*/
parseFilter_(filterStr) {
if (!filterStr) {
Expand All @@ -163,6 +171,10 @@ export class VariableService {
* @return {Promise<string>}
*/
applyFilters_(value, filters) {
if (!this.isFilterExperimentOn_()) {
return Promise.resolve(value);
}

let result = Promise.resolve(value);
for (let i = 0; i < filters.length; i++) {
const {filter, args} = this.parseFilter_(filters[i].trim());
Expand All @@ -180,7 +192,7 @@ export class VariableService {
}

/**
* @param {!string} template The template to expand
* @param {string} template The template to expand
* @param {!ExpansionOptions} options configuration to use for expansion
* @return {!Promise<!string>} The expanded string
*/
Expand Down Expand Up @@ -218,8 +230,8 @@ export class VariableService {
}

p = p.then(expandedValue =>
// First apply filters
this.applyFilters_(expandedValue, tokens))
// First apply filters
this.applyFilters_(expandedValue, tokens))
.then(finalRawValue => {
// Then encode the value
const val = options.noEncode
Expand All @@ -237,7 +249,6 @@ export class VariableService {

// Since the replacement will happen later, return the original template.
return match;

});

// Once all the promises are complete, return the expanded value.
Expand All @@ -248,7 +259,7 @@ export class VariableService {
* Returns an array containing two values: name and args parsed from the key.
*
* @param {string} key The key to be parsed.
* @return {!Object<string>}
* @return {!FunctionNameArgsDef}
* @private
*/
getNameArgs_(key) {
Expand Down Expand Up @@ -280,11 +291,14 @@ export class VariableService {

/**
* @param {string} value
* @return {!Promise<!string>}
* @return {!Promise<string>}
*/
hashFilter_(value) {
return cryptoFor(this.win_).then(crypto =>
crypto.sha384Base64(user().assertString(value)));
return cryptoFor(this.win_).then(crypto => crypto.sha384Base64(value));
}

isFilterExperimentOn_() {
return isExperimentOn(this.win_, 'variable-filters');
}
}

Expand Down
5 changes: 5 additions & 0 deletions tools/experiments/experiments.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,11 @@ const EXPERIMENTS = [
name: 'AMP Accordion attribute to opt out of preserved state.',
Spec: 'https://github.com/ampproject/amphtml/issues/3813',
},
{
id: 'variable-filters',
name: 'Format to apply filters to analytics variables',
cleanupIssue: 'https://github.com/ampproject/amphtml/issues/2198',
},
];

if (getMode().localDev) {
Expand Down

0 comments on commit e51b599

Please sign in to comment.