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

Simplify the code to get issue count #19380

Merged
merged 4 commits into from
Apr 25, 2022
Merged

Simplify the code to get issue count #19380

merged 4 commits into from
Apr 25, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Apr 12, 2022 •
edited
Loading

Since it's a count SQL, we just need to use Count.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 12, 2022
@lunny lunny added this to the 1.17.0 milestone Apr 12, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 12, 2022
@delvh
Copy link
Member

delvh commented Apr 12, 2022

Wait, does that actually work?
I mean, it compiles, but will the runtime behavior be the same as before?

delvh
delvh reviewed Apr 12, 2022
View reviewed changes
models/issue.go Outdated Show resolved Hide resolved
@lunny lunny mentioned this pull request Apr 12, 2022
Closed
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 12, 2022 •
edited
Loading

This PR is not ideal and the underlying problem is still not resolved.

  • If this PR is correct, then it means the old code sess.Find(&countsSlice) is wrong. how can it be? then there are more problems:
    • are all legacy code like this correct?
    • should it be a bug in XORM or MySQL? if yes, should it be fixed in XORM side or should detect the buggy MySQL version?
  • If this PR is incorrect, it only hides the underlying bug and returns the incorrect 0 in some cases, which causes that the problem becomes more difficult to be debugged.

@lunny lunny force-pushed the lunny/simple_issue_code branch from b78428b to 16103bb Compare April 12, 2022 11:41
@lunny
Copy link
Member Author

lunny commented Apr 12, 2022

This PR is not ideal and the underlying problem is still not resolved.

  • If this PR is correct, then it means the old code sess.Find(&countsSlice) is wrong. how can it be? then there are more problems:

    • are all legacy code like this correct?
    • should it be a bug in XORM or MySQL? if yes, should it be fixed in XORM side or should detect the buggy MySQL version?

  • If this PR is incorrect, it only hides the underlying bug and returns the incorrect 0 in some cases, which causes that the problem becomes more difficult to be debugged.

As I said, this PR is not a bug fix. I haven't found the problem of the previous code.
It's a refactor which make the code more simple and more readability.

@wxiaoguang
Copy link
Contributor

Does XORM report an error if Count meets 0 row in result? If no, how do you know that if the bug still exists or is resolved after this PR? There will be no error in future even there is still a related bugs.

@lunny
Copy link
Member Author

lunny commented Apr 12, 2022

Does XORM report an error if Count meets 0 row in result? If no, how do you know that if the bug still exists or is resolved after this PR? There will be no error in future even there is still a related bugs.

If there is a bug there, it's not related XORM or Gitea, it's an upstream problem. Mysql-driver or mysql server.
As a SQL standard, it should always return 1 record if there is no error.

@codecov-commenter

This comment was marked as off-topic.

Sign in to view
@wxiaoguang
Copy link
Contributor

Does XORM report an error if Count meets 0 row in result? If no, how do you know that if the bug still exists or is resolved after this PR? There will be no error in future even there is still a related bugs.

If there is a bug there, it's not related XORM or Gitea, it's an upstream problem. Mysql-driver or mysql server. As a SQL standard, it should always return 1 record if there is no error.

Then should Gitea hide such bug, or report such problem to end users (at least in logs)?

@techknowlogick techknowlogick changed the title Simple the code to get issue count Simplify the code to get issue count Apr 12, 2022
@lunny
Copy link
Member Author

lunny commented Apr 13, 2022

Does XORM report an error if Count meets 0 row in result? If no, how do you know that if the bug still exists or is resolved after this PR? There will be no error in future even there is still a related bugs.

If there is a bug there, it's not related XORM or Gitea, it's an upstream problem. Mysql-driver or mysql server. As a SQL standard, it should always return 1 record if there is no error.

Then should Gitea hide such bug, or report such problem to end users (at least in logs)?

I would like we do the later.

@wxiaoguang
Copy link
Contributor

Now we have a good chance to find the root case about the bug that SELECT COUNT() returns 0 row.

If this PR gets merged, no way and nobody would continue the debug work.

I prefer to find the root case of the bug first, then take this refactoring.

wxiaoguang
wxiaoguang approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The issue (#19368) itself is a MariaDB bug with prepared statements.

#19368 (comment)

To handle all similar problems gracefully in future, some improvements from XORM side:

  1. Make XORM's Count return an error if the result is 0 row.
  2. Make XORM report warnings for SQLs with more than xxxx parameters (eg: 16384), then users/developers can know there is something needed to be refactored, they have enough time to optimize the query before the hard limit comes.

@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 Apr 25, 2022
@wxiaoguang
Copy link
Contributor

I do not think it's worth to be backported, it's not a bug fix.

@lunny lunny merged commit fc00286 into go-gitea:main Apr 25, 2022
@lunny lunny deleted the lunny/simple_issue_code branch April 25, 2022 07:04
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 25, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
59edd92 
* giteaofficial/main:
  Simplify the code to get issue count (go-gitea#19380)
  use IsLoopback (go-gitea#19477)
  [skip ci] Updated translations via Crowdin
  Add RSS Feed buttons to Repo, User and Org pages (go-gitea#19370)
  [doctor] authorized-keys: fix displayed check name (go-gitea#19464)
  [skip ci] Updated translations via Crowdin
  Use horizontal tabs for repo header on mobile (go-gitea#19468)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@lunny @AbdulrhmnGhanem
Simplify the code to get issue count (go-gitea#19380)
6a4c910 
* Simple the code to get issue count

* Improve codes
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
5 participants
@lunny @delvh @wxiaoguang @codecov-commenter @GiteaBot