🐛Fix broken args in expander. by calebcordry · Pull Request #22508 · ampproject/amphtml

calebcordry · 2019-05-24T22:23:49Z

The expander works by walking the input string. When it sees a macro it pushes the promise of a value (async) or the immediate resulting value (sync) into an array, that will be joined at the end of string or the next ).

For example in a async call:
'abcMACROdef' => ['abc', Promise, 'def']

This can become a problem in nested context because this array is what is passed to the function that resolves the function through function.apply.
'OUTER(INNER)' => OUTER.apply([INNER]) // good
'OUTER(foo, INNER)' => OUTER.apply(['foo', INNER]) // good
'OUTER(fooINNER, bar)' => OUTER.apply(['foo', INNER, 'bar']) // bad - three args passed to OUTER instead of two

This fix changes this to use an array of arrays, where we can keep track of each argument in its own array, and therefore pass the correct number of args. The inner array(s) are then flattened before passing to .apply().

'OUTER(fooINNER, bar)' => [['foo', INNER], ['bar']] => flatten =>
OUTER.apply(['fooINNER', 'bar'] // good :)

An added bonus is that this casts any argument to a string before invoking the function :) This should help us with any changes to preserve types.

Also added a bunch of comments since this code is hairy, and its fresh in my mind for now.

Closes #20816

zhouyx

Thank you for the summary. LGTM

zhouyx · 2019-05-30T18:37:46Z

src/service/url-expander/expander.js

+            // Trim space in between args that builder has collected.
            if (builder.trim().length) {
-              results.push(builder);
+              results.push(builder.trim());


If I understand the comment correctly, we only want to trip space between arguments. If so I don't think it's necessary to trim at (? For example abc MACRO(test). Please let me know if I miss anything.

After talking offline I tried to move this trimming all to the processArgs* calls, but there is a problem with handling the back tick functionality. At time of resolving we do not know if the arg should be not trimmed. For example REPLACE(foo bar, ` `, -) at call time we have some arg list like ['foo bar', ' ', '-'] but we have lost track of what should be raw or not.

This problem also exists with trying to only trim when we see a ) or ,. I thought about keeping track of which args should be trimmed and passing this data around, but I think it is cleaner to just trim the args when we see a macro and we know we are in nested context. I updated the code to do so. Let me know if you think there is a better way.

zhouyx · 2019-06-03T20:09:01Z

src/service/url-expander/expander.js


+          // Collect any chars that may be prefixing the macro, if we are in
+          // a nested context trim the args.
+          if (builder.trim().length) {


nit: Since we parse the string from left to right could we append the builder to result first before we handle the matched binding.

Also I understand this is used for

CLIENT_ID( SUBSTR(abc, 1, 2))

or

CLIENT_ID( testSUBSTR(abc, 1, 2))

However it also applies to

CLIENT_ID ( test SUBSTR(abc, 1, 2))

What do you think should be the different between these cases?

for the first comment, sure we can move it up before finding the binding. For the second comment you are right these will all behave the same. The hard part is the distinction between CLIENT_ID ( test SUBSTR(abc, 1, 2)) and CLIENT_ID (a, test SUBSTR(abc, 1, 2)). We could only trim the first space instead of trimming the whole thing. What do you think?

Hmmm CLIENT_ID ( test SUBSTR(abc, 1, 2)) maybe a bad example, because there's a space between CLIENT_ID and (. I don't think we can handle that. Let me know if that's true and expected.

CLIENT_ID(a, test SUBSTR(abc, 1, 2)). So I assume the test will be trimmed to test. And it equals to CLIENT_ID(a,testSUBSTR(abc, 1, 2)). Is that right?
Do you think we should only remove the trialing space in such case?

you are correct these would be equivalent. Do you mean the leading space? If so I think that may be a good idea.

just so we are on the same page, I am thinking something like trimLeft instead of trim.

Ah, you're right. leading space it is : )

👍changed to trimStart and moved the builder push before the binding stuff.

also added test with the space. $IF(QUERY_PARAM(test), foo TIMESTAMP, ``)

zhouyx · 2019-06-03T20:12:32Z

src/service/url-expander/expander.js

+      const args = [];

      while (urlIndex < url.length && matchIndex <= matches.length) {
        if (match && urlIndex === match.start) {


Have a question about the usage of backtick.
What's the expected output with

abc`TIMESTAMP`def

I couldn't find where we respect the PARSER_IGNORE_FLAG when handling with macros. Do you think that's an issue?

You are right this macro is still resolved. The backtick only really works for spaces, parens, and commas. I think this is ok as we note in docs that macro names are reserved, but happy to revisit this. If we were to change it would need to be a separate PR.

I think this is ok as we note in docs that macro names are reserved

As long as we've made that decision, I think it makes sense. could you please remind me when did we add support for backtick (before nested macro support, or after).

Cool. I think reserve macros makes sense because I believe we have no plan to support keeping the ${vars} anyway.

zhouyx

LGTM. Thank you for the fix

googlebot added the cla: yes label May 24, 2019

calebcordry marked this pull request as ready for review May 24, 2019 23:03

calebcordry requested review from lannka and zhouyx May 25, 2019 01:25

zhouyx approved these changes May 30, 2019

View reviewed changes

zhouyx suggested changes Jun 3, 2019

View reviewed changes

zhouyx approved these changes Jun 4, 2019

View reviewed changes

calebcordry added 7 commits June 3, 2019 19:17

better handling of args

094a5b6

no space.

6551723

comments

f03efed

types

38ef390

handle double comma correctly.

e85044c

trim before args only nested

c0e4617

move builder before and trimStart

ac8874e

calebcordry merged commit 9fc30f9 into ampproject:master Jun 4, 2019

calebcordry deleted the expander-args-bug branch June 4, 2019 15:02

zhouyx mentioned this pull request Jun 12, 2019

Remove unnecessary type cast in macro resolvers #22799

Merged

calebcordry mentioned this pull request Jan 8, 2020

Expander: backtick should respect empty space #26247

Closed

Conversation

calebcordry commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

calebcordry commented May 24, 2019 •

edited

Loading

zhouyx Jun 3, 2019 •

edited

Loading