Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions changelog.d/749.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix is_master parsing error for write separation in sentinel mode
25 changes: 18 additions & 7 deletions django_redis/pool.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from typing import Dict
from urllib.parse import parse_qs, urlparse
from urllib.parse import parse_qs, urlencode, urlparse, urlunparse

from django.conf import settings
from django.core.exceptions import ImproperlyConfigured
Expand Down Expand Up @@ -163,16 +163,27 @@
# explicitly set service_name and sentinel_manager for the
# SentinelConnectionPool constructor since will be called by from_url
cp_params = dict(params)
cp_params.update(service_name=url.hostname, sentinel_manager=self._sentinel)
pool = super().get_connection_pool(cp_params)

# convert "is_master" to a boolean if set on the URL, otherwise if not
# provided it defaults to True.
is_master = parse_qs(url.query).get("is_master")
query_params = parse_qs(url.query)
is_master = query_params.get("is_master")
if is_master:
pool.is_master = to_bool(is_master[0])
cp_params["is_master"] = to_bool(is_master[0])

Check warning on line 171 in django_redis/pool.py

View check run for this annotation

Codecov / codecov/patch

django_redis/pool.py#L171

Added line #L171 was not covered by tests
# then remove the "is_master" query string from the URL
# so it doesn't interfere with the SentinelConnectionPool constructor
if "is_master" in query_params:
del query_params["is_master"]

Check warning on line 175 in django_redis/pool.py

View check run for this annotation

Codecov / codecov/patch

django_redis/pool.py#L175

Added line #L175 was not covered by tests
new_query = urlencode(query_params, doseq=True)
Copy link
Member

Choose a reason for hiding this comment

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

why do you need this if you deleted the key from the original query_params variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this parameter is not deleted, it cannot be correctly parsed as bool type in redis-py, but as str type. This will cause it to use the master node for reading and writing even if is_master=0 is set.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, sure, my bad

Copy link
Member

Choose a reason for hiding this comment

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

please merge master into your branch and solve conflicts then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have merged and resolved the conflicts


new_url = urlunparse(
(url.scheme, url.netloc, url.path, url.params, new_query, url.fragment)

Check warning on line 179 in django_redis/pool.py

View check run for this annotation

Codecov / codecov/patch

django_redis/pool.py#L179

Added line #L179 was not covered by tests
)

return pool
cp_params.update(
service_name=url.hostname, sentinel_manager=self._sentinel, url=new_url

Check warning on line 183 in django_redis/pool.py

View check run for this annotation

Codecov / codecov/patch

django_redis/pool.py#L183

Added line #L183 was not covered by tests
)

return super().get_connection_pool(cp_params)


def get_connection_factory(path=None, options=None):
Expand Down
2 changes: 2 additions & 0 deletions tests/test_connection_string.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
[
"unix://tmp/foo.bar?db=1",
"redis://localhost/2",
"redis://redis-master/0?is_master=0",
"redis://redis-master/2?is_master=False",
"rediss://localhost:3333?db=2",
],
)
Expand Down
Loading