Proposal: CollectOneRow and QueryRow should return an error if multiple rows are returned

[jackc]

Is your feature request related to a problem? Please describe.

CollectOneRow() and QueryRow() return an error if no rows are returned by the query. However, if multiple rows are returned the first row is used and all other rows are ignored. These functions imply that the caller expects exactly one row in response. Multiple rows being returned is (almost?) always a bug.

Describe the solution you'd like

A new error (e.g. pgx.ErrTooManyRows) should be returned if more than one row is found. The first row would still be scanned or returned.

Describe alternatives you've considered

• New functions could be introduced with this behavior. e.g. CollectExactlyOneRow() or CollectOneRowStrict(). QueryRow would be left alone in this case.
• A strict mode flag could be added to connection config that enabled / disabled this behavior. (I think this is probably a bad idea.)

Additional context

This is a behavior change so it's arguable whether this change should be allowed in a minor version change. But I can't think of any legitimate use for the existing behavior. I think it is always a bug and any breakage would actually be revealing that bug.

Also, it's not clear to me whether, how, or if this behavior should cascade to database/sql (stdlib).

[mwchambers]

@jackc

A new error (e.g. pgx.ErrTooManyRows) should be returned if more than one row is found. The first row would still be scanned or returned.

👍

New functions could be introduced with this behavior. e.g. CollectExactlyOneRow() or CollectOneRowStrict(). QueryRow would be left alone in this case. A strict mode flag could be added to connection config that enabled / disabled this behavior.

👎 I vote to fix CollectOneRow (even with behaviour change)

Multiple rows being returned is (almost?) always a bug.

Certainly true for my uses of CollectOneRow and QueryRow.

This is a behavior change so it's arguable whether this change should be allowed in a minor version change. But I can't think of any legitimate use for the existing behavior. I think it is always a bug and any breakage would actually be revealing that bug.

You could argue that it's not always a bug. Some people might be relying on the behaviour of getting any row of many. But how many are unintentionally doing this?

If the intent is to take the first of many we can be explicit and LIMIT 1

[eandre]

I think it would be a mistake to make QueryRow return an error for this, because it's a backwards incompatible change. Breaking a query that has worked in the past, both with pgx and the stdlib, is quite troubling to me. It's fair to argue the user should have used LIMIT 1 or similar, but given that it's always worked in the past I can see why one might think it's redundant if the results are deterministically sorted and QueryRow is used.

For what it's worth, we try to keep Encore up to date with the latest pgx minor release, but making that change would make us reconsider. We take backwards compatibility seriously and we don't want to break an application that works fine just because we upgrade our dependency on pgx to a new minor version.

[jackc]

Breaking a query that has worked in the past, both with pgx and the stdlib, is quite troubling to me. It's fair to argue the user should have used LIMIT 1 or similar, but given that it's always worked in the past I can see why one might think it's redundant if the results are deterministically sorted and QueryRow is used.

I guess that's the key question -- was it actually working in the past? Or is the current behavior hiding an error?

I agree that if the query has an order by then the current behavior is working, but I would also say that if there isn't an order by the current behavior is hiding an application bug.

[eandre]

I agree that if the query has an order by then the current behavior is working, but I would also say that if there isn't an order by the current behavior is hiding an application bug.

Yes, I agree. I don't think it's the right decision to break working code because some other code may be broken, though.

It's always nice to prevent a bug in the first place as a library author by means of designing an API that prevents misuse, and if this API was brand-new I would totally be in favor of the behavior you're advocating for.

But this API isn't new, and has a bunch of users, and the name QueryRow has many years of established semantics, I think that changes the equation significantly. There are valid, well-defined queries and code written that relies on the current behavior, that would be broken by making this change. And while I agree with the goal of preventing misuse, the existing semantics are well-documented and have a long history, so ultimately the application authors using QueryRow have to take responsibility for using it correctly.

In my opinion the negatives of changing QueryRow outweigh the positives. But this is just my opinion; do with it as you wish :)

[jackc]

CollectExactlyOneRow now exists for people to opt-in to that behavior. No change will be made to the current behavior of CollectOneRow and QueryRow.