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

cmd/vet: warn when sql queries don't have the right amount of arguments #19471

Closed
mattetti opened this issue Mar 9, 2017 · 11 comments

Comments

Projects
None yet
10 participants
@mattetti
Copy link
Contributor

commented Mar 9, 2017

I'd like to see go vet warn me if I'm crafting a sql query using placeholders and the number of arguments isn't matching. This is a very common issue that obviously isn't caught by the compiler. Ideally, it would work similarly to the way printf vetting works.

Example:

rows, err := db.Query("SELECT name FROM users WHERE age=?", age, "matt")

The above line should get a go vet warning letting me know that 2 arguments are passed when only 1 is expected. The chance of bug increases as the number of placeholder increases such as in an update or insert.

@mdlayher mdlayher changed the title go vet warning when sql queries don't have the right amount of arguments cmd/vet: warn when sql queries don't have the right amount of arguments Mar 9, 2017

@mdlayher mdlayher added the GoCommand label Mar 9, 2017

@mdlayher

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

CC @dominikh. At a glance I didn't see a staticcheck check for this, but maybe it'd be appropriate there, if not in vet itself.

@dgryski

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2017

Or maybe https://github.com/stripe/safesql that's already focused on checking SQL?

@robpike

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2017

Vet shouldn't be checking statements from another language. It's a Go tool.

It seems there is plenty of opportunity - and existing tools - for checking SQL queries, but for my money vet is not the place for them.

@mattetti

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2017

@dgryski the stripe package is great but it's designed for security. Here I'm suggesting more of a basic check on the number of arguments provided to the database/sql package when preparing a query: https://golang.org/pkg/database/sql/#DB.Query
To be fair, I'm not even sure safesql does check the number of arguments.

@robpike I agree but I'm not asking to validate statement from other languages, just the number of arguments sent. Looking at the last 4 years of using Go in production it has been our number one cause of random (and stupid) bugs. I'm pretty confident others are in the same camp. I could certainly create a sql vet tool or make sure safesql supports that check. But most new gophers wouldn't know about the issue until it's too late (and wouldn't know about the existence of safesql or my new tool).
database/sql is a very commonly used package, I'm convinced that offering such a basic check (which would be a copy of the printf vet check) would have a big impact. Is it slightly outside of the scope of vetting Go code? yes, but doesn't affect the purity of the language, it won't require a lot of maintenance and it will save a lot of gophers' butts.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2017

@mattetti

I agree but I'm not asking to validate statement from other languages, just the number of arguments sent.

How would be the correct number of args passed to sql.DB.Query determined? Which SQL standard/dialect should vet parse?

@robpike

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2017

@mattetti You realize that means vet would need an SQL parser. Even leaving aside the dialect issue, that seems a bridge too far.

@mattetti

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2017

@robpike I didn't realize that, I thought placeholders (?, $x..) were well defined and could be looked up the same way we check printf arguments. I'd be glad to do some Big Query research to confirm that in real life, a couple placeholders are used in 90%+ cases. If I can prove that is the case and we can do a straightforward count check, would you reconsider?

@cznic

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2017

  • The grammar of printf format string is regular.
  • The grammar of SQL (whatever it means) is not, regexp will not help.
  • Named SQL parameters further complicate everything.
@shanna

This comment has been minimized.

Copy link

commented Mar 9, 2017

@mattetti I don't think you'll have much luck. SQL is too complex just to move the error from go test to go vet.

I did a code generation for postgres SQL queries yak shave recently. Because of the complexity, embedded languages, pl/pgsql, pl/perl, execute statements (sql within sql) the only practical and somewhat reliable way I found to parse postgres SQL for bind values was to throw it at the databases own parser using C bindings and traverse the AST.

@josharian josharian removed the GoCommand label Mar 9, 2017

@thomasheller

This comment has been minimized.

Copy link

commented Mar 9, 2017

@mattetti Just to confirm this, I ran your example through gometalinter --enable-all, and none of the 26 linters (including safesql) complain about it. I agree it would be worthwhile to add this check to a linter that already deals with SQL. Maybe gas or safesql, but none of them actually parse SQL, they just check if a concatenated string is passed to one of the query-related methods at the moment.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

Sounds like this isn't going to happen. Closing.

@bradfitz bradfitz closed this Mar 21, 2017

@golang golang locked and limited conversation to collaborators Mar 21, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.