sqlx
sqlx copied to clipboard
jmoiron
Validate single row count for Row.Scan
Validate that a single row only is returned by queries used for Row.Scan.
This avoids unexpected results when the query has an issue such as a missing join criteria or limit in conjunction with functions which expect only on row returned e.g. Get(...).
Also:
- Fixed missing \n's for test output of
ConnectAll.
Pull Request Test Coverage Report for Build 53
- 4 of 6 (66.67%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage decreased (-0.06%) to 71.761%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| sqlx.go | 4 | 6 | 66.67% |
| <!-- | Total: | 4 | 6 |
| Totals | |
|---|---|
| Change from base Build 32: | -0.06% |
| Covered Lines: | 1080 |
| Relevant Lines: | 1505 |
💛 - Coveralls
Pull Request Test Coverage Report for Build 55
- 4 of 6 (66.67%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage decreased (-0.06%) to 71.761%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| sqlx.go | 4 | 6 | 66.67% |
| <!-- | Total: | 4 | 6 |
| Totals | |
|---|---|
| Change from base Build 32: | -0.06% |
| Covered Lines: | 1080 |
| Relevant Lines: | 1505 |
💛 - Coveralls
How about instead of making this optional you create a 1.2.0 release and merge this after the release? It might still break some users code but it also reminds them to stick to a released version. Otherwise you end up never merging features that fix broken behavior.
I think its important this isn't gated and does cause failures as the users code is currently broken in an unpredictable way and they just don't know it.
With this fix in our code base every single case was a legitimate bug.
@glaslos not a fan of that process, as it means no one will notice and fix till the next release.
I would always expect those tracking master to be aware there may be noticeable changes, such as this one, and to avoid those one should be tracking a release branch.
@stevenh I very much agree with your sentiment. Something very similar happened to uuid: https://github.com/satori/go.uuid/issues/66 which eventually resulted in the community forking the project, leaving behind a lot of broken code and outrage.
Having read that thread it was an API breaking change where this is more a correction of missing error, which I don't think anyone would object to knowing about.
There's also a very easy fix if you SQL is broken and you don't want to fix it properly add LIMIT 1.
As a way of an update this fix has found more subtle bugs in our codebase since being deployed so continuing to have a positive impact.
I know this is really old, but just checking to see if we can get this one in?
@ardan-bkennedy this looks like a good change, but probably in a new major release since it breaks behavior. Wdyt?
