Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add url REPLACE macro #13130

Merged
merged 4 commits into from Jan 30, 2018
Merged

Add url REPLACE macro #13130

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e6d0399
add-replace-filter
calebcordry Jan 29, 2018
6603a85
Refactor space handling
calebcordry Jan 30, 2018
b66c2ae
Toggle off next arg
calebcordry Jan 30, 2018
bba812d
Do what test describes
calebcordry Jan 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions extensions/amp-analytics/0.1/test/test-variables.js
View file
Open in desktop
Expand Up @@ -193,6 +193,14 @@ describe('amp-analytics.VariableService', function() {
'S7Uc5ZmODduHWdplzrZ7Jsnqx')
);
});

it('replaces', () => {
return check('REPLACE(this-is-a-test, `-`, )', 'thisisatest');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought this should gives this is a test (with spaces)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are currently stripping spaces so people can do things like DEFAULT(one, two) vs only allowing DEFAULT(one,two). We could change this but I think allowing it is better? It does make it tricky to pass in a space though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that both tests are testing replace with no third arg? here newSubStr = undefined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, newSubStr === '' in the first. I will add a few more tests to make this more clear

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so how do i get this is a test? Is it supported?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was spending some time thinking about this last night. Right now it is not supported without using a different macro that would return a space. We could support something like REPLACE(this-is-a-test, `-`, ` `) If we do that should we make the back ticks on the third parameter required? Also no matter what the spaces will be encoded so it would return this%20is%20a%20test

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now respecting whitespaces inside all back ticks. More tests were written to cover this behavior.

});

it('replace with no third arg', () => {
return check('REPLACE(thi@s-is-a-te@st, `-|@`)', 'thisisatest');
});
});

describe('getNameArgs:', () => {
Expand Down
12 changes: 12 additions & 0 deletions extensions/amp-analytics/0.1/variables.js
View file
Open in desktop
Expand Up @@ -98,6 +98,17 @@ function defaultMacro(value, defaultValue) {
return value;
}

function replaceFilter(string, matchPattern, newSubStr) {
Copy link
Contributor

@zhouyx zhouyx Jan 30, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsDoc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Done.

if (!matchPattern) {
user().warn(TAG, 'Replace must have two or more arguments');
}
if (!newSubStr) {
newSubStr = '';
}
const regex = new RegExp(matchPattern, 'g');
return string.replace(regex, newSubStr);
}


/**
* Provides support for processing of advanced variable syntax like nested
Expand Down Expand Up @@ -126,6 +137,7 @@ export class VariableService {
this.register_('HASH', this.hashMacro_.bind(this));
this.register_('IF',
(value, thenValue, elseValue) => value ? thenValue : elseValue);
this.register_('REPLACE', replaceFilter);
}

/**
Expand Down
13 changes: 10 additions & 3 deletions src/service/url-expander/expander.js
View file
Open in desktop
Expand Up @@ -16,6 +16,8 @@

import {rethrowAsync} from '../../log';

export const PARSER_IGNORE_FLAG = '`';

/** Rudamentary parser to handle nested Url replacement. */
export class Expander {

Expand Down Expand Up @@ -86,13 +88,13 @@ export class Expander {
let matchIndex = 0;
let match = matches[matchIndex];
let numOfPendingCalls = 0;
let ignoringChars = false;

const evaluateNextLevel = () => {
let builder = '';
const results = [];

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

if (match && urlIndex === match.start) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can we get a match in the middle of a literal string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand your question correctly, this is to support things like example.com?cid=CLIENT_ID(__ga)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean why would CLIENT_ID match inside of

cid=`CLIENT_ID(__GA)`

the tick marks a literal string ("don't parse me" section).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way it was designed is so that the ticks are only respected inside a macro call (inside parenthesis). We are currently reserving keywords like CLIENT_ID to be replaced anywhere in the input string. We could potentially change the parser to look for this if you think its a valid use case?

let binding;
// find out where this keyword is coming from
Expand Down Expand Up @@ -127,7 +129,12 @@ export class Expander {
builder = '';
}

else if (numOfPendingCalls && url[urlIndex] === ',') {
else if (url[urlIndex] === PARSER_IGNORE_FLAG) {
ignoringChars = !ignoringChars;
urlIndex++;
}

else if (numOfPendingCalls && url[urlIndex] === ',' && !ignoringChars) {
if (builder.length) {
results.push(builder.trim());
}
Expand All @@ -141,7 +148,7 @@ export class Expander {
urlIndex++;
}

else if (url[urlIndex] === ')') {
else if (url[urlIndex] === ')' && !ignoringChars) {
urlIndex++;
numOfPendingCalls--;
const binding = stack.pop();
Expand Down
18 changes: 18 additions & 0 deletions test/functional/url-expander/test-expander.js
View file
Open in desktop
Expand Up @@ -174,6 +174,24 @@ describes.realWin('Expander', {
return expect(expander.expand('TRIM(FAKE(aaaaa))', mockBindings))
.to.eventually.equal('');
});

it('ignores commas within backticks', () => {
const url = 'CONCAT(`he,llo`,UPPERCASE(world)';
return expect(expander.expand(url, mockBindings))
.to.eventually.equal('he,lloWORLD');
});

it('ignores left parentheses within backticks', () => {
const url = 'CONCAT(hello, `wo((rld`)';
return expect(expander.expand(url, mockBindings))
.to.eventually.equal('hellowo((rld');
});

it('ignores right parentheses within backticks', () => {
const url = 'CONCAT(`hello)`,UPPERCASE(world)';
return expect(expander.expand(url, mockBindings))
.to.eventually.equal('hello)WORLD');
});
});
});