Merge pull request #1352 from JohnVillalovos/jlvillal/fix_mro

fix: add a check to ensure the MRO is correct

6 files changed, 127 insertions, 5 deletions

diff --git a/gitlab/tests/objects/test_mro.py b/gitlab/tests/objects/test_mro.py
new file mode 100644
index 0000000..8f67b77
--- /dev/null
+++ b/gitlab/tests/objects/test_mro.py
@@ -0,0 +1,122 @@
+"""
+Ensure objects defined in gitlab.v4.objects have REST* as last item in class
+definition
+
+Original notes by John L. Villalovos
+
+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 ^^^^^^^^^^
+
+
+Why this is an issue:
+
+  When we do type-checking for gitlab/mixins.py we make RESTObject or
+  RESTManager the base class for the mixins
+
+  Here is how our classes look when type-checking:
+
+      class RESTObject(object):
+          def __init__(self, manager: "RESTManager", attrs: Dict[str, Any]) -> None:
+              ...
+
+      class Mixin(RESTObject):
+          ...
+
+      # Wrong ordering here
+      class Wrongv4Object(RESTObject, RefreshMixin):
+          ...
+
+  If we actually ran this in Python we would get the following error:
+         class Wrongv4Object(RESTObject, Mixin):
+    TypeError: Cannot create a consistent method resolution
+    order (MRO) for bases RESTObject, Mixin
+
+  When we are type-checking it fails to understand the class Wrongv4Object
+  and thus we can't type check it correctly.
+
+Almost all classes in gitlab/v4/objects/*py were already correct before this
+check was added.
+"""
+import inspect
+
+import pytest
+
+import gitlab.v4.objects
+
+
+def test_show_issue():
+    """Test case to demonstrate the TypeError that occurs"""
+
+    class RESTObject(object):
+        def __init__(self, manager: str, attrs: int) -> None:
+            ...
+
+    class Mixin(RESTObject):
+        ...
+
+    with pytest.raises(TypeError) as exc_info:
+        # Wrong ordering here
+        class Wrongv4Object(RESTObject, Mixin):
+            ...
+
+    # The error message in the exception should be:
+    #   TypeError: Cannot create a consistent method resolution
+    #   order (MRO) for bases RESTObject, Mixin
+
+    # Make sure the exception string contains "MRO"
+    assert "MRO" in exc_info.exconly()
+
+    # Correctly ordered class, no exception
+    class Correctv4Object(Mixin, RESTObject):
+        ...
+
+
+def test_mros():
+    """Ensure objects defined in gitlab.v4.objects have REST* as last item in
+    class definition.
+
+    We do this as we need to ensure the MRO (Method Resolution Order) is
+    correct.
+    """
+
+    failed_messages = []
+    for module_name, module_value in inspect.getmembers(gitlab.v4.objects):
+        if not inspect.ismodule(module_value):
+            # We only care about the modules
+            continue
+        # Iterate through all the classes in our module
+        for class_name, class_value in inspect.getmembers(module_value):
+            if not inspect.isclass(class_value):
+                continue
+
+            # Ignore imported classes from gitlab.base
+            if class_value.__module__ == "gitlab.base":
+                continue
+
+            mro = class_value.mro()
+
+            # We only check classes which have a 'gitlab.base' class in their MRO
+            has_base = False
+            for count, obj in enumerate(mro, start=1):
+                if obj.__module__ == "gitlab.base":
+                    has_base = True
+                    base_classname = obj.__name__
+            if has_base:
+                filename = inspect.getfile(class_value)
+                # NOTE(jlvillal): The very last item 'mro[-1]' is always going
+                # to be 'object'. That is why we are checking 'mro[-2]'.
+                if mro[-2].__module__ != "gitlab.base":
+                    failed_messages.append(
+                        (
+                            f"class definition for {class_name!r} in file {filename!r} "
+                            f"must have {base_classname!r} as the last class in the "
+                            f"class definition"
+                        )
+                    )
+    failed_msg = "\n".join(failed_messages)
+    assert not failed_messages, failed_msg
diff --git a/gitlab/v4/objects/commits.py b/gitlab/v4/objects/commits.py
index 037a90d..6176a08 100644
--- a/gitlab/v4/objects/commits.py
+++ b/gitlab/v4/objects/commits.py
@@ -159,7 +159,7 @@ class ProjectCommitCommentManager(ListMixin, CreateMixin, RESTManager):
     )
 
 
-class ProjectCommitStatus(RESTObject, RefreshMixin):
+class ProjectCommitStatus(RefreshMixin, RESTObject):
     pass
 
 
diff --git a/gitlab/v4/objects/deployments.py b/gitlab/v4/objects/deployments.py
index 395bc24..64d779f 100644
--- a/gitlab/v4/objects/deployments.py
+++ b/gitlab/v4/objects/deployments.py
@@ -8,7 +8,7 @@ __all__ = [
 ]
 
 
-class ProjectDeployment(RESTObject, SaveMixin):
+class ProjectDeployment(SaveMixin, RESTObject):
     pass
 
 
diff --git a/gitlab/v4/objects/jobs.py b/gitlab/v4/objects/jobs.py
index 6513d75..e6e04e1 100644
--- a/gitlab/v4/objects/jobs.py
+++ b/gitlab/v4/objects/jobs.py
@@ -10,7 +10,7 @@ __all__ = [
 ]
 
 
-class ProjectJob(RESTObject, RefreshMixin):
+class ProjectJob(RefreshMixin, RESTObject):
     @cli.register_custom_action("ProjectJob")
     @exc.on_http_error(exc.GitlabJobCancelError)
     def cancel(self, **kwargs):
diff --git a/gitlab/v4/objects/pipelines.py b/gitlab/v4/objects/pipelines.py
index bafab9b..724c5e8 100644
--- a/gitlab/v4/objects/pipelines.py
+++ b/gitlab/v4/objects/pipelines.py
@@ -30,7 +30,7 @@ __all__ = [
 ]
 
 
-class ProjectPipeline(RESTObject, RefreshMixin, ObjectDeleteMixin):
+class ProjectPipeline(RefreshMixin, ObjectDeleteMixin, RESTObject):
     _managers = (
         ("jobs", "ProjectPipelineJobManager"),
         ("bridges", "ProjectPipelineBridgeManager"),
diff --git a/gitlab/v4/objects/releases.py b/gitlab/v4/objects/releases.py
index ea74adb..9c94187 100644
--- a/gitlab/v4/objects/releases.py
+++ b/gitlab/v4/objects/releases.py
@@ -24,7 +24,7 @@ class ProjectReleaseManager(NoUpdateMixin, RESTManager):
     )
 
 
-class ProjectReleaseLink(RESTObject, ObjectDeleteMixin, SaveMixin):
+class ProjectReleaseLink(ObjectDeleteMixin, SaveMixin, RESTObject):
     pass