Repository: plotly/dash
Pull request #753 (2019-05-30 01:16:40)

Props blacklist
Fixes https://github.com/plotly/dash-core-components/issues/440

Pares down the attributes of dash component classes on the Python side by doing two things:
- Remove the `MutableMapping` inheritance - we weren't using any of the mixins from that class anyway, except in tests. This stops setting attributes: `clear`, `get`, `items`, `keys`, `pop`, `popitem`, `setdefault`, `update`, `values` - all of these are now allowable as prop names. We still set the magic methods of the mapping interface.
- Prefix a couple of other methods (`traverse` and `traverse_with_paths`) with underscores, so these are allowable as well.

Then what's left we blacklist: during `extract-meta`, if any prop has a leading underscore or matches one of the remaining disallowed names, we fail (not just print a warning) and refuse to generate the python file. The names we forbid are:
- `UNDEFINED`
- `REQUIRED`
- `to_plotly_json`
- `available_properties`
- `available_wildcard_properties`

These are all used within generated components, or (in the case of `to_plotly_json`) in other packages, so it would be difficult to allow these without forcing all packages to be rebuilt. Fortunately I don't see these as particularly valuable prop names (unlike, say, `values`, `items`, and some of the other `MutableMapping` methods)

Error messages look like:
```
ERROR: "_foo" matches forbidden prop name pattern: /^_.*$/

extract-meta failed.


Error generating metadata in dash_core_components (status=0)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
...
```

- [X] I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR
- [x] I have added entry in the `CHANGELOG.md`


Patch:
diff --git a/dash/CHANGELOG.md b/dash/CHANGELOG.md
index ad5d1a350..ef8b9027a 100644
--- a/dash/CHANGELOG.md
+++ b/dash/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## Unreleased
 ### Changed
+- [#753](https://github.com/plotly/dash/pull/753) `Component` no longer inherits `MutableMapping`, so `values`, `keys`, and more are no longer methods. This fixed an issue reported in [dcc](https://github.com/plotly/dash-core-components/issues/440) where components with certain prop names defined but not provided would cause a failure to render. During component generation we now disallow all props with leading underscores or matching a few remaining reserved words: `UNDEFINED`, `REQUIRED`, `to_plotly_json`, `available_properties`, and `available_wildcard_properties`.
+
 - [#739](https://github.com/plotly/dash/pull/739) Allow the Flask app to be provided to Dash after object initialization. This allows users to define Dash layouts etc when using the app factory pattern, or any other pattern that inhibits access to the app object. This broadly complies with the flask extension API, allowing Dash to be considered as a Flask extension where it needs to be.
 
 - [#722](https://github.com/plotly/dash/pull/722) Assets are served locally by default. Both JS scripts and CSS files are affected. This improves robustness and flexibility in numerous situations, but in certain cases initial loading could be slowed. To restore the previous CDN serving, set `app.scripts.config.serve_locally = False` (and similarly with `app.css`, but this is generally less important).
diff --git a/dash/dash.py b/dash/dash.py
index 1e31a74ff..292d8f207 100644
--- a/dash/dash.py
+++ b/dash/dash.py
@@ -914,7 +914,8 @@ class Dash(object):
         def _validate_value(val, index=None):
             # val is a Component
             if isinstance(val, Component):
-                for p, j in val.traverse_with_paths():
+                # pylint: disable=protected-access
+                for p, j in val._traverse_with_paths():
                     # check each component value in the tree
                     if not _value_is_valid(j):
                         _raise_invalid(
@@ -1194,7 +1195,8 @@ class Dash(object):
         layout_id = getattr(self.layout, 'id', None)
 
         component_ids = {layout_id} if layout_id else set()
-        for component in to_validate.traverse():
+        # pylint: disable=protected-access
+        for component in to_validate._traverse():
             component_id = getattr(component, 'id', None)
             if component_id and component_id in component_ids:
                 raise exceptions.DuplicateIdError(
diff --git a/dash/development/base_component.py b/dash/development/base_component.py
index 01f8189d3..79e7110c0 100644
--- a/dash/development/base_component.py
+++ b/dash/development/base_component.py
@@ -59,7 +59,7 @@ def _check_if_has_indexable_children(item):
 
 
 @six.add_metaclass(ComponentMeta)
-class Component(patch_collections_abc('MutableMapping')):
+class Component(object):
     class _UNDEFINED(object):
         def __repr__(self):
             return 'undefined'
@@ -184,7 +184,7 @@ class Component(patch_collections_abc('MutableMapping')):
         # If we were in a list, then this exception will get caught
         raise KeyError(id)
 
-    # Supply ABC methods for a MutableMapping:
+    # Magic methods for a mapping interface:
     # - __getitem__
     # - __setitem__
     # - __delitem__
@@ -208,12 +208,12 @@ class Component(patch_collections_abc('MutableMapping')):
         """Delete items by ID in the tree of children."""
         return self._get_set_or_delete(id, 'delete')
 
-    def traverse(self):
+    def _traverse(self):
         """Yield each item in the tree."""
-        for t in self.traverse_with_paths():
+        for t in self._traverse_with_paths():
             yield t[1]
 
-    def traverse_with_paths(self):
+    def _traverse_with_paths(self):
         """Yield each item with its path in the tree."""
         children = getattr(self, 'children', None)
         children_type = type(children).__name__
@@ -224,7 +224,8 @@ class Component(patch_collections_abc('MutableMapping')):
         # children is just a component
         if isinstance(children, Component):
             yield "[*] " + children_string, children
-            for p, t in children.traverse_with_paths():
+            # pylint: disable=protected-access
+            for p, t in children._traverse_with_paths():
                 yield "\n".join(["[*] " + children_string, p]), t
 
         # children is a list of components
@@ -238,12 +239,13 @@ class Component(patch_collections_abc('MutableMapping')):
                 yield list_path, i
 
                 if isinstance(i, Component):
-                    for p, t in i.traverse_with_paths():
+                    # pylint: disable=protected-access
+                    for p, t in i._traverse_with_paths():
                         yield "\n".join([list_path, p]), t
 
     def __iter__(self):
         """Yield IDs in the tree of children."""
-        for t in self.traverse():
+        for t in self._traverse():
             if (isinstance(t, Component) and
                     getattr(t, 'id', None) is not None):
 
diff --git a/dash/development/component_generator.py b/dash/development/component_generator.py
index 69dfafb28..48b88c092 100644
--- a/dash/development/component_generator.py
+++ b/dash/development/component_generator.py
@@ -19,6 +19,16 @@ from ._py_components_generation import generate_imports
 from ._py_components_generation import generate_classes_files
 
 
+reserved_words = [
+    'UNDEFINED',
+    'REQUIRED',
+    'to_plotly_json',
+    'available_properties',
+    'available_wildcard_properties',
+    '_.*'
+]
+
+
 class _CombinedFormatter(
         argparse.ArgumentDefaultsHelpFormatter,
         argparse.RawDescriptionHelpFormatter
@@ -47,9 +57,13 @@ def generate_components(
 
     extract_path = pkg_resources.resource_filename("dash", "extract-meta.js")
 
+    reserved_patterns = '|'.join('^{}$'.format(p) for p in reserved_words)
+
     os.environ["NODE_PATH"] = "node_modules"
     cmd = shlex.split(
-        "node {} {} {}".format(extract_path, ignore, components_source),
+        "node {} {} {} {}".format(
+            extract_path, ignore, reserved_patterns, components_source
+        ),
         posix=not is_windows,
     )
 
@@ -138,8 +152,8 @@ def cli():
         "-p",
         "--package-info-filename",
         default="package.json",
-        help="The filename of the copied `package.json` \
-to `project_shortname`",
+        help="The filename of the copied `package.json` "
+        "to `project_shortname`",
     )
     parser.add_argument(
         "-i",
diff --git a/dash/extract-meta.js b/dash/extract-meta.js
index b667a58ad..3567161be 100644
--- a/dash/extract-meta.js
+++ b/dash/extract-meta.js
@@ -4,8 +4,11 @@ const fs = require('fs');
 const path = require('path');
 const reactDocs = require('react-docgen');
 
-const componentPaths = process.argv.slice(3);
+const componentPaths = process.argv.slice(4);
 const ignorePattern = new RegExp(process.argv[2]);
+const reservedPatterns = process.argv[3].split('|').map(part => new RegExp(part));
+
+let failed = false;
 
 const excludedDocProps = [
     'setProps', 'id', 'className', 'style'
@@ -20,13 +23,18 @@ const metadata = Object.create(null);
 componentPaths.forEach(componentPath =>
     collectMetadataRecursively(componentPath)
 );
-writeOut(metadata);
+if (failed) {
+    console.error('\nextract-meta failed.\n');
+}
+else {
+    writeOut(metadata);
+}
 
 function help() {
     console.error('usage: ');
     console.error(
-        'extract-meta path/to/component(s) ' +
-            ' [path/to/more/component(s), ...] > metadata.json'
+        'extract-meta ^fileIgnorePattern ^forbidden$|^props$|^patterns$' +
+        ' path/to/component(s) [path/to/more/component(s) ...] > metadata.json'
     );
 }
 
@@ -55,6 +63,20 @@ function docstringWarning(doc) {
     );
 }
 
+function propError(doc) {
+    for(const propName in doc.props) {
+        reservedPatterns.forEach(reservedPattern => {
+            if (reservedPattern.test(propName)) {
+                process.stderr.write(
+                    `\nERROR: "${propName}" matches reserved word ` +
+                    `pattern: ${reservedPattern.toString()}\n`
+                );
+                failed = true;
+            }
+        });
+    }
+}
+
 
 function parseFile(filepath) {
     const urlpath = filepath.split(path.sep).join('/');
@@ -68,6 +90,7 @@ function parseFile(filepath) {
         src = fs.readFileSync(filepath);
         const doc = metadata[urlpath] = reactDocs.parse(src);
         docstringWarning(doc);
+        propError(doc);
     } catch (error) {
         writeError(error, filepath);
     }


Test patch:
diff --git a/tests/unit/dash/development/test_base_component.py b/tests/unit/dash/development/test_base_component.py
index d901189ec..739590ccf 100644
--- a/tests/unit/dash/development/test_base_component.py
+++ b/tests/unit/dash/development/test_base_component.py
@@ -1,5 +1,4 @@
 from collections import OrderedDict
-import collections
 import inspect
 import json
 import os
@@ -8,8 +7,17 @@ import unittest
 import plotly
 
 from dash.development.base_component import Component
-from dash.development._py_components_generation import generate_class_string, generate_class_file, generate_class, \
-    create_docstring, prohibit_events, js_to_py_type
+from dash.development.component_generator import reserved_words
+from dash.development._py_components_generation import (
+    generate_class_string,
+    generate_class_file,
+    generate_class,
+    create_docstring,
+    prohibit_events,
+    js_to_py_type
+)
+
+_dir = os.path.dirname(os.path.abspath(__file__))
 
 Component._prop_names = ('id', 'a', 'children', 'style', )
 Component._type = 'TestComponent'
@@ -85,8 +93,9 @@ class TestComponent(unittest.TestCase):
 
     def test_get_item_with_nested_children_with_mixed_strings_and_without_lists(self):  # noqa: E501
         c, c1, c2, c3, c4, c5 = nested_tree()
+        keys = [k for k in c]
         self.assertEqual(
-            list(c.keys()),
+            keys,
             [
                 '0.0',
                 '0.1',
@@ -135,7 +144,8 @@ class TestComponent(unittest.TestCase):
 
     def test_del_item_with_nested_children_with_mixed_strings_and_without_lists(self):  # noqa: E501
         c = nested_tree()[0]
-        for key in reversed(list(c.keys())):
+        keys = reversed([k for k in c])
+        for key in keys:
             c[key]
             del c[key]
             with self.assertRaises(KeyError):
@@ -143,7 +153,7 @@ class TestComponent(unittest.TestCase):
 
     def test_traverse_with_nested_children_with_mixed_strings_and_without_lists(self):  # noqa: E501
         c, c1, c2, c3, c4, c5 = nested_tree()
-        elements = [i for i in c.traverse()]
+        elements = [i for i in c._traverse()]
         self.assertEqual(
             elements,
             c.children + [c3] + [c2] + c2.children
@@ -153,19 +163,12 @@ class TestComponent(unittest.TestCase):
         c, c1, c2, c3, c4, c5 = nested_tree()
         c2.children = tuple(c2.children)
         c.children = tuple(c.children)
-        elements = [i for i in c.traverse()]
+        elements = [i for i in c._traverse()]
         self.assertEqual(
             elements,
             list(c.children) + [c3] + [c2] + list(c2.children)
         )
 
-    def test_iter_with_nested_children_with_mixed_strings_and_without_lists(self):  # noqa: E501
-        c = nested_tree()[0]
-        keys = list(c.keys())
-        # get a list of ids that __iter__ provides
-        iter_keys = [i for i in c]
-        self.assertEqual(keys, iter_keys)
-
     def test_to_plotly_json_with_nested_children_with_mixed_strings_and_without_lists(self):  # noqa: E501
         c = nested_tree()[0]
         Component._namespace
@@ -244,17 +247,6 @@ class TestComponent(unittest.TestCase):
         with self.assertRaises(KeyError):
             c3['0']
 
-    def test_equality(self):
-        # TODO - Why is this the case? How is == being performed?
-        # __eq__ only needs __getitem__, __iter__, and __len__
-        self.assertTrue(Component() == Component())
-        self.assertTrue(Component() is not Component())
-
-        c1 = Component(id='1')
-        c2 = Component(id='2', children=[Component()])
-        self.assertTrue(c1 == c2)
-        self.assertTrue(c1 is not c2)
-
     def test_set_item(self):
         c1a = Component(id='1', children='Hello world')
         c2 = Component(id='2', children=c1a)
@@ -450,6 +442,9 @@ class TestComponent(unittest.TestCase):
         ])), 3)
 
     def test_iter(self):
+        # The mixin methods from MutableMapping were cute but probably never
+        # used - at least not by us. Test that they're gone
+
         # keys, __contains__, items, values, and more are all mixin methods
         # that we get for free by inheriting from the MutableMapping
         # and behave as according to our implementation of __iter__
@@ -469,40 +464,37 @@ class TestComponent(unittest.TestCase):
                 Component(children=[Component(id='8')]),
             ]
         )
-        # test keys()
-        keys = [k for k in list(c.keys())]
-        self.assertEqual(keys, ['2', '3', '4', '5', '6', '7', '8'])
-        self.assertEqual([i for i in c], keys)
 
-        # test values()
-        components = [i for i in list(c.values())]
-        self.assertEqual(components, [c[k] for k in keys])
+        mixins = ['clear', 'get', 'items', 'keys', 'pop', 'popitem',
+                  'setdefault', 'update', 'values']
+
+        for m in mixins:
+            assert not hasattr(c, m), 'should not have method ' + m
+
+        keys = ['2', '3', '4', '5', '6', '7', '8']
 
-        # test __iter__()
         for k in keys:
             # test __contains__()
-            self.assertTrue(k in c)
+            assert k in c, 'should find key ' + k
+            # test __getitem__()
+            assert c[k].id == k, 'key {} points to the right item'.format(k)
 
-        # test __items__
-        items = [i for i in list(c.items())]
-        self.assertEqual(list(zip(keys, components)), items)
+        # test __iter__()
+        keys2 = []
+        for k in c:
+            keys2.append(k)
+            assert k in keys, 'iteration produces key ' + k
 
-    def test_pop(self):
-        c2 = Component(id='2')
-        c = Component(id='1', children=c2)
-        c2_popped = c.pop('2')
-        self.assertTrue('2' not in c)
-        self.assertTrue(c2_popped is c2)
+        assert len(keys) == len(keys2), 'iteration produces no extra keys'
 
 
 class TestGenerateClassFile(unittest.TestCase):
     def setUp(self):
-        json_path = os.path.join(
-            'tests', 'unit', 'dash', 'development', 'metadata_test.json')
+        json_path = os.path.join(_dir, 'metadata_test.json')
         with open(json_path) as data_file:
             json_string = data_file.read()
             data = json\
-                .JSONDecoder(object_pairs_hook=collections.OrderedDict)\
+                .JSONDecoder(object_pairs_hook=OrderedDict)\
                 .decode(json_string)
             self.data = data
 
@@ -537,9 +529,7 @@ class TestGenerateClassFile(unittest.TestCase):
             self.written_class_string = f.read()
 
         # The expected result for both class string and class file generation
-        expected_string_path = os.path.join(
-            'tests', 'unit', 'dash', 'development', 'metadata_test.py'
-        )
+        expected_string_path = os.path.join(_dir, 'metadata_test.py')
         with open(expected_string_path, 'r') as f:
             self.expected_class_string = f.read()
 
@@ -567,12 +557,11 @@ class TestGenerateClassFile(unittest.TestCase):
 
 class TestGenerateClass(unittest.TestCase):
     def setUp(self):
-        path = os.path.join(
-            'tests', 'unit', 'dash', 'development', 'metadata_test.json')
+        path = os.path.join(_dir, 'metadata_test.json')
         with open(path) as data_file:
             json_string = data_file.read()
             data = json\
-                .JSONDecoder(object_pairs_hook=collections.OrderedDict)\
+                .JSONDecoder(object_pairs_hook=OrderedDict)\
                 .decode(json_string)
             self.data = data
 
@@ -583,14 +572,11 @@ class TestGenerateClass(unittest.TestCase):
             namespace='TableComponents'
         )
 
-        path = os.path.join(
-            'tests', 'unit', 'dash', 'development',
-            'metadata_required_test.json'
-        )
+        path = os.path.join(_dir, 'metadata_required_test.json')
         with open(path) as data_file:
             json_string = data_file.read()
             required_data = json\
-                .JSONDecoder(object_pairs_hook=collections.OrderedDict)\
+                .JSONDecoder(object_pairs_hook=OrderedDict)\
                 .decode(json_string)
             self.required_data = required_data
 
@@ -753,15 +739,36 @@ class TestGenerateClass(unittest.TestCase):
         with self.assertRaises(Exception):
             self.ComponentClassRequired(children='test')
 
+    def test_attrs_match_forbidden_props(self):
+        assert '_.*' in reserved_words, 'props cannot have leading underscores'
+
+        # props are not added as attrs unless explicitly provided
+        # except for children, which is always set if it's a prop at all.
+        expected_attrs = set(reserved_words + ['children']) - set(['_.*'])
+        c = self.ComponentClass()
+        base_attrs = set(dir(c))
+        extra_attrs = set(a for a in base_attrs if a[0] != '_')
+
+        assert extra_attrs == expected_attrs, \
+            'component has only underscored and reserved word attrs'
+
+        # setting props causes them to show up as attrs
+        c2 = self.ComponentClass('children', id='c2', optionalArray=[1])
+        prop_attrs = set(dir(c2))
+
+        assert base_attrs - prop_attrs == set([]), 'no attrs were removed'
+        assert (
+            prop_attrs - base_attrs == set(['id', 'optionalArray'])
+        ), 'explicit props were added as attrs'
+
 
 class TestMetaDataConversions(unittest.TestCase):
     def setUp(self):
-        path = os.path.join(
-            'tests', 'unit', 'dash', 'development', 'metadata_test.json')
+        path = os.path.join(_dir, 'metadata_test.json')
         with open(path) as data_file:
             json_string = data_file.read()
             data = json\
-                .JSONDecoder(object_pairs_hook=collections.OrderedDict)\
+                .JSONDecoder(object_pairs_hook=OrderedDict)\
                 .decode(json_string)
             self.data = data
 
@@ -940,12 +947,11 @@ def assert_docstring(assertEqual, docstring):
 
 class TestFlowMetaDataConversions(unittest.TestCase):
     def setUp(self):
-        path = os.path.join(
-            'tests', 'unit', 'dash', 'development', 'flow_metadata_test.json')
+        path = os.path.join(_dir, 'flow_metadata_test.json')
         with open(path) as data_file:
             json_string = data_file.read()
             data = json\
-                .JSONDecoder(object_pairs_hook=collections.OrderedDict)\
+                .JSONDecoder(object_pairs_hook=OrderedDict)\
                 .decode(json_string)
             self.data = data