Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade xorm #27673

Merged
merged 7 commits into from Oct 19, 2023
Merged

Upgrade xorm #27673

merged 7 commits into from Oct 19, 2023

Conversation

lng2020
Copy link
Member

@lng2020 lng2020 commented Oct 18, 2023 •
edited by wolfogre

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 18, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 18, 2023
@lng2020 lng2020 added the backport/v1.21 This PR should be backported to Gitea 1.21 label Oct 18, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 18, 2023
@lng2020
Copy link
Member Author

lng2020 commented Oct 18, 2023

Need to backport because PR #27609 is backported.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 18, 2023
@lng2020
Copy link
Member Author

lng2020 commented Oct 18, 2023

CI failure is related. Seems like inappropriate usage of group by
ci_failure

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 18, 2023
@lng2020
Copy link
Member Author

lng2020 commented Oct 18, 2023

Update: when using the function GetCount, avoid using the OrderBy clause since there is no field selected.

@lng2020
Copy link
Member Author

lng2020 commented Oct 18, 2023

CI failure is related.
the latest version of xorm changes the implementation of limit when the database is mssql.
So the SQL query gets error when there is no orderby

SELECT count(*) FROM [commit_status] WHERE (repo_id = ?) AND (sha = ?) OFFSET 0 ROWS FETCH NEXT 30 ROWS ONLY

see https://gitea.com/xorm/xorm/pulls/2188 for more detail

@lunny
Copy link
Member

lunny commented Oct 18, 2023

CI failure is related. the latest version of xorm changes the implementation of limit when the database is mssql. So the SQL query gets error when there is no orderby

SELECT count(*) FROM [commit_status] WHERE (repo_id = ?) AND (sha = ?) OFFSET 0 ROWS FETCH NEXT 30 ROWS ONLY

see https://gitea.com/xorm/xorm/pulls/2188 for more detail

Looks like it's xorm's responsibility to fix the problem.

@lng2020
Copy link
Member Author

lng2020 commented Oct 18, 2023

Looks like it's xorm's responsibility to fix the problem.

Yes. That PR seems to forget to add orderby 1 in some cases. Though manually adding orderby 1 in Gitea probably will solve the problem, it's better for xorm to implement that.

lng2020
lng2020 commented Oct 18, 2023
View reviewed changes
models/unittest/unit_tests.go
@@ -96,7 +96,15 @@ func AssertExistsAndLoadMap(t assert.TestingT, table string, conditions ...any)
// GetCount get the count of a bean
func GetCount(t assert.TestingT, bean any, conditions ...any) int {
e := db.GetEngine(db.DefaultContext)
count, err := whereOrderConditions(e, conditions).Count(bean)
for _, condition := range conditions {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the limit implementation of xorm is not fixed, this code will run into a bug too. But in our cases, we just don't use limit when writing test cases so the bug is not triggered.

@lng2020
Copy link
Member Author

lng2020 commented Oct 19, 2023 •
edited

CI is passing. Merge this or wait for xorm?
Some noticeable changes:

  1. When using the function GetCount for test usage, avoid using the OrderBy id clause since id is not selected.
  2. Manually add OrderBy 1 when there is no OrderBy.

@delvh
Copy link
Member

delvh commented Oct 19, 2023

If we want to backport this PR, we should merge it soon.
I want to prepare 1.21.0-rc2.

@lunny
Copy link
Member

lunny commented Oct 19, 2023

We haven't used counting with limit yet in production codes, so I'm OK to merge this and I'm also sending https://gitea.com/xorm/xorm/pulls/2342 to try to fix it.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 19, 2023
@lunny lunny enabled auto-merge (squash) October 19, 2023 09:53
@lunny lunny merged commit e91d4f1 into go-gitea:main Oct 19, 2023
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Oct 19, 2023
@GiteaBot GiteaBot mentioned this pull request Oct 19, 2023
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Oct 19, 2023
@lng2020 @GiteaBot
Upgrade xorm (go-gitea#27673)
ff40ea4 
Related to https://gitea.com/xorm/xorm/pulls/2341
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Oct 19, 2023
silverwind pushed a commit that referenced this pull request Oct 19, 2023
@GiteaBot @lng2020
Upgrade xorm (#27673) (#27691)
89d3766 
Backport #27673 by @lng2020

Related to https://gitea.com/xorm/xorm/pulls/2341

Co-authored-by: Nanguan Lin <70063547+lng2020@users.noreply.github.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 20, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
8879756 
* upstream/main:
  Adapt `.changelog.yml` to new labeling system (go-gitea#27701)
  Update and add new eslint plugins (go-gitea#27698)
  Show total TrackedTime on issue/pull/milestone lists (go-gitea#26672)
  Add doctor dbconsistency fix to delete repos with no owner (go-gitea#27290)
  Delete repos of org when purge delete user (go-gitea#27273)
  Fix required checkboxes in issue forms (go-gitea#27592)
  Upgrade xorm (go-gitea#27673)
  [docs] Add note that PROTOCOL config is case-sensitive (go-gitea#25685)
  Remove network call on `make clean` (go-gitea#27689)
  Add unit tests for action runner token (go-gitea#27670)
  Typos fixed in documentation files (go-gitea#27687)
@lng2020 lng2020 deleted the update_xorm branch October 21, 2023 03:17
@lng2020 lng2020 mentioned this pull request Oct 27, 2023
Merged
KN4CK3R pushed a commit that referenced this pull request Oct 27, 2023
@lng2020
Upgrade xorm to 1.3.4 (#27807)
e75b89e 
Noticeable change: 
Remove the `OrderBy("1") `
[patch](#27673 (comment))
for mssql since xorm has [fixed
it](https://gitea.com/xorm/xorm/commit/0f085408afd85707635eadb2294ab52be04f3c0f).
@GiteaBot GiteaBot mentioned this pull request Oct 27, 2023
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Oct 27, 2023
@lng2020 @GiteaBot
Upgrade xorm to 1.3.4 (go-gitea#27807)
335432b 
Noticeable change: 
Remove the `OrderBy("1") `
[patch](go-gitea#27673 (comment))
for mssql since xorm has [fixed
it](https://gitea.com/xorm/xorm/commit/0f085408afd85707635eadb2294ab52be04f3c0f).
KN4CK3R pushed a commit that referenced this pull request Oct 27, 2023
@GiteaBot @lng2020
Upgrade xorm to 1.3.4 (#27807) (#27813)
2d2a565 
Backport #27807 by @lng2020

Noticeable change: 
Remove the `OrderBy("1") `
[patch](#27673 (comment))
for mssql since xorm has [fixed
it](https://gitea.com/xorm/xorm/commit/0f085408afd85707635eadb2294ab52be04f3c0f).

Co-authored-by: Nanguan Lin <70063547+lng2020@users.noreply.github.com>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@wolfogre wolfogre wolfogre approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/bug
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lng2020 @lunny @delvh @wolfogre @GiteaBot @silverwind