Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DhavalGojiya wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ae7185
fix: Avoid sending request body with GET (issue #318)
DhavalGojiya Dec 15, 2025
1b44a58
feat: Add requests.Session support to SolrCoreAdmin and update tests
DhavalGojiya Dec 17, 2025
e8e32df
resp.raise_for_status()
cclauss Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 73 additions & 13 deletions pysolr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -1250,18 +1257,71 @@ 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 = {"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,
)
Copy link
Contributor

@cclauss cclauss Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
)
)
resp.raise_for_status()

Copy link
Contributor

@cclauss cclauss Dec 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix our tests to properly handle this change.

Copy link
Contributor Author

@DhavalGojiya DhavalGojiya Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cclauss will do.

resp.raise_for_status()
return resp.json()
Copy link
Collaborator

@acdha acdha Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call raise_for_status() here? Otherwise the caller doesn't have a simple way to tell whether the request was a success or error response unless we refactor the return signature to be status_code, decoded_json.

Copy link
Contributor Author

@DhavalGojiya DhavalGojiya Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acdha The previous implementation also didn't have this feature.
Users can simply inspect the Solr JSON response’s status key if they want to know the HTTP status of the request.

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):
"""
Expand All @@ -1274,7 +1334,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"
Expand All @@ -1291,7 +1351,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
"""
Expand All @@ -1300,7 +1360,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):
"""
Expand All @@ -1309,7 +1369,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):
"""
Expand All @@ -1318,7 +1378,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):
"""
Expand All @@ -1327,7 +1387,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.")
Expand Down
100 changes: 42 additions & 58 deletions tests/test_admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import contextlib
import json
import unittest

from pysolr import SolrCoreAdmin, SolrError
Expand Down Expand Up @@ -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."""
Expand All @@ -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):
"""
Expand All @@ -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):
"""
Expand All @@ -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."""
Expand All @@ -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):
"""
Expand All @@ -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."""
Expand All @@ -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."""
Expand All @@ -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"])
Loading