Expose query `metadata` #504

MahdiBM · 2024-08-22T19:24:01Z

Currently the functions that return PostgresRowSequence have no way of reporting back the metadata of the query.
This PR exposes the metadata.

Checklist

Add PostgresConnection tests for the new query and execute funcs
Add PostgresClient funcs + tests

codecov · 2024-08-22T19:28:29Z

Codecov Report

Attention: Patch coverage is 35.21127% with 92 lines in your changes missing coverage. Please review.

Project coverage is 61.01%. Comparing base (5d817be) to head (f388f5e).

Files with missing lines	Patch %	Lines
...es/PostgresNIO/Connection/PostgresConnection.swift	0.00%	55 Missing ⚠️
Sources/PostgresNIO/Pool/PostgresClient.swift	0.00%	34 Missing ⚠️
Sources/PostgresNIO/New/PSQLRowStream.swift	94.33%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #504      +/-   ##
==========================================
- Coverage   61.39%   61.01%   -0.39%     
==========================================
  Files         125      125              
  Lines       10149    10291     +142     
==========================================
+ Hits         6231     6279      +48     
- Misses       3918     4012      +94

Files with missing lines	Coverage Δ
Sources/PostgresNIO/New/PostgresRowSequence.swift	`90.38% <ø> (ø)`
Sources/PostgresNIO/New/PSQLRowStream.swift	`87.53% <94.33%> (+1.17%)`	⬆️
Sources/PostgresNIO/Pool/PostgresClient.swift	`0.00% <0.00%> (ø)`
...es/PostgresNIO/Connection/PostgresConnection.swift	`34.46% <0.00%> (-3.99%)`	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sources/PostgresNIO/New/PostgresRowSequence.swift

Tests/IntegrationTests/AsyncTests.swift

MahdiBM · 2024-08-27T16:10:06Z

@fabianfett I've thought a bit more about this and possibly we might want to have another type that wraps PostgresRowSequence in the next major version at least? so we can expose more stuff in the response. Not only the metadata, but maybe even the connection properties? Or who knows what.
I don't have an immediate use-case for connection properties but it looks like some lost info. Maybe someone finds a value in them although they can also be individually queried.

bridger · 2024-10-09T08:08:31Z

I'd love this addition! I have queries that should update exactly 1 row. I'd like to check that the update was successful, but I can't check without the metadata. (These queries currently don't return the row itself.)

fabianfett

I'm not a fan of the proposed API changes: They either get everything into memory, or they force users to consume the rows synchronously. I consider both bad options.

However I do have an alternative in mind:

extension PostgresConnection {
    // use this for queries where you don't expect any rows back.
    @discardableResult
    func execute(_ query: PostgresQuery) async throws -> PostgresQueryMetadata

    // use this for queries where you want to consume the rows.
    // we can use the `consume` scope to better ensure structured concurrency when consuming the rows.
    func query<Result>(
        _ query: PostgresQuery, 
        _ consume: (PostgresRowSequence) async throws -> Result
    ) async throws -> (Result, PostgresQueryMetadata)
}

@MahdiBM Wdyt?

MahdiBM · 2024-12-08T11:25:49Z

I'm not a fan of the proposed API changes: They either get everything into memory, or they force users to consume the rows synchronously. I consider both bad options.

I honestly don't think this is a problem. We're not really taking any options away.

However I do have an alternative in mind:

To be honest this looks better, at least at the first glance. So I'd be happy to move this way.

duncangroenewald · 2025-02-25T05:18:59Z

@MahdiBM what is the expected timeframe for this change to be included in a release ? Just wondering whether I should wait or fall back to using Connection.query in the interim ? I don't mind using a pre-release branch if there is one since we are in development still.

MahdiBM · 2025-03-02T18:07:08Z

@duncangroenewald IIRC you found a solution for now using that PostgresDatabase function that has a onMetadata?
Anyway, I'm not sure about when this'll be released. I finished the PR now, @fabianfett will need to review and approve when he has time, then we'll see.

MahdiBM · 2025-03-02T18:08:44Z

CodeCov report is not accurate about those funcs in the first 2 listed files not having tests.
For some reason even Xcode can't correctly find the new funcs' symbols.

I've added both unit and integration tests.

duncangroenewald · 2025-03-02T18:11:50Z

@MahdiBM - OK well if there is any chance of creating a beta branch with this added let me know as I would rather use that than have to go back and simplify things later on.

MahdiBM · 2025-03-02T18:19:53Z

@duncangroenewald I took a look and this branch is up to date with main, so you could temporarily depend on this branch (repo: https://github.com/MahdiBM/postgres-nio.git, branch: mmbm-row-seq-expose-metadata) if you want.
Or did you mean a "beta release"?

duncangroenewald · 2025-03-02T18:23:29Z

thanks that branch of yours should be fine - just don't deleted it :)

I'll let you know how it goes.

fabianfett

Quick review

fabianfett · 2025-03-03T08:54:01Z

Sources/PostgresNIO/New/PSQLRowStream.swift

+    // MARK: Drain on EventLoop
+
+    func drain() -> EventLoopFuture<Void> {
+        if self.eventLoop.inEventLoop {


This is already implemented with cancel.

cancel is not enough:

1- it doesn't wait until the query is over so metadata is available.
If you try to get the metadata too soon, the PostgresNIO code will crash.

2- it doesn't cover all cases that might happen. For example when downstream is waiting for consumer.
That might happen when the user hasn't even tried to consume the rows.

fabianfett · 2025-03-03T08:54:09Z

Sources/PostgresNIO/New/PSQLRowStream.swift

+        }
+    }
+
+    private func drain0() -> EventLoopFuture<Void> {


This is already implemented with cancel0.

See the other comment.

fabianfett · 2025-03-03T08:55:05Z

.gitignore

@@ -1,5 +1,6 @@
 .DS_Store
 /.build
+/.index-build


why have you added this one?

This is like a .build directory but for sourcekit-lsp.
In the sourcekit-lsp bundled with swift 6.1 it'll be moved into .build dir itself, but that's not out.

you need to have Swift 6.0.x and have experimental-indexing enabled (e.g. in the VSCode extension or using the sourcekit-lsp config file).

SwiftNIO already has it as well:
https://github.com/apple/swift-nio/blob/main/.gitignore#L3

fabianfett · 2025-03-03T08:55:30Z

Sources/PostgresNIO/Connection/PostgresConnection.swift

+        _ query: PostgresQuery,
+        logger: Logger,
+        file: String = #fileID,
+        line: Int = #line,


@FranzBusch what is the #isolation magic that we need here?

MahdiBM · 2025-03-03T10:00:26Z

Tests/IntegrationTests/AsyncTests.swift

+            let metadata = try await connection.execute(insertionQuery, logger: .psqlTest)
+            XCTAssertEqual(metadata.rows, rowsCount)


This is a test i originally wrote myself in another PR. Moved it to use query metadata since that's what should have been done anyway.

MahdiBM · 2025-03-03T10:02:14Z

Tests/PostgresNIOTests/New/PSQLRowStreamTests.swift

@@ -19,20 +19,29 @@ final class PSQLRowStreamTests: XCTestCase {

        XCTAssertEqual(try stream.all().wait(), [])
        XCTAssertEqual(stream.commandTag, "INSERT 0 1")
+
+        XCTAssertNoThrow(try stream.drain().wait())


all these tests are covering real-world use-cases.

we hand the postgres-sequence over to users. If they have fully consumed the rows, a drain() call should simply just do nothing. Not crash or ....

MahdiBM · 2025-03-03T10:03:08Z

Tests/PostgresNIOTests/New/PSQLRowStreamTests.swift

+        // Drain should work
+        XCTAssertThrowsError(try stream.drain().wait()) {
+            XCTAssertEqual($0 as? PSQLError, expectedError)


in case of error, drain will just throw the same error.
Though realistically if there is an error users will not reach this far in the code.

MahdiBM · 2025-03-03T10:09:15Z

Tests/PostgresNIOTests/New/PSQLRowStreamTests.swift

+        // attach consumers
+        let rowSequence = stream.asyncSequence()
+        XCTAssertEqual(dataSource.hitDemand, 0)
+        let drainFuture = stream.drain()
+        XCTAssertEqual(dataSource.hitDemand, 1)


This might happen when someone is using the new query function. an async-seq is attached but user might simply ignore it.

MahdiBM · 2025-03-03T10:18:10Z

Tests/PostgresNIOTests/New/PSQLRowStreamTests.swift

+        // attach consumers
+        let allFuture = stream.all()
+        XCTAssertEqual(dataSource.hitDemand, 1)
+        let drainFuture = stream.drain()
+        XCTAssertEqual(dataSource.hitDemand, 2)


I reckon this case won't actually happen at all with the current query functions and technically should only happen if users are misusing something like the postgres-seq by escaping it and trying to consume it weirdly or in a Task. It hits the case .waitingForAll code in drain0().

Should i remove it or just let it be incase?

I'd personally put a different preconditionFailure in case .waitingForAll and try to let users know what they did wrong + delete this test 🤔.

Or maybe we should keep that code for the sake of completeness in drain0() and having it not be like "only supposed to be used before you want to get the metadata"

MahdiBM requested review from fabianfett and gwynne as code owners August 22, 2024 19:24

MahdiBM commented Aug 23, 2024

View reviewed changes

Sources/PostgresNIO/New/PostgresRowSequence.swift Outdated Show resolved Hide resolved

MahdiBM commented Aug 23, 2024

View reviewed changes

Sources/PostgresNIO/New/PostgresRowSequence.swift Outdated Show resolved Hide resolved

MahdiBM commented Aug 23, 2024

View reviewed changes

Tests/IntegrationTests/AsyncTests.swift Outdated Show resolved Hide resolved

MahdiBM mentioned this pull request Nov 30, 2024

Problem getting query metadata from a PostgresClient query #525

Open

fabianfett requested changes Dec 8, 2024

View reviewed changes

MahdiBM force-pushed the mmbm-row-seq-expose-metadata branch from d1492cc to 61da70d Compare February 20, 2025 00:31

MahdiBM changed the title ~~Expose query metadata in PostgresRowSequence~~ Expose query metadata Feb 20, 2025

MahdiBM marked this pull request as draft February 20, 2025 00:32

Implement PostgresConnection.query and .execute with metadata

f388f5e

MahdiBM marked this pull request as ready for review March 2, 2025 18:07

MahdiBM force-pushed the mmbm-row-seq-expose-metadata branch from 5313614 to f388f5e Compare March 2, 2025 18:13

fabianfett reviewed Mar 3, 2025

View reviewed changes

MahdiBM commented Mar 3, 2025

View reviewed changes

MahdiBM requested a review from fabianfett March 3, 2025 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose query `metadata` #504

Expose query `metadata` #504

MahdiBM commented Aug 22, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading

MahdiBM commented Aug 27, 2024

bridger commented Oct 9, 2024

fabianfett left a comment •

edited

Loading

MahdiBM commented Dec 8, 2024

duncangroenewald commented Feb 25, 2025

MahdiBM commented Mar 2, 2025

MahdiBM commented Mar 2, 2025 •

edited

Loading

duncangroenewald commented Mar 2, 2025

MahdiBM commented Mar 2, 2025 •

edited

Loading

duncangroenewald commented Mar 2, 2025

fabianfett left a comment

fabianfett Mar 3, 2025

MahdiBM Mar 3, 2025

fabianfett Mar 3, 2025

MahdiBM Mar 3, 2025

fabianfett Mar 3, 2025

MahdiBM Mar 3, 2025

MahdiBM Mar 3, 2025

MahdiBM Mar 3, 2025

fabianfett Mar 3, 2025

MahdiBM Mar 3, 2025

MahdiBM Mar 3, 2025

MahdiBM Mar 3, 2025

MahdiBM Mar 3, 2025

MahdiBM Mar 3, 2025

MahdiBM Mar 3, 2025

MahdiBM Mar 4, 2025

		let metadata = try await connection.execute(insertionQuery, logger: .psqlTest)
		XCTAssertEqual(metadata.rows, rowsCount)

Expose query metadata #504

Are you sure you want to change the base?

Expose query metadata #504

Conversation

MahdiBM commented Aug 22, 2024 • edited Loading

Checklist

codecov bot commented Aug 22, 2024 • edited Loading

Codecov Report

MahdiBM commented Aug 27, 2024

bridger commented Oct 9, 2024

fabianfett left a comment • edited Loading

Choose a reason for hiding this comment

MahdiBM commented Dec 8, 2024

duncangroenewald commented Feb 25, 2025

MahdiBM commented Mar 2, 2025

MahdiBM commented Mar 2, 2025 • edited Loading

duncangroenewald commented Mar 2, 2025

MahdiBM commented Mar 2, 2025 • edited Loading

duncangroenewald commented Mar 2, 2025

fabianfett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Expose query `metadata` #504

Expose query `metadata` #504

MahdiBM commented Aug 22, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading

fabianfett left a comment •

edited

Loading

MahdiBM commented Mar 2, 2025 •

edited

Loading

MahdiBM commented Mar 2, 2025 •

edited

Loading