xorm/xorm
xorm/xorm
63
357
Fork 207
Code Issues 373 Pull Requests 18 Actions 9 Releases 35 Activity

order by function support empty value #2423

Closed
leiwingqueen wants to merge 2 commits from leiwingqueen/xorm:fix_order_by_empty into v1
pull from: leiwingqueen/xorm:fix_order_by_empty
merge into: xorm:v1
Conversation 0 Commits 2 Files Changed 2 +37 -13
leiwingqueen commented 2024-03-01 10:13:15 +00:00
First-time contributor
Copy Link

what happen

xorm 1.3.2 support order by clause support empty string and v1.3.8 throw an error to the caller. I want xorm support this feature in the future.

it will throw an error "order by string is empty" when I invoke the function OrderBy("").

We need this feature in the case that user can change order clause and empty string means no need to order the rows

expectation

testEngine.Where("age =1 ").OrderBy("") return success

# what happen xorm 1.3.2 support order by clause support empty string and v1.3.8 throw an error to the caller. I want xorm support this feature in the future. it will throw an error "order by string is empty" when I invoke the function OrderBy(""). We need this feature in the case that user can change order clause and empty string means no need to order the rows # expectation testEngine.Where("age =1 ").OrderBy("") return success
leiwingqueen added 1 commit 2024-03-01 10:13:15 +00:00
order by function support empty value
Some checks failed
test cockroach / test cockroach (pull_request) Has been cancelled
Details
test mariadb / test mariadb (pull_request) Has been cancelled
Details
test mssql / test mssql with collation (pull_request) Has been cancelled
Details
test mssql / test mssql (pull_request) Has been cancelled
Details
test mysql / test mysql (pull_request) Has been cancelled
Details
test mysql8 / test mysql8 (pull_request) Has been cancelled
Details
test postgres / test postgres (pull_request) Has been cancelled
Details
test sqlite / unit test & test sqlite (pull_request) Has been cancelled
Details
test tidb / test tidb (pull_request) Has been cancelled
Details
b48e273924
leiwingqueen added 1 commit 2024-03-01 10:35:42 +00:00
fix asc and desc clause
Some checks failed
test cockroach / test cockroach (pull_request) Has been cancelled
Details
test mariadb / test mariadb (pull_request) Has been cancelled
Details
test mssql / test mssql with collation (pull_request) Has been cancelled
Details
test mssql / test mssql (pull_request) Has been cancelled
Details
test mysql / test mysql (pull_request) Has been cancelled
Details
test mysql8 / test mysql8 (pull_request) Has been cancelled
Details
test postgres / test postgres (pull_request) Has been cancelled
Details
test sqlite / unit test & test sqlite (pull_request) Has been cancelled
Details
test tidb / test tidb (pull_request) Has been cancelled
Details
70e3838441
lunny reviewed 2024-03-02 13:30:59 +00:00
tests/types_null_test.go
@ -273,0 +275,4 @@
assertSync(t, new(NullStruct))
if true {
err := testEngine.Where("`age` IS NOT NULL").OrderBy("").Iterate(new(NullStruct),
lunny commented 2024-03-02 13:30:59 +00:00
Owner
Copy Link

Maybe we should keep orderByStr as *string. I think OrderBy("") should return error.

Maybe we should keep orderByStr as `*string`. I think `OrderBy("")` should return error.
leiwingqueen commented 2024-03-02 14:23:57 +00:00
Author
First-time contributor
Copy Link

I suppose to be ok that if the old version throw an error. But I find the version 1.3.2 will ignore the empty string. May stay the same action with the old version is a better way

I suppose to be ok that if the old version throw an error. But I find the version 1.3.2 will ignore the empty string. May stay the same action with the old version is a better way
lunny commented 2024-03-03 06:36:49 +00:00
Owner
Copy Link

The old version will hide the possible error which I don't think it's better. Returning the error will let you find the code problem. If it hidden it, the error may never be found.

The old version will hide the possible error which I don't think it's better. Returning the error will let you find the code problem. If it hidden it, the error may never be found.
leiwingqueen commented 2024-03-04 02:41:09 +00:00
Author
First-time contributor
Copy Link

yeah, maybe you're right. I will fix it today, thanks for your advice.

yeah, maybe you're right. I will fix it today, thanks for your advice.
👍 1
leiwingqueen commented 2024-03-04 04:01:40 +00:00
Author
First-time contributor
Copy Link

Do we need to treat the orderStr as *string nor string in orderBy struct?

func (ob orderBy) CheckValid() error {
	if ob.orderStr == nil {
		return fmt.Errorf("order by string is nil")
	}
	switch t := ob.orderStr.(type) {
	case *string:
		if *t == "" {
			return fmt.Errorf("order by string is empty")
		}
		return nil
	case *builder.Expression:
		if t.Content() == "" {
			return fmt.Errorf("order by string is empty")
		}
		return nil
	default:
		return fmt.Errorf("order by string is not string point or builder.Expression")
	}
}
Do we need to treat the orderStr as *string nor string in orderBy struct? ```go func (ob orderBy) CheckValid() error { if ob.orderStr == nil { return fmt.Errorf("order by string is nil") } switch t := ob.orderStr.(type) { case *string: if *t == "" { return fmt.Errorf("order by string is empty") } return nil case *builder.Expression: if t.Content() == "" { return fmt.Errorf("order by string is empty") } return nil default: return fmt.Errorf("order by string is not string point or builder.Expression") } } ```
leiwingqueen commented 2024-03-04 04:04:49 +00:00
Author
First-time contributor
Copy Link

The caller need to change OrderBy("abc") to OrderBy(&"abc") when they upgrade the new version

The caller need to change OrderBy("abc") to OrderBy(&"abc") when they upgrade the new version
leiwingqueen commented 2024-03-05 02:18:46 +00:00
Author
First-time contributor
Copy Link

@lunny

@lunny
lunny commented 2024-03-05 16:56:47 +00:00
Owner
Copy Link

Looks like a break change.

Looks like a break change.
leiwingqueen commented 2024-03-06 10:58:14 +00:00
Author
First-time contributor
Copy Link

Same idea. That makes user confuse about this change.

So xorm framework stay the same is a better way? If user use a variable to pass the "order by param" in old version, they will only find that error in special case when they upgrade the new version.

It seems that both two solutions are not good enough.

Same idea. That makes user confuse about this change. So xorm framework stay the same is a better way? If user use a variable to pass the "order by param" in old version, they will only find that error in special case when they upgrade the new version. It seems that both two solutions are not good enough.
leiwingqueen closed this pull request 2024-03-13 16:33:18 +00:00
Some checks failed
test cockroach / test cockroach (pull_request) Has been cancelled
Required
Details
test mariadb / test mariadb (pull_request) Has been cancelled
Required
Details
test mssql / test mssql with collation (pull_request) Has been cancelled
Required
Details
test mssql / test mssql (pull_request) Has been cancelled
Required
Details
test mysql / test mysql (pull_request) Has been cancelled
Required
Details
test mysql8 / test mysql8 (pull_request) Has been cancelled
Required
Details
test postgres / test postgres (pull_request) Has been cancelled
Required
Details
test sqlite / unit test & test sqlite (pull_request) Has been cancelled
Required
Details
test tidb / test tidb (pull_request) Has been cancelled
Required
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
backport/done

   
backport/v1

   
blocked
  
db
oracle

  
db
sqlserver

  
duplicate
  
feature
cache

  
frontport/done

   
frontport/main

   
invalid
  
kind
breaking

  
kind
bug

  
kind
build

  
kind
dependencies

  
kind
docs

  
kind
driver

  
kind
enhancement

  
kind
feature

  
kind
performance

  
kind
proposal

  
kind
question

  
kind
refactor

  
kind
testing

  
need
feedback

  
need
test

  
proposal:accepted

   
RaspBerry Pi

   
regression

   
skip-changelog

   
upstream

   
wip

   
wontfix
No Label
backport/done
backport/v1
blocked
db
oracle
db
sqlserver
duplicate
feature
cache
frontport/done
frontport/main
invalid
kind
breaking
kind
bug
kind
build
kind
dependencies
kind
docs
kind
driver
kind
enhancement
kind
feature
kind
performance
kind
proposal
kind
question
kind
refactor
kind
testing
need
feedback
need
test
proposal:accepted
RaspBerry Pi
regression
skip-changelog
upstream
wip
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
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#2423
No description provided.
Delete Branch "leiwingqueen/xorm:fix_order_by_empty"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?