Merge pull request #154 from kyleknap/merge-dicts-collections

Fix filter method from clobering identifier params

boto3/resources/collection.py

@@ -15,6 +15,7 @@
 import logging
 
 from botocore import xform_name
+from botocore.utils import merge_dicts
 
 from .action import BatchAction
 from .params import create_request_parameters
@@ -106,7 +107,7 @@ def _clone(self, **kwargs):
         :return: A clone of this resource collection
         """
         params = copy.deepcopy(self._params)
-        params.update(kwargs)
+        merge_dicts(params, kwargs, append_lists=True)
         clone = self.__class__(self._model, self._parent,
                                self._handler, **params)
         return clone
@@ -135,10 +136,9 @@ def pages(self):
         cleaned_params = self._params.copy()
         limit = cleaned_params.pop('limit', None)
         page_size = cleaned_params.pop('page_size', None)
-
         params = create_request_parameters(
             self._parent, self._model.request)
-        params.update(cleaned_params)
+        merge_dicts(params, cleaned_params, append_lists=True)
 
         # Is this a paginated operation? If so, we need to get an
         # iterator for the various pages. If not, then we simply

tests/unit/resources/test_collection.py

@@ -480,6 +480,43 @@ def test_filters_paginated(self, handler):
             PaginationConfig={'PageSize': None, 'MaxItems': 2},
             Param1='foo', Param2=3)
 
+    @mock.patch('boto3.resources.collection.ResourceHandler')
+    def test_filter_does_not_clobber_existing_list_values(self, handler):
+        self.collection_def = {
+            'request': {
+                'operation': 'GetFrobs',
+                "params": [
+                    {"target": "Filters[0].Name", "source": "string",
+                     "value": "frob-id"},
+                    {"target": "Filters[0].Values[0]", "source": "identifier",
+                     "name": "Id"}
+                ]
+            },
+            'resource': {
+                'type': 'Frob',
+                'identifiers': [
+                    {'target': 'Id', 'source': 'response',
+                     'path': 'Frobs[].Id'}
+                ]
+            }
+        }
+        self.client.can_paginate.return_value = True
+        self.client.get_paginator.return_value.paginate.return_value = []
+        handler.return_value.return_value = []
+        collection = self.get_collection()
+
+        self.parent.id = 'my-id'
+        list(collection.filter(
+            Filters=[{'Name': 'another-filter', 'Values': ['foo']}]))
+        paginator = self.client.get_paginator.return_value
+        paginator.paginate.assert_called_with(
+            PaginationConfig={'PageSize': None, 'MaxItems': None},
+            Filters=[
+                {'Values': ['my-id'], 'Name': 'frob-id'},
+                {'Values': ['foo'], 'Name': 'another-filter'}
+            ]
+        )
+
     @mock.patch('boto3.resources.collection.ResourceHandler')
     def test_page_size_param(self, handler):
         self.client.can_paginate.return_value = True
@@ -553,6 +590,46 @@ def test_chaining_copies_parameters(self, handler):
         paginator.paginate.assert_called_with(
             PaginationConfig={'PageSize': 3, 'MaxItems': 3}, CustomArg=1)
 
+    @mock.patch('boto3.resources.collection.ResourceHandler')
+    def test_chaining_filters_does_not_clobber_list_values(self, handler):
+        self.collection_def = {
+            'request': {
+                'operation': 'GetFrobs',
+                "params": [
+                    {"target": "Filters[0].Name", "source": "string",
+                     "value": "frob-id"},
+                    {"target": "Filters[0].Values[0]", "source": "identifier",
+                     "name": "Id"}
+                ]
+            },
+            'resource': {
+                'type': 'Frob',
+                'identifiers': [
+                    {'target': 'Id', 'source': 'response',
+                     'path': 'Frobs[].Id'}
+                ]
+            }
+        }
+        self.client.can_paginate.return_value = True
+        self.client.get_paginator.return_value.paginate.return_value = []
+        handler.return_value.return_value = []
+        collection = self.get_collection()
+
+        self.parent.id = 'my-id'
+        collection = collection.filter(
+            Filters=[{'Name': 'second-filter', 'Values': ['foo']}])
+        list(collection.filter(
+            Filters=[{'Name': 'third-filter', 'Values': ['bar']}]))
+        paginator = self.client.get_paginator.return_value
+        paginator.paginate.assert_called_with(
+            PaginationConfig={'PageSize': None, 'MaxItems': None},
+            Filters=[
+                {'Values': ['my-id'], 'Name': 'frob-id'},
+                {'Values': ['foo'], 'Name': 'second-filter'},
+                {'Values': ['bar'], 'Name': 'third-filter'}
+            ]
+        )
+
     def test_chained_repr(self):
         collection = self.get_collection()