Refactor avatar package, support default avatar fallback (#36788)

* Fix #34715

modules/assetfs/embed.go

@@ -260,7 +260,7 @@ func (fi *embeddedFileInfo) Mode() fs.FileMode {
 }
 
 func (fi *embeddedFileInfo) ModTime() time.Time {
-	return getExecutableModTime()
+	return GetExecutableModTime()
 }
 
 func (fi *embeddedFileInfo) IsDir() bool {
@@ -279,9 +279,9 @@ func (fi *embeddedFileInfo) Info() (fs.FileInfo, error) {
 	return fi, nil
 }
 
-// getExecutableModTime returns the modification time of the executable file.
+// GetExecutableModTime returns the modification time of the executable file.
 // In bindata, we can't use the ModTime of the files because we need to make the build reproducible
-var getExecutableModTime = sync.OnceValue(func() (modTime time.Time) {
+var GetExecutableModTime = sync.OnceValue(func() (modTime time.Time) {
 	exePath, err := os.Executable()
 	if err != nil {
 		return modTime

modules/avatar/avatar.go

@@ -28,21 +28,18 @@ import (
 // than the size after resizing.
 const DefaultAvatarSize = 256
 
-// RandomImageSize generates and returns a random avatar image unique to input data
+// RandomImageWithSize generates and returns a random avatar image unique to input data
 // in custom size (height and width).
-func RandomImageSize(size int, data []byte) (image.Image, error) {
+func RandomImageWithSize(size int, data []byte) image.Image {
 	// we use white as background, and use dark colors to draw blocks
-	imgMaker, err := identicon.New(size, color.White, identicon.DarkColors...)
-	if err != nil {
-		return nil, fmt.Errorf("identicon.New: %w", err)
-	}
-	return imgMaker.Make(data), nil
+	imgMaker := identicon.New(size, color.White, identicon.DarkColors)
+	return imgMaker.Make(data)
 }
 
-// RandomImage generates and returns a random avatar image unique to input data
+// RandomImageDefaultSize generates and returns a random avatar image unique to input data
 // in default size (height and width).
-func RandomImage(data []byte) (image.Image, error) {
-	return RandomImageSize(DefaultAvatarSize*setting.Avatar.RenderedSizeFactor, data)
+func RandomImageDefaultSize(data []byte) image.Image {
+	return RandomImageWithSize(DefaultAvatarSize*setting.Avatar.RenderedSizeFactor, data)
 }
 
 // processAvatarImage process the avatar image data, crop and resize it if necessary.

modules/avatar/avatar_test.go

@@ -15,19 +15,6 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func Test_RandomImageSize(t *testing.T) {
-	_, err := RandomImageSize(0, []byte("gitea@local"))
-	assert.Error(t, err)
-
-	_, err = RandomImageSize(64, []byte("gitea@local"))
-	assert.NoError(t, err)
-}
-
-func Test_RandomImage(t *testing.T) {
-	_, err := RandomImage([]byte("gitea@local"))
-	assert.NoError(t, err)
-}
-
 func Test_ProcessAvatarPNG(t *testing.T) {
 	setting.Avatar.MaxWidth = 4096
 	setting.Avatar.MaxHeight = 4096
@@ -134,3 +121,18 @@ func Test_ProcessAvatarImage(t *testing.T) {
 	_, err = processAvatarImage(origin, 262144)
 	assert.ErrorContains(t, err, "image width is too large: 10 > 5")
 }
+
+func BenchmarkRandomImage(b *testing.B) {
+	b.Run("size-48", func(b *testing.B) {
+		for b.Loop() {
+			// BenchmarkRandomImage/size-48-12         	   49549	     22899 ns/op
+			RandomImageWithSize(48, []byte("test-content"))
+		}
+	})
+	b.Run("size-96", func(b *testing.B) {
+		for b.Loop() {
+			// BenchmarkRandomImage/size-96-12         	   13816	     88187 ns/op
+			RandomImageWithSize(96, []byte("test-content"))
+		}
+	})
+}

modules/avatar/identicon/identicon.go

@@ -8,13 +8,14 @@ package identicon
 
 import (
 	"crypto/sha256"
-	"errors"
-	"fmt"
 	"image"
 	"image/color"
 )
 
-const minImageSize = 16
+const (
+	minImageSize = 16
+	maxImageSize = 2048
+)
 
 // Identicon is used to generate pseudo-random avatars
 type Identicon struct {
@@ -24,25 +25,17 @@ type Identicon struct {
 	rect       image.Rectangle
 }
 
-// New returns an Identicon struct with the correct settings
-// size image size
-// back background color
-// fore all possible foreground colors. only one foreground color will be picked randomly for one image
-func New(size int, back color.Color, fore ...color.Color) (*Identicon, error) {
-	if len(fore) == 0 {
-		return nil, errors.New("foreground is not set")
-	}
-
-	if size < minImageSize {
-		return nil, fmt.Errorf("size %d is smaller than min size %d", size, minImageSize)
-	}
-
+// New returns an Identicon struct.
+// Only one foreground color will be picked randomly for one image.
+func New(size int, backColor color.Color, foreColors []color.Color) *Identicon {
+	size = max(size, minImageSize)
+	size = min(size, maxImageSize)
 	return &Identicon{
-		foreColors: fore,
-		backColor:  back,
+		foreColors: foreColors,
+		backColor:  backColor,
 		size:       size,
 		rect:       image.Rect(0, 0, size, size),
-	}, nil
+	}
 }
 
 // Make generates an avatar by data

modules/avatar/identicon/identicon_test.go

@@ -23,7 +23,7 @@ func TestGenerate(t *testing.T) {
 	}
 
 	backColor := color.White
-	imgMaker, err := New(64, backColor, DarkColors...)
+	imgMaker, err := New(64, backColor, DarkColors)
 	assert.NoError(t, err)
 	for i := 0; i < 100; i++ {
 		s := strconv.Itoa(i)

modules/httpcache/httpcache.go

@@ -60,21 +60,6 @@ func CacheControlForPrivateStatic() *CacheControlOptions {
 	}
 }
 
-// HandleGenericETagCache handles ETag-based caching for a HTTP request.
-// It returns true if the request was handled.
-func HandleGenericETagCache(req *http.Request, w http.ResponseWriter, etag string) (handled bool) {
-	if len(etag) > 0 {
-		w.Header().Set("Etag", etag)
-		if checkIfNoneMatchIsValid(req, etag) {
-			w.WriteHeader(http.StatusNotModified)
-			return true
-		}
-	}
-	// not sure whether it is a public content, so just use "private" (old behavior)
-	SetCacheControlInHeader(w.Header(), CacheControlForPrivateStatic())
-	return false
-}
-
 // checkIfNoneMatchIsValid tests if the header If-None-Match matches the ETag
 func checkIfNoneMatchIsValid(req *http.Request, etag string) bool {
 	ifNoneMatch := req.Header.Get("If-None-Match")
@@ -89,10 +74,18 @@ func checkIfNoneMatchIsValid(req *http.Request, etag string) bool {
 	return false
 }
 
-// HandleGenericETagTimeCache handles ETag-based caching with Last-Modified caching for a HTTP request.
+func HandleGenericETagPublicCache(req *http.Request, w http.ResponseWriter, etag string, lastModified *time.Time) bool {
+	return handleGenericETagTimeCache(req, w, etag, lastModified, CacheControlForPublicStatic())
+}
+
+func HandleGenericETagPrivateCache(req *http.Request, w http.ResponseWriter, etag string, lastModified *time.Time) bool {
+	return handleGenericETagTimeCache(req, w, etag, lastModified, CacheControlForPrivateStatic())
+}
+
+// handleGenericETagTimeCache handles ETag-based caching with Last-Modified caching for the HTTP request.
 // It returns true if the request was handled.
-func HandleGenericETagTimeCache(req *http.Request, w http.ResponseWriter, etag string, lastModified *time.Time) (handled bool) {
-	if len(etag) > 0 {
+func handleGenericETagTimeCache(req *http.Request, w http.ResponseWriter, etag string, lastModified *time.Time, cacheControlOpts *CacheControlOptions) (handled bool) {
+	if etag != "" {
 		w.Header().Set("Etag", etag)
 	}
 	if lastModified != nil && !lastModified.IsZero() {
@@ -100,7 +93,7 @@ func HandleGenericETagTimeCache(req *http.Request, w http.ResponseWriter, etag s
 		w.Header().Set("Last-Modified", lastModified.UTC().Format(http.TimeFormat))
 	}
 
-	if len(etag) > 0 {
+	if etag != "" {
 		if checkIfNoneMatchIsValid(req, etag) {
 			w.WriteHeader(http.StatusNotModified)
 			return true
@@ -117,7 +110,6 @@ func HandleGenericETagTimeCache(req *http.Request, w http.ResponseWriter, etag s
 		}
 	}
 
-	// not sure whether it is a public content, so just use "private" (old behavior)
-	SetCacheControlInHeader(w.Header(), CacheControlForPrivateStatic())
+	SetCacheControlInHeader(w.Header(), cacheControlOpts)
 	return false
 }

modules/httpcache/httpcache_test.go

@@ -6,92 +6,73 @@ package httpcache
 import (
 	"net/http"
 	"net/http/httptest"
-	"strings"
 	"testing"
+	"time"
+
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 )
 
-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 TestHandleGenericETagCache(t *testing.T) {
-	etag := `"test"`
+	matchedEtag := `"matched-etag"`
+	lastModifiedTime := new(time.Date(2021, time.January, 2, 15, 4, 5, 0, time.FixedZone("test-zone", 8*3600)))
+	lastModified := lastModifiedTime.UTC().Format(http.TimeFormat)
+	cacheControl := "max-age=0, private, must-revalidate, no-transform"
+	type testCase struct {
+		name        string
+		reqHeaders  map[string]string
+		wantHandled bool
+		wantHeaders map[string]string
+		wantStatus  int
+	}
+	cases := []testCase{
+		{
+			name:        "No If-None-Match",
+			wantHandled: false,
+			wantHeaders: map[string]string{"Last-Modified": lastModified, "Cache-Control": cacheControl, "Etag": matchedEtag},
+		},
+		{
+			name:        "Mismatched If-None-Match",
+			reqHeaders:  map[string]string{"If-None-Match": `"mismatched-etag"`},
+			wantHandled: false,
+			wantHeaders: map[string]string{"Last-Modified": lastModified, "Cache-Control": cacheControl, "Etag": matchedEtag},
+		},
+		{
+			name:        "Matched If-None-Match",
+			reqHeaders:  map[string]string{"If-None-Match": matchedEtag},
+			wantHandled: true,
+			wantHeaders: map[string]string{"Last-Modified": lastModified, "Cache-Control": "", "Etag": matchedEtag},
+			wantStatus:  http.StatusNotModified,
+		},
+		{
+			name:        "Multiple Mismatched If-None-Match",
+			reqHeaders:  map[string]string{"If-None-Match": `"mismatched-etag1", "mismatched-etag2"`},
+			wantHandled: false,
+			wantHeaders: map[string]string{"Last-Modified": lastModified, "Cache-Control": cacheControl, "Etag": matchedEtag},
+		},
+		{
+			name:        "Multiple Matched If-None-Match",
+			reqHeaders:  map[string]string{"If-None-Match": `"mismatched-etag", ` + matchedEtag},
+			wantHandled: true,
+			wantHeaders: map[string]string{"Last-Modified": lastModified, "Cache-Control": "", "Etag": matchedEtag},
+			wantStatus:  http.StatusNotModified,
+		},
+	}
 
-	t.Run("No_If-None-Match", func(t *testing.T) {
-		req := &http.Request{Header: make(http.Header)}
-		w := httptest.NewRecorder()
-
-		handled := HandleGenericETagCache(req, w, etag)
-
-		assert.False(t, handled)
-		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"))
-	})
-	t.Run("Wrong_If-None-Match", func(t *testing.T) {
-		req := &http.Request{Header: make(http.Header)}
-		w := httptest.NewRecorder()
-
-		req.Header.Set("If-None-Match", `"wrong etag"`)
-
-		handled := HandleGenericETagCache(req, w, etag)
-
-		assert.False(t, handled)
-		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"))
-	})
-	t.Run("Correct_If-None-Match", func(t *testing.T) {
-		req := &http.Request{Header: make(http.Header)}
-		w := httptest.NewRecorder()
-
-		req.Header.Set("If-None-Match", etag)
-
-		handled := HandleGenericETagCache(req, w, etag)
-
-		assert.True(t, handled)
-		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)
-	})
-	t.Run("Multiple_Wrong_If-None-Match", func(t *testing.T) {
-		req := &http.Request{Header: make(http.Header)}
-		w := httptest.NewRecorder()
-
-		req.Header.Set("If-None-Match", `"wrong etag", "wrong etag "`)
-
-		handled := HandleGenericETagCache(req, w, etag)
-
-		assert.False(t, handled)
-		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"))
-	})
-	t.Run("Multiple_Correct_If-None-Match", func(t *testing.T) {
-		req := &http.Request{Header: make(http.Header)}
-		w := httptest.NewRecorder()
-
-		req.Header.Set("If-None-Match", `"wrong etag", `+etag)
-
-		handled := HandleGenericETagCache(req, w, etag)
-
-		assert.True(t, handled)
-		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)
-	})
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			req := httptest.NewRequest(http.MethodGet, "http://example.com/test", nil)
+			for k, v := range tc.reqHeaders {
+				req.Header.Set(k, v)
+			}
+			w := httptest.NewRecorder()
+			assert.Equal(t, tc.wantHandled, HandleGenericETagPrivateCache(req, w, matchedEtag, lastModifiedTime))
+			resp := w.Result()
+			for k, v := range tc.wantHeaders {
+				assert.Equal(t, v, resp.Header.Get(k))
+			}
+			assert.Equal(t, tc.wantStatus, util.Iif(resp.StatusCode == http.StatusOK, 0, resp.StatusCode))
+		})
+	}
 }