From 22290b768b642ba0672dc4be953982c5e2f576f5 Mon Sep 17 00:00:00 2001 From: William Chou Date: Thu, 22 Dec 2016 17:17:36 -0500 Subject: [PATCH] Add variable references in action method arg values (#6723) * implement var dereferencing in action arg values * fix type error * add unit tests for applyActionInfoArgs() * fix lint error * PR comments * fix lint errors --- src/service/action-impl.js | 161 +++++++++++++++++++++++++++------ test/functional/test-action.js | 120 ++++++++++++++++++++---- 2 files changed, 231 insertions(+), 50 deletions(-) diff --git a/src/service/action-impl.js b/src/service/action-impl.js index f42ffe6cb673..1a38afd5dbb7 100644 --- a/src/service/action-impl.js +++ b/src/service/action-impl.js @@ -40,18 +40,25 @@ const ELEMENTS_ACTIONS_MAP_ = { 'AMP': ['setState'], }; +/** + * A map of method argument keys to functions that generate the argument values + * given a local scope object. The function allows argument values to reference + * data in the event that generated the action. + * @typedef {Object} + */ +let ActionInfoArgsDef; + /** * @typedef {{ * event: string, * target: string, * method: string, - * args: ?JSONType, + * args: ?ActionInfoArgsDef, * str: string * }} */ let ActionInfoDef; - /** * The structure that contains all details of the action method invocation. * @struct @@ -240,26 +247,29 @@ export class ActionService { const actionInfo = action.actionInfo; + // Replace any variables in args with data in `event`. + const args = applyActionInfoArgs(actionInfo.args, event); + // Global target, e.g. `AMP`. const globalTarget = this.globalTargets_[actionInfo.target]; if (globalTarget) { const invocation = new ActionInvocation( this.root_, actionInfo.method, - actionInfo.args, + args, action.node, event); globalTarget(invocation); return; } - const target = this.root_.getElementById(action.actionInfo.target); + const target = this.root_.getElementById(actionInfo.target); if (!target) { - this.actionInfoError_('target not found', action.actionInfo, target); + this.actionInfoError_('target not found', actionInfo, target); return; } - this.invoke_(target, action.actionInfo.method, action.actionInfo.args, - action.node, event, action.actionInfo); + this.invoke_(target, actionInfo.method, args, + action.node, event, actionInfo); } /** @@ -395,7 +405,7 @@ export function parseActionMap(s, context) { if (tok.type == TokenType.EOF || tok.type == TokenType.SEPARATOR && tok.value == ';') { // Expected, ignore. - } else if (tok.type == TokenType.LITERAL) { + } else if (tok.type == TokenType.LITERAL || tok.type == TokenType.ID) { // Format: event:target.method @@ -403,8 +413,9 @@ export function parseActionMap(s, context) { const event = tok.value; // Target: ":target." - assertToken(toks.next(), TokenType.SEPARATOR, ':'); - const target = assertToken(toks.next(), TokenType.LITERAL).value; + assertToken(toks.next(), [TokenType.SEPARATOR], ':'); + const target = assertToken( + toks.next(), [TokenType.LITERAL, TokenType.ID]).value; // Method: ".method". Method is optional. let method = DEFAULT_METHOD_; @@ -412,7 +423,8 @@ export function parseActionMap(s, context) { peek = toks.peek(); if (peek.type == TokenType.SEPARATOR && peek.value == '.') { toks.next(); // Skip '.' - method = assertToken(toks.next(), TokenType.LITERAL).value || method; + method = assertToken( + toks.next(), [TokenType.LITERAL, TokenType.ID]).value || method; // Optionally, there may be arguments: "(key = value, key = value)". peek = toks.peek(); @@ -425,13 +437,26 @@ export function parseActionMap(s, context) { if (tok.type == TokenType.SEPARATOR && (tok.value == ',' || tok.value == ')')) { // Expected: ignore. - } else if (tok.type == TokenType.LITERAL) { + } else if (tok.type == TokenType.LITERAL || + tok.type == TokenType.ID) { // Key: "key = " const argKey = tok.value; - assertToken(toks.next(), TokenType.SEPARATOR, '='); - const argValue = - assertToken(toks.next(/* convertValue */ true), - TokenType.LITERAL).value; + assertToken(toks.next(), [TokenType.SEPARATOR], '='); + // Value is either a literal or a variable: "foo.bar.baz" + tok = assertToken(toks.next(/* convertValue */ true), + [TokenType.LITERAL, TokenType.ID]); + const argValueTokens = [tok]; + // Variables have one or more dereferences: ".identifier" + if (tok.type == TokenType.ID) { + for (peek = toks.peek(); + peek.type == TokenType.SEPARATOR && peek.value == '.'; + peek = toks.peek()) { + tok = toks.next(); // Skip '.'. + tok = assertToken(toks.next(false), [TokenType.ID]); + argValueTokens.push(tok); + } + } + const argValue = getActionInfoArgValue(argValueTokens); if (!args) { args = map(); } @@ -471,6 +496,67 @@ export function parseActionMap(s, context) { return actionMap; } +/** + * Returns a function that generates a method argument value for a given token. + * The function takes a single object argument `data`. + * If the token is an identifier `foo`, the function returns `data[foo]`. + * Otherwise, the function returns the token value. + * @param {Array} tokens + * @return {?function(!Object):string} + * @private + */ +function getActionInfoArgValue(tokens) { + if (tokens.length == 0) { + return null; + } + if (tokens.length == 1) { + return () => tokens[0].value; + } else { + return data => { + let current = data; + // Traverse properties of `data` per token values. + for (let i = 0; i < tokens.length; i++) { + const value = tokens[i].value; + if (current && current.hasOwnProperty(value)) { + current = current[value]; + } else { + return null; + } + } + // Only allow dereferencing of primitives. + const type = typeof current; + if (type === 'string' || type === 'number' || type === 'boolean') { + return current; + } else { + return null; + } + }; + } +} + +/** + * Generates method arg values for each key in the given ActionInfoArgsDef + * with the data in the given event. + * @param {?ActionInfoArgsDef} args + * @param {?Event} event + * @return {?JSONType} + * @private Visible for testing only. + */ +export function applyActionInfoArgs(args, event) { + if (!args) { + return args; + } + const data = {}; + if (event && event.detail) { + data['event'] = event.detail; + } + const applied = map(); + Object.keys(args).forEach(key => { + applied[key] = args[key].call(null, data); + }); + return applied; +} + /** * @param {string} s * @param {!Element} context @@ -488,24 +574,23 @@ function assertActionForParser(s, context, condition, opt_message) { /** * @param {string} s * @param {!Element} context - * @param {!{type: TokenType, value: *}} tok - * @param {TokenType} type + * @param {!TokenDef} tok + * @param {Array} types * @param {*=} opt_value - * @return {!{type: TokenType, value: *}} + * @return {!TokenDef} * @private */ -function assertTokenForParser(s, context, tok, type, opt_value) { +function assertTokenForParser(s, context, tok, types, opt_value) { if (opt_value !== undefined) { assertActionForParser(s, context, - tok.type == type && tok.value == opt_value, + types.indexOf(tok.type) >= 0 && tok.value == opt_value, `; expected [${opt_value}]`); } else { - assertActionForParser(s, context, tok.type == type); + assertActionForParser(s, context, types.indexOf(tok.type) >= 0); } return tok; } - /** * @enum {number} */ @@ -514,8 +599,14 @@ const TokenType = { EOF: 1, SEPARATOR: 2, LITERAL: 3, + ID: 4, }; +/** + * @typedef {{type: TokenType, value: *}} + */ +let TokenDef; + /** @private @const {string} */ const WHITESPACE_SET = ' \t\n\r\f\v\u00A0\u2028\u2029'; @@ -528,7 +619,6 @@ const STRING_SET = '"\''; /** @private @const {string} */ const SPECIAL_SET = WHITESPACE_SET + SEPARATOR_SET + STRING_SET; - /** @private */ class ParserTokenizer { /** @@ -545,7 +635,7 @@ class ParserTokenizer { /** * Returns the next token and advances the position. * @param {boolean=} opt_convertValues - * @return {!{type: TokenType, value: *}} + * @return {!TokenDef} */ next(opt_convertValues) { const tok = this.next_(opt_convertValues || false); @@ -556,7 +646,7 @@ class ParserTokenizer { /** * Returns the next token but keeps the current position. * @param {boolean=} opt_convertValues - * @return {!{type: TokenType, value: *}} + * @return {!TokenDef} */ peek(opt_convertValues) { return this.next_(opt_convertValues || false); @@ -632,7 +722,7 @@ class ParserTokenizer { return {type: TokenType.LITERAL, value, index: newIndex}; } - // A key + // Advance until next special character. let end = newIndex + 1; for (; end < this.str_.length; end++) { if (SPECIAL_SET.indexOf(this.str_.charAt(end)) != -1) { @@ -640,10 +730,21 @@ class ParserTokenizer { } } const s = this.str_.substring(newIndex, end); - const value = convertValues && (s == 'true' || s == 'false') ? - s == 'true' : s; newIndex = end - 1; - return {type: TokenType.LITERAL, value, index: newIndex}; + + // Boolean literal. + if (convertValues && (s == 'true' || s == 'false')) { + const value = (s == 'true'); + return {type: TokenType.LITERAL, value, index: newIndex}; + } + + // Identifier. + if (!isNum(s.charAt(0))) { + return {type: TokenType.ID, value: s, index: newIndex}; + } + + // Key. + return {type: TokenType.LITERAL, value: s, index: newIndex}; } } diff --git a/test/functional/test-action.js b/test/functional/test-action.js index 3a1ca42bc9f1..e1d64b3e3fbc 100644 --- a/test/functional/test-action.js +++ b/test/functional/test-action.js @@ -14,7 +14,11 @@ * limitations under the License. */ -import {ActionService, parseActionMap} from '../../src/service/action-impl'; +import { + ActionService, + applyActionInfoArgs, + parseActionMap, +} from '../../src/service/action-impl'; import {AmpDocSingle} from '../../src/service/ampdoc-impl'; import {setParentWindow} from '../../src/service'; import * as sinon from 'sinon'; @@ -64,7 +68,7 @@ describe('ActionService parseAction', () => { expect(a.event).to.equal('event1'); expect(a.target).to.equal('target1'); expect(a.method).to.equal('method1'); - expect(a.args['key1']).to.equal('value1'); + expect(a.args['key1']()).to.equal('value1'); }); it('should parse with multiple args', () => { @@ -72,8 +76,8 @@ describe('ActionService parseAction', () => { expect(a.event).to.equal('event1'); expect(a.target).to.equal('target1'); expect(a.method).to.equal('method1'); - expect(a.args['key1']).to.equal('value1'); - expect(a.args['key2']).to.equal('value2'); + expect(a.args['key1']()).to.equal('value1'); + expect(a.args['key2']()).to.equal('value2'); }); it('should parse with multiple args with whitespace', () => { @@ -82,8 +86,8 @@ describe('ActionService parseAction', () => { expect(a.event).to.equal('event1'); expect(a.target).to.equal('target1'); expect(a.method).to.equal('method1'); - expect(a.args['key1']).to.equal('value1'); - expect(a.args['key2']).to.equal('value2'); + expect(a.args['key1']()).to.equal('value1'); + expect(a.args['key2']()).to.equal('value2'); }); it('should parse with no args', () => { @@ -108,8 +112,8 @@ describe('ActionService parseAction', () => { expect(a.event).to.equal('event1'); expect(a.target).to.equal('target1'); expect(a.method).to.equal('method1'); - expect(a.args['key1']).to.equal('value1'); - expect(a.args['key2']).to.equal('value (2)\''); + expect(a.args['key1']()).to.equal('value1'); + expect(a.args['key2']()).to.equal('value (2)\''); }); it('should parse with single-quoted args', () => { @@ -118,8 +122,8 @@ describe('ActionService parseAction', () => { expect(a.event).to.equal('event1'); expect(a.target).to.equal('target1'); expect(a.method).to.equal('method1'); - expect(a.args['key1']).to.equal('value1'); - expect(a.args['key2']).to.equal('value (2)"'); + expect(a.args['key1']()).to.equal('value1'); + expect(a.args['key2']()).to.equal('value (2)"'); }); it('should parse with args with trailing comma', () => { @@ -128,23 +132,23 @@ describe('ActionService parseAction', () => { expect(a.event).to.equal('event1'); expect(a.target).to.equal('target1'); expect(a.method).to.equal('method1'); - expect(a.args['key1']).to.equal('value1'); + expect(a.args['key1']()).to.equal('value1'); expect(Object.keys(a.args)).to.have.length(1); }); it('should parse with boolean args', () => { - expect(parseAction('e:t.m(k=true)').args['k']).to.equal(true); - expect(parseAction('e:t.m(k=false)').args['k']).to.equal(false); + expect(parseAction('e:t.m(k=true)').args['k']()).to.equal(true); + expect(parseAction('e:t.m(k=false)').args['k']()).to.equal(false); }); it('should parse with numeric args', () => { - expect(parseAction('e:t.m(k=123)').args['k']).to.equal(123); - expect(parseAction('e:t.m(k=1.23)').args['k']).to.equal(1.23); - expect(parseAction('e:t.m(k=.123)').args['k']).to.equal(.123); + expect(parseAction('e:t.m(k=123)').args['k']()).to.equal(123); + expect(parseAction('e:t.m(k=1.23)').args['k']()).to.equal(1.23); + expect(parseAction('e:t.m(k=.123)').args['k']()).to.equal(.123); }); it('should parse with term semicolon', () => { - expect(parseAction('e:t.m(k=1); ').args['k']).to.equal(1); + expect(parseAction('e:t.m(k=1); ').args['k']()).to.equal(1); }); it('should parse with args as a proto-less object', () => { @@ -155,8 +159,85 @@ describe('ActionService parseAction', () => { expect(parseAction('true:t.m').event).to.equal('true'); expect(parseAction('e:true.m').target).to.equal('true'); expect(parseAction('e:t.true').method).to.equal('true'); - expect(parseAction('e:t.m(true=1)').args['true']).to.equal(1); - expect(parseAction('e:t.m(01=1)').args['01']).to.equal(1); + expect(parseAction('e:t.m(true=1)').args['true']()).to.equal(1); + expect(parseAction('e:t.m(01=1)').args['01']()).to.equal(1); + }); + + it('should dereference vars in arg value identifiers', () => { + const data = {foo: {bar: 'baz'}}; + const a = parseAction('e:t.m(key1=foo.bar)'); + expect(a.args['key1']()).to.equal(null); + expect(a.args['key1'](data)).to.equal('baz'); + }); + + it('should NOT dereference vars in identifiers without "." operator', () => { + const a = parseAction('e:t.m(key1=foo)'); + expect(a.args['key1']({foo: 'bar'})).to.equal('foo'); + }); + + it('should NOT dereference vars in arg value strings', () => { + const a = parseAction('e:t.m(key1="abc")'); + expect(a.args['key1']()).to.equal('abc'); + expect(a.args['key1']({foo: 'bar'})).to.equal('abc'); + expect(() => { + parseAction('e:t.m(key1="abc".foo)'); + }).to.throw(/Expected either/); + }); + + it('should NOT dereference vars in arg value booleans', () => { + const a = parseAction('e:t.m(key1=true)'); + expect(a.args['key1']()).to.equal(true); + expect(a.args['key1']({true: 'bar'})).to.equal(true); + expect(() => { + parseAction('e:t.m(key1=true.bar)'); + }).to.throw(/Expected either/); + }); + + it('should NOT dereference vars in arg value numerics', () => { + const a = parseAction('e:t.m(key1=123)'); + expect(a.args['key1']()).to.equal(123); + expect(a.args['key1']({123: 'bar'})).to.equal(123); + expect(() => { + parseAction('e:t.m(key1=123.bar)'); + }).to.throw(/Expected either/); + }); + + it('should return null for undefined references in arg values', () => { + const a = parseAction('e:t.m(key1=foo.bar)'); + expect(a.args['key1'](null)).to.equal(null); + expect(a.args['key1']({})).to.equal(null); + expect(a.args['key1']({foo: null})).to.equal(null); + }); + + it('should NOT dereference non-primitives in arg values', () => { + const a = parseAction('e:t.m(key1=foo.bar)'); + expect(a.args['key1']({foo: {bar: undefined}})).to.equal(null); + expect(a.args['key1']({foo: {bar: {}}})).to.equal(null); + expect(a.args['key1']({foo: {bar: []}})).to.equal(null); + expect(a.args['key1']({foo: {bar: () => {}}})).to.equal(null); + }); + + it('should apply arg functions with no event', () => { + const a = parseAction('e:t.m(key1=foo)'); + expect(applyActionInfoArgs(a.args, null)).to.deep.equal({key1: 'foo'}); + }); + + it('applied arg values should be proto-less objects', () => { + const a = parseAction('e:t.m(key1=foo)'); + expect(applyActionInfoArgs(a.args, null).__proto__).to.be.undefined; + expect(applyActionInfoArgs(a.args, null).constructor).to.be.undefined; + }); + + it('should apply arg value functions with an event with data', () => { + const a = parseAction('e:t.m(key1=event.foo)'); + const event = new CustomEvent('MyEvent', {detail: {foo: 'bar'}}); + expect(applyActionInfoArgs(a.args, event)).to.deep.equal({key1: 'bar'}); + }); + + it('should apply arg value functions with an event without data', () => { + const a = parseAction('e:t.m(key1=foo)'); + const event = new CustomEvent('MyEvent'); + expect(applyActionInfoArgs(a.args, event)).to.deep.equal({key1: 'foo'}); }); it('should parse empty to null', () => { @@ -225,7 +306,6 @@ describe('ActionService parseAction', () => { }); }); - describe('Action parseActionMap', () => { it('should parse with a single action', () => {