Skip to content

Commit a7a0487

Browse files
JohnVillalovosJohnVillalovos
committed
fix: add a check to ensure the MRO is correct
Add a check to ensure the MRO (Method Resolution Order) is correct for classes in gitlab.v4.objects when doing type-checking. An example of an incorrect definition: class ProjectPipeline(RESTObject, RefreshMixin, ObjectDeleteMixin): ^^^^^^^^^^ This should be at the end. Correct way would be: class ProjectPipeline(RefreshMixin, ObjectDeleteMixin, RESTObject): Correctly at the end ^^^^^^^^^^ Also fix classes which have the issue.
1 parent 96d2805 commit a7a0487

File tree

6 files changed

+137
-5
lines changed

6 files changed

+137
-5
lines changed

‎gitlab/tests/objects/test_mro.py

+132
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
"""
2+
Ensure objects defined in gitlab.v4.objects have REST* as last item in class
3+
definition
4+
"""
5+
import inspect
6+
7+
import pytest
8+
9+
import gitlab.v4.objects
10+
11+
# Original notes by John L. Villalovos
12+
#
13+
# An example of an incorrect definition:
14+
# class ProjectPipeline(RESTObject, RefreshMixin, ObjectDeleteMixin):
15+
# ^^^^^^^^^^ This should be at the end.
16+
#
17+
# Correct way would be:
18+
# class ProjectPipeline(RefreshMixin, ObjectDeleteMixin, RESTObject):
19+
# Correctly at the end ^^^^^^^^^^
20+
#
21+
#
22+
# Why this is an issue:
23+
#
24+
# When we do type-checking for gitlab/mixins.py we make RESTObject or
25+
# RESTManager the base class for the mixins
26+
#
27+
# Here is how our classes look when type-checking:
28+
#
29+
# class RESTObject(object):
30+
# def __init__(self, manager: "RESTManager", attrs: Dict[str, Any]) -> None:
31+
# ...
32+
#
33+
# class Mixin(RESTObject):
34+
# ...
35+
#
36+
# # Wrong ordering here
37+
# class Somev4Object(RESTObject, RefreshMixin):
38+
# ...
39+
#
40+
# If we actually ran this in Python we would get the following error:
41+
# class Somev4Object(RESTObject, Mixin):
42+
# TypeError: Cannot create a consistent method resolution
43+
# order (MRO) for bases RESTObject, Mixin
44+
#
45+
# When we are type-checking it fails to understand the class Somev4Object
46+
# and thus we can't type check it correctly.
47+
#
48+
# Almost all classes in gitlab/v4/objects/*py were already correct before this
49+
# check was added.
50+
51+
52+
def test_show_issue():
53+
"""Test case to demonstrate the TypeError that occurs"""
54+
55+
class RESTObject(object):
56+
def __init__(self, manager: str, attrs: int) -> None:
57+
...
58+
59+
class Mixin(RESTObject):
60+
...
61+
62+
with pytest.raises(TypeError) as exc_info:
63+
# Wrong ordering here
64+
class Somev4Object(RESTObject, Mixin):
65+
...
66+
67+
# The error message in the exception should be:
68+
# TypeError: Cannot create a consistent method resolution
69+
# order (MRO) for bases RESTObject, Mixin
70+
71+
# Make sure the exception string contains "MRO"
72+
assert "MRO" in exc_info.exconly()
73+
74+
# Correctly ordered class, no exception
75+
class Somev4Object(Mixin, RESTObject):
76+
...
77+
78+
79+
def test_mros():
80+
"""Ensure objects defined in gitlab.v4.objects have REST* as last item in
81+
class definition.
82+
83+
We do this as we need to ensure the MRO (Method Resolution Order) is
84+
correct.
85+
"""
86+
87+
debug = False
88+
failed_messages = []
89+
for module_name, module_value in inspect.getmembers(gitlab.v4.objects):
90+
if not inspect.ismodule(module_value):
91+
# We only care about the modules
92+
continue
93+
if debug:
94+
print("Module:", module_name, type(module_value), module_value)
95+
# Iterate through all the classes in our module
96+
for class_name, class_value in inspect.getmembers(module_value):
97+
if not inspect.isclass(class_value):
98+
continue
99+
100+
# Ignore imported classes from gitlab.base
101+
if class_value.__module__ == "gitlab.base":
102+
continue
103+
104+
if debug:
105+
print("\tClass:", class_name, type(class_value), class_value)
106+
mro = class_value.mro()
107+
108+
# We only check classes which have a 'gitlab.base' class in their MRO
109+
has_base = False
110+
for count, obj in enumerate(mro, start=1):
111+
if debug:
112+
print("\t\tMRO:", count, obj)
113+
if obj.__module__ == "gitlab.base":
114+
has_base = True
115+
base_classname = obj.__name__
116+
# print(f"\t{obj.__name__}: {obj.__module__}")
117+
if has_base:
118+
filename = inspect.getfile(class_value)
119+
# NOTE(jlvillal): The very last item 'mro[-1]' is always going
120+
# to be 'object'. That is why we are checking 'mro[-2]'.
121+
if mro[-2].__module__ != "gitlab.base":
122+
failed_messages.append(
123+
(
124+
f"class definition for {class_name!r} in file {filename!r} "
125+
f"must have {base_classname!r} as the last class in the "
126+
f"class definition"
127+
)
128+
)
129+
failed_msg = "\n".join(failed_messages)
130+
if failed_messages and debug:
131+
print(failed_msg)
132+
assert not failed_messages, failed_msg

‎gitlab/v4/objects/commits.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ class ProjectCommitCommentManager(ListMixin, CreateMixin, RESTManager):
157157
_create_attrs = (("note",), ("path", "line", "line_type"))
158158

159159

160-
class ProjectCommitStatus(RESTObject, RefreshMixin):
160+
class ProjectCommitStatus(RefreshMixin, RESTObject):
161161
pass
162162

163163

‎gitlab/v4/objects/deployments.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
]
99

1010

11-
class ProjectDeployment(RESTObject, SaveMixin):
11+
class ProjectDeployment(SaveMixin, RESTObject):
1212
pass
1313

1414

‎gitlab/v4/objects/jobs.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
]
1111

1212

13-
class ProjectJob(RESTObject, RefreshMixin):
13+
class ProjectJob(RefreshMixin, RESTObject):
1414
@cli.register_custom_action("ProjectJob")
1515
@exc.on_http_error(exc.GitlabJobCancelError)
1616
def cancel(self, **kwargs):

‎gitlab/v4/objects/pipelines.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
]
3131

3232

33-
class ProjectPipeline(RESTObject, RefreshMixin, ObjectDeleteMixin):
33+
class ProjectPipeline(RefreshMixin, ObjectDeleteMixin, RESTObject):
3434
_managers = (
3535
("jobs", "ProjectPipelineJobManager"),
3636
("bridges", "ProjectPipelineBridgeManager"),

‎gitlab/v4/objects/releases.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ class ProjectReleaseManager(NoUpdateMixin, RESTManager):
2424
_create_attrs = (("name", "tag_name", "description"), ("ref", "assets"))
2525

2626

27-
class ProjectReleaseLink(RESTObject, ObjectDeleteMixin, SaveMixin):
27+
class ProjectReleaseLink(ObjectDeleteMixin, SaveMixin, RESTObject):
2828
pass
2929

3030

0 commit comments

Comments
 (0)