Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

database/sql: QueryRow should verify that we have only one row #20163

Closed
RaduBerinde opened this issue Apr 28, 2017 · 4 comments
Closed

database/sql: QueryRow should verify that we have only one row #20163

RaduBerinde opened this issue Apr 28, 2017 · 4 comments
Assignees
Milestone

Comments

@RaduBerinde
Copy link
Contributor

@RaduBerinde RaduBerinde commented Apr 28, 2017

QueryRow is documented as follows:

// QueryRow executes a query that is expected to return at most one row.
// QueryRow always returns a non-nil value. Errors are deferred until
// Row's Scan method is called.

This suggests that we should get an error if the query returns more than one row. However Row.Scan() doesn't check that r.rows.Next() returns false after processing one row, so it would ignore any remaining rows.

Conversely "at most one row" suggests that getting no rows is somehow acceptable, but an error is returned in that case. I think it should say "exactly one row".

@kardianos
Copy link
Contributor

@kardianos kardianos commented Apr 28, 2017

I'm not convinced there is a code change that needs to happen. I do think the docs could be clearer on results. I think the current behavior is what is most often desired, but that the docs could make that behavior clearer.

@slrz
Copy link

@slrz slrz commented Apr 28, 2017

Conversely "at most one row" suggests that getting no rows is somehow acceptable, but an error is returned in that case. I think it should say "exactly one row".

No, it should not. If the query returns no rows, you get a ErrNoRows on Scan. That's useful and constitutes valid usage of QueryRow,

Also, I think you're reading this the wrong way: the "expected to return at most one row" part places an obligation on the user of QueryRow and not on its implementation. You're not supposed to use it in the first place if your query can return more than one row.

As the existing implementation allowed it, changing it to return an error now has the potential to break code that relied on it returning the first result (whatever that is).

@RaduBerinde
Copy link
Contributor Author

@RaduBerinde RaduBerinde commented Apr 29, 2017

I can see why changing the behavior is undesirable. but at least the documentation should be clarified. It needs to say that it scans the first row (ignoring others), and that ErrNoRows is returned if there are no rows.

@RaduBerinde RaduBerinde reopened this Apr 29, 2017
@bradfitz bradfitz added this to the Go1.9 milestone Apr 29, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 5, 2017

CL https://golang.org/cl/44890 mentions this issue.

@adams-sarah adams-sarah self-assigned this Jun 5, 2017
@gopherbot gopherbot closed this in 3bcdbe5 Jun 12, 2017
@golang golang locked and limited conversation to collaborators Jun 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.