Refactor orderby and support arguments #2150

Merged
lunny merged 14 commits from lunny/orderby into master 3 months ago
lunny commented 3 months ago
Owner

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

To support OrderBy with arguments, this PR have a total refactor about how to generate SQL of Select, Delete and Update.
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is failing Details
f3def69fd2
Refactor orderby and support arguments
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is failing Details
6e9e65056f
function parameter
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is failing Details
8c10203fb5
more refactor
lunny force-pushed lunny/orderby from 8c10203fb5 to 213e4a7775 3 months ago
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is failing Details
17af3f4ada
refactor query
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is failing Details
fd3434d5a4
Fix bug
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is failing Details
425af34ced
more refactors
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is failing Details
6a9ce2e4f5
Refactor
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is failing Details
7b98365630
Fix test
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is passing Details
6638a6219e
Fix test
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is passing Details
d3e98adee8
Fix bug
lunny added the
kind/breaking
label 3 months ago
lunny added this to the 1.3.1 milestone 3 months ago
lunny force-pushed lunny/orderby from d3e98adee8 to 5fa0a473c7 3 months ago
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is passing Details
09156d1ecb
Make orderStr private
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is passing Details
c78313e08d
fix bug
Collaborator

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

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
kind/enhancement
kind/refactor
labels 3 months ago
Poster
Owner

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
Collaborator

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`.
lunny added 1 commit 3 months ago
continuous-integration/drone/pr Build is passing Details
e2d1e0c83b
OrderBy support builder.Express
lunny merged commit f9a6990ecb into master 3 months ago
lunny deleted branch lunny/orderby 3 months ago
Collaborator

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

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

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

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

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?
continuous-integration/drone/pr Build is passing
The pull request has been merged as f9a6990ecb.
Sign in to join this conversation.
Loading…
There is no content yet.