Fix connection secure port management. by stankudrow · Pull Request #162 · long2ice/asynch

stankudrow · 2026-06-15T22:28:05Z

Fix errors in the connection security parameter + minor (un)related corrections

This PR was inspired by #138, and I thought it would be a good idea to split the secure connection case and query parametrisation, focusing only on the first part. It wasn't straightforward, but perhaps that's for the best.

@barakor, I hope I'm not stepping on your toes — I fully acknowledge your contribution. I'd appreciate it if you could also review this PR; your suggestions will be greatly appreciated and carefully addressed.

Some tests used to fail due to issues with parameter substitution processing (related to #147). @baconfield, @dmkulazhenko, could you please review these changes?

I experimented with configuring connections in the tests, which caused tests using a bare Connection() or Pool(...) without connection_kwargs to fail. This is actually a positive outcome, as it revealed the fragility of those tests.

@long2ice, @nils-borrmann-tacto, @DaniilAnichin, I had some difficulty specifying the pytest-asyncio asyncio_default_fixture_loop_scope attribute and addressing the warning from pytest: neither the "session" nor the "function" scope resolves the issue. By the way, your reviews would be very welcome.

…ot tests

stankudrow · 2026-06-16T17:52:35Z

@baconfield . @nils-borrmann-tacto , @long2ice , damn, this PR should go first because the failed CI in the dev - https://github.com/long2ice/asynch/actions/runs/27552079391/job/81440866403 . @barakor, @dmkulazhenko , @DaniilAnichin , @pohmelie, please help with reviews.

DaniilAnichin · 2026-06-17T17:22:07Z

Seems to be ok; about the pytest-asyncio: mostly I've dealt with it by switching to anyio, but I don't think this is a PR to do it in.

stankudrow · 2026-06-17T20:34:21Z

Seems to be ok; about the pytest-asyncio: mostly I've dealt with it by switching to anyio, but I don't think this is a PR to do it in.

Yeah, you are right, this PR is not about anyio. However, before proposing a PR, feel free to open an issue and lay out pros and cons of anyio, examples etc.

barakor

Seems ok overall, but the change in asynch/connection.py that I commented about is problematic

Co-authored-by: Barak <26878518+barakor@users.noreply.github.com>

baconfield · 2026-06-18T01:04:23Z

@baconfield . @nils-borrmann-tacto , @long2ice , damn, this PR should go first because the failed CI in the dev - https://github.com/long2ice/asynch/actions/runs/27552079391/job/81440866403 . @barakor, @dmkulazhenko , @DaniilAnichin , @pohmelie, please help with reviews.

Resolved with #163

stankudrow · 2026-06-18T05:01:07Z

@baconfield . @nils-borrmann-tacto , @long2ice , damn, this PR should go first because the failed CI in the dev - https://github.com/long2ice/asynch/actions/runs/27552079391/job/81440866403 . @barakor, @dmkulazhenko , @DaniilAnichin , @pohmelie, please help with reviews.

Resolved with #163

The changes from the #163 were here before - https://github.com/long2ice/asynch/pull/162/changes#diff-94026d54d9ecff66c0362322fe026b6995eb0bdbb56bb8bf84447fc7fa148b04 . You can close the #163 PR.

…fied

stankudrow · 2026-06-24T03:40:03Z

@DaniilAnichin , @pohmelie , hello.

In spite of these changes, could you re-review and re-approve them in case they are really OK? After that this PR can go to the final-boss review. Sorry for these bureaucratic troubles.

stankudrow · 2026-07-15T19:53:46Z

@long2ice , hello. This PR is ready to be merged.

@stankudrow

…dling, PEP 249 - #162 (@stankudrow): a secure connection without an explicit port went to 9000 instead of 9440. Reworked rather than ported: the original patch resolved the port before reading `secure` out of the DSN, so `clickhouses://host/db` - its own motivating case - still got 9000, and it regressed bare `Connection()` by passing None for every unset field. Here the port simply stays None until the proto layer resolves it from the scheme, and the facade reads the effective port back. - #150 (@nils-borrmann-tacto): a rejected query disconnected the connection, so every SQL error cost a pooled connection. Server exceptions now leave it open (the exception packet leaves the stream at a packet boundary), with `is_query_executing` cleared so the next query is accepted. Deviation from the original: a failed query no longer updates the tracked database - the patch as proposed let a failed `USE db` move the client's idea of it. - #146 (@vizor-games): None in a non-Nullable column surfaced as whatever primitive failed first. Placed in the slow path of `prepare_items` (which already iterates) plus a translation at the dispatcher for columns that bypass it, rather than the proposed `None in items` scan on every column: that would have taxed the fast path and, running before `prepare_items`, would have broken `input_format_null_as_default`. - #159 (@turquoisehealth): took the additive part of the PEP 249 work - module globals, `connect()`, exception re-exports, `Cursor.arraysize`. Left out the behaviour changes (`commit()` as a no-op, `description` returning type objects) and the pool cross-loop reset, which leaks connections and reads private asyncio attributes. threadsafety is 1, not the 2 the PR proposed: connections own a socket and a parse buffer and are not shareable between threads.

long2ice · 2026-08-14T14:13:14Z

Thanks — the bug is real and is fixed on dev in 682d5e1, though I reworked the approach rather than porting the patch, for two reasons:

In _get_proto_connection_kwargs the port was resolved before secure was read out of the DSN, so clickhouses://host/db — the motivating case — still resolved to 9000. The parametrised tests all passed an explicit port, so that path wasn't covered.
The defaults were only filled in inside the if dsn: branch, so a bare Connection() passed user=None/host=None down to ProtoConnection and overrode its signature defaults. The tests were changed to always pass a DSN, which hid it (this is also what @barakor was pointing at).

The fix on dev keeps it minimal: port stays None until the proto layer resolves it via get_default_port(secure=...), and the facade reads the effective port back from the connection. Tests cover all seven combinations (default, secure=True, explicit port, secure+explicit, and the three DSN forms).

I left the parameter-vs-DSN precedence inversion and the _get_proto_connection_kwargs refactor out — those are behaviour changes worth their own discussion rather than riding along with a port fix. Closing; thanks for finding it.

Stanley Kudrow added 4 commits June 16, 2026 01:05

handle connection params/attrs and secure connection case + update ro…

8db4d1f

…ot tests

change .env credentials

fb43371

add project notes (NOTES.md) with 'How to test' section

c7bb6a2

update pool tests

0328a96

stankudrow commented Jun 15, 2026

View reviewed changes

Comment thread asynch/proto/connection.py

stankudrow commented Jun 15, 2026

View reviewed changes

Comment thread asynch/connection.py Outdated

stankudrow commented Jun 15, 2026

View reviewed changes

Comment thread asynch/connection.py

add PR long2ice#162 (mention long2ice#138)

59e55ab

pohmelie reviewed Jun 17, 2026

View reviewed changes

Comment thread asynch/connection.py Outdated

DaniilAnichin approved these changes Jun 17, 2026

View reviewed changes

Stanley Kudrow added 3 commits June 17, 2026 23:50

update root files

2974455

add test_escape module (from #138m thanx to @barakor)

a8ca162

add get_default_port helper

02952d6

stankudrow requested a review from pohmelie June 17, 2026 20:59

fix get_default_port helper docstring

53f87a3

stankudrow commented Jun 17, 2026

View reviewed changes

Comment thread tests/test_proto/utils/test_escape.py

barakor reviewed Jun 17, 2026

View reviewed changes

Comment thread asynch/proto/connection.py

Comment thread asynch/proto/connection.py Outdated

Comment thread asynch/connection.py Outdated

Comment thread asynch/connection.py Outdated

Comment thread tests/test_proto/utils/test_escape.py

pohmelie approved these changes Jun 17, 2026

View reviewed changes

stankudrow and others added 2 commits June 18, 2026 02:37

Update asynch/proto/connection.py

e45f3d7

Co-authored-by: Barak <26878518+barakor@users.noreply.github.com>

Update asynch/connection.py

5d6e43c

Co-authored-by: Barak <26878518+barakor@users.noreply.github.com>

stankudrow requested a review from barakor June 18, 2026 04:58

Stanley Kudrow added 3 commits June 18, 2026 11:09

swap CHANGELOG records for v0.3.2

72950f5

fix connection configuration when DSN and connection params are speci…

13a5148

…fied

add test_connection_params

35e6f4c

stankudrow requested a review from DaniilAnichin June 18, 2026 20:03

stankudrow requested a review from pohmelie June 18, 2026 20:03

mention @barakor as coauthor in long2ice#162

d2b1c29

barakor approved these changes Jun 21, 2026

View reviewed changes

barakor mentioned this pull request Jun 21, 2026

Fix default port on secure connection + Query Parameterization #138

Closed

DaniilAnichin approved these changes Jun 24, 2026

View reviewed changes

long2ice closed this Aug 14, 2026

stankudrow deleted the fix-connection-default-secure-port branch August 15, 2026 07:28

Conversation

stankudrow commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stankudrow commented Jun 16, 2026

Uh oh!

Uh oh!

DaniilAnichin commented Jun 17, 2026

Uh oh!

stankudrow commented Jun 17, 2026

Uh oh!

Uh oh!

barakor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

baconfield commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stankudrow commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stankudrow commented Jun 24, 2026

Uh oh!

stankudrow commented Jul 15, 2026

Uh oh!

long2ice commented Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stankudrow commented Jun 15, 2026 •

edited

Loading

baconfield commented Jun 18, 2026 •

edited

Loading

stankudrow commented Jun 18, 2026 •

edited

Loading