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

[mattetti]

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]

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]

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

[robpike]

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]

@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]

@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]

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

[mattetti]

@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]

• 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]

@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.

[thomasheller]

@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]

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