Skip to content

Commit e8780f8

Browse files
author
Konstantinos Bairaktaris
committed
(jsonapi) Fix mutability issue with collections
Here is the problem: Lets assume you have a paginated collection: ```python organization = transifex_api.Organization.get(...) first_page = organization.fetch("projects") first_page._params # <<< {} for project in first_page: do_something_with(project) ``` Now lets suppose that you get the next page: ```python next_page = first_page.next() for project in next_page: do_something_with(project) ``` Now, lets inspect the params of the first page: ```python first_page._params # <<< {'page[cursor]': "..."} ``` Here is the problem: `next` does this ```python def next(self): return self.__class__(self.API, self.next_url, self._params) ``` `self.__class__()` will call `__init__` which will do: ```python def __init__(self, API, url, params=None): ... query_params = ... params.update(query_params) ... ``` There are two problems here: - The first one is that the `params.update(...)` call will modify the params variable which is a reference to the `._params` attribute of the **first** page. This explains the bug. We should make sure that the initializer modifies a copy of the parameters to protect from this issue. - The second problem is that the first page's `.next()` call has no business providing (a copy of) its parameters to the initializer; it's the job of the server to include any parameters in the pagination URL itself. This can also occur if you use `.all()` or `.all_pages()` (and if there are more than one pages) before using the parameters of the first page again, for example if you want to apply a filter to the collection.
1 parent 60cd3a3 commit e8780f8

1 file changed

Lines changed: 9 additions & 12 deletions

File tree

transifex/api/jsonapi/collections.py

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ class Collection(abc.MutableSequence):
88
def __init__(self, API, url, params=None):
99
if params is None:
1010
params = {}
11+
else:
12+
params = dict(params)
1113

1214
parsed = urlparse(url)
1315
path, query = parsed.path, parsed.query
@@ -55,8 +57,7 @@ def _evaluate(self, response_body=None):
5557
return
5658

5759
if response_body is None:
58-
response_body = self.API.request(
59-
"get", self._url, params=self._params)
60+
response_body = self.API.request("get", self._url, params=self._params)
6061
included = {}
6162
if "included" in response_body:
6263
included = {
@@ -69,14 +70,12 @@ def _evaluate(self, response_body=None):
6970
for (name, relationship) in item.get("relationships", {}).items():
7071
if relationship is None or "data" not in relationship:
7172
continue
72-
key = (relationship["data"]["type"],
73-
relationship["data"]["id"])
73+
key = (relationship["data"]["type"], relationship["data"]["id"])
7474
if key in included:
7575
related[name] = self.API.new(included[key])
7676
relationships = item.pop("relationships", {})
7777
relationships.update(related)
78-
self._data.append(self.API.new(
79-
relationships=relationships, **item))
78+
self._data.append(self.API.new(relationships=relationships, **item))
8079

8180
self._next_url = response_body.get("links", {}).get("next")
8281
self._previous_url = response_body.get("links", {}).get("previous")
@@ -104,8 +103,7 @@ def to_dict(self):
104103
self_url = self._url
105104
if self._params:
106105
self_url += "?" + "&".join(
107-
("{}={}".format(key, value)
108-
for key, value in self._params.items())
106+
("{}={}".format(key, value) for key, value in self._params.items())
109107
)
110108

111109
links = {"self": self_url}
@@ -125,13 +123,13 @@ def has_next(self):
125123
return bool(self.next_url)
126124

127125
def next(self):
128-
return self.__class__(self.API, self.next_url, self._params)
126+
return self.__class__(self.API, self.next_url)
129127

130128
def has_previous(self):
131129
return bool(self.previous_url)
132130

133131
def previous(self):
134-
return self.__class__(self.API, self.previous_url, self._params)
132+
return self.__class__(self.API, self.previous_url)
135133

136134
def all_pages(self):
137135
if self.data:
@@ -153,8 +151,7 @@ def filter(self, **filters):
153151
params = dict(self._params)
154152

155153
for key, value in filters.items():
156-
key = "filter" + "".join(("[{}]".format(part)
157-
for part in key.split("__")))
154+
key = "filter" + "".join(("[{}]".format(part) for part in key.split("__")))
158155
if isinstance(value, Resource):
159156
value = value.id
160157

0 commit comments

Comments
 (0)