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

Commit 63a6f1e

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 aa32ea5 commit 63a6f1e

File tree

4 files changed

+194
-7
lines changed

4 files changed

+194
-7
lines changed

‎gitlab/client.py

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,15 @@
1818

1919
import os
2020
import time
21+
import traceback
22+
import warnings
2123
from typing import Any, cast, Dict, List, Optional, Tuple, TYPE_CHECKING, Union
2224

2325
import requests
2426
import requests.utils
2527
from requests_toolbelt.multipart.encoder import MultipartEncoder # type: ignore
2628

29+
import gitlab
2730
import gitlab.config
2831
import gitlab.const
2932
import gitlab.exceptions
@@ -35,6 +38,12 @@
3538
"{source!r} to {target!r}"
3639
)
3740

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

3948
class Gitlab:
4049
"""Represents a GitLab server connection.
@@ -808,7 +817,7 @@ def http_list(
808817
# In case we want to change the default behavior at some point
809818
as_list = True if as_list is None else as_list
810819

811-
get_all = kwargs.pop("all", False)
820+
get_all = kwargs.pop("all", None)
812821
url = self._build_url(path)
813822

814823
page = kwargs.get("page")
@@ -818,7 +827,39 @@ def http_list(
818827

819828
if page or as_list is True:
820829
# pagination requested, we return a list
821-
return list(GitlabList(self, url, query_data, get_next=False, **kwargs))
830+
gl_list = GitlabList(self, url, query_data, get_next=False, **kwargs)
831+
items = list(gl_list)
832+
if (
833+
page is None
834+
and get_all is None
835+
and (gl_list.per_page is not None and len(items) >= gl_list.per_page)
836+
and (gl_list.total is None or len(items) < gl_list.total)
837+
):
838+
# Get `stacklevel` for user code so we indicate where issue is in their
839+
# code.
840+
pg_dir = os.path.abspath(os.path.dirname(__file__))
841+
stack = traceback.extract_stack()
842+
for stacklevel, frame in enumerate(reversed(stack), start=1):
843+
frame_dir = os.path.abspath(os.path.dirname(frame.filename))
844+
if not frame_dir.startswith(pg_dir):
845+
break
846+
847+
total_items = "10,000+" if gl_list.total is None else gl_list.total
848+
# Warn the user that they are only going to retrieve `per_page` maximum
849+
# items. This is a common cause of issues filed.
850+
warnings.warn(
851+
(
852+
f"Calling a `list()` method without specifying `all=True` or "
853+
f"`as_list=False` will return a maximum of {gl_list.per_page} "
854+
f"items. Your query returned {len(items)} of {total_items} "
855+
f"items. See {_PAGINATION_URL} for more details. If this was "
856+
A3E2 f"done intentionally, then this warning can be supressed by "
857+
f"adding the argument `all=False` to the `list()` call."
858+
),
859+
category=UserWarning,
860+
stacklevel=stacklevel,
861+
)
862+
return items
822863

823864
# No pagination, generator requested
824865
return GitlabList(self, url, query_data, **kwargs)

‎tests/functional/api/test_gitlab.py

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

35
import gitlab
@@ -81,13 +83,13 @@ def test_template_dockerfile(gl):
8183

8284

8385
def test_template_gitignore(gl):
84-
assert gl.gitignores.list()
86+
assert gl.gitignores.list(all=True)
8587
gitignore = gl.gitignores.get("Node")
8688
assert gitignore.content is not None
8789

8890

8991
def test_template_gitlabciyml(gl):
90-
assert gl.gitlabciymls.list()
92+
assert gl.gitlabciymls.list(all=True)
9193
gitlabciyml = gl.gitlabciymls.get("Nodejs")
9294
assert gitlabciyml.content is not None
9395

@@ -181,3 +183,48 @@ 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(caught_warnings[0].message)
197+
assert "Calling" in message
198+
assert "return a maximum of" in message
199+
assert "readthedocs" in message
200+
assert __file__ == warning.filename
201+
202+
203+
def test_list_page_nowarning(gl):
204+
"""Using `page=X` will disable the warning"""
205+
with warnings.catch_warnings(record=True) as caught_warnings:
206+
gl.gitlabciymls.list(page=1)
207+
assert len(caught_warnings) == 0
208+
209+
210+
def test_list_all_false_nowarning(gl):
211+
"""Using `all=False` will disable the warning"""
212+
with warnings.catch_warnings(record=True) as caught_warnings:
213+
gl.gitlabciymls.list(all=False)
214+
assert len(caught_warnings) == 0
215+
216+
217+
def test_list_all_true_nowarning(gl):
218+
"""Using `all=True` will disable the warning"""
219+
with warnings.catch_warnings(record=True) as caught_warnings:
220+
items = gl.gitlabciymls.list(all=True)
221+
assert len(caught_warnings) == 0
222+
assert len(items) > 20
223+
224+
225+
def test_list_as_list_false_nowarning(gl):
226+
"""Using `as_list=False` will disable the warning"""
227+
with warnings.catch_warnings(record=True) as caught_warnings:
228+
items = gl.gitlabciymls.list(as_list=False)
229+
assert len(caught_warnings) == 0
230+
assert len(list(items)) > 20

‎tests/functional/fixtures/docker-compose.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ services:
1414
GITLAB_ROOT_PASSWORD: 5iveL!fe
1515
GITLAB_SHARED_RUNNERS_REGISTRATION_TOKEN: registration-token
1616
GITLAB_OMNIBUS_CONFIG: |
17-
external_url 'http://127.0.0.1:8080'
17+
external_url 'http://localhost:8080'
1818
registry['enable'] = false
1919
nginx['redirect_http_to_https'] = false
2020
nginx['listen_port'] = 80

‎tests/unit/test_gitlab_http_methods.py

Lines changed: 101 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,116 @@ 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(caught_warnings[0].message)
392+
assert "Calling" in message
393+
assert "return a maximum of" in message
394+
assert "readthedocs" in message
395+
assert __file__ == warning.filename
396+
assert isinstance(result, list)
397+
assert len(result) == 20
398+
assert len(responses.calls) == 1
399+
400+
401+
@responses.activate
402+
def test_list_request_as_list_false_nowarning(gl):
403+
responses.add(**large_list_response)
404+
with warnings.catch_warnings(record=True) as caught_warnings:
405+
result = gl.http_list("/projects", as_list=False)
406+
assert len(caught_warnings) == 0
407+
assert isinstance(result, GitlabList)
408+
assert len(list(result)) == 20
409+
assert len(responses.calls) == 1
410+
411+
412+
@responses.activate
413+
def test_list_request_all_true_nowarning(gl):
414+
responses.add(**large_list_response)
415+
with warnings.catch_warnings(record=True) as caught_warnings:
416+
result = gl.http_list("/projects", all=True)
417+
assert len(caught_warnings) == 0
418+
assert isinstance(result, list)
419+
assert len(result) == 20
420+
assert len(responses.calls) == 1
421+
422+
423+
@responses.activate
424+
def test_list_request_all_false_nowarning(gl):
425+
responses.add(**large_list_response)
426+
with warnings.catch_warnings(record=True) as caught_warnings:
427+
result = gl.http_list("/projects", all=False)
428+
assert len(caught_warnings) == 0
429+
assert isinstance(result, list)
430+
assert len(result) == 20
431+
assert len(responses.calls) == 1
432+
433+
434+
@responses.activate
435+
def test_list_request_page_nowarning(gl):
436+
response_dict = copy.deepcopy(large_list_response)
437+
response_dict["match"] = [responses.matchers.query_param_matcher({"page": "1"})]
438+
responses.add(**response_dict)
439+
with warnings.catch_warnings(record=True) as caught_warnings:
440+
gl.http_list("/projects", page=1)
441+
assert len(caught_warnings) == 0
442+
assert len(responses.calls) == 1
443+
444+
346445
@responses.activate
347446
def test_list_request_404(gl):
348447
url = "http://localhost/api/v4/not_there"

0 commit comments

Comments
 (0)
0