From c71d9f565a70ee6638b0c35ff44b5fc8637d7489 Mon Sep 17 00:00:00 2001 From: Mohammad Khatib Date: Wed, 23 Nov 2016 16:49:09 -0800 Subject: [PATCH] Introduce expandStringAsync and expandStringSync in url-replacement (#6323) --- build-system/tasks/presubmit-checks.js | 12 ++++ src/service/url-replacements-impl.js | 77 +++++++++++++++++++++--- test/functional/test-url-replacements.js | 28 +++++++++ 3 files changed, 110 insertions(+), 7 deletions(-) diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 4aaf706ff379..4586773e6f56 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -610,6 +610,18 @@ var forbiddenTermsSrcInclusive = { '\\.getTime\\(\\)': { message: 'Unless you do weird date math (whitelist), use Date.now().', }, + '\\.expandStringSync\\(': { + message: requiresReviewPrivacy, + whitelist: [ + 'src/service/url-replacements-impl.js', + ] + }, + '\\.expandStringAsync\\(': { + message: requiresReviewPrivacy, + whitelist: [ + 'src/service/url-replacements-impl.js', + ] + }, }; // Terms that must appear in a source file. diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index f4e4304e90d2..ca9eb18386a0 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -524,6 +524,8 @@ export class UrlReplacements { * Synchronously expands the provided URL by replacing all known variables with * their resolved values. Optional `opt_bindings` can be used to add new * variables or override existing ones. Any async bindings are ignored. + * + * TODO(mkhatib, #6322): Deprecate and please use expandUrlSync or expandStringSync. * @param {string} url * @param {!Object=} opt_bindings * @param {!Object=} opt_collectVars @@ -532,21 +534,83 @@ export class UrlReplacements { * @return {string} */ expandSync(url, opt_bindings, opt_collectVars, opt_whiteList) { - return /** @type {string} */( - this.expand_(url, opt_bindings, opt_collectVars, /* opt_sync */ true, - opt_whiteList)); + return this.expandUrlSync( + url, opt_bindings, opt_collectVars, opt_whiteList); } /** * Expands the provided URL by replacing all known variables with their * resolved values. Optional `opt_bindings` can be used to add new variables * or override existing ones. + * + * TODO(mkhatib, #6322): Deprecate and please use expandUrlAsync or expandStringAsync. * @param {string} url * @param {!Object=} opt_bindings * @return {!Promise} */ expandAsync(url, opt_bindings) { - return /** @type {!Promise} */(this.expand_(url, opt_bindings)); + return this.expandUrlAsync(url, opt_bindings); + } + + + /** + * Synchronously expands the provided source by replacing all known variables with + * their resolved values. Optional `opt_bindings` can be used to add new + * variables or override existing ones. Any async bindings are ignored. + * @param {string} source + * @param {!Object=} opt_bindings + * @param {!Object=} opt_collectVars + * @param {!Object=} opt_whiteList Optional white list of names + * that can be substituted. + * @return {string} + */ + expandStringSync(source, opt_bindings, opt_collectVars, opt_whiteList) { + return /** @type {string} */ ( + this.expand_(source, opt_bindings, opt_collectVars, /* opt_sync */ true, + opt_whiteList)); + } + + /** + * Expands the provided source by replacing all known variables with their + * resolved values. Optional `opt_bindings` can be used to add new variables + * or override existing ones. + * @param {string} source + * @param {!Object=} opt_bindings + * @return {!Promise} + */ + expandStringAsync(source, opt_bindings) { + return /** @type {!Promise} */ (this.expand_(source, opt_bindings)); + } + + /** + * Synchronously expands the provided URL by replacing all known variables with + * their resolved values. Optional `opt_bindings` can be used to add new + * variables or override existing ones. Any async bindings are ignored. + * @param {string} url + * @param {!Object=} opt_bindings + * @param {!Object=} opt_collectVars + * @param {!Object=} opt_whiteList Optional white list of names + * that can be substituted. + * @return {string} + */ + expandUrlSync(url, opt_bindings, opt_collectVars, opt_whiteList) { + return this.ensureProtocolMatches_(url, /** @type {string} */ (this.expand_( + url, opt_bindings, opt_collectVars, /* opt_sync */ true, + opt_whiteList))); + } + + /** + * Expands the provided URL by replacing all known variables with their + * resolved values. Optional `opt_bindings` can be used to add new variables + * or override existing ones. + * @param {string} url + * @param {!Object=} opt_bindings + * @return {!Promise} + */ + expandUrlAsync(url, opt_bindings) { + return /** @type {!Promise} */ ( + this.expand_(url, opt_bindings).then( + replacement => this.ensureProtocolMatches_(url, replacement))); } /** @@ -687,10 +751,9 @@ export class UrlReplacements { } if (opt_sync) { - return this.ensureProtocolMatches_(url, replacement); + return replacement; } - return (replacementPromise || Promise.resolve(replacement)) - .then(replacement => this.ensureProtocolMatches_(url, replacement)); + return replacementPromise || Promise.resolve(replacement); } /** diff --git a/test/functional/test-url-replacements.js b/test/functional/test-url-replacements.js index 5040666308df..e32a8b95af82 100644 --- a/test/functional/test-url-replacements.js +++ b/test/functional/test-url-replacements.js @@ -1074,4 +1074,32 @@ describe('UrlReplacements', () => { }); }); }); + + describe('Expanding String', () => { + it('should not reject protocol changes with expandStringSync', () => { + const win = getFakeWindow(); + const urlReplacements = installUrlReplacementsServiceForDoc(win.ampdoc); + let expanded = urlReplacements.expandStringSync( + 'PROTOCOL://example.com/?r=RANDOM', { + 'PROTOCOL': 'abc', + }); + expect(expanded).to.match(/abc:\/\/example\.com\/\?r=(\d+(\.\d+)?)$/); + expanded = urlReplacements.expandStringSync( + 'FUNCT://example.com/?r=RANDOM', { + 'FUNCT': function() { return 'abc'; }, + }); + expect(expanded).to.match(/abc:\/\/example\.com\/\?r=(\d+(\.\d+)?)$/); + }); + + it('should not check protocol changes with expandStringAsync', () => { + const win = getFakeWindow(); + const urlReplacements = installUrlReplacementsServiceForDoc(win.ampdoc); + return urlReplacements.expandStringAsync( + 'RANDOM:X:Y', { + 'RANDOM': Promise.resolve('abc'), + }).then(expanded => { + expect(expanded).to.equal('abc:X:Y'); + }); + }); + }); });