Fix default port on secure connection + Query Parameterization#138
Fix default port on secure connection + Query Parameterization#138barakor wants to merge 1 commit intolong2ice:devfrom
Conversation
barakor
changed the title
barakor
force-pushed
the
fix-default-port-on-secure
branch
from
d7efd64 to
ebfe24e
Compare
asynch/connection.py
Outdated
| user = config.get("user", None) or user | ||
| password = config.get("password", None) or password | ||
| host = config.get("host", None) or host | ||
| port = config.get("port", None) or port |
There was a problem hiding this comment.
What if it's secure and no port was specified?
There was a problem hiding this comment.
this is what this pr is about, Protoconnection handles this already
There was a problem hiding this comment.
But it doesn't check secure from dsn and so port will always be constants.DEFAULT_PORT if it's not specified in dsn or maybe I don't understand something?
There was a problem hiding this comment.
One of my changes was to add a check for secure from the dsn
There was a problem hiding this comment.
Python 3.12.10 (main, Apr 8 2025, 11:35:47) [GCC 14.2.1 20250322] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from asynch.proto import constants
>>> from asynch.proto.utils.dsn import parse_dsn
>>>
>>> dsn = 'clickhouses://user:pass@hostname/database'
>>> secure = False
>>> port = None
>>> if secure and port is None:
... port = constants.DEFAULT_SECURE_PORT
... elif not secure and port is None:
... port = constants.DEFAULT_PORT
...
>>> config = parse_dsn(dsn)
>>> user = config.get("user") or user
>>> password = config.get("password") or password
>>> host = config.get("host") or host
>>> port = config.get("port") or port
>>> database = config.get("database") or database
>>> secure = config.get("secure") or secure
>>> print(port, secure)
9000 True
>>>
|
@DaniilAnichin , @pohmelie, hello 👋 . Could you possibly review this PR? |
| "stack_track": stack_track, | ||
| } | ||
|
|
||
| self._connection = ProtoConnection(**config, stack_track=stack_track, **kwargs) |
There was a problem hiding this comment.
Why we need stack_track=stack_track in the middle?
There was a problem hiding this comment.
Based on what I see above, because stack_track is not part of dsn, so will be missing if we follow the first if branch above. Probably for same reason it was explicitly passed in prev version.
There was a problem hiding this comment.
It should be excluded from config then, I think
DaniilAnichin
left a comment
DaniilAnichin
left a comment
There was a problem hiding this comment.
Minor comment on styling on the port check, other than that lgtm
| "stack_track": stack_track, | ||
| } | ||
|
|
||
| self._connection = ProtoConnection(**config, stack_track=stack_track, **kwargs) |
There was a problem hiding this comment.
Based on what I see above, because stack_track is not part of dsn, so will be missing if we follow the first if branch above. Probably for same reason it was explicitly passed in prev version.
|
@barakor , hello. Any updates? Are you willing to drag this PR to the finish line? |
What's missing @stankudrow ? I thought I addressed everything |
Added new suggestions and outlined the existent. Also, is this case is implemented in a unit test? |
tests and query params bug fix fix bug with default port for secure connection typing and moving function to module Update asynch/proto/connection.py Nones and such Update test_escape.py styling Co-Authored-By: Stanley Kudrow <stankudrow@yandex.ru>
barakor
force-pushed
the
fix-default-port-on-secure
branch
from
bf3f6e0 to
ef5fb97
Compare
|
Rebased |
Probably the PR #147 may be merged soon enough, feel free to have a look at it if you like. |
5 participants
I found and fixed 2 bugs:
Connectionwithout specifying the port, it default to9000even though there's logic to handle it..format()with a dict)