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

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.

integrations/user_avatar_test.go

@@ -11,7 +11,6 @@ import (
 	"mime/multipart"
 	"net/http"
 	"net/url"
-	"strings"
 	"testing"
 
 	"code.gitea.io/gitea/models"
@@ -75,14 +74,8 @@ func TestUserAvatar(t *testing.T) {
 		user2 = db.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) // owner of the repo3, is an org
 
 		req = NewRequest(t, "GET", user2.AvatarLink())
-		resp := session.MakeRequest(t, req, http.StatusFound)
+		_ = session.MakeRequest(t, req, http.StatusOK)
-		location := resp.Header().Get("Location")
-		if !strings.HasPrefix(location, "/avatars") {
-			assert.Fail(t, "Avatar location is not local: %s", location)
-		}
-		req = NewRequest(t, "GET", location)
-		session.MakeRequest(t, req, http.StatusOK)
 
-		// Can't test if the response matches because the image is regened on upload but checking that this at least doesn't give a 404 should be enough.
+		// Can't test if the response matches because the image is re-generated on upload but checking that this at least doesn't give a 404 should be enough.
 	})
 }

models/avatar.go

@@ -1,148 +0,0 @@
-// Copyright 2020 The Gitea Authors. All rights reserved.
-// Use of this source code is governed by a MIT-style
-// license that can be found in the LICENSE file.
-
-package models
-
-import (
-	"context"
-	"crypto/md5"
-	"fmt"
-	"net/url"
-	"path"
-	"strconv"
-	"strings"
-
-	"code.gitea.io/gitea/models/db"
-	"code.gitea.io/gitea/modules/base"
-	"code.gitea.io/gitea/modules/cache"
-	"code.gitea.io/gitea/modules/log"
-	"code.gitea.io/gitea/modules/setting"
-)
-
-// EmailHash represents a pre-generated hash map
-type EmailHash struct {
-	Hash  string `xorm:"pk varchar(32)"`
-	Email string `xorm:"UNIQUE NOT NULL"`
-}
-
-func init() {
-	db.RegisterModel(new(EmailHash))
-}
-
-// DefaultAvatarLink the default avatar link
-func DefaultAvatarLink() string {
-	u, err := url.Parse(setting.AppSubURL)
-	if err != nil {
-		log.Error("GetUserByEmail: %v", err)
-		return ""
-	}
-
-	u.Path = path.Join(u.Path, "/assets/img/avatar_default.png")
-	return u.String()
-}
-
-// DefaultAvatarSize is a sentinel value for the default avatar size, as
-// determined by the avatar-hosting service.
-const DefaultAvatarSize = -1
-
-// DefaultAvatarPixelSize is the default size in pixels of a rendered avatar
-const DefaultAvatarPixelSize = 28
-
-// AvatarRenderedSizeFactor is the factor by which the default size is increased for finer rendering
-const AvatarRenderedSizeFactor = 4
-
-// HashEmail hashes email address to MD5 string.
-// https://en.gravatar.com/site/implement/hash/
-func HashEmail(email string) string {
-	return base.EncodeMD5(strings.ToLower(strings.TrimSpace(email)))
-}
-
-// GetEmailForHash converts a provided md5sum to the email
-func GetEmailForHash(md5Sum string) (string, error) {
-	return cache.GetString("Avatar:"+md5Sum, func() (string, error) {
-		emailHash := EmailHash{
-			Hash: strings.ToLower(strings.TrimSpace(md5Sum)),
-		}
-
-		_, err := db.GetEngine(db.DefaultContext).Get(&emailHash)
-		return emailHash.Email, err
-	})
-}
-
-// LibravatarURL returns the URL for the given email. This function should only
-// be called if a federated avatar service is enabled.
-func LibravatarURL(email string) (*url.URL, error) {
-	urlStr, err := setting.LibravatarService.FromEmail(email)
-	if err != nil {
-		log.Error("LibravatarService.FromEmail(email=%s): error %v", email, err)
-		return nil, err
-	}
-	u, err := url.Parse(urlStr)
-	if err != nil {
-		log.Error("Failed to parse libravatar url(%s): error %v", urlStr, err)
-		return nil, err
-	}
-	return u, nil
-}
-
-// HashedAvatarLink returns an avatar link for a provided email
-func HashedAvatarLink(email string, size int) string {
-	lowerEmail := strings.ToLower(strings.TrimSpace(email))
-	sum := fmt.Sprintf("%x", md5.Sum([]byte(lowerEmail)))
-	_, _ = cache.GetString("Avatar:"+sum, func() (string, error) {
-		emailHash := &EmailHash{
-			Email: lowerEmail,
-			Hash:  sum,
-		}
-		// OK we're going to open a session just because I think that that might hide away any problems with postgres reporting errors
-		if err := db.WithTx(func(ctx context.Context) error {
-			has, err := db.GetEngine(ctx).Where("email = ? AND hash = ?", emailHash.Email, emailHash.Hash).Get(new(EmailHash))
-			if has || 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 nil
-			}
-			_, _ = db.GetEngine(ctx).Insert(emailHash)
-			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
-		}
-		return lowerEmail, nil
-	})
-	if size > 0 {
-		return setting.AppSubURL + "/avatar/" + url.PathEscape(sum) + "?size=" + strconv.Itoa(size)
-	}
-	return setting.AppSubURL + "/avatar/" + url.PathEscape(sum)
-}
-
-// MakeFinalAvatarURL constructs the final avatar URL string
-func MakeFinalAvatarURL(u *url.URL, size int) string {
-	vals := u.Query()
-	vals.Set("d", "identicon")
-	if size != DefaultAvatarSize {
-		vals.Set("s", strconv.Itoa(size))
-	}
-	u.RawQuery = vals.Encode()
-	return u.String()
-}
-
-// SizedAvatarLink returns a sized link to the avatar for the given email address.
-func SizedAvatarLink(email string, size int) string {
-	var avatarURL *url.URL
-	if setting.EnableFederatedAvatar && setting.LibravatarService != nil {
-		// This is the slow path that would need to call LibravatarURL() which
-		// does DNS lookups. Avoid it by issuing a redirect so we don't block
-		// the template render with network requests.
-		return HashedAvatarLink(email, size)
-	} else if !setting.DisableGravatar {
-		// copy GravatarSourceURL, because we will modify its Path.
-		copyOfGravatarSourceURL := *setting.GravatarSourceURL
-		avatarURL = &copyOfGravatarSourceURL
-		avatarURL.Path = path.Join(avatarURL.Path, HashEmail(email))
-	} else {
-		return DefaultAvatarLink()
-	}
-
-	return MakeFinalAvatarURL(avatarURL, size)
-}

models/avatars/avatar.go

@@ -0,0 +1,180 @@
+// Copyright 2021 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package avatars
+
+import (
+	"context"
+	"net/url"
+	"path"
+	"strconv"
+	"strings"
+
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/base"
+	"code.gitea.io/gitea/modules/cache"
+	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/setting"
+)
+
+// DefaultAvatarPixelSize is the default size in pixels of a rendered avatar
+const DefaultAvatarPixelSize = 28
+
+// AvatarRenderedSizeFactor is the factor by which the default size is increased for finer rendering
+const AvatarRenderedSizeFactor = 4
+
+// EmailHash represents a pre-generated hash map (mainly used by LibravatarURL, it queries email server's DNS records)
+type EmailHash struct {
+	Hash  string `xorm:"pk varchar(32)"`
+	Email string `xorm:"UNIQUE NOT NULL"`
+}
+
+func init() {
+	db.RegisterModel(new(EmailHash))
+}
+
+// DefaultAvatarLink the default avatar link
+func DefaultAvatarLink() string {
+	u, err := url.Parse(setting.AppSubURL)
+	if err != nil {
+		log.Error("GetUserByEmail: %v", err)
+		return ""
+	}
+
+	u.Path = path.Join(u.Path, "/assets/img/avatar_default.png")
+	return u.String()
+}
+
+// HashEmail hashes email address to MD5 string. https://en.gravatar.com/site/implement/hash/
+func HashEmail(email string) string {
+	return base.EncodeMD5(strings.ToLower(strings.TrimSpace(email)))
+}
+
+// GetEmailForHash converts a provided md5sum to the email
+func GetEmailForHash(md5Sum string) (string, error) {
+	return cache.GetString("Avatar:"+md5Sum, func() (string, error) {
+		emailHash := EmailHash{
+			Hash: strings.ToLower(strings.TrimSpace(md5Sum)),
+		}
+
+		_, err := db.GetEngine(db.DefaultContext).Get(&emailHash)
+		return emailHash.Email, err
+	})
+}
+
+// LibravatarURL returns the URL for the given email. Slow due to the DNS lookup.
+// This function should only be called if a federated avatar service is enabled.
+func LibravatarURL(email string) (*url.URL, error) {
+	urlStr, err := setting.LibravatarService.FromEmail(email)
+	if err != nil {
+		log.Error("LibravatarService.FromEmail(email=%s): error %v", email, err)
+		return nil, err
+	}
+	u, err := url.Parse(urlStr)
+	if err != nil {
+		log.Error("Failed to parse libravatar url(%s): error %v", urlStr, err)
+		return nil, err
+	}
+	return u, nil
+}
+
+// saveEmailHash returns an avatar link for a provided email,
+// the email and hash are saved into database, which will be used by GetEmailForHash later
+func saveEmailHash(email string) string {
+	lowerEmail := strings.ToLower(strings.TrimSpace(email))
+	emailHash := HashEmail(lowerEmail)
+	_, _ = cache.GetString("Avatar:"+emailHash, func() (string, error) {
+		emailHash := &EmailHash{
+			Email: lowerEmail,
+			Hash:  emailHash,
+		}
+		// OK we're going to open a session just because I think that that might hide away any problems with postgres reporting errors
+		if err := db.WithTx(func(ctx context.Context) error {
+			has, err := db.GetEngine(ctx).Where("email = ? AND hash = ?", emailHash.Email, emailHash.Hash).Get(new(EmailHash))
+			if has || 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 nil
+			}
+			_, _ = db.GetEngine(ctx).Insert(emailHash)
+			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
+		}
+		return lowerEmail, nil
+	})
+	return emailHash
+}
+
+// GenerateUserAvatarFastLink returns a fast link (302) to the user's avatar: "/user/avatar/${User.Name}/${size}"
+func GenerateUserAvatarFastLink(userName string, size int) string {
+	if size < 0 {
+		size = 0
+	}
+	return setting.AppSubURL + "/user/avatar/" + userName + "/" + strconv.Itoa(size)
+}
+
+// GenerateUserAvatarImageLink returns a link for `User.Avatar` image file: "/avatars/${User.Avatar}"
+func GenerateUserAvatarImageLink(userAvatar string, size int) string {
+	if size > 0 {
+		return setting.AppSubURL + "/avatars/" + userAvatar + "?size=" + strconv.Itoa(size)
+	}
+	return setting.AppSubURL + "/avatars/" + userAvatar
+}
+
+// generateRecognizedAvatarURL generate a recognized avatar (Gravatar/Libravatar) URL, it modifies the URL so the parameter is passed by a copy
+func generateRecognizedAvatarURL(u url.URL, size int) string {
+	urlQuery := u.Query()
+	urlQuery.Set("d", "identicon")
+	if size > 0 {
+		urlQuery.Set("s", strconv.Itoa(size))
+	}
+	u.RawQuery = urlQuery.Encode()
+	return u.String()
+}
+
+// generateEmailAvatarLink returns a email avatar link.
+// if final is true, it may use a slow path (eg: query DNS).
+// 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 == "" {
+		return DefaultAvatarLink()
+	}
+
+	var err error
+	if setting.EnableFederatedAvatar && setting.LibravatarService != nil {
+		emailHash := saveEmailHash(email)
+		if final {
+			// for final link, we can spend more time on slow external query
+			var avatarURL *url.URL
+			if avatarURL, err = LibravatarURL(email); err != nil {
+				return DefaultAvatarLink()
+			}
+			return generateRecognizedAvatarURL(*avatarURL, size)
+		}
+		// for non-final link, we should return fast (use a 302 redirection link)
+		urlStr := setting.AppSubURL + "/avatar/" + emailHash
+		if size > 0 {
+			urlStr += "?size=" + strconv.Itoa(size)
+		}
+		return urlStr
+	} else if !setting.DisableGravatar {
+		// copy GravatarSourceURL, because we will modify its Path.
+		avatarURLCopy := *setting.GravatarSourceURL
+		avatarURLCopy.Path = path.Join(avatarURLCopy.Path, HashEmail(email))
+		return generateRecognizedAvatarURL(avatarURLCopy, size)
+	}
+	return DefaultAvatarLink()
+}
+
+//GenerateEmailAvatarFastLink returns a avatar link (fast, the link may be a delegated one: "/avatar/${hash}")
+func GenerateEmailAvatarFastLink(email string, size int) string {
+	return generateEmailAvatarLink(email, size, false)
+}
+
+//GenerateEmailAvatarFinalLink returns a avatar final link (maybe slow)
+func GenerateEmailAvatarFinalLink(email string, size int) string {
+	return generateEmailAvatarLink(email, size, true)
+}

models/avatars/avatar_test.go

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a MIT-style
 // license that can be found in the LICENSE file.
 
-package models
+package avatars
 
 import (
 	"net/url"
@@ -44,11 +44,11 @@ func TestSizedAvatarLink(t *testing.T) {
 
 	disableGravatar()
 	assert.Equal(t, "/testsuburl/assets/img/avatar_default.png",
-		SizedAvatarLink("gitea@example.com", 100))
+		GenerateEmailAvatarFastLink("gitea@example.com", 100))
 
 	enableGravatar(t)
 	assert.Equal(t,
 		"https://secure.gravatar.com/avatar/353cbad9b58e69c96154ad99f92bedc7?d=identicon&s=100",
-		SizedAvatarLink("gitea@example.com", 100),
+		GenerateEmailAvatarFastLink("gitea@example.com", 100),
 	)
 }

models/user_avatar.go

@@ -9,9 +9,8 @@ import (
 	"fmt"
 	"image/png"
 	"io"
-	"strconv"
-	"strings"
 
+	"code.gitea.io/gitea/models/avatars"
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/modules/avatar"
 	"code.gitea.io/gitea/modules/log"
@@ -40,7 +39,7 @@ func (u *User) generateRandomAvatar(e db.Engine) error {
 		return fmt.Errorf("RandomImage: %v", err)
 	}
 
-	u.Avatar = HashEmail(seed)
+	u.Avatar = avatars.HashEmail(seed)
 
 	// Don't share the images so that we can delete them easily
 	if err := storage.SaveFrom(storage.Avatars, u.CustomAvatarRelativePath(), func(w io.Writer) error {
@@ -60,61 +59,41 @@ func (u *User) generateRandomAvatar(e db.Engine) error {
 	return nil
 }
 
-// SizedRelAvatarLink returns a link to the user's avatar via
+// AvatarLinkWithSize returns a link to the user's avatar with size. size <= 0 means default size
-// the local explore page. Function returns immediately.
+func (u *User) AvatarLinkWithSize(size int) string {
-// When applicable, the link is for an avatar of the indicated size (in pixels).
-func (u *User) SizedRelAvatarLink(size int) string {
-	return setting.AppSubURL + "/user/avatar/" + u.Name + "/" + strconv.Itoa(size)
-}
-
-// RealSizedAvatarLink returns a link to the user's avatar. When
-// applicable, the link is for an avatar of the indicated size (in pixels).
-//
-// This function make take time to return when federated avatars
-// are in use, due to a DNS lookup need
-//
-func (u *User) RealSizedAvatarLink(size int) string {
 	if u.ID == -1 {
-		return DefaultAvatarLink()
+		// ghost user
+		return avatars.DefaultAvatarLink()
 	}
 
+	useLocalAvatar := false
+	autoGenerateAvatar := false
+
 	switch {
 	case u.UseCustomAvatar:
-		if u.Avatar == "" {
+		useLocalAvatar = true
-			return DefaultAvatarLink()
-		}
-		if size > 0 {
-			return setting.AppSubURL + "/avatars/" + u.Avatar + "?size=" + strconv.Itoa(size)
-		}
-		return setting.AppSubURL + "/avatars/" + u.Avatar
 	case setting.DisableGravatar, setting.OfflineMode:
-		if u.Avatar == "" {
+		useLocalAvatar = true
+		autoGenerateAvatar = true
+	}
+
+	if useLocalAvatar {
+		if u.Avatar == "" && autoGenerateAvatar {
 			if err := u.GenerateRandomAvatar(); err != nil {
 				log.Error("GenerateRandomAvatar: %v", err)
 			}
 		}
-		if size > 0 {
+		if u.Avatar == "" {
-			return setting.AppSubURL + "/avatars/" + u.Avatar + "?size=" + strconv.Itoa(size)
+			return avatars.DefaultAvatarLink()
 		}
-		return setting.AppSubURL + "/avatars/" + u.Avatar
+		return avatars.GenerateUserAvatarImageLink(u.Avatar, size)
 	}
-	return SizedAvatarLink(u.AvatarEmail, size)
+	return avatars.GenerateEmailAvatarFastLink(u.AvatarEmail, size)
 }
 
-// RelAvatarLink returns a relative link to the user's avatar. The link
+// AvatarLink returns a avatar link with default size
-// may either be a sub-URL to this site, or a full URL to an external avatar
-// service.
-func (u *User) RelAvatarLink() string {
-	return u.SizedRelAvatarLink(DefaultAvatarSize)
-}
-
-// AvatarLink returns user avatar absolute link.
 func (u *User) AvatarLink() string {
-	link := u.RelAvatarLink()
+	return u.AvatarLinkWithSize(0)
-	if link[0] == '/' && link[1] != '/' {
-		return setting.AppURL + strings.TrimPrefix(link, setting.AppSubURL)[1:]
-	}
-	return link
 }
 
 // UploadAvatar saves custom avatar for user.

modules/httpcache/httpcache.go

@@ -16,12 +16,17 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 )
 
-// GetCacheControl returns a suitable "Cache-Control" header value
+// AddCacheControlToHeader adds suitable cache-control headers to response
-func GetCacheControl() string {
+func AddCacheControlToHeader(h http.Header, d time.Duration) {
-	if !setting.IsProd() {
+	if setting.IsProd() {
-		return "no-store"
+		h.Set("Cache-Control", "private, max-age="+strconv.Itoa(int(d.Seconds())))
+	} else {
+		h.Set("Cache-Control", "no-store")
+		// to remind users they are using non-prod setting.
+		// some users may be confused by "Cache-Control: no-store" in their setup if they did wrong to `RUN_MODE` in `app.ini`.
+		h.Add("X-Gitea-Debug", "RUN_MODE="+setting.RunMode)
+		h.Add("X-Gitea-Debug", "CacheControl=no-store")
 	}
-	return "private, max-age=" + strconv.FormatInt(int64(setting.StaticCacheTime.Seconds()), 10)
 }
 
 // generateETag generates an ETag based on size, filename and file modification time
@@ -32,7 +37,7 @@ func generateETag(fi os.FileInfo) string {
 
 // HandleTimeCache handles time-based caching for a HTTP request
 func HandleTimeCache(req *http.Request, w http.ResponseWriter, fi os.FileInfo) (handled bool) {
-	w.Header().Set("Cache-Control", GetCacheControl())
+	AddCacheControlToHeader(w.Header(), setting.StaticCacheTime)
 
 	ifModifiedSince := req.Header.Get("If-Modified-Since")
 	if ifModifiedSince != "" {
@@ -63,7 +68,7 @@ func HandleGenericETagCache(req *http.Request, w http.ResponseWriter, etag strin
 			return true
 		}
 	}
-	w.Header().Set("Cache-Control", GetCacheControl())
+	AddCacheControlToHeader(w.Header(), setting.StaticCacheTime)
 	return false
 }
 

modules/httpcache/httpcache_test.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -24,6 +25,17 @@ func (m mockFileInfo) ModTime() time.Time { return time.Time{} }
 func (m mockFileInfo) IsDir() bool        { return false }
 func (m mockFileInfo) Sys() interface{}   { return nil }
 
+func countFormalHeaders(h http.Header) (c int) {
+	for k := range h {
+		// ignore our headers for internal usage
+		if strings.HasPrefix(k, "X-Gitea-") {
+			continue
+		}
+		c++
+	}
+	return c
+}
+
 func TestHandleFileETagCache(t *testing.T) {
 	fi := mockFileInfo{}
 	etag := `"MTBnaXRlYS50ZXN0TW9uLCAwMSBKYW4gMDAwMSAwMDowMDowMCBHTVQ="`
@@ -35,7 +47,7 @@ func TestHandleFileETagCache(t *testing.T) {
 		handled := HandleFileETagCache(req, w, fi)
 
 		assert.False(t, handled)
-		assert.Len(t, w.Header(), 2)
+		assert.Equal(t, 2, countFormalHeaders(w.Header()))
 		assert.Contains(t, w.Header(), "Cache-Control")
 		assert.Contains(t, w.Header(), "Etag")
 		assert.Equal(t, etag, w.Header().Get("Etag"))
@@ -49,7 +61,7 @@ func TestHandleFileETagCache(t *testing.T) {
 		handled := HandleFileETagCache(req, w, fi)
 
 		assert.False(t, handled)
-		assert.Len(t, w.Header(), 2)
+		assert.Equal(t, 2, countFormalHeaders(w.Header()))
 		assert.Contains(t, w.Header(), "Cache-Control")
 		assert.Contains(t, w.Header(), "Etag")
 		assert.Equal(t, etag, w.Header().Get("Etag"))
@@ -63,7 +75,7 @@ func TestHandleFileETagCache(t *testing.T) {
 		handled := HandleFileETagCache(req, w, fi)
 
 		assert.True(t, handled)
-		assert.Len(t, w.Header(), 1)
+		assert.Equal(t, 1, countFormalHeaders(w.Header()))
 		assert.Contains(t, w.Header(), "Etag")
 		assert.Equal(t, etag, w.Header().Get("Etag"))
 		assert.Equal(t, http.StatusNotModified, w.Code)
@@ -80,7 +92,7 @@ func TestHandleGenericETagCache(t *testing.T) {
 		handled := HandleGenericETagCache(req, w, etag)
 
 		assert.False(t, handled)
-		assert.Len(t, w.Header(), 2)
+		assert.Equal(t, 2, countFormalHeaders(w.Header()))
 		assert.Contains(t, w.Header(), "Cache-Control")
 		assert.Contains(t, w.Header(), "Etag")
 		assert.Equal(t, etag, w.Header().Get("Etag"))
@@ -94,7 +106,7 @@ func TestHandleGenericETagCache(t *testing.T) {
 		handled := HandleGenericETagCache(req, w, etag)
 
 		assert.False(t, handled)
-		assert.Len(t, w.Header(), 2)
+		assert.Equal(t, 2, countFormalHeaders(w.Header()))
 		assert.Contains(t, w.Header(), "Cache-Control")
 		assert.Contains(t, w.Header(), "Etag")
 		assert.Equal(t, etag, w.Header().Get("Etag"))
@@ -108,7 +120,7 @@ func TestHandleGenericETagCache(t *testing.T) {
 		handled := HandleGenericETagCache(req, w, etag)
 
 		assert.True(t, handled)
-		assert.Len(t, w.Header(), 1)
+		assert.Equal(t, 1, countFormalHeaders(w.Header()))
 		assert.Contains(t, w.Header(), "Etag")
 		assert.Equal(t, etag, w.Header().Get("Etag"))
 		assert.Equal(t, http.StatusNotModified, w.Code)
@@ -122,7 +134,7 @@ func TestHandleGenericETagCache(t *testing.T) {
 		handled := HandleGenericETagCache(req, w, etag)
 
 		assert.False(t, handled)
-		assert.Len(t, w.Header(), 2)
+		assert.Equal(t, 2, countFormalHeaders(w.Header()))
 		assert.Contains(t, w.Header(), "Cache-Control")
 		assert.Contains(t, w.Header(), "Etag")
 		assert.Equal(t, etag, w.Header().Get("Etag"))
@@ -136,7 +148,7 @@ func TestHandleGenericETagCache(t *testing.T) {
 		handled := HandleGenericETagCache(req, w, etag)
 
 		assert.True(t, handled)
-		assert.Len(t, w.Header(), 1)
+		assert.Equal(t, 1, countFormalHeaders(w.Header()))
 		assert.Contains(t, w.Header(), "Etag")
 		assert.Equal(t, etag, w.Header().Get("Etag"))
 		assert.Equal(t, http.StatusNotModified, w.Code)

modules/repository/commits.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/avatars"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	api "code.gitea.io/gitea/modules/structs"
@@ -139,14 +140,14 @@ func (pc *PushCommits) AvatarLink(email string) string {
 		return avatar
 	}
 
-	size := models.DefaultAvatarPixelSize * models.AvatarRenderedSizeFactor
+	size := avatars.DefaultAvatarPixelSize * avatars.AvatarRenderedSizeFactor
 
 	u, ok := pc.emailUsers[email]
 	if !ok {
 		var err error
 		u, err = models.GetUserByEmail(email)
 		if err != nil {
-			pc.avatars[email] = models.SizedAvatarLink(email, size)
+			pc.avatars[email] = avatars.GenerateEmailAvatarFastLink(email, size)
 			if !models.IsErrUserNotExist(err) {
 				log.Error("GetUserByEmail: %v", err)
 				return ""
@@ -156,7 +157,7 @@ func (pc *PushCommits) AvatarLink(email string) string {
 		}
 	}
 	if u != nil {
-		pc.avatars[email] = u.RealSizedAvatarLink(size)
+		pc.avatars[email] = u.AvatarLinkWithSize(size)
 	}
 
 	return pc.avatars[email]

modules/templates/helper.go

@@ -23,6 +23,7 @@ import (
 	"unicode"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/avatars"
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/emoji"
 	"code.gitea.io/gitea/modules/git"
@@ -550,16 +551,16 @@ func SVG(icon string, others ...interface{}) template.HTML {
 
 // Avatar renders user avatars. args: user, size (int), class (string)
 func Avatar(item interface{}, others ...interface{}) template.HTML {
-	size, class := parseOthers(models.DefaultAvatarPixelSize, "ui avatar image", others...)
+	size, class := parseOthers(avatars.DefaultAvatarPixelSize, "ui avatar image", others...)
 
 	if user, ok := item.(*models.User); ok {
-		src := user.RealSizedAvatarLink(size * models.AvatarRenderedSizeFactor)
+		src := user.AvatarLinkWithSize(size * avatars.AvatarRenderedSizeFactor)
 		if src != "" {
 			return AvatarHTML(src, size, class, user.DisplayName())
 		}
 	}
 	if user, ok := item.(*models.Collaborator); ok {
-		src := user.RealSizedAvatarLink(size * models.AvatarRenderedSizeFactor)
+		src := user.AvatarLinkWithSize(size * avatars.AvatarRenderedSizeFactor)
 		if src != "" {
 			return AvatarHTML(src, size, class, user.DisplayName())
 		}
@@ -575,7 +576,7 @@ func AvatarByAction(action *models.Action, others ...interface{}) template.HTML
 
 // RepoAvatar renders repo avatars. args: repo, size(int), class (string)
 func RepoAvatar(repo *models.Repository, others ...interface{}) template.HTML {
-	size, class := parseOthers(models.DefaultAvatarPixelSize, "ui avatar image", others...)
+	size, class := parseOthers(avatars.DefaultAvatarPixelSize, "ui avatar image", others...)
 
 	src := repo.RelAvatarLink()
 	if src != "" {
@@ -586,8 +587,8 @@ func RepoAvatar(repo *models.Repository, others ...interface{}) template.HTML {
 
 // AvatarByEmail renders avatars by email address. args: email, name, size (int), class (string)
 func AvatarByEmail(email string, name string, others ...interface{}) template.HTML {
-	size, class := parseOthers(models.DefaultAvatarPixelSize, "ui avatar image", others...)
+	size, class := parseOthers(avatars.DefaultAvatarPixelSize, "ui avatar image", others...)
-	src := models.SizedAvatarLink(email, size*models.AvatarRenderedSizeFactor)
+	src := avatars.GenerateEmailAvatarFastLink(email, size*avatars.AvatarRenderedSizeFactor)
 
 	if src != "" {
 		return AvatarHTML(src, size, class, name)

routers/web/repo/issue.go

@@ -2614,5 +2614,5 @@ func handleTeamMentions(ctx *context.Context) {
 
 	ctx.Data["MentionableTeams"] = ctx.Repo.Owner.Teams
 	ctx.Data["MentionableTeamsOrg"] = ctx.Repo.Owner.Name
-	ctx.Data["MentionableTeamsOrgAvatar"] = ctx.Repo.Owner.RelAvatarLink()
+	ctx.Data["MentionableTeamsOrgAvatar"] = ctx.Repo.Owner.AvatarLink()
 }

routers/web/user/avatar.go

@@ -5,100 +5,50 @@
 package user
 
 import (
-	"errors"
-	"net/url"
-	"path"
-	"strconv"
 	"strings"
+	"time"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/avatars"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/httpcache"
-	"code.gitea.io/gitea/modules/log"
-	"code.gitea.io/gitea/modules/setting"
 )
 
 func cacheableRedirect(ctx *context.Context, location string) {
-	ctx.Resp.Header().Set("Cache-Control", httpcache.GetCacheControl())
+	// here we should not use `setting.StaticCacheTime`, it is pretty long (default: 6 hours)
+	// we must make sure the redirection cache time is short enough, otherwise a user won't see the updated avatar in 6 hours
+	// it's OK to make the cache time short, it is only a redirection, and doesn't cost much to make a new request
+	httpcache.AddCacheControlToHeader(ctx.Resp.Header(), 5*time.Minute)
 	ctx.Redirect(location)
 }
 
-// Avatar redirect browser to user avatar of requested size
+// AvatarByUserName redirect browser to user avatar of requested size
-func Avatar(ctx *context.Context) {
+func AvatarByUserName(ctx *context.Context) {
 	userName := ctx.Params(":username")
-	size, err := strconv.Atoi(ctx.Params(":size"))
+	size := int(ctx.ParamsInt64(":size"))
-	if err != nil {
-		ctx.ServerError("Invalid avatar size", err)
-		return
-	}
-
-	log.Debug("Asked avatar for user %v and size %v", userName, size)
 
 	var user *models.User
 	if strings.ToLower(userName) != "ghost" {
-		user, err = models.GetUserByName(userName)
+		var err error
-		if err != nil {
+		if user, err = models.GetUserByName(userName); err != nil {
-			if models.IsErrUserNotExist(err) {
+			ctx.ServerError("Invalid user: "+userName, err)
-				ctx.ServerError("Requested avatar for invalid user", err)
-			} else {
-				ctx.ServerError("Retrieving user by name", err)
-			}
 			return
 		}
 	} else {
 		user = models.NewGhostUser()
 	}
 
-	cacheableRedirect(ctx, user.RealSizedAvatarLink(size))
+	cacheableRedirect(ctx, user.AvatarLinkWithSize(size))
 }
 
-// AvatarByEmailHash redirects the browser to the appropriate Avatar link
+// AvatarByEmailHash redirects the browser to the email avatar link
 func AvatarByEmailHash(ctx *context.Context) {
-	var err error
-
 	hash := ctx.Params(":hash")
-	if len(hash) == 0 {
+	email, err := avatars.GetEmailForHash(hash)
-		ctx.ServerError("invalid avatar hash", errors.New("hash cannot be empty"))
-		return
-	}
-
-	var email string
-	email, err = models.GetEmailForHash(hash)
 	if err != nil {
-		ctx.ServerError("invalid avatar hash", err)
+		ctx.ServerError("invalid avatar hash: "+hash, err)
-		return
-	}
-	if len(email) == 0 {
-		cacheableRedirect(ctx, models.DefaultAvatarLink())
 		return
 	}
 	size := ctx.FormInt("size")
-	if size == 0 {
+	cacheableRedirect(ctx, avatars.GenerateEmailAvatarFinalLink(email, size))
-		size = models.DefaultAvatarSize
-	}
-
-	var avatarURL *url.URL
-
-	if setting.EnableFederatedAvatar && setting.LibravatarService != nil {
-		avatarURL, err = models.LibravatarURL(email)
-		if err != nil {
-			avatarURL, err = url.Parse(models.DefaultAvatarLink())
-			if err != nil {
-				ctx.ServerError("invalid default avatar url", err)
-				return
-			}
-		}
-	} else if !setting.DisableGravatar {
-		copyOfGravatarSourceURL := *setting.GravatarSourceURL
-		avatarURL = &copyOfGravatarSourceURL
-		avatarURL.Path = path.Join(avatarURL.Path, hash)
-	} else {
-		avatarURL, err = url.Parse(models.DefaultAvatarLink())
-		if err != nil {
-			ctx.ServerError("invalid default avatar url", err)
-			return
-		}
-	}
-
-	cacheableRedirect(ctx, models.MakeFinalAvatarURL(avatarURL, size))
 }

routers/web/web.go

@@ -366,7 +366,7 @@ func RegisterRoutes(m *web.Route) {
 		m.Get("/activate", user.Activate, reqSignIn)
 		m.Post("/activate", user.ActivatePost, reqSignIn)
 		m.Any("/activate_email", user.ActivateEmail)
-		m.Get("/avatar/{username}/{size}", user.Avatar)
+		m.Get("/avatar/{username}/{size}", user.AvatarByUserName)
 		m.Get("/email2user", user.Email2User)
 		m.Get("/recover_account", user.ResetPasswd)
 		m.Post("/recover_account", user.ResetPasswdPost)

services/auth/sspi_windows.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/avatars"
 	"code.gitea.io/gitea/models/login"
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/log"
@@ -193,7 +194,7 @@ func (s *SSPI) newUser(username string, cfg *sspi.Source) (*models.User, error)
 		IsActive:                     cfg.AutoActivateUsers,
 		Language:                     cfg.DefaultLanguage,
 		UseCustomAvatar:              true,
-		Avatar:                       models.DefaultAvatarLink(),
+		Avatar:                       avatars.DefaultAvatarLink(),
 		EmailNotificationsPreference: models.EmailNotificationsDisabled,
 	}
 	if err := models.CreateUser(user); err != nil {

templates/base/head.tmpl

@@ -48,11 +48,11 @@
 			tributeValues: Array.from(new Map([
 				{{ range .Participants }}
 				['{{.Name}}', {key: '{{.Name}} {{.FullName}}', value: '{{.Name}}',
-				name: '{{.Name}}', fullname: '{{.FullName}}', avatar: '{{.RelAvatarLink}}'}],
+				name: '{{.Name}}', fullname: '{{.FullName}}', avatar: '{{.AvatarLink}}'}],
 				{{ end }}
 				{{ range .Assignees }}
 				['{{.Name}}', {key: '{{.Name}} {{.FullName}}', value: '{{.Name}}',
-				name: '{{.Name}}', fullname: '{{.FullName}}', avatar: '{{.RelAvatarLink}}'}],
+				name: '{{.Name}}', fullname: '{{.FullName}}', avatar: '{{.AvatarLink}}'}],
 				{{ end }}
 				{{ range .MentionableTeams }}
 					['{{$.MentionableTeamsOrg}}/{{.Name}}', {key: '{{$.MentionableTeamsOrg}}/{{.Name}}', value: '{{$.MentionableTeamsOrg}}/{{.Name}}',

templates/repo/issue/view_content/comments.tmpl

@@ -746,7 +746,7 @@
 		<div class="timeline-item-group">
 			<div class="timeline-item event" id="{{.HashTag}}">
 				<a class="timeline-avatar"{{if gt .Poster.ID 0}} href="{{.Poster.HomeLink}}"{{end}}>
-					<img src="{{.Poster.RelAvatarLink}}">
+					<img src="{{.Poster.AvatarLink}}">
 				</a>
 				<span class="badge grey">{{svg "octicon-x" 16}}</span>
 				<span class="text grey">