fix: properly pass kwargs to gitlab client and graphql client (gitlabform#1151)

In python-gitlab client, arbitrary parameters to graphql client is not accepted gracefully. This change fixes this issue by inspecting the library's signature and passing valid parameters to it. This way, we still maintain ability to pass accepted parameters/configurations through gitlabform's config.

Related to gitlabform#1140

Author: thomascizeron

docs/reference/index.md

@@ -41,17 +41,20 @@ gitlab:
   ssl_verify: true
   # timeout for the whole requests to the GitLab API, in seconds
   timeout: 10
+
   # ** advanced parameters **
-  # Any additional parameters specified here will be passed directly to the python-gitlab library.
+  # Any additional parameters specified here will be passed to the python-gitlab library if they are supported. 
   # See https://python-gitlab.readthedocs.io/en/stable/api-usage.html#gitlab-client for available options.
   #
-  # GitlabForm hasn't yet 100% migrated to use python-gitlab. Some processors are not using it and wont use these parameters.
+  # GitlabForm hasn't yet 100% migrated to use python-gitlab. Some processors are not using it and won't use these parameters.
   # For following migration to python-gitlab, see: https://github.com/gitlabform/gitlabform/issues/73
   #
-  # For example:
-  # retry_transient_errors: false # (defaults to `true`) retry requests that fail due to transient errors
-  # obey_rate_limit: true  # automatically wait if rate limit is hit
-  # max_retries: 5      # number of times to retry failed requests
+  # REST API parameters (passed to python-gitlab's Gitlab client):
+  # retry_transient_errors: false  # (defaults to `true`) retry requests that fail due to transient errors (5xx)
+  #
+  # GraphQL API parameters (passed to python-gitlab's GraphQL client):
+  # max_retries: 10       # number of times to retry failed GraphQL requests
+  # obey_rate_limit: true # automatically wait and retry if rate limit is hit
 
 # Configuration to apply to GitLab projects, groups and subgroups
 projects_and_groups:

gitlabform/gitlab/__init__.py

@@ -1,8 +1,9 @@
 import enum
+import inspect
 
 from typing import List
 
-from gitlab import GraphQL
+from gitlab import Gitlab as GitlabClient, GraphQL
 
 from gitlabform.gitlab.commits import GitLabCommits
 from gitlabform.gitlab.group_badges import GitLabGroupBadges
@@ -67,28 +68,41 @@ class GitLab(
 
 
 class GitlabWrapper:
+    # Parameters accepted by python-gitlab's Gitlab.__init__
+    # Other config keys (like max_retries) are used elsewhere in gitlabform
+    # or passed to specific components like GraphQL
+    GITLAB_CLIENT_PARAMS = set(inspect.signature(GitlabClient.__init__).parameters.keys()) - {"self"}
+
+    # Parameters accepted by python-gitlab's GraphQL.__init__
+    GRAPHQL_PARAMS = set(inspect.signature(GraphQL.__init__).parameters.keys()) - {"self"}
+
     def __init__(self, gitlabform: GitLab):
         session = gitlabform.session
 
-        graphql = GraphQL(url=gitlabform.gitlab_config["url"], token=gitlabform.gitlab_config["token"])
+        graphql_kwargs = {k: v for k, v in gitlabform.gitlab_config.items() if k in self.GRAPHQL_PARAMS}
+        graphql = GraphQL(**graphql_kwargs)
 
-        default_kwargs = {
+        default_gitlab_kwargs = {
             "retry_transient_errors": True,
         }
-        renamed_kwargs = {
+        renamed_gitlab_kwargs = {
             "token": "private_token",
         }
-        extra_kwargs = {
-            **default_kwargs,
-            **{k: v for k, v in gitlabform.gitlab_config.items() if k not in renamed_kwargs},
-            **{renamed_kwargs[k]: v for k, v in gitlabform.gitlab_config.items() if k in renamed_kwargs},
+        extra_gitlab_kwargs = {
+            **default_gitlab_kwargs,
+            **{
+                k: v
+                for k, v in gitlabform.gitlab_config.items()
+                if k not in renamed_gitlab_kwargs and k in self.GITLAB_CLIENT_PARAMS
+            },
+            **{renamed_gitlab_kwargs[k]: v for k, v in gitlabform.gitlab_config.items() if k in renamed_gitlab_kwargs},
         }
 
         self._gitlab: PythonGitlab = PythonGitlab(
             api_version="4",
             graphql=graphql,
             session=session,
-            **extra_kwargs,
+            **extra_gitlab_kwargs,
         )
 
     def get_gitlab(self):

tests/unit/gitlab/test_python_gitlab.py

@@ -68,50 +68,124 @@ def test_convert_result_to_member_roles_removes_GitlabGraphQl_specific_prefix_fr
 
 class TestGitlabWrapperExtraKwargs:
     """
-    Tests for extra kwargs support in GitlabWrapper that should be passed to PythonGitlab.
-    This includes retry_transient_errors, obey_rate_limit, and any other additional parameters
-    defined in the configuration.
+    Tests for GitlabWrapper kwargs filtering.
+    Ensures that only valid parameters are passed to python-gitlab's Gitlab client
+    and GraphQL client, preventing TypeError from unsupported kwargs.
     """
 
+    @patch("gitlabform.gitlab.GraphQL")
+    @patch("gitlabform.gitlab.PythonGitlab")
+    def test_max_retries_not_passed_to_gitlab_client(self, mock_python_gitlab, mock_graphql):
+        """Test that max_retries is NOT passed to PythonGitlab (would cause TypeError)"""
+        mock_gitlab = MagicMock(spec=GitLab)
+        mock_gitlab.session = MagicMock()
+        mock_gitlab.gitlab_config = {
+            "url": "https://gitlab.example.com",
+            "token": "test-token",
+            "max_retries": 5,  # This should NOT be passed to PythonGitlab
+        }
+
+        _ = GitlabWrapper(mock_gitlab)
+
+        call_kwargs = mock_python_gitlab.call_args.kwargs
+        assert "max_retries" not in call_kwargs
+
+    @patch("gitlabform.gitlab.GraphQL")
+    @patch("gitlabform.gitlab.PythonGitlab")
+    def test_max_retries_passed_to_graphql_client(self, mock_python_gitlab, mock_graphql):
+        """Test that max_retries IS passed to GraphQL client"""
+        mock_gitlab = MagicMock(spec=GitLab)
+        mock_gitlab.session = MagicMock()
+        mock_gitlab.gitlab_config = {
+            "url": "https://gitlab.example.com",
+            "token": "test-token",
+            "max_retries": 5,
+        }
+
+        _ = GitlabWrapper(mock_gitlab)
+
+        call_kwargs = mock_graphql.call_args.kwargs
+        assert call_kwargs["max_retries"] == 5
+
+    @patch("gitlabform.gitlab.GraphQL")
     @patch("gitlabform.gitlab.PythonGitlab")
-    def test_extra_kwargs_are_passed_to_python_gitlab(self, mock_python_gitlab):
-        """Test that extra kwargs from config are passed to PythonGitlab initialization"""
-        # Setup mock GitLab object with extra config parameters
+    def test_only_valid_gitlab_client_params_passed(self, mock_python_gitlab, mock_graphql):
+        """Test that only valid Gitlab client parameters are passed"""
         mock_gitlab = MagicMock(spec=GitLab)
         mock_gitlab.session = MagicMock()
         mock_gitlab.gitlab_config = {
             "url": "https://gitlab.example.com",
             "token": "test-token",
             "ssl_verify": True,
             "timeout": 10,
-            "retry_transient_errors": True,
-            "obey_rate_limit": True,
-            "foo": "bar",  # extra param for testing
+            "max_retries": 5,  # GraphQL only
+            "obey_rate_limit": True,  # GraphQL only
+            "some_unknown_param": "value",  # Should be filtered out
         }
 
-        # Create wrapper
         _ = GitlabWrapper(mock_gitlab)
 
-        # Verify PythonGitlab was called with the correct arguments
-        mock_python_gitlab.assert_called_once()
         call_kwargs = mock_python_gitlab.call_args.kwargs
 
-        # Check standard parameters
+        # Valid params should be present
         assert call_kwargs["url"] == "https://gitlab.example.com"
         assert call_kwargs["private_token"] == "test-token"
         assert call_kwargs["ssl_verify"] is True
         assert call_kwargs["timeout"] == 10
-        assert call_kwargs["api_version"] == "4"
 
-        # Check extra kwargs are passed
+        # Invalid params should NOT be present
+        assert "max_retries" not in call_kwargs
+        assert "obey_rate_limit" not in call_kwargs
+        assert "some_unknown_param" not in call_kwargs
+
+    @patch("gitlabform.gitlab.GraphQL")
+    @patch("gitlabform.gitlab.PythonGitlab")
+    def test_graphql_params_passed_correctly(self, mock_python_gitlab, mock_graphql):
+        """Test that GraphQL-specific parameters are passed to GraphQL client"""
+        mock_gitlab = MagicMock(spec=GitLab)
+        mock_gitlab.session = MagicMock()
+        mock_gitlab.gitlab_config = {
+            "url": "https://gitlab.example.com",
+            "token": "test-token",
+            "max_retries": 5,
+            "obey_rate_limit": False,
+            "retry_transient_errors": True,
+            "timeout": 30,
+            "ssl_verify": False,
+        }
+
+        _ = GitlabWrapper(mock_gitlab)
+
+        call_kwargs = mock_graphql.call_args.kwargs
+
+        assert call_kwargs["max_retries"] == 5
+        assert call_kwargs["obey_rate_limit"] is False
         assert call_kwargs["retry_transient_errors"] is True
-        assert call_kwargs["obey_rate_limit"] is True
-        assert call_kwargs["foo"] == "bar"
+        assert call_kwargs["timeout"] == 30
+        assert call_kwargs["ssl_verify"] is False
+
+    @patch("gitlabform.gitlab.GraphQL")
+    @patch("gitlabform.gitlab.PythonGitlab")
+    def test_token_renamed_to_private_token(self, mock_python_gitlab, mock_graphql):
+        """Test that 'token' config key is renamed to 'private_token' for Gitlab client"""
+        mock_gitlab = MagicMock(spec=GitLab)
+        mock_gitlab.session = MagicMock()
+        mock_gitlab.gitlab_config = {
+            "url": "https://gitlab.example.com",
+            "token": "my-secret-token",
+        }
+
+        _ = GitlabWrapper(mock_gitlab)
+
+        call_kwargs = mock_python_gitlab.call_args.kwargs
+
+        assert "token" not in call_kwargs
+        assert call_kwargs["private_token"] == "my-secret-token"
 
     @patch("gitlabform.gitlab.GraphQL")
     @patch("gitlabform.gitlab.PythonGitlab")
-    def test_default_values_for_gitlab_config(self, mock_python_gitlab, mock_graphql):
-        """Test that when only standard keys are provided, default values are used for others"""
+    def test_retry_transient_errors_default_true(self, mock_python_gitlab, mock_graphql):
+        """Test that retry_transient_errors defaults to True for Gitlab client"""
         mock_gitlab = MagicMock(spec=GitLab)
         mock_gitlab.session = MagicMock()
         mock_gitlab.gitlab_config = {
@@ -122,17 +196,39 @@ def test_default_values_for_gitlab_config(self, mock_python_gitlab, mock_graphql
         _ = GitlabWrapper(mock_gitlab)
 
         call_kwargs = mock_python_gitlab.call_args.kwargs
+        assert call_kwargs["retry_transient_errors"] is True
 
-        print(call_kwargs)
-        # Verify standard keys are passed directly as named parameters with defaults for others
-        assert call_kwargs["url"] == "https://gitlab.example.com"
-        assert call_kwargs["private_token"] == "test-token"
-        assert call_kwargs["retry_transient_errors"] is True  # default value of kwarg
+    @patch("gitlabform.gitlab.GraphQL")
+    @patch("gitlabform.gitlab.PythonGitlab")
+    def test_retry_transient_errors_can_be_overridden(self, mock_python_gitlab, mock_graphql):
+        """Test that retry_transient_errors can be overridden from config"""
+        mock_gitlab = MagicMock(spec=GitLab)
+        mock_gitlab.session = MagicMock()
+        mock_gitlab.gitlab_config = {
+            "url": "https://gitlab.example.com",
+            "token": "test-token",
+            "retry_transient_errors": False,
+        }
 
-        # Verify no extra kwargs beyond the expected parameters
-        expected_keys = {"url", "private_token", "retry_transient_errors", "api_version", "graphql", "session"}
-        actual_keys = set(call_kwargs.keys())
-        extra_keys = actual_keys - expected_keys
+        _ = GitlabWrapper(mock_gitlab)
+
+        call_kwargs = mock_python_gitlab.call_args.kwargs
+        assert call_kwargs["retry_transient_errors"] is False
+
+    @patch("gitlabform.gitlab.GraphQL")
+    @patch("gitlabform.gitlab.PythonGitlab")
+    def test_graphql_url_and_token_passed(self, mock_python_gitlab, mock_graphql):
+        """Test that URL and token are passed to GraphQL client"""
+        mock_gitlab = MagicMock(spec=GitLab)
+        mock_gitlab.session = MagicMock()
+        mock_gitlab.gitlab_config = {
+            "url": "https://gitlab.example.com",
+            "token": "test-token",
+        }
+
+        _ = GitlabWrapper(mock_gitlab)
 
-        # Only extra keys should be those not in excluded_keys (should be empty in this case)
-        assert len(extra_keys) == 0
+        mock_graphql.assert_called_once()
+        call_args = mock_graphql.call_args
+        assert call_args.kwargs["url"] == "https://gitlab.example.com"
+        assert call_args.kwargs["token"] == "test-token"