Fix requesting urls containing parameters with parameters dict (PyGithub#2929)

Requesting an url that contains parameters (query part of the url) did
not support giving a parameters dict:

Requester.requestJson(verb, "https://api.github.com/?per_page=10",
{"per_page": 20})

Now, parameters given in the URL have precedence over the dict.

Iterating over reversed `PaginatedList` is affected by this.

Fixes PyGithub#1136.

Author: EnricoMi

github/PaginatedList.py

@@ -236,6 +236,13 @@ def __reverse(self) -> None:
         lastUrl = self._getLastPageUrl()
         if lastUrl:
             self.__nextUrl = lastUrl
+            if self.__nextParams:
+                # #2929: remove all parameters from self.__nextParams contained in self.__nextUrl
+                self.__nextParams = {
+                    k: v
+                    for k, v in self.__nextParams.items()
+                    if k not in Requester.get_parameters_of_url(self.__nextUrl).keys()
+                }
 
     def _couldGrow(self) -> bool:
         return self.__nextUrl is not None

github/Requester.py

@@ -471,6 +471,29 @@ def get_graphql_prefix(path: Optional[str]) -> str:
             path = Requester.remove_suffix(path, "/v3")
         return path + "/graphql"
 
+    @staticmethod
+    def get_parameters_of_url(url: str) -> Dict[str, list]:
+        query = urllib.parse.urlparse(url)[4]
+        return urllib.parse.parse_qs(query)
+
+    @staticmethod
+    def add_parameters_to_url(
+        url: str,
+        parameters: Dict[str, Any],
+    ) -> str:
+        scheme, netloc, url, params, query, fragment = urllib.parse.urlparse(url)
+        url_params = urllib.parse.parse_qs(query)
+        # union parameters in url with given parameters, the latter have precedence
+        url_params.update(**{k: v if isinstance(v, list) else [v] for k, v in parameters.items()})
+        parameter_list = [(key, value) for key, values in url_params.items() for value in values]
+        # remove query from url
+        url = urllib.parse.urlunparse((scheme, netloc, url, params, "", fragment))
+
+        if len(parameter_list) == 0:
+            return url
+        else:
+            return f"{url}?{urllib.parse.urlencode(parameter_list)}"
+
     def close(self) -> None:
         """
         Close the connection to the server.
@@ -860,7 +883,7 @@ def __requestEncode(
         requestHeaders["User-Agent"] = self.__userAgent
 
         url = self.__makeAbsoluteUrl(url)
-        url = self.__addParametersToUrl(url, parameters)
+        url = Requester.add_parameters_to_url(url, parameters)
 
         encoded_input = None
         if input is not None:
@@ -996,16 +1019,6 @@ def __makeAbsoluteUrl(self, url: str) -> str:
                 url += f"?{o.query}"
         return url
 
-    def __addParametersToUrl(
-        self,
-        url: str,
-        parameters: Dict[str, Any],
-    ) -> str:
-        if len(parameters) == 0:
-            return url
-        else:
-            return f"{url}?{urllib.parse.urlencode(parameters)}"
-
     def __createConnection(
         self,
     ) -> Union[HTTPRequestsConnectionClass, HTTPSRequestsConnectionClass]:

tests/PaginatedList.py

@@ -37,6 +37,8 @@
 #                                                                              #
 ################################################################################
 
+from datetime import datetime, timezone
+
 from github.PaginatedList import PaginatedList as PaginatedListImpl
 
 from . import Framework
@@ -326,6 +328,40 @@ def testCustomPerPageWithGetPage(self):
         self.g.per_page = 100
         self.assertEqual(len(self.repo.get_issues().get_page(2)), 100)
 
+    def testCustomPerPageIteration(self):
+        self.g.per_page = 3
+        repo = self.g.get_repo("PyGithub/PyGithub")
+        comments = repo.get_issue(1136).get_comments()
+        self.assertEqual(
+            [
+                datetime(2019, 8, 10, 18, 16, 46, tzinfo=timezone.utc),
+                datetime(2024, 1, 6, 16, 4, 34, tzinfo=timezone.utc),
+                datetime(2024, 1, 6, 17, 34, 11, tzinfo=timezone.utc),
+                datetime(2024, 3, 20, 15, 24, 15, tzinfo=timezone.utc),
+                datetime(2024, 3, 21, 10, 55, 14, tzinfo=timezone.utc),
+                datetime(2024, 3, 21, 14, 2, 22, tzinfo=timezone.utc),
+                datetime(2024, 3, 24, 13, 58, 57, tzinfo=timezone.utc),
+            ],
+            [comment.created_at for comment in comments],
+        )
+
+    def testCustomPerPageReversedIteration(self):
+        self.g.per_page = 3
+        repo = self.g.get_repo("PyGithub/PyGithub")
+        comments = repo.get_issue(1136).get_comments().reversed
+        self.assertEqual(
+            [
+                datetime(2024, 3, 24, 13, 58, 57, tzinfo=timezone.utc),
+                datetime(2024, 3, 21, 14, 2, 22, tzinfo=timezone.utc),
+                datetime(2024, 3, 21, 10, 55, 14, tzinfo=timezone.utc),
+                datetime(2024, 3, 20, 15, 24, 15, tzinfo=timezone.utc),
+                datetime(2024, 1, 6, 17, 34, 11, tzinfo=timezone.utc),
+                datetime(2024, 1, 6, 16, 4, 34, tzinfo=timezone.utc),
+                datetime(2019, 8, 10, 18, 16, 46, tzinfo=timezone.utc),
+            ],
+            [comment.created_at for comment in comments],
+        )
+
     def testNoFirstPage(self):
         self.assertFalse(next(iter(self.list), None))
 

tests/ReplayData/PaginatedList.testCustomPerPageIteration.txt

@@ -28,6 +28,7 @@
 from unittest import mock
 
 import github
+from github import Requester as gr
 
 from . import Framework
 from .GithubIntegration import APP_ID, PRIVATE_KEY
@@ -137,6 +138,40 @@ class TestAuth(github.Auth.AppAuth):
             ),
         )
 
+    def testGetParametersOfUrl(self):
+        self.assertEqual({}, gr.Requester.get_parameters_of_url("https://github.com/api"))
+        self.assertEqual({"per_page": ["10"]}, gr.Requester.get_parameters_of_url("https://github.com/api?per_page=10"))
+        self.assertEqual(
+            {"per_page": ["10"], "page": ["2"]},
+            gr.Requester.get_parameters_of_url("https://github.com/api?per_page=10&page=2"),
+        )
+        self.assertEqual(
+            {"item": ["1", "2", "3"]}, gr.Requester.get_parameters_of_url("https://github.com/api?item=1&item=2&item=3")
+        )
+
+    def testAddParametersToUrl(self):
+        self.assertEqual("https://github.com/api", gr.Requester.add_parameters_to_url("https://github.com/api", {}))
+        self.assertEqual(
+            "https://github.com/api?per_page=10",
+            gr.Requester.add_parameters_to_url("https://github.com/api", {"per_page": 10}),
+        )
+        self.assertEqual(
+            "https://github.com/api?per_page=10&page=2",
+            gr.Requester.add_parameters_to_url("https://github.com/api", {"per_page": 10, "page": 2}),
+        )
+        self.assertEqual(
+            "https://github.com/api?per_page=10&page=2",
+            gr.Requester.add_parameters_to_url("https://github.com/api?per_page=10", {"page": 2}),
+        )
+        self.assertEqual(
+            "https://github.com/api?per_page=10&page=2",
+            gr.Requester.add_parameters_to_url("https://github.com/api?per_page=10&page=1", {"page": 2}),
+        )
+        self.assertEqual(
+            "https://github.com/api?item=3&item=4",
+            gr.Requester.add_parameters_to_url("https://github.com/api?item=1&item=2&item=3", {"item": [3, 4]}),
+        )
+
     def testCloseGithub(self):
         mocked_connection = mock.MagicMock()
         mocked_custom_connection = mock.MagicMock()