search

nats-io / nats.py

Python3 client for NATS
https://nats-io.github.io/nats.py/
Apache License 2.0
865 stars 177 forks source link

Client does not handle connect URL containing multiple servers #545

Open mprimi opened 6 months ago

mprimi commented 6 months ago

Observed behavior

Providing more than one server URL to connect to results in an error.

This works: 

nc = await nats.connect("nats://demo.nats.io:4222")

This does not:

nc = await nats.connect("nats://demo1.nats.io:4222,nats://demo2.nats.io:4222,")

It throws an error:

File "[...]/main.py", line 8, in main
    nc = await nats.connect("nats://demo.nats.io:4222,nats://demo.nats.io:4223")
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "[...]/.venv/lib/python3.11/site-packages/nats/__init__.py", line 45, in connect
    await nc.connect(servers, **options)
  File "[...]/.venv/lib/python3.11/site-packages/nats/aio/client.py", line 417, in connect
    self._setup_server_pool(servers)
  File "[...]/.venv/lib/python3.11/site-packages/nats/aio/client.py", line 1265, in _setup_server_pool
    raise errors.Error("nats: invalid connect url option")
nats.errors.Error: nats: invalid connect url option

Expected behavior

For parity with other clients, multiple server URLs should be accepted

Server and client version

nats-py-2.7.2 (current release)

Server version is not relevant.

Host environment

Python 3.11.3

The rest is not relevant

Steps to reproduce

Create a connection with a string that contains multiple server URLs, e.g.: nats.connect("nats://demo1.nats.io:4222,nats://demo2.nats.io:4222,")

charbonnierg commented 6 months ago

Aren't you using a string instead of a list here ? e.g., "nats://demo1.nats.io:4222,nats://demo2.nats.io:4222" instead of ["nats://demo1.nats.io:4222", "nats://demo2.nats.io:4222"]

mprimi commented 6 months ago

Aren't you using a string instead of a list here ? e.g., "nats://demo1.nats.io:4222,nats://demo2.nats.io:4222" instead of ["nats://demo1.nats.io:4222", "nats://demo2.nats.io:4222"]

nats.connect("nats://127.0.0.1:18001", "nats://127.0.0.1:18001")

Produces:

TypeError: connect() takes from 0 to 1 positional arguments but 2 were given
lekaf974 commented 6 months ago

@mprimi

Based on the code https://github.com/nats-io/nats.py/blob/ae09ea437813b01548938e80f19b81989411cd31/nats/__init__.py#L21

Looks you should do

nats.connect(["nats://demo1.nats.io:4222", "nats://demo2.nats.io:4222"])
mprimi commented 6 months ago

Duh. Thank you for pointing that out.

I'm changing this from 'defect' to enhancement and removing accepted, I'll let the client maintainers chime in on whether to keep it at all.

(on the plus side, just got a lot easier if someone wants to submit a PR!)

caspervonb commented 2 months ago

Don't think we should be splitting on delimited strings, we accept lists. Closing as not planned.

wallyqs commented 2 months ago

@caspervonb I think this is fine to add though so that it is closer to what the go client does

mprimi commented 2 months ago

Parity with other clients is also why I'd (soft) suggest adding this. Tutorials, docs, tools & co may provide users with a connect string and it would be nice if it works out of the box with Python too, without having the user having to split the string.