Try to make the SQL queries cleaner and more secure #2893
Conversation
| if len(cond) > 0 { | ||
| sess.And(cond) | ||
| } | ||
| sess := x.Limit(20, (page-1)*20).Where("is_closed=?", isClosed).In("repo_id", rids) |
There was a problem hiding this comment.
More personal preference than anything else, but I like to have each sentence in each line:
sess := x.
Limit(20, (page-1)*20).
Where("is_closed = ?", isClosed).
In("repo_id", rids)Apart from that, 👍
There was a problem hiding this comment.
As @unknwon have not done that so far I won't change it like that :)
There was a problem hiding this comment.
Just to verify, what happens if rids is empty?
There was a problem hiding this comment.
Just to verify, what happens if rids is empty?
https://github.com/go-xorm/xorm/blob/master/statement.go#L743-L745
Need to check what happens.
There was a problem hiding this comment.
Just to verify, what happens if rids is empty?
Look above within the function, you have done already a check, e.g. if len(rids) == 0 {
|
@unknwon if you are fine with that you can already merge that. If I can improve also other parts I will create more followup pull requests. |
| if opts.UserID > 0 { | ||
| queryStr += " AND issue_user.uid=" + com.ToStr(opts.UserID) | ||
| sess.Where("issue_user.uid = ?", opts.UserID) |
There was a problem hiding this comment.
Shouldn't this be?:
sess = sess.Where("issue_user.uid = ?", opts.UserID)At least is how it would work on Gorm.
There was a problem hiding this comment.
As you can see on all the query updates above this seems to be not required.
There was a problem hiding this comment.
You may need to change to sess.And.
|
Thanks! |
| } | ||
|
|
||
| if opts.IsMention { | ||
| queryStr := "issue.id=issue_user.issue_id AND issue_user.is_mentioned=1" | ||
| sess.Join("INNER", "issue_user", "issue.id = issue_user.issue_id AND issue_user.is_mentioned = 1") |
There was a problem hiding this comment.
Why join here where it did not?
It's a start as a followup to #2892