Avatar refactor, move avatar code from models to models.avatars, remove duplicated code #17123

wxiaoguang · 2021-09-22T13:58:09Z

wxiaoguang commented

2021-09-22 13:58:09 +00:00

Why this refactor

The goal is to move most files from models package to models.xxx package. Many models depend on avatar model, so just move this first.

And the existing logic is not clear, there are too many function like AvatarLink, RelAvatarLink, SizedRelAvatarLink, SizedAvatarLink, MakeFinalAvatarURL, HashedAvatarLink, etc. This refactor make everything clear:

user.AvatarLink()
user.AvatarLinkWithSize(size)
avatars.GenerateEmailAvatarFastLink(email, size)
avatars.GenerateEmailAvatarFinalLink(email, size)

And many duplicated code are deleted in route handler, the handler and the model share the same avatar logic now.

Why this refactor The goal is to move most files from `models` package to `models.xxx` package. Many models depend on avatar model, so just move this first. And the existing logic is not clear, there are too many function like `AvatarLink`, `RelAvatarLink`, `SizedRelAvatarLink`, `SizedAvatarLink`, `MakeFinalAvatarURL`, `HashedAvatarLink`, etc. This refactor make everything clear: * user.AvatarLink() * user.AvatarLinkWithSize(size) * avatars.GenerateEmailAvatarFastLink(email, size) * avatars.GenerateEmailAvatarFinalLink(email, size) And many duplicated code are deleted in route handler, the handler and the model share the same avatar logic now.

👍 2 ❤️ 1

delvh reviewed 2021-09-22 17:27:53 +00:00

models/avatars/avatar.go

						
				@ -0,0 +102,4 @@

							// Seriously we don't care about any DB problems just return the lowerEmail - we expect the transaction to fail most of the time

							return lowerEmail, nil

						}

						return lowerEmail, nil

delvh commented

2021-09-22 16:55:24 +00:00

Can't it be written as the following, or am I misunderstanding something here?

	return setting.AppURL + "/user/avatar/" + userName + "/" + strconv.Itoa(size)

Alternatively, if I interpret the [1:] correctly:

	return setting.AppURL + "user/avatar/" + userName + "/" + strconv.Itoa(size)

Alternatively, the version using formatted strings would be possible here but that would require the re-import of fmt.

Can't it be written as the following, or am I misunderstanding something here? ```suggestion return setting.AppURL + "/user/avatar/" + userName + "/" + strconv.Itoa(size) ``` Alternatively, if I interpret the `[1:]` correctly: ```suggestion return setting.AppURL + "user/avatar/" + userName + "/" + strconv.Itoa(size) ``` Alternatively, the version using formatted strings would be possible here but that would require the re-import of `fmt`.

models/avatars/avatar.go

delvh commented

2021-09-22 17:11:37 +00:00

Why is the size here lower bounded to 0, and in line 126 below to DefaultAvatarSize? I think these two should be set to the same value.

Why is the size here lower bounded to `0`, and in line 126 below to `DefaultAvatarSize`? I think these two should be set to the same value.

models/user_avatar.go

						
				@ -60,61 +59,41 @@ func (u *User) generateRandomAvatar(e db.Engine) error {

					return nil

delvh commented

2021-09-22 17:21:26 +00:00

		return avatars.GenerateEmailAvatarFastLink(u.AvatarEmail, size)

By doing that, you can remove the whole useLocalAvatar variable and make the calls below a little easier to read. Especially the return in 96 can then be removed.

```suggestion return avatars.GenerateEmailAvatarFastLink(u.AvatarEmail, size) ``` By doing that, you can remove the whole `useLocalAvatar` variable and make the calls below a little easier to read. Especially the return in 96 can then be removed.

delvh commented

2021-09-22 17:22:01 +00:00

Does something speak against

		else if size > 0 {

here?

Does something speak against ```suggestion else if size > 0 { ``` here?

delvh commented

2021-09-22 17:22:35 +00:00

And this would then be the else block.

wxiaoguang reviewed 2021-09-22 17:43:58 +00:00

models/avatars/avatar.go

						
				@ -0,0 +102,4 @@

							// Seriously we don't care about any DB problems just return the lowerEmail - we expect the transaction to fail most of the time

							return lowerEmail, nil

						}

						return lowerEmail, nil

wxiaoguang commented

2021-09-22 17:43:58 +00:00

They are from the legacy code. I do not want to change the logic now.

func (u *User) SizedRelAvatarLink(size int) string {
	return setting.AppSubURL + "/user/avatar/" + u.Name + "/" + strconv.Itoa(size)
}
func (u *User) RelAvatarLink() string {
	return u.SizedRelAvatarLink(DefaultAvatarSize)
}
func (u *User) AvatarLink() string {
	link := u.RelAvatarLink()
	if link[0] == '/' && link[1] != '/' {
		return setting.AppURL + strings.TrimPrefix(link, setting.AppSubURL)[1:]
	}
	return link
}

They are from the legacy code. I do not want to change the logic now. ``` func (u *User) SizedRelAvatarLink(size int) string { return setting.AppSubURL + "/user/avatar/" + u.Name + "/" + strconv.Itoa(size) } func (u *User) RelAvatarLink() string { return u.SizedRelAvatarLink(DefaultAvatarSize) } func (u *User) AvatarLink() string { link := u.RelAvatarLink() if link[0] == '/' && link[1] != '/' { return setting.AppURL + strings.TrimPrefix(link, setting.AppSubURL)[1:] } return link } ```

wxiaoguang reviewed 2021-09-22 17:46:04 +00:00

models/user_avatar.go

						
				@ -60,61 +59,41 @@ func (u *User) generateRandomAvatar(e db.Engine) error {

					return nil

wxiaoguang commented

2021-09-22 17:46:04 +00:00

I'd like to put all generated / returned URL together, it makes the flow more clear. There may be more switch-cases which doesn't use local avatar and needs more logic, current flow can be extended easily.

wxiaoguang reviewed 2021-09-22 17:48:27 +00:00

models/user_avatar.go

						
				@ -60,61 +59,41 @@ func (u *User) generateRandomAvatar(e db.Engine) error {

					return nil

wxiaoguang commented

2021-09-22 17:48:27 +00:00

The else doesn't help about the logic. And the size and u.Avatar are independent variables, it's better to keep them in different if.

The `else` doesn't help about the logic. And the `size` and `u.Avatar` are independent variables, it's better to keep them in different `if`.

wxiaoguang reviewed 2021-09-22 17:48:55 +00:00

models/user_avatar.go

						
				@ -60,61 +59,41 @@ func (u *User) generateRandomAvatar(e db.Engine) error {

					return nil

wxiaoguang commented

2021-09-22 17:48:55 +00:00

Explained above.

wxiaoguang reviewed 2021-09-22 17:50:38 +00:00

models/avatars/avatar.go

wxiaoguang commented

2021-09-22 17:50:38 +00:00

0 and -1 have same meaning in related logic, all will be default avatar size.

I prefer to use 0 here because the generated URL /user/avatar/user-name/0 is more beautiful than /user/avatar/user-name/-1

`0` and `-1` have same meaning in related logic, all will be default avatar size. I prefer to use 0 here because the generated URL `/user/avatar/user-name/0` is more beautiful than `/user/avatar/user-name/-1`

wxiaoguang reviewed 2021-09-23 06:25:45 +00:00

models/avatars/avatar.go

						
				@ -0,0 +102,4 @@

							// Seriously we don't care about any DB problems just return the lowerEmail - we expect the transaction to fail most of the time

							return lowerEmail, nil

						}

						return lowerEmail, nil

wxiaoguang commented

2021-09-23 06:25:44 +00:00

@delvh just searched a while, I think we use setting.AppSubURL + "/user/avatar/" + userName + "/" + strconv.Itoa(size) can be better here. I am not sure about should we use AppURL or StaticURLPrefix. Anyway, it just works as before now.

@delvh just searched a while, I think we use `setting.AppSubURL + "/user/avatar/" + userName + "/" + strconv.Itoa(size)` can be better here. I am not sure about `should we use AppURL or StaticURLPrefix`. Anyway, it just works as before now.

wxiaoguang reviewed 2021-09-23 06:28:18 +00:00

models/avatars/avatar.go

wxiaoguang commented

2021-09-23 06:28:18 +00:00

Now I removed the useless constant DefaultAvatarSize. Only 0 .

Now I removed the useless constant `DefaultAvatarSize`. Only `0` .

silverwind (Migrated from github.com) reviewed 2021-09-23 15:04:32 +00:00

routers/web/user/avatar.go

silverwind (Migrated from github.com) commented

2021-09-23 15:04:31 +00:00

I don't agree to lowering this because it creates different caching behaviour of local vs. remote (Gravatar) avatars. Local avatars don't hit this code path so are still subject to setting.StaticCacheTime.

I think we should introduce a new config option for avatar cache time (default 5 minutes), which should apply to both this redirect and the cache headers of local avatars.

I don't agree to lowering this because it creates different caching behaviour of local vs. remote (Gravatar) avatars. Local avatars don't hit this code path so are still subject to `setting.StaticCacheTime`. I think we should introduce a new config option for avatar cache time (default 5 minutes), which should apply to both this redirect and the cache headers of local avatars.

wxiaoguang reviewed 2021-09-23 15:12:56 +00:00

routers/web/user/avatar.go

wxiaoguang commented

2021-09-23 15:12:56 +00:00

It doesn't create different caching behavior of local vs. remote (Gravatar) avatars, because we can not control or can not depend on Gravatar's cache behavior, so the difference is not important at all.

5 minutes is still too long. Why should we make a user can't see his new avatar in 5 minutes? And I do not think we should introduce so many config options, it only makes users more confused.

The cacheableRedirect was just introduced recently, there is no evidence any user benefits from it. The original issue was not likely related to it, either. The server has worked correctly for a long time before, a one-minute cacheable redirection is already better than before, and won't make users confused too much. If the cahce is not a must, we can choose to remove the cacheableRedirect and revert to the old behavior.

It doesn't create different caching behavior of local vs. remote (Gravatar) avatars, because we can not control or can not depend on Gravatar's cache behavior, so the difference is not important at all. 5 minutes is still too long. Why should we make a user can't see his new avatar in 5 minutes? And I do not think we should introduce so many config options, it only makes users more confused. The `cacheableRedirect` was just introduced recently, there is no evidence any user benefits from it. The original issue was not likely related to it, either. The server has worked correctly for a long time before, a one-minute cacheable redirection is already better than before, and won't make users confused too much. If the cahce is not a must, we can choose to remove the `cacheableRedirect` and revert to the old behavior.

delvh reviewed 2021-09-23 15:26:49 +00:00

routers/web/user/avatar.go

delvh commented

2021-09-23 15:26:49 +00:00

But to be fair: How often do you change your avatar?
Once or twice a year? If at all?
So I can definitely understand caching it for a longer time.
If needed, the user can still force his cache to be cleared.

But to be fair: How often do you change your avatar? Once or twice a year? If at all? So I can definitely understand caching it for a longer time. If needed, the user can still force his cache to be cleared.

wxiaoguang reviewed 2021-09-23 15:32:27 +00:00

routers/web/user/avatar.go

wxiaoguang commented

2021-09-23 15:32:27 +00:00

But the 302 request doesn't cost much, it's not necessary to pay much attention to a fast 302 request. And even more, with this refactor, most local avatar images are responded by a final URL, they do not need the 302 redirection anymore.

The 302 redirection only occurs in some rare cases. So my first preference is to revert to the old behavior (302 without cache). Second preference is to make the 302 cache time not more than one minute (or 5 minutes at most ?). It's not worthy to introduce more config options.

But the 302 request doesn't cost much, it's not necessary to pay much attention to a fast 302 request. And even more, with this refactor, most local avatar images are responded by a `final` URL, they do not need the 302 redirection anymore. The 302 redirection only occurs in some rare cases. So my first preference is to revert to the old behavior (302 without cache). Second preference is to make the 302 cache time not more than one minute (or 5 minutes at most ?). It's not worthy to introduce more config options.

silverwind (Migrated from github.com) reviewed 2021-09-23 15:58:28 +00:00

routers/web/user/avatar.go

silverwind (Migrated from github.com) commented

2021-09-23 15:58:27 +00:00

Yes, we should get rid of this redirect, at least for the cases where the URL can be synchonously obtained, which is certainly possible for Gravatar, but not for some other federated services like libravatar.org, IIRC.

So Gravatar should just render img src="<gravatar-url>". Is this the case after this refactor?

I guess I'm fine with this short cache on the cases where redirect needs to stay. It's not like I'd use services like libravatar.org :)

Yes, we should get rid of this redirect, at least for the cases where the URL can be synchonously obtained, which is certainly possible for Gravatar, but not for some other federated services like libravatar.org, IIRC. So Gravatar should just render `img src="<gravatar-url>"`. Is this the case after this refactor? I guess I'm fine with this short cache on the cases where redirect needs to stay. It's not like I'd use services like libravatar.org :)

wxiaoguang reviewed 2021-09-23 16:09:50 +00:00

routers/web/user/avatar.go

wxiaoguang commented

2021-09-23 16:09:50 +00:00

Yes, in most cases user's Gravatar image will be rendered as img src="<gravatar-url>", and user's custom avatar will be rendered as img src="<real-local-avatars-url>".

The only cases that our 302 redirection occurs:

In some cases we only have user name, then we use /user/avatar/${name}/${size} (currently no such case, there is no caller to GenerateUserAvatarFastLink now, this function is for potential further cases)
In some cases we only have E-mail hash (eg: git commit log) AND LibravatarService is used, then we use /avatar/${hash}

I can not find more cases that need the 302 redirection. So the cases are rare enough.

Yes, in most cases user's Gravatar image will be rendered as `img src="<gravatar-url>"`, and user's custom avatar will be rendered as `img src="<real-local-avatars-url>"`. The only cases that our 302 redirection occurs: 1. In some cases we only have user name, then we use `/user/avatar/${name}/${size}` (currently no such case, there is no caller to `GenerateUserAvatarFastLink` now, this function is for potential further cases) 2. In some cases we only have E-mail hash (eg: git commit log) AND `LibravatarService` is used, then we use `/avatar/${hash}` I can not find more cases that need the 302 redirection. So the cases are rare enough.

lunny approved these changes 2021-09-29 06:40:32 +00:00

zeripath reviewed 2021-09-29 19:22:42 +00:00

models/avatars/avatar.go

						
				@ -0,0 +100,4 @@

							return nil

						}); err != nil {

							// Seriously we don't care about any DB problems just return the lowerEmail - we expect the transaction to fail most of the time

							return lowerEmail, nil

zeripath commented

2021-09-29 19:22:42 +00:00

You're pointing to /user/avatar/:username/:size here so AppSubURL is the correct thing.

You're pointing to /user/avatar/:username/:size here so AppSubURL is the correct thing. ```suggestion ```

zeripath reviewed 2021-09-29 19:40:12 +00:00

models/avatars/avatar.go

						
				@ -0,0 +100,4 @@

							return nil

						}); err != nil {

							// Seriously we don't care about any DB problems just return the lowerEmail - we expect the transaction to fail most of the time

							return lowerEmail, nil

zeripath commented

2021-09-29 19:40:12 +00:00

might be better to extract this out to a function then you can return the result of this function at 144 and at 154.

That would allow you to simplify some of the logic above.

might be better to extract this out to a function then you can return the result of this function at 144 and at 154. That would allow you to simplify some of the logic above.

wxiaoguang reviewed 2021-09-30 03:55:12 +00:00

models/avatars/avatar.go

						
				@ -0,0 +100,4 @@

							return nil

						}); err != nil {

							// Seriously we don't care about any DB problems just return the lowerEmail - we expect the transaction to fail most of the time

							return lowerEmail, nil

wxiaoguang commented

2021-09-30 03:55:11 +00:00

~~But there is no caller to these codes now. It only works for gravatar-like URLs. The line 114 doesn't share the same logic as here.~~

~~I think we can postpone the function extraction to next time when others need it?~~

~~But there is no caller to these codes now. It only works for gravatar-like URLs. The line 114 doesn't share the same logic as here.~~ ~~I think we can postpone the function extraction to next time when others need it?~~

wxiaoguang reviewed 2021-09-30 03:55:36 +00:00

models/avatars/avatar.go

						
				@ -0,0 +100,4 @@

							return nil

						}); err != nil {

							// Seriously we don't care about any DB problems just return the lowerEmail - we expect the transaction to fail most of the time

							return lowerEmail, nil

wxiaoguang commented

2021-09-30 03:55:36 +00:00

Oh, I know your meaning, just a minute ~~~

delvh reviewed 2021-09-30 07:33:47 +00:00

models/avatars/avatar.go

						
				@ -0,0 +139,4 @@

				// if final is false, it always uses a fast path.

				func generateEmailAvatarLink(email string, size int, final bool) string {

					email = strings.TrimSpace(email)

					if email == "" {

delvh commented

2021-09-30 07:24:21 +00:00

I know of someone who wants to change that once he sees it 😉

I know of someone who wants to change that once he sees it :wink:

modules/httpcache/httpcache_test.go

						
				@ -27,0 +33,4 @@

						}

						c++

					}

					return c

delvh commented

2021-09-30 07:26:42 +00:00

Yeah, I can see that, but maybe we should rename the Formal to Official or Standard.

Yeah, I can see that, but maybe we should rename the `Formal` to `Official` or `Standard`.

modules/httpcache/httpcache_test.go

						
				@ -27,1 +36,4 @@

					return c

				}

				func TestHandleFileETagCache(t *testing.T) {

delvh commented

2021-09-30 07:28:45 +00:00

I wouldn't even check for X-Gitea,
I would instead check only for X- as according to my knowledge any header starting with that is not standardized.
You do not know if there will ever be a custom header that is not X-Gitea.

Oh wait, I just noticed that this is a testing method where you can indeed influence what headers are being sent.
I guess you can then ignore this comment.

I wouldn't even check for `X-Gitea`, I would instead check only for `X-` as according to my knowledge any header starting with that is not standardized. You do not know if there will ever be a custom header that is not `X-Gitea`. Oh wait, I just noticed that this is a testing method where you can indeed influence what headers are being sent. I guess you can then ignore this comment.

routers/web/user/avatar.go

delvh commented

2021-09-30 07:33:35 +00:00

Isn't there a return missing here?

			ctx.ServerError("Invalid user: "+userName, err)
			return

Isn't there a return missing here? ```suggestion ctx.ServerError("Invalid user: "+userName, err) return ```

wxiaoguang reviewed 2021-09-30 08:14:12 +00:00

modules/httpcache/httpcache_test.go

						
				@ -27,0 +33,4 @@

						}

						c++

					}

					return c

wxiaoguang commented

2021-09-30 08:14:11 +00:00

I didn't use "standard" because we may want some customization in future, which contains our customized headers should be counted. It's just a personal preference, and this function just appears in the unit test privately, so I think this name doesn't matter much.

wxiaoguang reviewed 2021-09-30 08:14:32 +00:00

modules/httpcache/httpcache_test.go

						
				@ -27,1 +36,4 @@

					return c

				}

				func TestHandleFileETagCache(t *testing.T) {

wxiaoguang commented

2021-09-30 08:14:31 +00:00

Yep, explained above.

wxiaoguang reviewed 2021-09-30 08:18:09 +00:00

routers/web/user/avatar.go

wxiaoguang commented

2021-09-30 08:18:09 +00:00

fixed

delvh approved these changes 2021-09-30 10:06:05 +00:00

delvh left a comment

Even though my approval won't count, here it is.

6543 (Migrated from github.com) reviewed 2021-10-05 19:43:57 +00:00

modules/httpcache/httpcache.go

6543 (Migrated from github.com) commented

2021-10-05 19:43:57 +00:00

		h.Add("X-Gitea-Debug", "CacheControl=no-store")

```suggestion h.Add("X-Gitea-Debug", "CacheControl=no-store") ```

6543 (Migrated from github.com) approved these changes 2021-10-05 19:47:15 +00:00

This repo is archived. You cannot comment on pull requests.

No reviewers

6543

No Label

hacktoberfest-accepted

kind/upstream-related

status/needs-feedback

No Milestone

No project

No Assignees

4 Participants

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: lunny/gitea#17123

				`@ -60,61 +59,41 @@ func (u *User) generateRandomAvatar(e db.Engine) error {`
				`return nil`

Avatar refactor, move avatar code from models to models.avatars, remove duplicated code #17123

Avatar refactor, move avatar code from `models` to `models.avatars`, remove duplicated code #17123