8000 feat: emit a warning when using a `list()` method returns max · python-gitlab/python-gitlab@f99cf74 · GitHub
[go: up one dir, main page]
Skip to content

Commit f99cf74

Browse files
JohnVillalovosJohnVillalovos
committed
feat: emit a warning when using a list() method returns max
A common cause of issues filed and questions raised is that a user will call a `list()` method and only get 20 items. As this is the default maximum of items that will be returned from a `list()` method. To help with this we now emit a warning when the result from a `list()` method is greater-than or equal to 20 (or the specified `per_page` value) and the user is not using either `all=True`, `all=False`, `as_list=False`, or `page=X`.
1 parent 9fe60f7 commit f99cf74

File tree

3 files changed

+195
-10
lines changed

3 files changed

+195
-10
lines changed

‎gitlab/client.py

Lines changed: 50 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import requests.utils
2525
from requests_toolbelt.multipart.encoder import MultipartEncoder # type: ignore
2626

27+
import gitlab
2728
import gitlab.config
2829
import gitlab.const
2930
import gitlab.exceptions
@@ -35,6 +36,12 @@
3536
"{source!r} to {target!r}"
3637
)
3738

39+
# https://docs.gitlab.com/ee/api/#offset-based-pagination
40+
_PAGINATION_URL = (
41+
f"https://python-gitlab.readthedocs.io/en/v{gitlab.__version__}/"
42+
f"api-usage.html#pagination"
43+
)
44+
3845

3946
class Gitlab:
4047
"""Represents a GitLab server connection.
@@ -807,25 +814,60 @@ def http_list(
807814
GitlabHttpError: When the return code is not 2xx
808815
GitlabParsingError: If the json data could not be parsed
809816
"""
817+
# pylint: disable=too-many-return-statements
810818
query_data = query_data or {}
811819

812820
# In case we want to change the default behavior at some point
813821
as_list = True if as_list is None else as_list
814822

815-
get_all = kwargs.pop("all", False)
823+
get_all = kwargs.pop("all", None)
816824
url = self._build_url(path)
817825

818826
page = kwargs.get("page")
819827

820-
if get_all is True and as_list is True:
821-
return list(GitlabList(self, url, query_data, **kwargs))
828+
if as_list is False:
829+
# Generator requested
830+
return GitlabList(self, url, query_data, **kwargs)
822831

823-
if page or as_list is True:
824-
# pagination requested, we return a list
825-
return list(GitlabList(self, url, query_data, get_next=False, **kwargs))
832+
if get_all is True:
833+
return list(GitlabList(self, url, query_data, **kwargs))
826834

827-
# No pagination, generator requested
828-
return GitlabList(self, url, query_data, **kwargs)
835+
# pagination requested, we return a list
836+
gl_list = GitlabList(self, url, query_data, get_next=False, **kwargs)
837+
items = list(gl_list)
838+
839+
# No warning is emitted if any of the following conditions apply:
840+
# * `all=False` was set in the `list()` call.
841+
# * `page` was set in the `list()` call.
842+
# * GitLab did not return the `x-per-page` header.
843+
# * Number of items received is less than per-page value.
844+
# * Number of items received is >= total available.
845+
if get_all is False:
846+
return items
847+
if page is not None:
848+
return items
849+
if gl_list.per_page is None:
850+
return items
851+
if len(items) < gl_list.per_page:
852+
return items
853+
if gl_list.total is not None and len(items) >= gl_list.total:
854+
return items
855+
856+
# Warn the user that they are only going to retrieve `per_page`
857+
# maximum items. This is a common cause of issues filed.
858+
total_items = "many" if gl_list.total is None else gl_list.total
859+
utils.warn(
860+
message=(
861+
f"Calling a `list()` method without specifying `all=True` or "
862+
f"`as_list=False` will return a maximum of {gl_list.per_page} items. "
863+
f"Your query returned {len(items)} of {total_items} items. See "
864+
f"{_PAGINATION_URL} for more details. If this was done intentionally, "
865+
f"then this warning can be supressed by adding the argument "
866+
f"`all=False` to the `list()` call."
867+
),
868+
category=UserWarning,
869+
)
870+
return items
829871

830872
def http_post(
831873
self,

‎tests/functional/api/test_gitlab.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import warnings
2+
13
import pytest
24

35
import gitlab
@@ -181,3 +183,46 @@ def test_rate_limits(gl):
181183
settings.throttle_authenticated_api_enabled = False
182184
settings.save()
183185
[project.delete() for project in projects]
186+
187+
188+
def test_list_default_warning(gl):
189+
"""When there are more than 20 items and use default `list()` then warning is
190+
generated"""
191+
with warnings.catch_warnings(record=True) as caught_warnings:
192+
gl.gitlabciymls.list()
193+
assert len(caught_warnings) == 1
194+
warning = caught_warnings[0]
195+
assert isinstance(warning.message, UserWarning)
196+
message = str(warning.message)
197+
assert "python-gitlab.readthedocs.io" in message
198+
assert __file__ == warning.filename
199+
200+
201+
def test_list_page_nowarning(gl):
202+
"""Using `page=X` will disable the warning"""
203+
with warnings.catch_warnings(record=True) as caught_warnings:
204+
gl.gitlabciymls.list(page=1)
205+
assert len(caught_warnings) == 0
206+
207+
208+
def test_list_all_false_nowarning(gl):
209+
"""Using `all=False` will disable the warning"""
210+
with warnings.catch_warnings(record=True) as caught_warnings:
211+
gl.gitlabciymls.list(all=False)
212+
assert len(caught_warnings) == 0
213+
214+
215+
def test_list_all_true_nowarning(gl):
216+
"""Using `all=True` will disable the warning"""
217+
with warnings.catch_warnings(record=True) as caught_warnings:
218+
items = gl.gitlabciymls.list(all=True)
219+
assert len(caught_warnings) == 0
220+
assert len(items) > 20
221+
222+
223+
def test_list_as_list_false_nowarning(gl):
224+
"""Using `as_list=False` will disable the warning"""
225+
with warnings.catch_warnings(record=True) as caught_warnings:
226+
items = gl.gitlabciymls.list(as_list=False)
227+
assert len(caught_warnings) == 0
228+
assert len(list(items)) > 20

‎tests/unit/test_gitlab_http_methods.py

Lines changed: 100 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
import copy
2+
import warnings
3+
14
import pytest
25
import requests
36
import responses
@@ -329,20 +332,115 @@ def test_list_request(gl):
329332
match=MATCH_EMPTY_QUERY_PARAMS,
330333
)
331334

332-
result = gl.http_list("/projects", as_list=True)
335+
with warnings.catch_warnings(record=True) as caught_warnings:
336+
result = gl.http_list("/projects", as_list=True)
337+
assert len(caught_warnings) == 0
333338
assert isinstance(result, list)
334339
assert len(result) == 1
335340

336341
result = gl.http_list("/projects", as_list=False)
337342
assert isinstance(result, GitlabList)
338-
assert len(result) == 1
343+
assert len(list(result)) == 1
339344

340345
result = gl.http_list("/projects", all=True)
341346
assert isinstance(result, list)
342347
assert len(result) == 1
343348
assert responses.assert_call_count(url, 3) is True
344349

345350

351+
large_list_response = {
352+
"method": responses.GET,
353+
"url": "http://localhost/api/v4/projects",
354+
"json": [
355+
{"name": "project01"},
356+
{"name": "project02"},
357+
{"name": "project03"},
358+
{"name": "project04"},
359+
{"name": "project05"},
360+
{"name": "project06"},
361+
{"name": "project07"},
362+
{"name": "project08"},
363+
{"name": "project09"},
364+
{"name": "project10"},
365+
{"name": "project11"},
366+
{"name": "project12"},
367+
{"name": "project13"},
368+
{"name": "project14"},
369+
{"name": "project15"},
370+
{"name": "project16"},
371+
{"name": "project17"},
372+
{"name": "project18"},
373+
{"name": "project19"},
374+
{"name": "project20"},
375+
],
376+
"headers": {"X-Total": "30", "x-per-page": "20"},
377+
"status": 200,
378+
"match": MATCH_EMPTY_QUERY_PARAMS,
379+
}
380+
381+
382+
@responses.activate
383+
def test_list_request_pagination_warning(gl):
384+
responses.add(**large_list_response)
385+
386+
with warnings.catch_warnings(record=True) as caught_warnings:
387+
result = gl.http_list("/projects", as_list=True)
388+
assert len(caught_warnings) == 1
389+
warning = caught_warnings[0]
390+
assert isinstance(warning.message, UserWarning)
391+
message = str(warning.message)
392+
assert "Calling a `list()` method" in message
393+
assert "python-gitlab.readthedocs.io" in message
394+
assert __file__ == warning.filename
395+
assert isinstance(result, list)
396+
assert len(result) == 20
397+
assert len(responses.calls) == 1
398+
399+
400+
@responses.activate
401+
def test_list_request_as_list_false_nowarning(gl):
402+
responses.add(**large_list_response)
403+
with warnings.catch_warnings(record=True) as caught_warnings:
404+
result = gl.http_list("/projects", as_list=False)
405+
assert len(caught_warnings) == 0
406+
assert isinstance(result, GitlabList)
407+
assert len(list(result)) == 20
408+
assert len(responses.calls) == 1
409+
410+
411+
@responses.activate
412+
def test_list_request_all_true_nowarning(gl):
413+
responses.add(**large_list_response)
414+
with warnings.catch_warnings(record=True) as caught_warnings:
415+
result = gl.http_list("/projects", all=True)
416+
assert len(caught_warnings) == 0
417+
assert isinstance(result, list)
418+
assert len(result) == 20
419+
assert len(responses.calls) == 1
420+
421+
422+
@responses.activate
423+
def test_list_request_all_false_nowarning(gl):
424+
responses.add(**large_list_response)
425+
with warnings.catch_warnings(record=True) as caught_warnings:
426+
result = gl.http_list("/projects", all=False)
427+
assert len(caught_warnings) == 0
428+
assert isinstance(result, list)
429+
assert len(result) == 20
430+
assert len(responses.calls) == 1
431+
432+
433+
@responses.activate
434+
def test_list_request_page_nowarning(gl):
435+
response_dict = copy.deepcopy(large_list_response)
436+
response_dict["match"] = [responses.matchers.query_param_matcher({"page": "1"})]
437+
responses.add(**response_dict)
438+
with warnings.catch_warnings(record=True) as caught_warnings:
439+
gl.http_list("/projects", page=1)
440+
assert len(caught_warnings) == 0
441+
assert len(responses.calls) == 1
442+
443+
346444
@responses.activate
347445
def test_list_request_404(gl):
348446
url = "http://localhost/api/v4/not_there"

0 commit comments

Comments
 (0)
0