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

Parameter support for "IN (...)" #85

Open
robfig opened this issue Sep 27, 2013 · 8 comments
Open

Parameter support for "IN (...)" #85

robfig opened this issue Sep 27, 2013 · 8 comments

Comments

@robfig
Copy link
Contributor

robfig commented Sep 27, 2013

Presently I have to manually muck around with the SQL for queries that involve an IN. It would be nice if this worked:

select * from foo where id in (?)

where the corresponding parameter is a slice.

Since slices are not allowed as parameters, the implementation for this could avoid touching the normal flow, and it could only activate when a slice parameter is detected.

I think it would have to iterate through the parameter placeholders in the query -- when it gets to the one corresponding to the slice, it would have to expand it to the length of the slice, e.g. (?, ?, ?). Since some DBs have numbered parameters, it would also have to renumber all subsequent parameter placeholders.

Only thing is that the implementation seems a little bit challenging -- for example, is it safe to assume any '?' is a placeholder, regardless of context? Maybe not. Would need a full test suite.

@purohit
Copy link
Contributor

purohit commented Sep 27, 2013

+1, I'm currently using weird work-arounds for IN support, also. Curious -- why expand to (?, ?, ?) and renumber all parameters, instead of keeping the parameter number the same, and escaping each slice value in-place? With Postgres:

ids := []int{4, 17, 3}
name := "Rob"
executor.Select("SELECT * FROM Users WHERE Id IN $1 AND Name = $2", ids, name)
// SELECT * FROM TABLE WHERE Id IN (4, 17, 3) AND Name = 'Rob';

@robfig
Copy link
Contributor Author

robfig commented Sep 27, 2013

At least in MySQL, parameters are sent separately from the query. The client library does not do the escaping; the server does.

@robfig
Copy link
Contributor Author

robfig commented Sep 27, 2013

That does bring up the possibility of either:

  • Implementing sql escaping for strings
  • Limiting the IN (?) syntax to ids ints, which do not require escaping.

@insasho
Copy link

insasho commented Oct 18, 2013

-1. I'm new to gorp but I've written numerous database abstraction layers in the past. Rewriting the user-provided query string would violate expectations of any users expecting Gorp to behave like other ORMs or database libraries, and it could reduce efficiency due to the inability to reuse pre-existing prepared statements. Implementing escaping in Go could also be less efficient and will result in challenges relating to character encodings. Using placeholders as they are designed to be used, and letting the underlying database libraries handle the details, will result in a cleaner and less surprising Gorp.

@robfig
Copy link
Contributor Author

robfig commented Oct 20, 2013

Your objections seem to be:

  1. The behavior would be surprising.
  2. Removes ability to use prepared statements
  3. Escaping in Go would be brittle and less efficient.

I'm not sure if you object to the overall idea or only the implementation suggested by @purohit -- in any case, I think none of those objections apply to the suggestion as originally posed (which does not involve SQL escaping in Go). I see no reason why the current behavior is less surprising than having IN (?) expand a slice parameter, there is no ability currently to use prepared statements (and Gorp could transparently add a prepared statement cache anyway), and the initial suggestion does not involve escaping in Go.

@insasho
Copy link

insasho commented Oct 28, 2013

Objection #1 still applies to @coopernurse's idea as well as your original suggestion, but as I've thought about this more I'm thinking the productivity wins, sometimes-confusing error messages, and other minor issues would perhaps be worth it if the manipulation is implemented in database-specific Dialects rather than on all operations.

The origin of my concern is that it requires Gorp to become confident in manipulating the query string provided by the user for the interpretation of the database. Not all query strings look like "SQL" -- for example, ODBC drivers support bizarre operations and syntaxes, complex statements like stored procedure definitions can contain ?s unsuitable for replacement, numbered placeholders, named placeholders, Postgres array notation, embedded scripting languages could use ? as an operator, etc -- but the majority of these could be addressed if Gorp only attempts to perform these operations when they are not likely to cause issues. Implementing them on Dialect might be the right place. Another benefit of adding it to Dialect is that it becomes user-configurable.

Also, the database/sql driver Valuer interface allows slices of bytes. It is thus reasonable to allow passing a slice of bytes as a query parameter when writing queries that touch blobs. There may be other cases like this that would require special treatment, so marking the slices to expand with a wrapper might be the safest thing to do.

I agree that the prepared statement cache could be added safely later.

@robfig
Copy link
Contributor Author

robfig commented Nov 2, 2013

Sure, part of the Dialect makes sense. I agree that expecting to manipulate the query in all cases is too challenging. For example, in SQLite alone there appear to be 5 different types of placeholders:
http://www.sqlite.org/c3ref/bind_blob.html

On the other side of the spectrum, MySQL just requires a sequence of '?' to be generated, which is absolutely trivial and which I could be pretty confident would not go wrong.

With respect to "How do we know if it's meant to be a slice or a single valued []byte", we could say that a list-valued parameter has to have a type marker wrapping, e.g.

type List interface{}

dbm.Select(&objs, "select id, name from foo where id in (?)", gorp.List(ids)) 

Shrug. It may be too brittle to implement for more complicated placeholders. On the other side, making everyone implement their own seems like a shame.

@insasho
Copy link

insasho commented Nov 4, 2013

+1 to the List wrapper.

Another option would be to support Gorp-specific markup that isn't specific to any dialect but would let the developer register handlers for well-defined tags. Example:

SELECT id, name FROM foo WHERE id IN ({{list "ids"}})

or even:

dbm.Exec("INSERT INTO t1 (c1, c2, c3) VALUES
  {{values t}}}", [][]interface{}{{"a","b","c"},{"d","e","f"}})

... which would prepare the following placeholder-applicable statement:

INSERT INTO t1 (c1, c2, c3) VALUES
  (?, ?, ?),
  (?, ?, ?);

The template-based approach shifts the burden of syntactical correctness onto the developer, which might be less surprising and require less magic overall.

@GeertJohan GeertJohan changed the title [Feature Request] Parameter support for "IN (...)" Parameter support for "IN (...)" Jul 1, 2015
@GeertJohan GeertJohan added this to the v2.1-maybe milestone Jul 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants