Forbid mixing named and positional parameters in SQL statements. #787
Conversation
| int index = i; | ||
| Argument a = params.findForPosition(i) | ||
| .orElseThrow(() -> { | ||
| String msg = String.format("Unable to execute, no positional parameter bound at position %s", |
There was a problem hiding this comment.
I would just use "Unable to execute, no positional parameter bound at position " + index. The index is the last place, so it doesn't seem useful to use String.format.
| a.apply(i + 1, statement, this.context); | ||
| } catch (SQLException e) { | ||
| throw new UnableToExecuteStatementException( | ||
| String.format("Exception while binding positional param at (0 based) position %d", |
There was a problem hiding this comment.
I think we could use a string concatenation here instead of String.format
| Argument a = params.findForName(param) | ||
| .orElseThrow(() -> { | ||
| String msg = String.format("Unable to execute, no named parameter matches \"%s\".", param); | ||
| return new UnableToExecuteStatementException(msg, context); |
There was a problem hiding this comment.
I wonder if we could use a more terse form:
() -> new UnableToExecuteStatementException(
String.format("Unable to execute, no named parameter matches \"%s\".", param),
context)| try { | ||
| a.apply(i + 1, statement, this.context); | ||
| } catch (SQLException e) { | ||
| throw new UnableToCreateStatementException(String.format("Exception while binding '%s'", |
There was a problem hiding this comment.
Nitpick: I think we need to decide with with quotes we wrap the parameter in error messages (single or double quotes).
There was a problem hiding this comment.
I like single quotes better, because then I can be lazy and not escape them
| return new UnableToExecuteStatementException(msg, context); | ||
| }); | ||
| try { | ||
| a.apply(i + 1, statement, this.context); |
| return new UnableToExecuteStatementException(msg, context); | ||
| }); | ||
| try { | ||
| a.apply(i + 1, statement, this.context); |
There was a problem hiding this comment.
I think this. superfluous, we could use just context.
| } | ||
|
|
||
| private void bindPositional(Binding params, PreparedStatement statement) { | ||
| int paramCount = stmt.getParams().size(); |
There was a problem hiding this comment.
I guess it doesn't make much sense to extract stmt.getParams().size(); to a variable. It's not used outside of the for loop condition.
| int paramCount = stmt.getParams().size(); | ||
| for (int i = 0; i < paramCount; i++) { | ||
| int index = i; | ||
| Argument a = params.findForPosition(i) |
There was a problem hiding this comment.
I think we could avoid declaring the argument variable and just invoke apply after orElseThrow.
|
Is this lined up for 2.x or 3.x We MUST NOT do this in 2.x |
|
3.0 only |
Addresses #500.
In a nutshell: if a named parameter didn't match up with e.g. the method parameter name, JDBI would attempt to match on a positional param. This could have very unexpected results if e.g. the method arguments aren't in the same order as the query.
I'm rather surprised how many of our existing tests relied on this. 馃槵
This PR adds the following restrictions:
You can still bind both positional and named, but the statement will only use one category of bindings or the other.