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.

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

Replace all instances of db.DefaultContext with a custom context #27065

Closed
delvh opened this issue Sep 13, 2023 · 7 comments
Closed

Replace all instances of db.DefaultContext with a custom context #27065

delvh opened this issue Sep 13, 2023 · 7 comments
Labels
type/refactoring Existing code has been cleaned up. There should be no new functionality.

Comments

@delvh
Copy link
Member

delvh commented Sep 13, 2023

Problem

Early on, people often used db.DefaultContext whenever they did not have access to to a context.Context (from the Go standard library).

Nowadays, we see the problems from this approach as it means that data is added to transactions it doesn't belong, or requests hang forever waiting for some condition to be met.

Solution

As such, any database call should be started from a fitting context that is not db.DefaultContext.
The only places where db.DefaultContext is still allowed, is inside tests (files ending with test.go).
Any other occurrence should be replaced with an additional ctx context.Context function parameter (that should be the first function parameter).

Where to get the ctx param?

If you don't know where to get it from, in the worst case, you need to pass the context down from the route handler (routers/ package, i.e. routers/web/web.go) until you reach the respective function.

How to find all occurrences in need of conversion?

On Posix systems (MacOS, Linux, BSD), simply execute the following in a shell in the root directory of the gitea source code:

grep -irnH 'db\.DefaultContext' {routers,services,models,modules} | grep -v 'test.*\.go'

or if you have ag installed:

ag 'db\.DefaultContext' | grep -v 'test.*\.go' 

Alternatively, you can use whatever graphical tool you like. This works on all systems.

@delvh delvh added type/refactoring Existing code has been cleaned up. There should be no new functionality. good first issue Likely to be an easy fix labels Sep 13, 2023
@JakobDev
Copy link
Contributor

I will try to fix it at some places

@JakobDev
Copy link
Contributor

JakobDev commented Sep 14, 2023

I opened a first PR. More will follow.

btw: If someone wants to use the GitHub search, here's a Link.

techknowlogick pushed a commit that referenced this issue Sep 14, 2023
Part of #27065

This reduces the usage of `db.DefaultContext`. I think I've got enough
files for the first PR. When this is merged, I will continue working on
this.

Considering how many files this PR affect, I hope it won't take to long
to merge, so I don't end up in the merge conflict hell.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
lunny pushed a commit that referenced this issue Sep 15, 2023
@JakobDev
Copy link
Contributor

@delvh How should I deal with functions that are used by templates?

@delvh
Copy link
Member Author

delvh commented Sep 15, 2023

Yes, those are unfortunately impossible yet.
We are currently working towards implementing ctxFunctions in the templates that could be called similar to the already existing mechanism of ctx.Locale.Tr, but that process isn't finished yet.

lunny pushed a commit that referenced this issue Sep 16, 2023
lunny pushed a commit that referenced this issue Sep 25, 2023
Part of #27065

---------

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
GiteaBot pushed a commit to GiteaBot/gitea that referenced this issue Sep 25, 2023
Part of go-gitea#27065

---------

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
silverwind pushed a commit that referenced this issue Sep 25, 2023
Backport #27103 by @JakobDev

Part of #27065

Co-authored-by: JakobDev <jakobdev@gmx.de>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
lunny pushed a commit that referenced this issue Sep 29, 2023
Part of #27065

This PR touches functions used in templates. As templates are not static
typed, errors are harder to find, but I hope I catch it all. I think
some tests from other persons do not hurt.
GiteaBot pushed a commit to GiteaBot/gitea that referenced this issue Sep 29, 2023
Part of go-gitea#27065

This PR touches functions used in templates. As templates are not static
typed, errors are harder to find, but I hope I catch it all. I think
some tests from other persons do not hurt.
silverwind pushed a commit that referenced this issue Sep 29, 2023
Backport #27265 by @JakobDev

Part of #27065

This PR touches functions used in templates. As templates are not static
typed, errors are harder to find, but I hope I catch it all. I think
some tests from other persons do not hurt.

Co-authored-by: JakobDev <jakobdev@gmx.de>
lunny added a commit that referenced this issue Oct 3, 2023
Part of #27065

---------

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: delvh <dev.lh@web.de>
lunny added a commit that referenced this issue Oct 11, 2023
Part of #27065

---------

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@wxiaoguang wxiaoguang removed the good first issue Likely to be an easy fix label Oct 11, 2023
lunny pushed a commit that referenced this issue Oct 14, 2023
@JakobDev
Copy link
Contributor

This can be closed now

@delvh
Copy link
Member Author

delvh commented Oct 14, 2023

All remaining 47 instances are inherently unfixable?

@lunny
Copy link
Member

lunny commented Oct 16, 2023

I think left db.DefaultContext is not easy to change and we can close this issue.

@delvh delvh closed this as completed Oct 16, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

No branches or pull requests

4 participants