From 8b2dfc646feb05f7ed895c80b04b8cdd67bab87a Mon Sep 17 00:00:00 2001 From: Dhaval Gojiya Date: Mon, 15 Dec 2025 19:06:29 +0530 Subject: [PATCH 1/4] fix: Avoid sending request body with GET (issue #318) --- pysolr.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pysolr.py b/pysolr.py index c4d59143..6a6e4c0a 100644 --- a/pysolr.py +++ b/pysolr.py @@ -1258,9 +1258,9 @@ def _get_url(self, url, params=None, headers=None): 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) + resp = requests.get(url, params=params, headers=headers) return force_unicode(resp.content) def status(self, core=None): From 168ea81827c22b4977b2607b1b390b8a8ed8de06 Mon Sep 17 00:00:00 2001 From: Dhaval Gojiya Date: Wed, 17 Dec 2025 16:47:38 +0530 Subject: [PATCH 2/4] feat: Add requests.Session support to SolrCoreAdmin and update tests --- pysolr.py | 83 ++++++++++++++++++++++++++++++------ tests/test_admin.py | 100 +++++++++++++++++++------------------------- 2 files changed, 113 insertions(+), 70 deletions(-) diff --git a/pysolr.py b/pysolr.py index 6a6e4c0a..a2dbc50f 100644 --- a/pysolr.py +++ b/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,70 @@ 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_url(self, url, params=None, headers=None): + def _get_log(self): + return LOG + + 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 = {} - resp = requests.get(url, params=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, + ) + return resp.json() + 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 +1333,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 +1350,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 +1359,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 +1368,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 +1377,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 +1386,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.") diff --git a/tests/test_admin.py b/tests/test_admin.py index e909ab08..609a7aad 100644 --- a/tests/test_admin.py +++ b/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,21 +113,19 @@ 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.""" @@ -144,23 +134,21 @@ def test_create__existing_core_raises_error(self): 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) + result = self.solr_admin.create("demo_core1") - self.assertEqual(data["responseHeader"]["status"], 500) + self.assertEqual(result["responseHeader"]["status"], 500) self.assertEqual( - data["error"]["msg"], "Core with name 'demo_core1' already exists." + result["error"]["msg"], "Core with name 'demo_core1' already exists." ) 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) + result = self.solr_admin.reload("not_exists") # 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"]) + self.assertEqual(result["responseHeader"]["status"], 400) + self.assertIn("No such core", result["error"]["msg"]) + self.assertIn("not_exists", result["error"]["msg"]) def test_rename__nonexistent_core_no_effect(self): """ @@ -175,12 +163,11 @@ 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.""" @@ -189,13 +176,12 @@ def test_swap__missing_source_core_returns_error(self): 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) + result = 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.assertEqual(result["responseHeader"]["status"], 400) + self.assertIn("No such core", result["error"]["msg"]) + self.assertIn("not_exists", result["error"]["msg"]) def test_swap__missing_target_core_returns_error(self): """Test swapping when the target core is missing returns a 400 error.""" @@ -204,22 +190,20 @@ def test_swap__missing_target_core_returns_error(self): 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) + result = 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.assertEqual(result["responseHeader"]["status"], 400) + self.assertIn("No such core", result["error"]["msg"]) + self.assertIn("not_exists", result["error"]["msg"]) def test_unload__nonexistent_core_returns_error(self): """Test unloading a non-existent core returns a 400 error response.""" # Attempt to unload a core that does not exist - raw_response = self.solr_admin.unload("not_exists") - data = json.loads(raw_response) + result = 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.assertEqual(result["responseHeader"]["status"], 400) + self.assertIn("Cannot unload non-existent core", result["error"]["msg"]) + self.assertIn("not_exists", result["error"]["msg"]) From 7d5550634629245056391ef97ed9a675af65f256 Mon Sep 17 00:00:00 2001 From: Christian Clauss Date: Thu, 18 Dec 2025 16:53:29 +0100 Subject: [PATCH 3/4] resp.raise_for_status() --- pysolr.py | 1 + 1 file changed, 1 insertion(+) diff --git a/pysolr.py b/pysolr.py index a2dbc50f..9a2fba18 100644 --- a/pysolr.py +++ b/pysolr.py @@ -1312,6 +1312,7 @@ def _send_request(self, url, params=None, headers=None): headers=headers, auth=self.auth, ) + resp.raise_for_status() return resp.json() except requests.exceptions.JSONDecodeError as e: self.log.exception("Failed to decode JSON response from Solr at %s", url) From 2dbb8a0e5abeeeb0665f6c00562e511d0ef5cbf3 Mon Sep 17 00:00:00 2001 From: Dhaval Gojiya Date: Fri, 19 Dec 2025 14:42:17 +0530 Subject: [PATCH 4/4] feat: Add HTTPError exception handling when Solr returns bad responses --- pysolr.py | 13 +++++++++ tests/test_admin.py | 66 ++++++++++++++++++++++----------------------- 2 files changed, 46 insertions(+), 33 deletions(-) diff --git a/pysolr.py b/pysolr.py index 9a2fba18..6f14be78 100644 --- a/pysolr.py +++ b/pysolr.py @@ -1314,6 +1314,19 @@ def _send_request(self, url, params=None, headers=None): ) 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( diff --git a/tests/test_admin.py b/tests/test_admin.py index 609a7aad..81192463 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -128,27 +128,30 @@ def test_status__nonexistent_core_returns_empty_response(self): 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 - result = self.solr_admin.create("demo_core1") + # Second create should raise SolrError + with self.assertRaises(SolrError) as ctx: + self.solr_admin.create("demo_core1") - self.assertEqual(result["responseHeader"]["status"], 500) - self.assertEqual( - result["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.""" - result = self.solr_admin.reload("not_exists") + """Test that reloading a non-existent core raises SolrError.""" + + with self.assertRaises(SolrError) as ctx: + self.solr_admin.reload("not_exists") - # Solr returns a 400 error for missing cores - self.assertEqual(result["responseHeader"]["status"], 400) - self.assertIn("No such core", result["error"]["msg"]) - self.assertIn("not_exists", result["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_rename__nonexistent_core_no_effect(self): """ @@ -170,40 +173,37 @@ def test_rename__nonexistent_core_no_effect(self): 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 - result = self.solr_admin.swap("not_exists", "demo_core2") + 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(result["responseHeader"]["status"], 400) - self.assertIn("No such core", result["error"]["msg"]) - self.assertIn("not_exists", result["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 - result = self.solr_admin.swap("demo_core1", "not_exists") + 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(result["responseHeader"]["status"], 400) - self.assertIn("No such core", result["error"]["msg"]) - self.assertIn("not_exists", result["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 - result = self.solr_admin.unload("not_exists") + with self.assertRaises(SolrError) as ctx: + self.solr_admin.unload("not_exists") - # Solr returns a 400 error for unloading a missing core - self.assertEqual(result["responseHeader"]["status"], 400) - self.assertIn("Cannot unload non-existent core", result["error"]["msg"]) - self.assertIn("not_exists", result["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))