Refactor orderby and support arguments #2150

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.

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.

zeripath commented

2022-05-30 12:06:48 +00:00

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>

lunny added the

labels 2022-05-30 14:14:43 +00:00

lunny commented

2022-05-30 15:07:48 +00:00

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

I don't think we should support `builder.Builder` but `builder.Express` depends on https://gitea.com/xorm/builder/pulls/86

zeripath commented

2022-05-30 15:49:29 +00:00

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.

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: ```mermaid 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`.

👍 1

lunny added 1 commit 2022-05-31 02:03:10 +00:00

OrderBy support builder.Express

continuous-integration/drone/pr Build is passing

Details

e2d1e0c83b

lunny merged commit f9a6990ecb into master

2022-05-31 03:00:29 +00:00

lunny referenced this issue from a commit

2022-05-31 03:00:29 +00:00

Refactor orderby and support arguments (#2150)

lunny deleted branch lunny/orderby

2022-05-31 03:00:29 +00:00

zeripath commented

2022-05-31 19:36:46 +00:00

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.

lunny commented

2022-06-01 07:03:58 +00:00

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.

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

zeripath commented

2022-06-01 18:18:58 +00:00

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

lunny commented

2022-06-02 07:48:34 +00:00

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

But what about other databases?

> I've linked the SQLite Documentation above. It does. But what about other databases?

Sign in to join this conversation.

No reviewers

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: xorm/xorm#2150