search

vapor / postgres-nio

🐘 Non-blocking, event-driven Swift client for PostgreSQL.
https://api.vapor.codes/postgresnio/documentation/postgresnio/
MIT License
304 stars 70 forks source link

Fixes `LISTEN` to quote channel name #466

Closed NeedleInAJayStack closed 3 months ago

NeedleInAJayStack commented 3 months ago

This adjusts the SQL LISTEN statement called by PostgresConnection.addListener to quote the channel name.

For example, previously addListener("abc") would run LISTEN abc; and it will now run LISTEN "abc";

As far as I can tell, this only matters when the channel name is a reserved Postgres keyword, like default.

NeedleInAJayStack commented 3 months ago

This behavior appears to have been introduced in v1.18.0. Interestingly, by watching SQL logs it appears that version also changed addListener to call LISTEN automatically, so some of the notification tests are calling LISTEN twice.

fabianfett commented 3 months ago

This behavior appears to have been introduced in v1.18.0. Interestingly, by watching SQL logs it appears that version also changed addListener to call LISTEN automatically, so some of the notification tests are calling LISTEN twice.

@NeedleInAJayStack Does this mean you actually used the old LISTEN API?

NeedleInAJayStack commented 3 months ago

Does this mean you actually used the old LISTEN API?

Yeah, we were using the old LISTEN API. However, my comment was more related to these tests, and specifically that the lines with conn?.simpleQuery("LISTEN example").wait() are technically no longer needed since addListener now calls LISTEN example;. When watching the SQL logs I could see two LISTEN example queries in a row.

NeedleInAJayStack commented 3 months ago

I'm able to recreate the TinyFastSequenceTests.testReserveCapacityIsForwarded CI failures in a swift:5.10-jammy container locally:

/repo/Tests/ConnectionPoolModuleTests/TinyFastSequenceTests.swift:37: error: TinyFastSequenceTests.testReserveCapacityIsForwarded : XCTAssertEqual failed: ("9") is not equal to ("8") - 
/repo/Tests/ConnectionPoolModuleTests/TinyFastSequenceTests.swift:46: error: TinyFastSequenceTests.testReserveCapacityIsForwarded : XCTAssertEqual failed: ("9") is not equal to ("8") -
/repo/Tests/ConnectionPoolModuleTests/TinyFastSequenceTests.swift:53: error: TinyFastSequenceTests.testReserveCapacityIsForwarded : XCTAssertEqual failed: ("9") is not equal to ("8") -

I'm able to recreate these errors on main, and going back to v1.19.0 where these tests were introduced. I'm not sure why the CI passed on previous MRs though. Do you guys have any idea what might be different now compared with those other MRs?

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 61.73%. Comparing base (8f8724e) to head (12c5d5d).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #466 +/- ## ========================================= + Coverage 0 61.73% +61.73% ========================================= Files 0 125 +125 Lines 0 10024 +10024 ========================================= + Hits 0 6188 +6188 - Misses 0 3836 +3836 ``` | [Files](https://app.codecov.io/gh/vapor/postgres-nio/pull/466?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor) | Coverage Ξ” | | |---|---|---| | [...urces/PostgresNIO/New/PostgresChannelHandler.swift](https://app.codecov.io/gh/vapor/postgres-nio/pull/466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9Qb3N0Z3Jlc05JTy9OZXcvUG9zdGdyZXNDaGFubmVsSGFuZGxlci5zd2lmdA==) | `84.31% <100.00%> (ΓΈ)` | | ... and [124 files with indirect coverage changes](https://app.codecov.io/gh/vapor/postgres-nio/pull/466/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor)