Skip to content

Commit 909aa9a

Browse files
nejchnejch
authored
Merge pull request #1352 from JohnVillalovos/jlvillal/fix_mro
fix: add a check to ensure the MRO is correct
2 parents dde01c7 + 0357c37 commit 909aa9a

File tree

6 files changed

+127
-5
lines changed

6 files changed

+127
-5
lines changed

‎gitlab/tests/objects/test_mro.py

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
"""
2+
Ensure objects defined in gitlab.v4.objects have REST* as last item in class
3+
definition
4+
5+
Original notes by John L. Villalovos
6+
7+
An example of an incorrect definition:
8+
class ProjectPipeline(RESTObject, RefreshMixin, ObjectDeleteMixin):
9+
^^^^^^^^^^ This should be at the end.
10+
11+
Correct way would be:
12+
class ProjectPipeline(RefreshMixin, ObjectDeleteMixin, RESTObject):
13+
Correctly at the end ^^^^^^^^^^
14+
15+
16+
Why this is an issue:
17+
18+
When we do type-checking for gitlab/mixins.py we make RESTObject or
19+
RESTManager the base class for the mixins
20+
21+
Here is how our classes look when type-checking:
22+
23+
class RESTObject(object):
24+
def __init__(self, manager: "RESTManager", attrs: Dict[str, Any]) -> None:
25+
...
26+
27+
class Mixin(RESTObject):
28+
...
29+
30+
# Wrong ordering here
31+
class Wrongv4Object(RESTObject, RefreshMixin):
32+
...
33+
34+
If we actually ran this in Python we would get the following error:
35+
class Wrongv4Object(RESTObject, Mixin):
36+
TypeError: Cannot create a consistent method resolution
37+
order (MRO) for bases RESTObject, Mixin
38+
39+
When we are type-checking it fails to understand the class Wrongv4Object
40+
and thus we can't type check it correctly.
41+
42+
Almost all classes in gitlab/v4/objects/*py were already correct before this
43+
check was added.
44+
"""
45+
import inspect
46+
47+
import pytest
48+
49+
import gitlab.v4.objects
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 Wrongv4Object(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 Correctv4Object(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+
failed_messages = []
88+
for module_name, module_value in inspect.getmembers(gitlab.v4.objects):
89+
if not inspect.ismodule(module_value):
90+
# We only care about the modules
91+
continue
92+
# Iterate through all the classes in our module
93+
for class_name, class_value in inspect.getmembers(module_value):
94+
if not inspect.isclass(class_value):
95+
continue
96+
97+
# Ignore imported classes from gitlab.base
98+
if class_value.__module__ == "gitlab.base":
99+
continue
100+
101+
mro = class_value.mro()
102+
103+
# We only check classes which have a 'gitlab.base' class in their MRO
104+
has_base = False
105+
for count, obj in enumerate(mro, start=1):
106+
if obj.__module__ == "gitlab.base":
107+
has_base = True
108+
base_classname = obj.__name__
109+
if has_base:
110+
filename = inspect.getfile(class_value)
111+
# NOTE(jlvillal): The very last item 'mro[-1]' is always going
112+
# to be 'object'. That is why we are checking 'mro[-2]'.
113+
if mro[-2].__module__ != "gitlab.base":
114+
failed_messages.append(
115+
(
116+
f"class definition for {class_name!r} in file {filename!r} "
117+
f"must have {base_classname!r} as the last class in the "
118+
f"class definition"
119+
)
120+
)
121+
failed_msg = "\n".join(failed_messages)
122+
assert not failed_messages, failed_msg

‎gitlab/v4/objects/commits.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ class ProjectCommitCommentManager(ListMixin, CreateMixin, RESTManager):
159159
)
160160

161161

162-
class ProjectCommitStatus(RESTObject, RefreshMixin):
162+
class ProjectCommitStatus(RefreshMixin, RESTObject):
163163
pass
164164

165165

‎gitlab/v4/objects/deployments.py

Lines changed: 1 addition & 1 deletion
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

Lines changed: 1 addition & 1 deletion
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

Lines changed: 1 addition & 1 deletion
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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ class ProjectReleaseManager(NoUpdateMixin, RESTManager):
2424
)
2525

2626

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

3030

0 commit comments

Comments
 (0)