fix: Avoid sending request body with GET (issue #318)

pysolr.py

@@ -279,9 +279,16 @@ def __init__(
         self.always_commit = always_commit
 
     def get_session(self):
+        """
+        Returns a requests Session object to use for sending requests to Solr.
+
+        The session is created lazily on first call to this method, and is
+        reused for all subsequent requests.
+
+        :return: requests.Session instance
+        """
         if self.session is None:
             self.session = requests.Session()
-            self.session.stream = False
             self.session.verify = self.verify
         return self.session
 
@@ -1250,18 +1257,84 @@ class SolrCoreAdmin(object):
        8. LOAD (not currently implemented)
     """
 
-    def __init__(self, url, *args, **kwargs):
-        super(SolrCoreAdmin, self).__init__(*args, **kwargs)
+    def __init__(self, url, timeout=60, auth=None, verify=True, session=None):
         self.url = url
+        self.timeout = timeout
+        self.log = self._get_log()
+        self.auth = auth
+        self.verify = verify
+        self.session = session
+
+    def get_session(self):
+        """
+        Returns a requests Session object to use for sending requests to Solr.
+
+        The session is created lazily on first call to this method, and is
+        reused for all subsequent requests.
+
+        :return: requests.Session instance
+        """
+        if self.session is None:
+            self.session = requests.Session()
+            self.session.verify = self.verify
+        return self.session
+
+    def _get_log(self):
+        return LOG
 
-    def _get_url(self, url, params=None, headers=None):
+    def _send_request(self, url, params=None, headers=None):
+        """
+        Internal method to send a GET request to Solr.
+
+        :param url: Full URL to query
+        :param params: Dictionary of query parameters
+        :param headers: Dictionary of HTTP headers
+        :return: JSON response from Solr
+        :raises SolrError: if the request fails or the JSON response cannot be decoded
+        """
         if params is None:
             params = {}
         if headers is None:
-            headers = {"Content-Type": "application/x-www-form-urlencoded"}
+            headers = {}
 
-        resp = requests.get(url, data=safe_urlencode(params), headers=headers)
-        return force_unicode(resp.content)
+        session = self.get_session()
+
+        self.log.debug(
+            "Starting Solr admin request to '%s' with params %s",
+            url,
+            params,
+        )
+
+        try:
+            resp = session.get(
+                url,
+                params=params,
+                headers=headers,
+                auth=self.auth,
+            )
+            resp.raise_for_status()
+            return resp.json()
+
+        except requests.exceptions.HTTPError as e:
+            error_url = e.response.url
+            error_msg = e.response.text
+            error_code = e.response.status_code
+
+            self.log.exception(
+                "Solr returned HTTP error %s for URL %s", error_code, error_url
+            )
+            raise SolrError(
+                f"Solr returned HTTP error {error_code}. Response body: {error_msg}"
+            )
+
+        except requests.exceptions.JSONDecodeError as e:
+            self.log.exception("Failed to decode JSON response from Solr at %s", url)
+            raise SolrError(
+                f"Failed to decode JSON response: {e}. Response text: {resp.text}"
+            )
+        except requests.exceptions.RequestException as e:
+            self.log.exception("Request to Solr failed for URL %s", url)
+            raise SolrError(f"Request failed: {e}")
 
     def status(self, core=None):
         """
@@ -1274,7 +1347,7 @@ def status(self, core=None):
         if core is not None:
             params.update(core=core)
 
-        return self._get_url(self.url, params=params)
+        return self._send_request(self.url, params=params)
 
     def create(
         self, name, instance_dir=None, config="solrconfig.xml", schema="schema.xml"
@@ -1291,7 +1364,7 @@ def create(
         else:
             params.update(instanceDir=instance_dir)
 
-        return self._get_url(self.url, params=params)
+        return self._send_request(self.url, params=params)
 
     def reload(self, core):  # NOQA: A003
         """
@@ -1300,7 +1373,7 @@ def reload(self, core):  # NOQA: A003
         See https://wiki.apache.org/solr/CoreAdmin#RELOAD
         """
         params = {"action": "RELOAD", "core": core}
-        return self._get_url(self.url, params=params)
+        return self._send_request(self.url, params=params)
 
     def rename(self, core, other):
         """
@@ -1309,7 +1382,7 @@ def rename(self, core, other):
         See http://wiki.apache.org/solr/CoreAdmin#RENAME
         """
         params = {"action": "RENAME", "core": core, "other": other}
-        return self._get_url(self.url, params=params)
+        return self._send_request(self.url, params=params)
 
     def swap(self, core, other):
         """
@@ -1318,7 +1391,7 @@ def swap(self, core, other):
         See http://wiki.apache.org/solr/CoreAdmin#SWAP
         """
         params = {"action": "SWAP", "core": core, "other": other}
-        return self._get_url(self.url, params=params)
+        return self._send_request(self.url, params=params)
 
     def unload(self, core):
         """
@@ -1327,7 +1400,7 @@ def unload(self, core):
         See http://wiki.apache.org/solr/CoreAdmin#UNLOAD
         """
         params = {"action": "UNLOAD", "core": core}
-        return self._get_url(self.url, params=params)
+        return self._send_request(self.url, params=params)
 
     def load(self, core):
         raise NotImplementedError("Solr 1.4 and below do not support this operation.")

tests/test_admin.py

@@ -1,5 +1,4 @@
 import contextlib
-import json
 import unittest
 
 from pysolr import SolrCoreAdmin, SolrError
@@ -46,31 +45,27 @@ def test_status(self):
         """Test the status endpoint returns details for all cores and specific cores."""
 
         # Status of all cores
-        raw_all = self.solr_admin.status()
-        all_data = json.loads(raw_all)
+        result = self.solr_admin.status()
 
-        self.assertIn("core0", all_data["status"])
+        self.assertIn("core0", result["status"])
 
         # Status of a specific core
-        raw_single = self.solr_admin.status(core="core0")
-        single_data = json.loads(raw_single)
+        result = self.solr_admin.status(core="core0")
 
-        self.assertEqual(single_data["status"]["core0"]["name"], "core0")
+        self.assertEqual(result["status"]["core0"]["name"], "core0")
 
     def test_create(self):
         """Test creating a core returns a successful response."""
-        raw_response = self.solr_admin.create("demo_core1")
-        data = json.loads(raw_response)
+        result = self.solr_admin.create("demo_core1")
 
-        self.assertEqual(data["responseHeader"]["status"], 0)
-        self.assertEqual(data["core"], "demo_core1")
+        self.assertEqual(result["responseHeader"]["status"], 0)
+        self.assertEqual(result["core"], "demo_core1")
 
     def test_reload(self):
         """Test reloading a core returns a successful response."""
-        raw_response = self.solr_admin.reload("core0")
-        data = json.loads(raw_response)
+        result = self.solr_admin.reload("core0")
 
-        self.assertEqual(data["responseHeader"]["status"], 0)
+        self.assertEqual(result["responseHeader"]["status"], 0)
 
     def test_rename(self):
         """Test renaming a core succeeds and the new name appears in the status."""
@@ -79,16 +74,14 @@ def test_rename(self):
         self.solr_admin.create("demo_core1")
 
         # Rename the core to a new name
-        raw_response = self.solr_admin.rename("demo_core1", "demo_core2")
-        data = json.loads(raw_response)
+        result = self.solr_admin.rename("demo_core1", "demo_core2")
 
-        self.assertEqual(data["responseHeader"]["status"], 0)
+        self.assertEqual(result["responseHeader"]["status"], 0)
 
         # Verify that the renamed core appears in the status response
-        raw_response2 = self.solr_admin.status(core="demo_core2")
-        data2 = json.loads(raw_response2)
+        result_2 = self.solr_admin.status(core="demo_core2")
 
-        self.assertEqual(data2["status"]["demo_core2"]["name"], "demo_core2")
+        self.assertEqual(result_2["status"]["demo_core2"]["name"], "demo_core2")
 
     def test_swap(self):
         """
@@ -107,10 +100,9 @@ def test_swap(self):
         self.solr_admin.create("demo_core2")
 
         # Perform swap
-        raw_swap = self.solr_admin.swap("demo_core1", "demo_core2")
-        swap_data = json.loads(raw_swap)
+        result = self.solr_admin.swap("demo_core1", "demo_core2")
 
-        self.assertEqual(swap_data["responseHeader"]["status"], 0)
+        self.assertEqual(result["responseHeader"]["status"], 0)
 
     def test_unload(self):
         """
@@ -121,46 +113,45 @@ def test_unload(self):
         """
         self.solr_admin.create("demo_core1")
 
-        raw_response = self.solr_admin.unload("demo_core1")
-        data = json.loads(raw_response)
+        result = self.solr_admin.unload("demo_core1")
 
-        self.assertEqual(data["responseHeader"]["status"], 0)
+        self.assertEqual(result["responseHeader"]["status"], 0)
 
     def test_load(self):
         self.assertRaises(NotImplementedError, self.solr_admin.load, "wheatley")
 
     def test_status__nonexistent_core_returns_empty_response(self):
         """Test that requesting status for a missing core returns an empty response."""
-        raw_response = self.solr_admin.status(core="not_exists")
-        data = json.loads(raw_response)
+        result = self.solr_admin.status(core="not_exists")
 
-        self.assertNotIn("name", data["status"]["not_exists"])
-        self.assertNotIn("instanceDir", data["status"]["not_exists"])
+        self.assertNotIn("name", result["status"]["not_exists"])
+        self.assertNotIn("instanceDir", result["status"]["not_exists"])
 
     def test_create__existing_core_raises_error(self):
-        """Test creating a core that already exists returns a 500 error."""
+        """Test creating a core that already exists raises SolrError."""
 
         # First create succeeds
         self.solr_admin.create("demo_core1")
 
-        # Creating the same core again should return a 500 error response
-        raw_response = self.solr_admin.create("demo_core1")
-        data = json.loads(raw_response)
+        # Second create should raise SolrError
+        with self.assertRaises(SolrError) as ctx:
+            self.solr_admin.create("demo_core1")
 
-        self.assertEqual(data["responseHeader"]["status"], 500)
-        self.assertEqual(
-            data["error"]["msg"], "Core with name 'demo_core1' already exists."
+        self.assertIn("Solr returned HTTP error 500", str(ctx.exception))
+        self.assertIn(
+            "Core with name 'demo_core1' already exists",
+            str(ctx.exception),
         )
 
     def test_reload__nonexistent_core_raises_error(self):
-        """Test that reloading a non-existent core returns a 400 error."""
-        raw_response = self.solr_admin.reload("not_exists")
-        data = json.loads(raw_response)
+        """Test that reloading a non-existent core raises SolrError."""
 
-        # Solr returns a 400 error for missing cores
-        self.assertEqual(data["responseHeader"]["status"], 400)
-        self.assertIn("No such core", data["error"]["msg"])
-        self.assertIn("not_exists", data["error"]["msg"])
+        with self.assertRaises(SolrError) as ctx:
+            self.solr_admin.reload("not_exists")
+
+        self.assertIn("Solr returned HTTP error 400", str(ctx.exception))
+        self.assertIn("No such core", str(ctx.exception))
+        self.assertIn("not_exists", str(ctx.exception))
 
     def test_rename__nonexistent_core_no_effect(self):
         """
@@ -175,51 +166,44 @@ def test_rename__nonexistent_core_no_effect(self):
         self.solr_admin.rename("not_exists", "demo_core99")
 
         # Check the status of the target core to verify the rename had no effect
-        raw_response = self.solr_admin.status(core="demo_core99")
-        data = json.loads(raw_response)
+        result = self.solr_admin.status(core="demo_core99")
 
         # The target core should not exist because the rename operation was ignored
-        self.assertNotIn("name", data["status"]["demo_core99"])
-        self.assertNotIn("instanceDir", data["status"]["demo_core99"])
+        self.assertNotIn("name", result["status"]["demo_core99"])
+        self.assertNotIn("instanceDir", result["status"]["demo_core99"])
 
     def test_swap__missing_source_core_returns_error(self):
-        """Test swapping when the source core is missing returns a 400 error."""
+        """Test swapping when the source core is missing raises SolrError."""
 
         # Create only the target core
         self.solr_admin.create("demo_core2")
 
-        # Attempt to swap a missing source core with an existing target core
-        raw_response = self.solr_admin.swap("not_exists", "demo_core2")
-        data = json.loads(raw_response)
+        with self.assertRaises(SolrError) as ctx:
+            self.solr_admin.swap("not_exists", "demo_core2")
 
-        # Solr returns a 400 error when the source core does not exist
-        self.assertEqual(data["responseHeader"]["status"], 400)
-        self.assertIn("No such core", data["error"]["msg"])
-        self.assertIn("not_exists", data["error"]["msg"])
+        self.assertIn("Solr returned HTTP error 400", str(ctx.exception))
+        self.assertIn("No such core", str(ctx.exception))
+        self.assertIn("not_exists", str(ctx.exception))
 
     def test_swap__missing_target_core_returns_error(self):
-        """Test swapping when the target core is missing returns a 400 error."""
+        """Test swapping when the target core is missing raises SolrError."""
 
         # Create only the source core
         self.solr_admin.create("demo_core1")
 
-        # Attempt to swap with a missing target core
-        raw_response = self.solr_admin.swap("demo_core1", "not_exists")
-        data = json.loads(raw_response)
+        with self.assertRaises(SolrError) as ctx:
+            self.solr_admin.swap("demo_core1", "not_exists")
 
-        # Solr returns a 400 error when the target core does not exist
-        self.assertEqual(data["responseHeader"]["status"], 400)
-        self.assertIn("No such core", data["error"]["msg"])
-        self.assertIn("not_exists", data["error"]["msg"])
+        self.assertIn("Solr returned HTTP error 400", str(ctx.exception))
+        self.assertIn("No such core", str(ctx.exception))
+        self.assertIn("not_exists", str(ctx.exception))
 
     def test_unload__nonexistent_core_returns_error(self):
-        """Test unloading a non-existent core returns a 400 error response."""
+        """Test unloading a non-existent core raises SolrError."""
 
-        # Attempt to unload a core that does not exist
-        raw_response = self.solr_admin.unload("not_exists")
-        data = json.loads(raw_response)
+        with self.assertRaises(SolrError) as ctx:
+            self.solr_admin.unload("not_exists")
 
-        # Solr returns a 400 error for unloading a missing core
-        self.assertEqual(data["responseHeader"]["status"], 400)
-        self.assertIn("Cannot unload non-existent core", data["error"]["msg"])
-        self.assertIn("not_exists", data["error"]["msg"])
+        self.assertIn("Solr returned HTTP error 400", str(ctx.exception))
+        self.assertIn("Cannot unload non-existent core", str(ctx.exception))
+        self.assertIn("not_exists", str(ctx.exception))