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

Avoid issue info panic #29625

Merged
merged 3 commits into from Mar 6, 2024
Merged

Avoid issue info panic #29625

merged 3 commits into from Mar 6, 2024

Conversation

wxiaoguang
Copy link
Contributor

Fix #29624

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 6, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 6, 2024
@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Mar 6, 2024
@wxiaoguang wxiaoguang added type/bug backport/v1.21 This PR should be backported to Gitea 1.21 labels Mar 6, 2024
@wxiaoguang wxiaoguang mentioned this pull request Mar 6, 2024
Closed
KN4CK3R
KN4CK3R reviewed Mar 6, 2024
View reviewed changes
models/activities/action.go
Comment on lines +399 to +402
for len(ret) < 3 {
ret = append(ret, "")
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we know in 2 years why that is needed? The whole delimiter concept is fragile because the values are not just numbers but user provided text which can contain the delimiter:

gitea/services/feed/action.go

Line 232 in a2b0fb1

Content: fmt.Sprintf("%d|%s", review.Issue.Index, strings.Split(comm.Content, "\n")[0]),

gitea/services/feed/action.go

Line 278 in a2b0fb1

Content: fmt.Sprintf("%d|%s", pr.Issue.Index, pr.Issue.Title),

gitea/services/feed/action.go

Line 310 in a2b0fb1

Content: fmt.Sprintf("%d|%s|%s", review.Issue.Index, reviewerName, comment.Content),

That field should be json or a struct which can be json serialized by xorm. Here for example it contains json text:

gitea/services/feed/action.go

Line 344 in a2b0fb1

Content: string(data),

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 6, 2024 •
edited

Choose a reason for hiding this comment

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

I totally agree with you, but this patch is done intentionally.

  1. The fragile | is as old 2014-07 (8dd07c0 Unknwon on 2014-07-26 at 12:24)
  2. This patch fixes the panic, and is easy to backport

Copy link
Member

Choose a reason for hiding this comment

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

I'd like don't show the comment which not have enough data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like don't show the comment which not have enough data.

Then it needs a lot of new {{if}} blocks in the templates. I don't think it's worth to make it that complex.

KN4CK3R
KN4CK3R reviewed Mar 6, 2024
View reviewed changes
models/activities/action.go Outdated Show resolved Hide resolved
@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 Mar 6, 2024
@wxiaoguang
Copy link
Contributor Author

I know there might be some different opinions about this "fix", IMO, it is good enough:

  1. It doesn't make anything worse, the proper fix could be proposed in the future.
  2. It fixes the panic for end users
  3. It is easy to backport

@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 Mar 6, 2024
@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 Mar 6, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) March 6, 2024 12:02
@wxiaoguang wxiaoguang merged commit 1d25489 into go-gitea:main Mar 6, 2024
26 checks passed
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 6, 2024
@wxiaoguang @GiteaBot
Avoid issue info panic (go-gitea#29625)
b2842df 
Fix go-gitea#29624
@GiteaBot GiteaBot mentioned this pull request Mar 6, 2024
Merged
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Mar 6, 2024
@wxiaoguang wxiaoguang deleted the fix-issue-info branch March 6, 2024 13:20
wxiaoguang added a commit that referenced this pull request Mar 6, 2024
@GiteaBot @wxiaoguang
Avoid issue info panic (#29625) (#29632)
6ee58a0 
Backport #29625 by wxiaoguang

Fix #29624

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 6, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into overflow-menu
de5074c 
* origin/main: (29 commits)
  Tweak actions color and borders (go-gitea#29640)
  Add download URL for executable files (go-gitea#28260)
  Move all login and account creation page labels to be above inputs (go-gitea#29432)
  Avoid issue info panic (go-gitea#29625)
  Cache repository default branch commit status to reduce query on commit status table (go-gitea#29444)
  Avoid unexpected panic in graceful manager (go-gitea#29629)
  Add a link for the recently pushed branch notification (go-gitea#29627)
  Fix wrong header of org project view page (go-gitea#29626)
  Detect broken git hooks (go-gitea#29494)
  Sync branches to DB immediately when handle git hook calling (go-gitea#29493)
  Fix wrong line number in code search result (go-gitea#29260)
  Make wiki default branch name changable (go-gitea#29603)
  A small refactor for agit implementation (go-gitea#29614)
  Update Twitter Logo (go-gitea#29621)
  Fix 500 error when adding PR comment (go-gitea#29622)
  Run editorconfig-checker on `locale_en-US.ini` (go-gitea#29608)
  bump protobuf module (go-gitea#29617)
  Add ac claim for old docker/build-push-action@v3 / current buildx gha cache (go-gitea#29584)
  Skip email domain check when admins edit user emails (go-gitea#29609)
  Improve natural sort (go-gitea#29611)
  ...
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 7, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
69ca301 
* giteaofficial/main:
  Use strict protocol check when redirect (go-gitea#29642)
  Update various logos and unify their filenames (go-gitea#29637)
  Tweak actions color and borders (go-gitea#29640)
  Add download URL for executable files (go-gitea#28260)
  Move all login and account creation page labels to be above inputs (go-gitea#29432)
  Avoid issue info panic (go-gitea#29625)
  Cache repository default branch commit status to reduce query on commit status table (go-gitea#29444)
  Avoid unexpected panic in graceful manager (go-gitea#29629)
  Add a link for the recently pushed branch notification (go-gitea#29627)
  Fix wrong header of org project view page (go-gitea#29626)
  Detect broken git hooks (go-gitea#29494)
  Sync branches to DB immediately when handle git hook calling (go-gitea#29493)
  Fix wrong line number in code search result (go-gitea#29260)
  Make wiki default branch name changable (go-gitea#29603)
  A small refactor for agit implementation (go-gitea#29614)
  Update Twitter Logo (go-gitea#29621)
  Fix 500 error when adding PR comment (go-gitea#29622)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KN4CK3R KN4CK3R KN4CK3R left review comments

@a1012112796 a1012112796 a1012112796 left review comments

@lunny lunny lunny 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
6 participants
@wxiaoguang @lunny @KN4CK3R @a1012112796 @delvh @GiteaBot