Skip to content

Conversation

@zacharysierakowski
Copy link

High level

Exposes two functions - ScanAll and ScanSingleRow that can be used with a Rows instance to safely/easily bind to a dest that's a struct or a scannable.

Use case

The reason for this addition is, I'm writing an wrapper around *sqlx.DB that essentially prepends a GTID check into a multi-statement. I'm trying to support GetContext and SelectContext so that they can be called without any difference to the function signature when using the wrapper (and no need for awareness of wrapper logic at all).

The code I've written so far that's running into this blocker is:

  • prepends a separate query to the incoming query
  • using QueryxContext to get the results of the multi-statement queries
  • reading the first result, doing things, then moving onto the second (NextResultSet)
  • once I'm onto the second, I basically want to bind to dest just like the standard GetContext would
  • [blocker] however, if I use something like rows.StructScan(dest), it panics if the provided dest isn't a struct

psuedo-code

func (g *DB) GetContext(ctx context.Context, dest interface{}, query string, args ...interface{}) error {
    ...
    multiStatementQuery, combinedArgs := prependQuery(query)
    rows, err := g.d.QueryxContext(ctx, multiStatementQuery, combinedArgs...)
    ...
    defer rows.Close()
    ... 
    rows.Scan(xxx) // read the first query results
    ... 
    rows.NextResultSet() // move to next result set
    ... 
    rows.StructScan(dest) // this is what fails, when a caller passes in a scannable to this wrapper
}

With this PR, the end state would be that I can swap my last line (rows.StructScan(dest)) to sqlx.ScanSingleRow(rows, dest, false).

I have the same issue with my SelectContext function where i'll need sqlx.ScanAll.

Details

This PR exposes scanAll as a public function. The function has been unmodified, other than updating the casing (so that it's public) and removing the sentence in the comment about the desirability to make it public 🙂 all calls to scanAll have been updated to ScanAll.

Additionally, this PR exposes a function called ScanSingleRow which is a subset of the existing scanAny function. It was slightly modified to support Rows or Row, but should not introduce a functional difference from the existing implementation.

Neither of these changes are breaking changes. They essentially expose the "scan x" functionality so that they can be called without needing to reflect on the dest before choosing to call something like StructScan.

Additional Context

If it means anything at all, this change is for code I'm writing @github. I'm hoping not to have to keep a sqlx fork just for this delta. If there's feedback in terms of naming, or, if you'd prefer this to somehow be put under a subdirectory/package, I'm happy to iterate here! If there's a way to accomplish this with the existing code, without reinventing a chunk of sqlx reflection/scanning, I'm happy to close this PR.

* Exposes scanAll and scanAny functionality to support multi-statement abstractions

* improve diff readability

* update func name

* fix tests

* Tidy comment

* improve test
@zacharysierakowski
Copy link
Author

@jmoiron do you have some time to review this anytime in the next week or so? 🙏

@zacharysierakowski
Copy link
Author

It doesn't look like I have permissions to request reviewers, so just going to @ mention a few folks from latest PR merges to see if anyone has the permissions to review/approve for merge.

@dlsniper @ardan-bkennedy @pkieltyka

Copy link

@cli1150 cli1150 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants