Skip to content
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

Client does not handle connect URL containing multiple servers #545

Open
mprimi opened this issue Feb 28, 2024 · 7 comments
Open

Client does not handle connect URL containing multiple servers #545

mprimi opened this issue Feb 28, 2024 · 7 comments
Labels
proposal Enhancement idea or proposal rejected Proposal is rejected

Comments

@mprimi
Copy link

mprimi commented Feb 28, 2024

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,")

@mprimi mprimi added the defect Suspected defect such as a bug or regression label Feb 28, 2024
@wallyqs wallyqs assigned wallyqs and unassigned wallyqs Feb 28, 2024
@wallyqs wallyqs added the good first issue Good for newcomers label Feb 28, 2024
@mprimi mprimi added accepted The defect or proposal as been accepted help wanted labels Feb 28, 2024
@charbonnierg
Copy link
Contributor

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
Copy link
Author

mprimi commented Feb 29, 2024

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
Copy link

lekaf974 commented Feb 29, 2024

@mprimi

Based on the code

servers: Union[str, List[str]] = ["nats://localhost:4222"],

Looks you should do

nats.connect(["nats://demo1.nats.io:4222", "nats://demo2.nats.io:4222"])

@mprimi mprimi added enhancement Enhancement to existing functionality and removed accepted The defect or proposal as been accepted labels Feb 29, 2024
@mprimi
Copy link
Author

mprimi commented Feb 29, 2024

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 caspervonb added rejected Proposal is rejected proposal Enhancement idea or proposal and removed enhancement Enhancement to existing functionality good first issue Good for newcomers defect Suspected defect such as a bug or regression help wanted labels Jun 12, 2024
@caspervonb
Copy link
Contributor

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

@caspervonb caspervonb closed this as not planned Won't fix, can't repro, duplicate, stale Jun 12, 2024
@wallyqs
Copy link
Member

wallyqs commented Jun 13, 2024

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

@wallyqs wallyqs reopened this Jun 13, 2024
@mprimi
Copy link
Author

mprimi commented Jun 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Enhancement idea or proposal rejected Proposal is rejected
Projects
None yet
Development

No branches or pull requests

5 participants