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

Next round of db.DefaultContext refactor #27089

Merged
merged 6 commits into from
Sep 16, 2023
Merged

Next round of db.DefaultContext refactor #27089

merged 6 commits into from
Sep 16, 2023

Conversation

JakobDev
Copy link
Contributor

Part of #27065

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 15, 2023
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 15, 2023
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Sep 15, 2023
lunny
lunny reviewed Sep 15, 2023
View reviewed changes
models/issues/milestone.go Outdated Show resolved Hide resolved
@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 Sep 15, 2023
delvh
delvh reviewed Sep 15, 2023
View reviewed changes
services/issue/label.go
func RemoveLabel(issue *issues_model.Issue, doer *user_model.User, label *issues_model.Label) error {
ctx, committer, err := db.TxContext(db.DefaultContext)
func RemoveLabel(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, label *issues_model.Label) error {
dbCtx, committer, err := db.TxContext(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

I summon @wxiaoguang

Copy link
Contributor

Choose a reason for hiding this comment

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

The "notification" framework has some legacy design problems.

I guess maybe the code could be like this (and the same for ReplaceLabels or others) to avoid the ctx trick.

    err := db.WithTx(ctx, func (...) {
        ... 
    })
    if err == nil {
         notify_service.......
    }
    return err

routers/web/repo/issue_stopwatch.go Show resolved Hide resolved
modules/auth/webauthn/webauthn.go
@@ -68,7 +68,7 @@ func (u *User) WebAuthnIcon() string {

// WebAuthnCredentials implementns the webauthn.User interface
func (u *User) WebAuthnCredentials() []webauthn.Credential {
dbCreds, err := auth.GetWebAuthnCredentialsByUID(u.ID)
dbCreds, err := auth.GetWebAuthnCredentialsByUID(db.DefaultContext, u.ID)
Copy link
Member

Choose a reason for hiding this comment

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

Pity that we can't fix that at the moment as it is used in the templates.

Copy link
Member

Choose a reason for hiding this comment

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

We have ctx in templates (#26254).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but not yet for any usecase, correct?
Or can it simply be used in every template even those that don't have it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's safe to use ctx in any template. I was going to refactor more, but there is an open PR #26745 which would conflict with the ctx refactoring. I do not want to introduce too many conflicts.

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 could not find a template that is using this function

Copy link
Contributor

@wxiaoguang wxiaoguang Sep 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There are a lot (ctx)

image

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 mean GetWebAuthnCredentialsByUID

Copy link
Contributor

@wxiaoguang wxiaoguang Sep 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You are right. I think it's safe to remove func WebAuthnCredentials

(While as a refactoring PR, search&replace is good enough, I don't consider the removal is a must, the removal could be done later)

@delvh delvh added type/refactoring Existing code has been cleaned up. There should be no new functionality. and removed modifies/api This PR adds API routes or modifies them labels Sep 15, 2023
wxiaoguang
wxiaoguang approved these changes Sep 16, 2023
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

As a refactoring PR, it looks good to me. No block from my side. Some nits (like dbCtx) could be left to the future. Actually there are some other dbCtx usages in code, they could be refactored together.

@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 Sep 16, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 16, 2023
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Sep 16, 2023
@lunny lunny enabled auto-merge (squash) September 16, 2023 14:34
@lunny lunny merged commit f91dbbb into go-gitea:main Sep 16, 2023
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Sep 16, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 16, 2023
@JakobDev JakobDev deleted the nodefaultctx branch September 16, 2023 14:45
@delvh delvh modified the milestones: 1.22.0, 1.21.0 Sep 16, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 17, 2023
@zjjhot
Merge remote-tracking branch 'giteaoffical/main'
1434f7a 
* giteaoffical/main: (23 commits)
  Search branches (go-gitea#27055)
  Fix wrong migration for email address (go-gitea#27106)
  [skip ci] Updated translations via Crowdin
  Support `.git-blame-ignore-revs` file (go-gitea#26395)
  Add `RemoteAddress` to mirrors (go-gitea#26952)
  Upgrading the actions/checkout@4 (go-gitea#27096)
  Next round of `db.DefaultContext` refactor (go-gitea#27089)
  Ui correction in mobile view nav bar left aligned items. (go-gitea#27046)
  Add missing deps to files-changed (go-gitea#27100)
  Use db.WithTx for AddTeamMember to avoid ctx abuse (go-gitea#27095)
  Drop Node.js 16 and update js dependencies (go-gitea#27094)
  Fix NPE when editing OAuth2 applications (go-gitea#27078)
  Use `print` instead of `printf` (go-gitea#27093)
  Add tests for db indexer in indexer_test.go (go-gitea#27087)
  [skip ci] Updated translations via Crowdin
  Allow empty Conan files (go-gitea#27092)
  Actions are no longer experimental, so enable them by default (go-gitea#27054)
  Update brew installation documentation since gitea moved to brew core package (go-gitea#27070)
  More refactoring of `db.DefaultContext` (go-gitea#27083)
  [skip ci] Updated translations via Crowdin
  ...
silverwind added a commit to silverwind/gitea that referenced this pull request Sep 17, 2023
@silverwind
Merge remote-tracking branch 'origin/main' into usefetch
8a6e43c 
* origin/main: (53 commits)
  Search branches (go-gitea#27055)
  Fix wrong migration for email address (go-gitea#27106)
  [skip ci] Updated translations via Crowdin
  Support `.git-blame-ignore-revs` file (go-gitea#26395)
  Add `RemoteAddress` to mirrors (go-gitea#26952)
  Upgrading the actions/checkout@4 (go-gitea#27096)
  Next round of `db.DefaultContext` refactor (go-gitea#27089)
  Ui correction in mobile view nav bar left aligned items. (go-gitea#27046)
  Add missing deps to files-changed (go-gitea#27100)
  Use db.WithTx for AddTeamMember to avoid ctx abuse (go-gitea#27095)
  Drop Node.js 16 and update js dependencies (go-gitea#27094)
  Fix NPE when editing OAuth2 applications (go-gitea#27078)
  Use `print` instead of `printf` (go-gitea#27093)
  Add tests for db indexer in indexer_test.go (go-gitea#27087)
  [skip ci] Updated translations via Crowdin
  Allow empty Conan files (go-gitea#27092)
  Actions are no longer experimental, so enable them by default (go-gitea#27054)
  Update brew installation documentation since gitea moved to brew core package (go-gitea#27070)
  More refactoring of `db.DefaultContext` (go-gitea#27083)
  [skip ci] Updated translations via Crowdin
  ...
This was referenced Sep 30, 2023
Closed
Merged
lunny pushed a commit that referenced this pull request Oct 1, 2023
@CaiCandong
Fix unexpected context canceled when migrating repository (#27368)
87aa552 
Fix #27363

Regression of #27089
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny left review comments

@KN4CK3R KN4CK3R KN4CK3R approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@delvh delvh delvh 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. modifies/api This PR adds API routes or modifies them size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.
6 participants
@JakobDev @lunny @KN4CK3R @wxiaoguang @delvh @GiteaBot