Refactor orderby and support arguments #2150

To support OrderBy with arguments, this PR have a total refactor about how to generate SQL of Select, Delete and Update.

The change to the signature of OrderBy is an API break... If someone has an interface that expects OrderBy(string) *xorm.Session that won't work anymore.

This would be rare - for example Gitea doesn't have such an interface - but it is still an API break.

If you're happy with the breaking change relating the function signature change - it may be advisable to further change the signature to OrderBy(order interface{}, args ...interface{}) at the same time:

Patch

diff --git a/engine.go b/engine.go
index 11bf930..81cfc7a 100644
--- a/engine.go
+++ b/engine.go
@@ -1006,7 +1006,7 @@ func (engine *Engine) Asc(colNames ...string) *Session {
 }
 
 // OrderBy will generate "ORDER BY order"
-func (engine *Engine) OrderBy(order string, args ...interface{}) *Session {
+func (engine *Engine) OrderBy(order interface{}, args ...interface{}) *Session {
 	session := engine.NewSession()
 	session.isAutoClose = true
 	return session.OrderBy(order, args...)
diff --git a/interface.go b/interface.go
index 5abff89..55ffebe 100644
--- a/interface.go
+++ b/interface.go
@@ -54,7 +54,7 @@ type Interface interface {
 	Nullable(...string) *Session
 	Join(joinOperator string, tablename interface{}, condition string, args ...interface{}) *Session
 	Omit(columns ...string) *Session
-	OrderBy(order string, args ...interface{}) *Session
+	OrderBy(order interface{}, args ...interface{}) *Session
 	Ping() error
 	Query(sqlOrArgs ...interface{}) (resultsSlice []map[string][]byte, err error)
 	QueryInterface(sqlOrArgs ...interface{}) ([]map[string]interface{}, error)
diff --git a/internal/statements/order_by.go b/internal/statements/order_by.go
index 97e7040..c059aa7 100644
--- a/internal/statements/order_by.go
+++ b/internal/statements/order_by.go
@@ -33,11 +33,25 @@ func (statement *Statement) WriteOrderBy(w builder.Writer) error {
 }
 
 // OrderBy generate "Order By order" statement
-func (statement *Statement) OrderBy(order string, args ...interface{}) *Statement {
+func (statement *Statement) OrderBy(order interface{}, args ...interface{}) *Statement {
 	if len(statement.orderStr) > 0 {
 		statement.orderStr += ", "
 	}
-	statement.orderStr += statement.ReplaceQuote(order)
+	var rawOrder string
+	switch order.(type) {
+	case (*builder.Builder):
+		var err error
+		rawOrder, args, err = order.(*builder.Builder).ToSQL()
+		if err != nil {
+			statement.LastError = err
+		}
+	case string:
+		rawOrder = order.(string)
+	default:
+		statement.LastError = ErrUnSupportedSQLType
+		return statement
+	}
+	statement.orderStr += statement.ReplaceQuote(rawOrder)
 	if len(args) > 0 {
 		statement.orderArgs = append(statement.orderArgs, args...)
 	}
diff --git a/session.go b/session.go
index 274b76b..388678c 100644
--- a/session.go
+++ b/session.go
@@ -275,7 +275,7 @@ func (session *Session) Limit(limit int, start ...int) *Session {
 
 // OrderBy provide order by query condition, the input parameter is the content
 // after order by on a sql statement.
-func (session *Session) OrderBy(order string, args ...interface{}) *Session {
+func (session *Session) OrderBy(order interface{}, args ...interface{}) *Session {
 	session.statement.OrderBy(order, args...)
 	return session
 }

If you're happy with the breaking change relating the function signature change - it may be advisable to further change the signature to `OrderBy(order interface{}, args ...interface{})` at the same time: <details> <summary>Patch</summary> ```patch diff --git a/engine.go b/engine.go index 11bf930..81cfc7a 100644 --- a/engine.go +++ b/engine.go @@ -1006,7 +1006,7 @@ func (engine *Engine) Asc(colNames ...string) *Session { } // OrderBy will generate "ORDER BY order" -func (engine *Engine) OrderBy(order string, args ...interface{}) *Session { +func (engine *Engine) OrderBy(order interface{}, args ...interface{}) *Session { session := engine.NewSession() session.isAutoClose = true return session.OrderBy(order, args...) diff --git a/interface.go b/interface.go index 5abff89..55ffebe 100644 --- a/interface.go +++ b/interface.go @@ -54,7 +54,7 @@ type Interface interface { Nullable(...string) *Session Join(joinOperator string, tablename interface{}, condition string, args ...interface{}) *Session Omit(columns ...string) *Session - OrderBy(order string, args ...interface{}) *Session + OrderBy(order interface{}, args ...interface{}) *Session Ping() error Query(sqlOrArgs ...interface{}) (resultsSlice []map[string][]byte, err error) QueryInterface(sqlOrArgs ...interface{}) ([]map[string]interface{}, error) diff --git a/internal/statements/order_by.go b/internal/statements/order_by.go index 97e7040..c059aa7 100644 --- a/internal/statements/order_by.go +++ b/internal/statements/order_by.go @@ -33,11 +33,25 @@ func (statement *Statement) WriteOrderBy(w builder.Writer) error { } // OrderBy generate "Order By order" statement -func (statement *Statement) OrderBy(order string, args ...interface{}) *Statement { +func (statement *Statement) OrderBy(order interface{}, args ...interface{}) *Statement { if len(statement.orderStr) > 0 { statement.orderStr += ", " } - statement.orderStr += statement.ReplaceQuote(order) + var rawOrder string + switch order.(type) { + case (*builder.Builder): + var err error + rawOrder, args, err = order.(*builder.Builder).ToSQL() + if err != nil { + statement.LastError = err + } + case string: + rawOrder = order.(string) + default: + statement.LastError = ErrUnSupportedSQLType + return statement + } + statement.orderStr += statement.ReplaceQuote(rawOrder) if len(args) > 0 { statement.orderArgs = append(statement.orderArgs, args...) } diff --git a/session.go b/session.go index 274b76b..388678c 100644 --- a/session.go +++ b/session.go @@ -275,7 +275,7 @@ func (session *Session) Limit(limit int, start ...int) *Session { // OrderBy provide order by query condition, the input parameter is the content // after order by on a sql statement. -func (session *Session) OrderBy(order string, args ...interface{}) *Session { +func (session *Session) OrderBy(order interface{}, args ...interface{}) *Session { session.statement.OrderBy(order, args...) return session } ``` </details>

I don't think we should support builder.Builder but builder.Express depends on xorm/builder#86

SQLite's documentation has a good BNF for ORDER BY:

https://www.sqlite.org/lang_select.html#x2027

An ORDER BY is followed by a comma separated list of ordering-terms

which is:

flowchart LR
node0(( ))
node1(( ))
node2(( ))
node3(( ))
collationname[collation-name]
node0 --> expr --> node1 
expr --> COLLATE --> collationname --> node1
node1 --> node2
node1 --> ASC --> node2
node1 --> DESC--> node2
node2 --> node3
node2 --> NULLS --> FIRST --> node3
NULLS --> LAST --> node3
node3 --> , --> node0

Now a *builder.cond is esssentially an expr here so it would be valid.

But we could make an explicit *builder.OrderingTerm.

My comment was that we should support general conditions - as they represent an SQL expression, see https://www.sqlite.org/syntax/expr.html

builder.Expression provides string way of doing this but actually builder.Cond is the general way of doing this.

My comment was that we should support general conditions - as they represent an SQL expression, see https://www.sqlite.org/syntax/expr.html

builder.Expression provides string way of doing this but actually builder.Cond is the general way of doing this.

I don't think Order By supports all builder.Cond. Here we use builder.Expression make the range minimal when possible.

I've linked the SQLite Documentation above. It does.

I've linked the SQLite Documentation above. It does.

But what about other databases?