whitelist of variables #4

hamousavi · 2018-02-07T23:02:25Z

If a whitelist of variables is provided using meta tags for example as follows
<meta name="amp-variable-substitution-whitelist" content="RANDOM,TIMEZONE">
then the runtime should respect it. In order to do this we parse the meta tag in GlobalVariableSource.

hamousavi · 2018-02-07T23:12:02Z

@raywainman @zhangsu

zhangsu · 2018-02-09T04:36:02Z

src/service/variable-source.js

@@ -109,7 +109,10 @@ export function getNavigationData(win, attribute) {
 * and override initialize() to add more supported variables.
 */
 export class VariableSource {
-  constructor() {
+  constructor(ampdoc) {
+    /** @const {!./ampdoc-impl.AmpDoc} */


@protected

zhangsu · 2018-02-09T04:37:17Z

src/service/variable-source.js

@@ -109,7 +109,10 @@ export function getNavigationData(win, attribute) {
 * and override initialize() to add more supported variables.
 */
 export class VariableSource {
-  constructor() {
+  constructor(ampdoc) {


Since the constructor is no longer nullary, it needs type annotation.

zhangsu · 2018-02-09T04:39:18Z

src/service/variable-source.js

@@ -121,6 +124,9 @@ export class VariableSource {

    /** @private {boolean} */
    this.initialized_ = false;
+
+    /** @const @private {?Array<string>} */
+    this.ampVariableSubstitutionWhitelist_ = this.createWhitelist_();


The method is named in a way to communicate that it creates a "whitelist", but the field is named ampVariableSubstitutionWhitelist_. Can we be consistent?

I find words like amp and substitution a bit redundant here. How about this:

this.whitelist_ = this.createWhitelist_();

zhangsu · 2018-02-09T04:40:11Z

src/service/variable-source.js

+   * (if provided in a meta tag).
+   * @private
+   */
+  createWhitelist_() {


Consider moving this to just below the constructor, since it only exists to support the constructor (footnote style).

zhangsu · 2018-02-09T04:42:31Z

src/service/variable-source.js

@@ -164,6 +170,11 @@ export class VariableSource {
   */
  set(varName, syncResolver) {
    dev().assert(varName.indexOf('RETURN') == -1);
+    if (this.ampVariableSubstitutionWhitelist_ &&
+      !this.ampVariableSubstitutionWhitelist_.includes(varName)) {


Given that the binary boolean operation here is duplicated, consider moving this to a private helper: if (this.isWhitelisted_(varName)).

zhangsu · 2018-02-09T04:58:15Z

src/service/variable-source.js

+    }
+
+    const head = this.ampdoc.getRootNode().head;
+    if (head) {


Consider exiting early here with the negative condition to make the precondition clear and the code below more width-efficient.

zhangsu · 2018-02-09T05:13:35Z

src/service/variable-source.js

+      const meta =
+        head.querySelector('meta[name="amp-variable-substitution-whitelist"]');
+
+      if (meta) {


zhangsu · 2018-02-09T05:14:23Z

test/functional/test-variable-source.js

@@ -84,6 +84,39 @@ describe('VariableSource', () => {
    });
  });

+  describes.fakeWin('#whitelist', {


I think I'd much prefer using describes.realWin to create a real <meta> tag and use real querySelector. The test would be much useful that way, since it actually tests the real thing that will be running under the code in prod.

zhangsu · 2018-02-09T05:16:37Z

src/service/variable-source.js

@@ -246,6 +261,11 @@ export class VariableSource {
   * @private
   */
  buildExpr_(keys, isV2) {
+    // If a whitelist is provided, the keys must all belong to the whitelist.


Could you clarify a bit more about why they keys must all belong to the whitelist?

Do you want me to explain this in the comments? This is basically because additional keys can be provided to the getExpr function (which then call the build_expr_). We already made sure that we filter those keys that are provided in the set and setAsync. So it is necessary to have this final filtering as well. And I insist that we do it in here rather than in getExpr. This was we make sure that nothing slips through. Does that make sense?

I feel like this this not very clear, so definitely worth clarifying this in the code instead of on Github. :)

zhangsu · 2018-02-09T05:17:39Z

src/service/variable-source.js

+
+      if (meta) {
+        return meta.getAttribute('content').split(',')
+            .map(variable => variable.trim());


Just to make sure: do we plan to move this to a utility after the two pull requests are merged (consider filing a bug so that you don't forget)?

Yes. That is the plan. I filed the bug ampproject#13497.

hamousavi · 2018-02-14T23:10:41Z

@zhangsu my linter is giving me some error

Error: Failed to load plugin sort-imports-es6-autofix: Cannot find module 'eslint-plugin-sort-imports-es6-autofix'

have you experienced something like this? It has been working with my previous PR.

zhangsu

Not sure what's going on with the linter failure, try merging the latest upstream and rerun the linter. I think there's also a way to clear your local Gulp build cache so that you can rebuild everything.

zhangsu · 2018-02-15T03:33:22Z

src/service/variable-source.js

+
+    // The whitelist of variables allowed for variable substitution.
+    /** @const @private {?Array<string>} */
+    this.whitelist_ = this.getVariableWhitelist_();


Should this be initialized to null to enable lazy initialization, just like your other PR?

That was my first approach but then I changed it because the lazy initialization in this PR will add a large number of unnecessary conditional checks: To lazy initialize we must move the initialization to set and setAsync. Now look at the initialize method of GlobalVariableSource. There are tens of calls to set and setAsync. So if we do lazy loading then runtime has to check a conditional for every one of these calls to set. It gets even worse if one notices that the check for lazy initialization must also appear in getExpr.

Also since the sole purpose of this class is to build a regex for variables that are allowed to be substituted, we can be certain that when the class is instantiated, a call that triggers the lazy initialization of whitelist will happen.

This last point is different in the action PR. You could have a document that contains no actions on the special AMP target, but the standard action handler is instantiated nevertheless (as there could be other types of actions).

These being said, we could just remove all the filterings happening in set as we are going to do one final filtering in buildExpr_ anyway. In that case we can just lazy initialize at buildExpr_ time. Let me know what you think.

I think we should generally avoid premature micro-(and macro-)optimizations unless we have profiling or benchmark data to support the claim for the performance difference. For conditional checks like this, I tend to trust the JIT compiler and the JavaScript virtual machine to perform the right optimization, so I'd aim for maximum code clarity instead of performance. In this case, the code is readable either ways, so I don't really feel strong about either ways.

zhangsu · 2018-02-15T03:34:45Z

src/service/variable-source.js

@@ -121,6 +127,43 @@ export class VariableSource {

    /** @private {boolean} */
    this.initialized_ = false;
+
+    // The whitelist of variables allowed for variable substitution.


Add this to the JSDoc:

/** * The whitelist of variables allowed for variable substitution. * @const @private {?Array<string>} */

zhangsu · 2018-02-15T03:35:58Z

src/service/variable-source.js

+   * @param {!./ampdoc-impl.AmpDoc} ampdoc
+   */
+  constructor(ampdoc) {
+    /** @protected @const {?./ampdoc-impl.AmpDoc} */


Nit: Let's be consistent on whether @const is before visibility tag or not. On line 131 below @const is before the visibility tag @private, but not here.

zhangsu · 2018-02-15T03:56:06Z

src/service/variable-source.js

@@ -109,7 +109,13 @@ export function getNavigationData(win, attribute) {
 * and override initialize() to add more supported variables.
 */
 export class VariableSource {
-  constructor() {
+  /**
+   * @param {!./ampdoc-impl.AmpDoc} ampdoc


This is marked as non-nullable, but the field below is marked as nullable, which doesn't make sense. The field is marked as @const, which means that it's never re-assigned, so it should always have the same nullability as the constructor parameter here.

Can we fix the test instead?

I fixed the variable-source test, but many of the other tests are failing unfortunately. I added back the check for this.ampdoc in getVariableWhitelist. I could create a bug maybe?

If the code is checking for whether this.ampdoc is truthy, then the type annotation here should really be consistent (i.e., ?AmpDoc. If the type is marked as non-nullable, then the code should do null check; if the code is doing null check, then the type should be marked as nullable.

zhangsu · 2018-02-15T03:57:45Z

src/service/variable-source.js

+    // If a whitelist is provided, the keys must all belong to the whitelist.
+    if (this.getVariableWhitelist_()) {
+      keys = keys.filter(key =>
+        this.getVariableWhitelist_().includes(key));


Nit: this seems to fit on the previous line.

zhangsu · 2018-02-15T04:01:08Z

test/functional/url-expander/test-expander.js

+        getAttribute: () => 'ABC,ABCD,CANONICAL',
+      };
+      sandbox.stub(env.win.document.head,
+          'querySelector').callsFake(() => fakeMeta);


This file is using a real window for each test, so it seems to me that we can use real querySelector and insert real meta tag here. This is especially valuable for a functional test as it's supposed to test the integration.

zhangsu · 2018-02-15T04:07:15Z

src/service/variable-source.js

+   */
+  isWhitelisted_(varName) {
+    return this.getVariableWhitelist_() &&
+      !this.getVariableWhitelist_().includes(varName);


This checks if the whitelist does not include the variable name, which contradicts with the method name isWhitelisted_.

I think this method should

return !this.getVariableWhitelist_() || this.getVariableWhitelist_().includes(varName);

And the call sites should be checking if (!this.isWhitelisted_(...)).

zhangsu · 2018-02-15T04:08:35Z

src/service/variable-source.js

+
+  /**
+   * @return {?Array<string>} The whitelist of allowed AMP variables. (if provided in 
+         a meta tag).


Nit: missing * at the start of the JSDoc column.

zhangsu · 2018-02-15T04:09:59Z

src/service/variable-source.js

+
+    // A meta[name="amp-variable-substitution-whitelist"] tag, if present, 
+    // contains, in its content attribute, a whitelist of variable
+    // substitution.


Nit: this seems to fit on the previous line.

zhangsu · 2018-02-15T04:12:21Z

src/service/variable-source.js

+  /**
+   * @param {string} varName
+   * @return {boolean} If a whitelist is provided and 
+         it contains the variable name returns true.


Nit: part of this line seems to fit on the previous line.

Also missing a starting *.

Actually, I think it'd be more naturally to just say:

/** * Returns `true` if a variable whitelist is *not* present or the present * whitelist contains the given variable name. * @param {string} varName * @return {boolean} */

zhangsu · 2018-02-15T23:21:48Z

src/service/variable-source.js

@@ -109,7 +109,13 @@ export function getNavigationData(win, attribute) {
 * and override initialize() to add more supported variables.
 */
 export class VariableSource {
-  constructor() {
+  /**
+   * @param {!./ampdoc-impl.AmpDoc} ampdoc


If the code is checking for whether this.ampdoc is truthy, then the type annotation here should really be consistent (i.e., ?AmpDoc. If the type is marked as non-nullable, then the code should do null check; if the code is doing null check, then the type should be marked as nullable.

zhangsu · 2018-02-15T23:24:33Z

src/service/variable-source.js

+   * @param {!./ampdoc-impl.AmpDoc} ampdoc
+   */
+  constructor(ampdoc) {
+    /** @protected @const {!./ampdoc-impl.AmpDoc} */


Can this be @private (and field renamed to ampdoc_)?

No. The derived GlobalVariableSource class uses ampdoc quite a lot.

zhangsu · 2018-02-15T23:28:04Z

src/service/variable-source.js

+     * The whitelist of variables allowed for variable substitution.
+     * @private @const {?Array<string>} 
+     */
+    this.whitelist_ = this.getVariableWhitelist_();


I still think the field name should be consistent with the method name, so either change the field name to variableWhitelist_ or change the method name to getWhitelist_.

zhangsu · 2018-02-15T23:28:32Z

src/service/variable-source.js

+      return this.whitelist_;
+    }
+
+    if (!this.ampdoc) {


Add a TODO to remove this after fixing the tests?

Done. I'll also create an issue when I create a pull request to upstream.

zhangsu · 2018-02-15T23:37:49Z

src/service/variable-source.js

+
+    // The whitelist of variables allowed for variable substitution.
+    /** @const @private {?Array<string>} */
+    this.whitelist_ = this.getVariableWhitelist_();


I think we should generally avoid premature micro-(and macro-)optimizations unless we have profiling or benchmark data to support the claim for the performance difference. For conditional checks like this, I tend to trust the JIT compiler and the JavaScript virtual machine to perform the right optimization, so I'd aim for maximum code clarity instead of performance. In this case, the code is readable either ways, so I don't really feel strong about either ways.

hamousavi

@zhangsu do you think we are ready to make PR to upstream?

hamousavi · 2018-02-16T20:54:39Z

src/service/variable-source.js

@@ -109,7 +109,13 @@ export function getNavigationData(win, attribute) {
 * and override initialize() to add more supported variables.
 */
 export class VariableSource {
-  constructor() {
+  /**
+   * @param {!./ampdoc-impl.AmpDoc} ampdoc


hamousavi · 2018-02-16T20:58:06Z

src/service/variable-source.js

+   * @param {!./ampdoc-impl.AmpDoc} ampdoc
+   */
+  constructor(ampdoc) {
+    /** @protected @const {!./ampdoc-impl.AmpDoc} */


No. The derived GlobalVariableSource class uses ampdoc quite a lot.

hamousavi · 2018-02-16T20:59:35Z

src/service/variable-source.js

+     * The whitelist of variables allowed for variable substitution.
+     * @private @const {?Array<string>} 
+     */
+    this.whitelist_ = this.getVariableWhitelist_();


hamousavi · 2018-02-16T21:06:15Z

src/service/variable-source.js

+      return this.whitelist_;
+    }
+
+    if (!this.ampdoc) {


Done. I'll also create an issue when I create a pull request to upstream.

zhangsu · 2018-02-16T22:40:47Z

Yes, go head!

hamousavi added 4 commits February 7, 2018 18:00

whitelist of variables

f8f3a51

revert variable-source test

713a397

reset

cd9d0c9

add tests for variable source

fc13f6b

zhangsu reviewed Feb 9, 2018

View reviewed changes

more code review changes

a7b42ac

zhangsu reviewed Feb 15, 2018

View reviewed changes

even more code review

222c848

zhangsu approved these changes Feb 15, 2018

View reviewed changes

change amp-doc to nullable

0bb598d

hamousavi commented Feb 16, 2018

View reviewed changes

whitelist of variables #4

Are you sure you want to change the base?

whitelist of variables #4

Conversation

hamousavi commented Feb 7, 2018

hamousavi commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangsu Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamousavi commented Feb 14, 2018

zhangsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamousavi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangsu commented Feb 16, 2018

zhangsu Feb 15, 2018 •

edited

Loading