-
-
Notifications
You must be signed in to change notification settings - Fork 913
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.
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
Add support for mongodb+srv scheme #1976
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please add some tests for the changes?
@auvipy Unless you have a valid connection using mongodb+srv, I don't think is possible to add tests due to how this scheme is resolved using DNS. |
I understand - would it be ok to ask for you to do some manual testing and at least share here the logs so we can have at least a minimal confirmation of it working? P.S |
c6d0ee9
to
a89072c
Compare
I did a monkey-patch on my code to use this change, I've been using since I opened this PR, this is the logs from yesterday: My monkey patch code is basically this: from kombu.transport.mongodb import Channel
from pymongo import uri_parser
def monkey_patch_channel_parse_uri(self, scheme="mongodb://"):
# See mongodb uri documentation:
# https://docs.mongodb.org/manual/reference/connection-string/
client = self.connection.client
hostname = client.hostname
if hostname.startswith("srv://"):
scheme = "mongodb+srv://"
hostname = "mongodb+" + hostname
client.hostname = hostname
if not hostname.startswith(scheme):
hostname = scheme + hostname
if not hostname[len(scheme) :]:
hostname += self.default_hostname
if client.userid and "@" not in hostname:
head, tail = hostname.split("://")
credentials = client.userid
if client.password:
credentials += ":" + client.password
hostname = head + "://" + credentials + "@" + tail
port = client.port if client.port else self.default_port
parsed = uri_parser.parse_uri(hostname, port)
dbname = parsed["database"] or client.virtual_host
if dbname in ("/", None):
dbname = self.default_database
options = {
"auto_start_request": True,
"ssl": self.ssl,
"connectTimeoutMS": (
int(self.connect_timeout * 1000) if self.connect_timeout else None
),
}
options.update(parsed["options"])
options = self._prepare_client_options(options)
if "tls" in options:
options.pop("ssl")
return hostname, dbname, options
def apply_kombu_monkey_patch():
Channel._parse_uri = monkey_patch_channel_parse_uri |
@H4ad Thank you! Much appreciated!
@auvipy I know you asked for tests, but I think this proves that it at least does what it is supposed to, in addition to also reviewing the code itself (which makes sense to me as-is). I think we can go through and come back if someone reports an issue - what do you say? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM then. We’ll wait for @auvipy to review it as well.
If we try run kombu with scheme
mongodb+srv
, it will break during URL parsing.Fixes: #1786
Fixes: #858