Add `ContextUser` #18798

KN4CK3R · 2022-02-17T16:06:45Z

This PR is a part of my package PR but I think it is better to have it standalone.

We have lots of places (mostly the API) where we call GetUserByParams to request the user for the current context. The method name is a bit misleading because it does not just gets the user but performs a redirect if the username was changed and so on.
This PR adds a middleware which sets a ContextUser in a single place which can be used by other methods. For routes which represent a repo or org the respective middlewares set the field too.

The change in modules/context/org.go is a bug I found while testing.
While most changes are straight forward, have a look at routers/web/user/profile.go where I had to change the Profile method with additional routes.

singuliere · 2022-02-18T13:47:54Z

routers/web/user/profile.go

-	// Show SSH keys.
-	if isShowKeys {
-		ShowSSHKeys(ctx, ctxUser.ID)
-		return
-	}
-
-	// Show GPG keys.
-	if isShowGPG {
-		ShowGPGKeys(ctx, ctxUser.ID)
-		return
-	}


Is this just removed or is it moved elsewhere? I can't find it but maybe I missed it. If it is removed, could please explain why?

Note to other reviewers: there was a hint in the description.

While most changes are straight forward, have a look at routers/web/user/profile.go where I had to change the Profile method with additional routes.

routers/web/web.go

singuliere

Very clean refactor, congrats. Easy to read too.

singuliere · 2022-02-18T13:54:30Z

@KN4CK3R did you verify all the lines where ctx.ContextUser is used instead of whatever was getting the user instead are covered by integration tests?

KN4CK3R · 2022-02-18T16:14:21Z

I would say 50:50. The following methods are not tested, the rest is covered:

admin.CreateRepo
admin.DeleteUser
feed.ShowUserFeedRSS
feed.ShowUserFeedAtom
user.ListFollowers
user.ListFollowing
user.CheckMyFollowing
user.Follow
user.Unfollow
user.GetStarredRepos
user.GetInfo
user.GetWatchedRepos

lunny · 2022-02-18T17:31:02Z

routers/api/v1/admin/org.go

@@ -67,7 +63,7 @@ func CreateOrg(ctx *context.APIContext) {
 		Visibility:  visibility,
 	}

-	if err := models.CreateOrganization(org, u); err != nil {
+	if err := models.CreateOrganization(org, ctx.ContextUser); err != nil {


This may should be login user but not contextUser? It's not this PR but before.

The comment says

username of the user that will own the created organization

so ContextUser is the correct user.

KN4CK3R · 2022-02-23T21:16:32Z

@singuliere I added the missing integration tests in #18872.

noerw

I think we could rename ctx.User together with this refactor, see my other comment.
Otherwise lgtm!

modules/context/context.go

…r-context-user

KN4CK3R · 2022-03-22T07:19:52Z

#19161 is merged. How should the ContextUser be named then? If there is a repo in the path we have ctx.Repo, if there is an organization in the path we have ctx.Org. So if we have an user in the path we should use ctx.User (that name is free now) but it may be confusing at first because it was a different user until today. Still my vote goes for ctx.User.

wxiaoguang · 2022-03-22T09:24:27Z

I still have a feeling that ctx.User is quite misleading, it's difficult to review and I'm afraid that there would be some serious bugs caused by the incorrect usage of the ctx.User .....

Since the user comes from org := ctx.Org.Organization; ctx.ContextUser = org.AsUser(), then can it be a field of Org? Then it becomes ctx.Org.User.

KN4CK3R · 2022-03-22T10:06:48Z

But /{user}/{repo} is no org. Personally I would not want it inside ctx.Org.

Since the user comes from org := ctx.Org.Organization; ctx.ContextUser = org.AsUser(), then can it be a field of Org? Then it becomes ctx.Org.User.

That is only a short path so I don't have to query the database again (similar short path is in repo assignment). But that code is not run if there is no org involved in the path.

wxiaoguang · 2022-03-22T10:19:08Z

I was thinking that Org is always non-nil and the /{user}/ can be considered as a single person org. And I have to admit that it's not ideal .....

If there is no better names, ContextUser is the choice in my mind, because it has been used widely in code and templates (I just did a grep). And maybe we can leave a clear comment for future developers that it is the user model in the request path ....

So I vote for ContextUser, the same as before in this PR (and 69 usages in templates: grep -r ContextUser templates | wc -l).

…r-context-user

zeripath · 2022-03-22T16:47:59Z

Is TargetUser a better name? Although actually ContextUser is not actually that bad really.

What is the ContextUser? Its the user that is referred to by the path in the URL - i.e. the URL Context.

KN4CK3R · 2022-03-22T17:55:33Z

I don't really like that

the context user is called ContextUser
the context organization is called Org (or Org.Organization)
the context repository is called Repo

All three should be named similar.

…r-context-user

wxiaoguang · 2022-03-24T04:16:27Z

Personally, I have a strong preference for ContextUser. (if others have other opinions, my opinion here is not a blocker).

Reasons:

The risk of misusing ContextUser is not the same as Repo or Org, Doer/User controls the access mode, and it's hard to debug if ContextUser is misused. Since most debug work is based on current doer which is the same as context user, it's hard to find the misuse.
The ContextUser has been widely used (69 lines) in templates, it is not a new concept. If we keep the ContextUser concept and name, everything is consistent and we do not need to refactor all templates. If we use User, it's hard to understand .User in templates.
In my mind, two-word variable is much easier to be grep-ed out and easier to be maintained.

And i think it's easier (and more clear) to rename Org/Repo to ContextOrg/ContextRepo (including templates) than renaming ContextUser to User ---- if necessary in future.

And TargetUser is also very accurate, if we choose it, I could contribute the future refactoring for the templates, then there is only TargetUser, no ContextUser.

zeripath · 2022-03-24T05:39:00Z

And i think it's easier (and more clear) to rename Org/Repo to ContextOrg/ContextRepo (including templates) than renaming ContextUser to User ---- if necessary in future.

Yup this would be the only consistent way of doing it.

The one slight flaw with sticking with context user is that it stutters somewhat in the go code. ctx.ContextUser Target doesn't do that but ... equally target is arguably not quite right as certainly I can imagine URLs referring to two users. Which one is the target? Base might be a better prefix but apart from the stuttering I think context user is probably fine.

wxiaoguang · 2022-03-25T03:41:44Z

Then .... if no objection, could we merge and use ContextUser at the moment, and try to find some new names and do whole refactor (templates/Org/Repo) in future?

* giteaofficial/main: Check go and nodejs version by go.mod and package.json (go-gitea#19197) Add `ContextUser` to http request context (go-gitea#18798) Set OpenGraph title to DisplayName in profile pages (go-gitea#19206)

This PR adds a middleware which sets a ContextUser (like GetUserByParams before) in a single place which can be used by other methods. For routes which represent a repo or org the respective middlewares set the field too. Also fix a bug in modules/context/org.go during refactoring.

Added ContextUser.

8337fc5

KN4CK3R added type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Feb 17, 2022

KN4CK3R added this to the 1.17.0 milestone Feb 17, 2022

Fixed swagger.

09618ac

singuliere reviewed Feb 18, 2022

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 18, 2022

singuliere reviewed Feb 18, 2022

View reviewed changes

routers/web/web.go Show resolved Hide resolved

singuliere approved these changes Feb 18, 2022

View reviewed changes

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 Feb 18, 2022

lunny reviewed Feb 18, 2022

View reviewed changes

KN4CK3R mentioned this pull request Feb 23, 2022

Add some api integration tests #18872

Merged

KN4CK3R mentioned this pull request Mar 1, 2022

Send 404 on /{org}.gpg #18959

Merged

noerw approved these changes Mar 21, 2022

View reviewed changes

modules/context/context.go Show resolved Hide resolved

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 21, 2022

KN4CK3R mentioned this pull request Mar 21, 2022

Rename ctx.User to ctx.Doer #19161

Merged

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

1ff193f

…r-context-user

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

e29433c

…r-context-user

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

a710228

…r-context-user

Merge branch 'main' into refactor-context-user

726e9c5

Merge branch 'main' into refactor-context-user

0ccc171

Merge branch 'main' into refactor-context-user

6e678a5

Merge branch 'main' into refactor-context-user

29ecbb0

wxiaoguang approved these changes Mar 26, 2022

View reviewed changes

wxiaoguang merged commit 59b867d into go-gitea:main Mar 26, 2022

wxiaoguang mentioned this pull request Mar 26, 2022

Add Package Registry #16510

Merged

KN4CK3R deleted the refactor-context-user branch March 30, 2022 19:08

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ContextUser` #18798

Add `ContextUser` #18798

KN4CK3R commented Feb 17, 2022

singuliere Feb 18, 2022

singuliere Feb 18, 2022

singuliere left a comment

singuliere commented Feb 18, 2022

KN4CK3R commented Feb 18, 2022 •

edited

Loading

lunny Feb 18, 2022

KN4CK3R Feb 18, 2022

KN4CK3R commented Feb 23, 2022

noerw left a comment

KN4CK3R commented Mar 22, 2022

wxiaoguang commented Mar 22, 2022 •

edited

Loading

KN4CK3R commented Mar 22, 2022

wxiaoguang commented Mar 22, 2022 •

edited

Loading

zeripath commented Mar 22, 2022 •

edited

Loading

KN4CK3R commented Mar 22, 2022

wxiaoguang commented Mar 24, 2022 •

edited

Loading

zeripath commented Mar 24, 2022

wxiaoguang commented Mar 25, 2022 •

edited

Loading

Add ContextUser #18798

Add ContextUser #18798

Conversation

KN4CK3R commented Feb 17, 2022

singuliere Feb 18, 2022

Choose a reason for hiding this comment

singuliere Feb 18, 2022

Choose a reason for hiding this comment

singuliere left a comment

Choose a reason for hiding this comment

singuliere commented Feb 18, 2022

KN4CK3R commented Feb 18, 2022 • edited Loading

lunny Feb 18, 2022

Choose a reason for hiding this comment

KN4CK3R Feb 18, 2022

Choose a reason for hiding this comment

KN4CK3R commented Feb 23, 2022

noerw left a comment

Choose a reason for hiding this comment

KN4CK3R commented Mar 22, 2022

wxiaoguang commented Mar 22, 2022 • edited Loading

KN4CK3R commented Mar 22, 2022

wxiaoguang commented Mar 22, 2022 • edited Loading

zeripath commented Mar 22, 2022 • edited Loading

KN4CK3R commented Mar 22, 2022

wxiaoguang commented Mar 24, 2022 • edited Loading

zeripath commented Mar 24, 2022

wxiaoguang commented Mar 25, 2022 • edited Loading

Add `ContextUser` #18798

Add `ContextUser` #18798

KN4CK3R commented Feb 18, 2022 •

edited

Loading

wxiaoguang commented Mar 22, 2022 •

edited

Loading

wxiaoguang commented Mar 22, 2022 •

edited

Loading

zeripath commented Mar 22, 2022 •

edited

Loading

wxiaoguang commented Mar 24, 2022 •

edited

Loading

wxiaoguang commented Mar 25, 2022 •

edited

Loading