Move name collision logic to the model

This change moves and formalizes the name collision logic from the factory
to the resource model. The following has changed:

* Documentation now better matches the factory output.
* Collision renaming order is formalized in one place
  * Order: reserved names (meta), load action, identifiers, actions,
    subresources, references, collections, and then attributes.
* Renaming resource model attributes/methods now happens at model loading
  time rather than class creation time.

The way this works is by creating a mapping of (type, name) tuples to
the renamed value, if it exists. Typically this mapping will be very
sparse and it's fast to create / access. In practice we currently only
have one or two names that collide across all the resource models.

Tests have been updated, some of which needed to define proper Botocore
shapes as the code now looks more closely at those at model load time.

Author: danielgtaylor

boto3/docs.py

@@ -177,6 +177,12 @@ def docs_for(service_name):
         for name, model in sorted(data['resources'].items(),
                                   key=lambda i:i[0]):
             resource_model = ResourceModel(name, model, data['resources'])
+
+            shape = None
+            if resource_model.shape:
+                shape = service_model.shape_for(resource_model.shape)
+            resource_model.load_rename_map(shape)
+
             if name not in models:
                 models[name] = {'type': 'resource', 'model': resource_model}
 
@@ -333,7 +339,7 @@ def document_resource(service_name, official_name, resource_model,
             docs += '   Attributes:\n\n'
             shape = service_model.shape_for(resource_model.shape)
 
-            for name, member in sorted(shape.members.items()):
+            for name, member in sorted(resource_model.get_attributes(shape).items()):
                 docs += ('   .. py:attribute:: {0}\n\n      (``{1}``)'
                          ' {2}\n\n').format(
                     xform_name(name), py_type_name(member.type_name),

boto3/resources/factory.py

@@ -74,6 +74,11 @@ def load_from_definition(self, service_name, resource_name, model,
 
         resource_model = ResourceModel(resource_name, model, resource_defs)
 
+        shape = None
+        if resource_model.shape:
+            shape = service_model.shape_for(resource_model.shape)
+        resource_model.load_rename_map(shape)
+
         self._load_identifiers(attrs, meta, resource_model)
         self._load_actions(attrs, resource_model, resource_defs,
                            service_model)
@@ -99,8 +104,6 @@ def _load_identifiers(self, attrs, meta, model):
         """
         for identifier in model.identifiers:
             snake_cased = xform_name(identifier.name)
-            snake_cased = self._check_allowed_name(
-                attrs, snake_cased, 'identifier', model.name)
             meta.identifiers.append(snake_cased)
             attrs[snake_cased] = None
 
@@ -118,8 +121,6 @@ def _load_actions(self, attrs, model, resource_defs, service_model):
 
         for action in model.actions:
             snake_cased = xform_name(action.name)
-            snake_cased = self._check_allowed_name(
-                attrs, snake_cased, 'action', model.name)
             attrs[snake_cased] = self._create_action(snake_cased,
                 action, resource_defs, service_model)
 
@@ -133,16 +134,8 @@ def _load_attributes(self, attrs, meta, model, service_model):
         if model.shape:
             shape = service_model.shape_for(model.shape)
 
-            for name, member in shape.members.items():
-                snake_cased = xform_name(name)
-                if snake_cased in meta.identifiers:
-                    # Skip identifiers, these are set through other means
-                    continue
-
-                snake_cased = self._check_allowed_name(
-                    attrs, snake_cased, 'attribute', model.name)
-                attrs[snake_cased] = self._create_autoload_property(name,
-                    snake_cased)
+            for name, member in model.get_attributes(shape).items():
+                attrs[name] = self._create_autoload_property(member.name, name)
 
     def _load_collections(self, attrs, model, resource_defs, service_model):
         """
@@ -153,8 +146,6 @@ def _load_collections(self, attrs, model, resource_defs, service_model):
         """
         for collection_model in model.collections:
             snake_cased = xform_name(collection_model.name)
-            snake_cased = self._check_allowed_name(
-                attrs, snake_cased, 'collection', model.name)
 
             attrs[snake_cased] = self._create_collection(
                 attrs['meta'].service_name, model.name, snake_cased,
@@ -177,8 +168,6 @@ def _load_has_relations(self, attrs, service_name, resource_name,
             # the data we need to create the resource, so
             # this instance becomes an attribute on the class.
             snake_cased = xform_name(reference.name)
-            snake_cased = self._check_allowed_name(
-                attrs, snake_cased, 'reference', model.name)
             attrs[snake_cased] = self._create_reference(
                 reference.resource.type, snake_cased, reference,
                 service_name, resource_name, model, resource_defs,
@@ -201,44 +190,8 @@ def _load_waiters(self, attrs, model):
         """
         for waiter in model.waiters:
             snake_cased = xform_name(waiter.resource_waiter_name)
-            snake_cased = self._check_allowed_name(
-                attrs, snake_cased, 'waiter', model.name)
             attrs[snake_cased] = self._create_waiter(waiter, snake_cased)
 
-    def _check_allowed_name(self, attrs, name, category, resource_name):
-        """
-        Determine if a given name is allowed on the instance, and if not,
-        then raise an exception. This prevents public attributes of the
-        class from being clobbered, e.g. since we define ``Resource.meta``,
-        no identifier may be named ``meta``. Another example: no action
-        named ``queue_items`` may be added after an identifier of the same
-        name has been added.
-
-        One attempt is made in the event of a collision to remedy the
-        situation. The ``category`` is appended to the name and the
-        check is performed again. For example, if an action named
-        ``get_frobs`` fails the test, then we try ``get_frobs_action``
-        after logging a warning.
-
-        :raises: ValueError
-        """
-        if name in attrs:
-            logger.warning('%s `%s` would clobber existing %s'
-                           ' resource attribute, going to try'
-                           ' %s instead...', category, name,
-                           resource_name, name + '_' + category)
-            # TODO: Move this logic into the model and strictly
-            #       define the loading order of categories. This
-            #       will make documentation much simpler.
-            name = name + '_' + category
-
-        if name in attrs:
-            raise ValueError('{0} `{1}` would clobber existing '
-                             '{2} resource attribute'.format(
-                                category, name, resource_name))
-
-        return name
-
     def _create_autoload_property(factory_self, name, snake_cased):
         """
         Creates a new property on the resource to lazy-load its value

boto3/resources/model.py

@@ -25,6 +25,8 @@
 
 import logging
 
+from botocore import xform_name
+
 
 logger = logging.getLogger(__name__)
 
@@ -250,12 +252,162 @@ class ResourceModel(object):
     def __init__(self, name, definition, resource_defs):
         self._definition = definition
         self._resource_defs = resource_defs
+        self._renamed = {}
 
         #: (``string``) The name of this resource
         self.name = name
         #: (``string``) The service shape name for this resource or ``None``
         self.shape = definition.get('shape')
 
+    def load_rename_map(self, shape=None):
+        """
+        Load a name translation map given a shape. This will set
+        up renamed values for any collisions, e.g. if the shape,
+        an action, and a subresource all are all named ``foo``
+        then the resource will have an action ``foo``, a subresource
+        named ``Foo`` and a property named ``foo_attribute``.
+        This is the order of precedence, from most important to
+        least important:
+
+        * Load action (resource.load)
+        * Identifiers
+        * Actions
+        * Subresources
+        * References
+        * Collections
+        * Attributes (shape members)
+
+        Batch actions are only exposed on collections, so do not
+        get modified here. Subresources use upper camel casing, so
+        are unlikely to collide with anything but other subresources.
+
+        Creates a structure like this::
+
+            renames = {
+                ('action', 'id'): 'id_action',
+                ('collection', 'id'): 'id_collection',
+                ('attribute', 'id'): 'id_attribute'
+            }
+
+            # Get the final name for an action named 'id'
+            name = renames.get(('action', 'id'), 'id')
+
+        :type shape: botocore.model.Shape
+        :param shape: The underlying shape for this resource.
+        """
+        # Meta is a reserved name for resources
+        names = set(['meta'])
+        self._renamed = {}
+
+        if self._definition.get('load'):
+            names.add('load')
+
+        for item in self._definition.get('identifiers', []):
+            self._load_name_with_category(names, item['name'], 'identifier')
+
+        for name in self._definition.get('actions', {}).keys():
+            self._load_name_with_category(names, name, 'action')
+
+        for name, ref in self._get_has_definition().items():
+            # Subresources require no data members, just typically
+            # identifiers and user input.
+            data_required = False
+            for identifier in ref['resource']['identifiers']:
+                if identifier['source'] == 'data':
+                    data_required = True
+                    break
+
+            if not data_required:
+                self._load_name_with_category(names, name, 'subresource',
+                                              snake_case=False)
+            else:
+                self._load_name_with_category(names, name, 'reference')
+
+        for name in self._definition.get('hasMany', {}).keys():
+            self._load_name_with_category(names, name, 'collection')
+
+        if shape is not None:
+            for name in shape.members.keys():
+                self._load_name_with_category(names, name, 'attribute')
+
+    def _load_name_with_category(self, names, name, category,
+                                 snake_case=True):
+        """
+        Load a name with a given category, possibly renaming it
+        if that name is already in use. The name will be stored
+        in ``names`` and possibly be set up in ``self._renamed``.
+
+        :type names: set
+        :param names: Existing names (Python attributes, properties, or
+                      methods) on the resource.
+        :type name: string
+        :param name: The original name of the value.
+        :type category: string
+        :param category: The value type, such as 'identifier' or 'action'
+        :type snake_case: bool
+        :param snake_case: True (default) if the name should be snake cased.
+        """
+        if snake_case:
+            name = xform_name(name)
+
+        if name in names:
+            logger.debug('Renaming %s %s %s' % (self.name, category, name))
+            self._renamed[(category, name)] = name + '_' + category
+            name += '_' + category
+
+            if name in names:
+                # This isn't good, let's raise instead of trying to keep
+                # renaming this value.
+                raise ValueError('Problem renaming {0} {1} to {2}!'.format(
+                    self.name, category, name))
+
+        names.add(name)
+
+    def _get_name(self, category, name, snake_case=True):
+        """
+        Get a possibly renamed value given a category and name. This
+        uses the rename map set up in ``load_rename_map``, so that
+        method must be called once first.
+
+        :type category: string
+        :param category: The value type, such as 'identifier' or 'action'
+        :type name: string
+        :param name: The original name of the value
+        :type snake_case: bool
+        :param snake_case: True (default) if the name should be snake cased.
+        :rtype: string
+        :return: Either the renamed value if it is set, otherwise the
+                 original name.
+        """
+        if snake_case:
+            name = xform_name(name)
+
+        return self._renamed.get((category, name), name)
+
+    def get_attributes(self, shape):
+        """
+        Get a dictionary of attribute names to shape models that
+        represent the attributes of this resource.
+
+        :type shape: botocore.model.Shape
+        :param shape: The underlying shape for this resource.
+        :rtype: dict
+        :return: Mapping of resource attributes.
+        """
+        attributes = {}
+        identifier_names = [i.name for i in self.identifiers]
+
+        for name, member in shape.members.items():
+            snake_cased = xform_name(name)
+            if snake_cased in identifier_names:
+                # Skip identifiers, these are set through other means
+                continue
+            snake_cased = self._get_name('attribute', snake_cased,
+                                         snake_case=False)
+            attributes[snake_cased] = member
+
+        return attributes
+
     @property
     def identifiers(self):
         """
@@ -266,7 +418,8 @@ def identifiers(self):
         identifiers = []
 
         for item in self._definition.get('identifiers', []):
-            identifiers.append(Identifier(item['name']))
+            name = self._get_name('identifier', item['name'])
+            identifiers.append(Identifier(name))
 
         return identifiers
 
@@ -294,6 +447,7 @@ def actions(self):
         actions = []
 
         for name, item in self._definition.get('actions', {}).items():
+            name = self._get_name('action', name)
             actions.append(Action(name, item, self._resource_defs))
 
         return actions
@@ -308,6 +462,7 @@ def batch_actions(self):
         actions = []
 
         for name, item in self._definition.get('batchActions', {}).items():
+            name = self._get_name('batch_action', name)
             actions.append(Action(name, item, self._resource_defs))
 
         return actions
@@ -387,6 +542,10 @@ def _get_related_resources(self, subresources):
         resources = []
 
         for name, definition in self._get_has_definition().items():
+            if subresources:
+                name = self._get_name('subresource', name, snake_case=False)
+            else:
+                name = self._get_name('reference', name)
             action = Action(name, definition, self._resource_defs)
 
             data_required = False
@@ -430,6 +589,7 @@ def collections(self):
         collections = []
 
         for name, item in self._definition.get('hasMany', {}).items():
+            name = self._get_name('collection', name)
             collections.append(Collection(name, item, self._resource_defs))
 
         return collections