feat: git/blob use NewTruncatedReader for profile and codeowners (#8243)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8243
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

models/issues/pull.go

@@ -5,6 +5,7 @@
 package issues
 
 import (
+	"bufio"
 	"context"
 	"errors"
 	"fmt"
@@ -923,31 +924,30 @@ func MergeBlockedByOutdatedBranch(protectBranch *git_model.ProtectedBranch, pr *
 	return protectBranch.BlockOnOutdatedBranch && pr.CommitsBehind > 0
 }
 
-// GetCodeOwnersFromContent returns the code owners configuration
-// Return empty slice if files missing
+// GetCodeOwnersFromReader returns the code owners configuration
 // Return warning messages on parsing errors
 // We're trying to do the best we can when parsing a file.
 // Invalid lines are skipped. Non-existent users and teams too.
-func GetCodeOwnersFromContent(ctx context.Context, data string) ([]*CodeOwnerRule, []string) {
-	if len(data) == 0 {
-		return nil, nil
-	}
+func GetCodeOwnersFromReader(ctx context.Context, rc io.ReadCloser, truncated bool) ([]*CodeOwnerRule, []string) {
+	defer rc.Close()
+	scanner := bufio.NewScanner(rc)
 
-	rules := make([]*CodeOwnerRule, 0)
-	lines := strings.Split(data, "\n")
-	warnings := make([]string, 0)
+	var rules []*CodeOwnerRule
+	var warnings []string
+	line := 0
+	for scanner.Scan() {
+		line++
 
-	for i, line := range lines {
-		tokens := TokenizeCodeOwnersLine(line)
+		tokens := TokenizeCodeOwnersLine(scanner.Text())
 		if len(tokens) == 0 {
 			continue
 		} else if len(tokens) < 2 {
-			warnings = append(warnings, fmt.Sprintf("Line: %d: incorrect format", i+1))
+			warnings = append(warnings, fmt.Sprintf("Line: %d: incorrect format", line))
 			continue
 		}
 		rule, wr := ParseCodeOwnersLine(ctx, tokens)
 		for _, w := range wr {
-			warnings = append(warnings, fmt.Sprintf("Line: %d: %s", i+1, w))
+			warnings = append(warnings, fmt.Sprintf("Line: %d: %s", line, w))
 		}
 		if rule == nil {
 			continue
@@ -955,6 +955,12 @@ func GetCodeOwnersFromContent(ctx context.Context, data string) ([]*CodeOwnerRul
 
 		rules = append(rules, rule)
 	}
+	if err := scanner.Err(); err != nil {
+		warnings = append(warnings, err.Error())
+	}
+	if truncated {
+		warnings = append(warnings, fmt.Sprintf("File too big: truncated while on line %d", line))
+	}
 
 	return rules, warnings
 }

modules/git/blob_test.go

@@ -35,7 +35,7 @@ func TestBlob_Data(t *testing.T) {
 	assert.Equal(t, output, string(data))
 }
 
-func TestBlob_GetBlobContent(t *testing.T) {
+func TestBlob(t *testing.T) {
 	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
 	repo, err := openRepositoryWithDefaultContext(bareRepo1Path)
 	require.NoError(t, err)
@@ -45,30 +45,93 @@ func TestBlob_GetBlobContent(t *testing.T) {
 	testBlob, err := repo.GetBlob("6c493ff740f9380390d5c9ddef4af18697ac9375")
 	require.NoError(t, err)
 
-	r, err := testBlob.GetBlobContent(100)
-	require.NoError(t, err)
-	require.Equal(t, "file2\n", r)
+	t.Run("GetBlobContent", func(t *testing.T) {
+		r, err := testBlob.GetBlobContent(100)
+		require.NoError(t, err)
+		require.Equal(t, "file2\n", r)
 
-	r, err = testBlob.GetBlobContent(-1)
-	require.NoError(t, err)
-	require.Empty(t, r)
+		r, err = testBlob.GetBlobContent(-1)
+		require.NoError(t, err)
+		require.Empty(t, r)
 
-	r, err = testBlob.GetBlobContent(4)
-	require.NoError(t, err)
-	require.Equal(t, "file", r)
+		r, err = testBlob.GetBlobContent(4)
+		require.NoError(t, err)
+		require.Equal(t, "file", r)
 
-	r, err = testBlob.GetBlobContent(6)
-	require.NoError(t, err)
-	require.Equal(t, "file2\n", r)
+		r, err = testBlob.GetBlobContent(6)
+		require.NoError(t, err)
+		require.Equal(t, "file2\n", r)
+	})
 
-	t.Run("non-existing blob", func(t *testing.T) {
-		inexistingBlob, err := repo.GetBlob("00003ff740f9380390d5c9ddef4af18690000000")
+	t.Run("NewTruncatedReader", func(t *testing.T) {
+		// read fewer than available
+		rc, size, err := testBlob.NewTruncatedReader(100)
+		require.NoError(t, err)
+		require.Equal(t, int64(6), size)
+
+		buf := make([]byte, 1)
+		n, err := rc.Read(buf)
+		require.NoError(t, err)
+		require.Equal(t, 1, n)
+		require.Equal(t, "f", string(buf))
+		n, err = rc.Read(buf)
+		require.NoError(t, err)
+		require.Equal(t, 1, n)
+		require.Equal(t, "i", string(buf))
+
+		require.NoError(t, rc.Close())
+
+		// read more than available
+		rc, size, err = testBlob.NewTruncatedReader(100)
+		require.NoError(t, err)
+		require.Equal(t, int64(6), size)
+
+		buf = make([]byte, 100)
+		n, err = rc.Read(buf)
+		require.NoError(t, err)
+		require.Equal(t, 6, n)
+		require.Equal(t, "file2\n", string(buf[:n]))
+
+		n, err = rc.Read(buf)
+		require.Error(t, err)
+		require.Equal(t, io.EOF, err)
+		require.Equal(t, 0, n)
+
+		require.NoError(t, rc.Close())
+
+		// read more than truncated
+		rc, size, err = testBlob.NewTruncatedReader(4)
+		require.NoError(t, err)
+		require.Equal(t, int64(6), size)
+
+		buf = make([]byte, 10)
+		n, err = rc.Read(buf)
+		require.NoError(t, err)
+		require.Equal(t, 4, n)
+		require.Equal(t, "file", string(buf[:n]))
+
+		n, err = rc.Read(buf)
+		require.Error(t, err)
+		require.Equal(t, io.EOF, err)
+		require.Equal(t, 0, n)
+
+		require.NoError(t, rc.Close())
+	})
+
+	t.Run("NonExisting", func(t *testing.T) {
+		nonExistingBlob, err := repo.GetBlob("00003ff740f9380390d5c9ddef4af18690000000")
 		require.NoError(t, err)
 
-		r, err := inexistingBlob.GetBlobContent(100)
+		r, err := nonExistingBlob.GetBlobContent(100)
 		require.Error(t, err)
 		require.IsType(t, ErrNotExist{}, err)
 		require.Empty(t, r)
+
+		rc, size, err := nonExistingBlob.NewTruncatedReader(100)
+		require.Error(t, err)
+		require.IsType(t, ErrNotExist{}, err)
+		require.Empty(t, rc)
+		require.Empty(t, size)
 	})
 }
 

modules/markup/markdown/markdown.go

@@ -267,8 +267,13 @@ func Render(ctx *markup.RenderContext, input io.Reader, output io.Writer) error
 
 // RenderString renders Markdown string to HTML with all specific handling stuff and return string
 func RenderString(ctx *markup.RenderContext, content string) (template.HTML, error) {
+	return RenderReader(ctx, strings.NewReader(content))
+}
+
+// RenderReader renders Markdown io.Reader to HTML with all specific handling stuff and return string
+func RenderReader(ctx *markup.RenderContext, input io.Reader) (template.HTML, error) {
 	var buf strings.Builder
-	if err := Render(ctx, strings.NewReader(content), &buf); err != nil {
+	if err := Render(ctx, input, &buf); err != nil {
 		return "", err
 	}
 	return template.HTML(buf.String()), nil

routers/web/org/home.go

@@ -175,10 +175,12 @@ func prepareOrgProfileReadme(ctx *context.Context, profileGitRepo *git.Repositor
 		return
 	}
 
-	if bytes, err := profileReadme.GetBlobContent(setting.UI.MaxDisplayFileSize); err != nil {
-		log.Error("failed to GetBlobContent: %v", err)
+	if rc, _, err := profileReadme.NewTruncatedReader(setting.UI.MaxDisplayFileSize); err != nil {
+		log.Error("failed to NewTruncatedReader: %v", err)
 	} else {
-		if profileContent, err := markdown.RenderString(&markup.RenderContext{
+		defer rc.Close()
+
+		if profileContent, err := markdown.RenderReader(&markup.RenderContext{
 			Ctx:     ctx,
 			GitRepo: profileGitRepo,
 			Links: markup.Links{
@@ -188,7 +190,7 @@ func prepareOrgProfileReadme(ctx *context.Context, profileGitRepo *git.Repositor
 				BranchPath: path.Join("branch", util.PathEscapeSegments(profileDbRepo.DefaultBranch)),
 			},
 			Metas: map[string]string{"mode": "document"},
-		}, bytes); err != nil {
+		}, rc); err != nil {
 			log.Error("failed to RenderString: %v", err)
 		} else {
 			ctx.Data["ProfileReadme"] = profileContent

routers/web/repo/view.go

@@ -439,8 +439,8 @@ func renderFile(ctx *context.Context, entry *git.TreeEntry) {
 			ctx.Data["FileError"] = ctx.Locale.Tr("actions.runs.invalid_workflow_helper", workFlowErr.Error())
 		}
 	} else if slices.Contains([]string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}, ctx.Repo.TreePath) {
-		if data, err := blob.GetBlobContent(setting.UI.MaxDisplayFileSize); err == nil {
-			_, warnings := issue_model.GetCodeOwnersFromContent(ctx, data)
+		if rc, size, err := blob.NewTruncatedReader(setting.UI.MaxDisplayFileSize); err == nil {
+			_, warnings := issue_model.GetCodeOwnersFromReader(ctx, rc, size > setting.UI.MaxDisplayFileSize)
 			if len(warnings) > 0 {
 				ctx.Data["FileWarning"] = strings.Join(warnings, "\n")
 			}

routers/web/user/profile.go

@@ -264,10 +264,12 @@ func prepareUserProfileTabData(ctx *context.Context, showPrivate bool, profileDb
 
 		total = int(count)
 	case "overview":
-		if bytes, err := profileReadme.GetBlobContent(setting.UI.MaxDisplayFileSize); err != nil {
-			log.Error("failed to GetBlobContent: %v", err)
+		if rc, _, err := profileReadme.NewTruncatedReader(setting.UI.MaxDisplayFileSize); err != nil {
+			log.Error("failed to NewTruncatedReader: %v", err)
 		} else {
-			if profileContent, err := markdown.RenderString(&markup.RenderContext{
+			defer rc.Close()
+
+			if profileContent, err := markdown.RenderReader(&markup.RenderContext{
 				Ctx:     ctx,
 				GitRepo: profileGitRepo,
 				Links: markup.Links{
@@ -280,7 +282,7 @@ func prepareUserProfileTabData(ctx *context.Context, showPrivate bool, profileDb
 					BranchPath: path.Join("branch", util.PathEscapeSegments(profileDbRepo.DefaultBranch)),
 				},
 				Metas: map[string]string{"mode": "document"},
-			}, bytes); err != nil {
+			}, rc); err != nil {
 				log.Error("failed to RenderString: %v", err)
 			} else {
 				ctx.Data["ProfileReadme"] = profileContent

services/issue/pull.go

@@ -43,8 +43,6 @@ type ReviewRequestNotifier struct {
 }
 
 func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) {
-	files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}
-
 	if pr.IsWorkInProgress(ctx) {
 		return nil, nil
 	}
@@ -72,18 +70,17 @@ func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue,
 		return nil, err
 	}
 
-	var data string
-	for _, file := range files {
+	var rules []*issues_model.CodeOwnerRule
+	for _, file := range []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} {
 		if blob, err := commit.GetBlobByPath(file); err == nil {
-			data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize)
+			rc, size, err := blob.NewTruncatedReader(setting.UI.MaxDisplayFileSize)
 			if err == nil {
+				rules, _ = issues_model.GetCodeOwnersFromReader(ctx, rc, size > setting.UI.MaxDisplayFileSize)
 				break
 			}
 		}
 	}
 
-	rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data)
-
 	// get the mergebase
 	mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())
 	if err != nil {

tests/integration/org_profile_test.go

@@ -0,0 +1,108 @@
+// Copyright 2024 The Forgejo Authors c/o Codeberg e.V.. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"net/http"
+	"net/url"
+	"strings"
+	"testing"
+
+	"forgejo.org/models/unittest"
+	user_model "forgejo.org/models/user"
+	"forgejo.org/modules/setting"
+	"forgejo.org/modules/test"
+	files_service "forgejo.org/services/repository/files"
+	"forgejo.org/tests"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestOrgProfile(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		checkReadme := func(t *testing.T, title, readmeFilename string, expectedCount int) {
+			t.Run(title, func(t *testing.T) {
+				defer tests.PrintCurrentTest(t)()
+
+				// Prepare the test repository
+				org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3})
+
+				var ops []*files_service.ChangeRepoFile
+				op := "create"
+				if readmeFilename != "README.md" {
+					ops = append(ops, &files_service.ChangeRepoFile{
+						Operation: "delete",
+						TreePath:  "README.md",
+					})
+				} else {
+					op = "update"
+				}
+				if readmeFilename != "" {
+					ops = append(ops, &files_service.ChangeRepoFile{
+						Operation:     op,
+						TreePath:      readmeFilename,
+						ContentReader: strings.NewReader("# Hi!\n"),
+					})
+				}
+
+				_, _, f := tests.CreateDeclarativeRepo(t, org3, ".profile", nil, nil, ops)
+				defer f()
+
+				// Perform the test
+				req := NewRequest(t, "GET", "/org3")
+				resp := MakeRequest(t, req, http.StatusOK)
+
+				doc := NewHTMLParser(t, resp.Body)
+				readmeCount := doc.Find("#readme_profile").Length()
+
+				assert.Equal(t, expectedCount, readmeCount)
+			})
+		}
+
+		checkReadme(t, "No readme", "", 0)
+		checkReadme(t, "README.md", "README.md", 1)
+		checkReadme(t, "readme.md", "readme.md", 1)
+		checkReadme(t, "ReadMe.mD", "ReadMe.mD", 1)
+		checkReadme(t, "readme.org does not render", "README.org", 0)
+
+		t.Run("readme-size", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			// Prepare the test repository
+			org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3})
+
+			_, _, f := tests.CreateDeclarativeRepo(t, org3, ".profile", nil, nil, []*files_service.ChangeRepoFile{
+				{
+					Operation: "update",
+					TreePath:  "README.md",
+					ContentReader: strings.NewReader(`## Lorem ipsum
+dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
+## Ut enim ad minim veniam
+quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum`),
+				},
+			})
+			defer f()
+
+			t.Run("full", func(t *testing.T) {
+				defer tests.PrintCurrentTest(t)()
+				defer test.MockVariableValue(&setting.UI.MaxDisplayFileSize, 500)()
+
+				req := NewRequest(t, "GET", "/org3")
+				resp := MakeRequest(t, req, http.StatusOK)
+				assert.Contains(t, resp.Body.String(), "Ut enim ad minim veniam")
+				assert.Contains(t, resp.Body.String(), "mollit anim id est laborum")
+			})
+
+			t.Run("truncated", func(t *testing.T) {
+				defer tests.PrintCurrentTest(t)()
+				defer test.MockVariableValue(&setting.UI.MaxDisplayFileSize, 146)()
+
+				req := NewRequest(t, "GET", "/org3")
+				resp := MakeRequest(t, req, http.StatusOK)
+				assert.Contains(t, resp.Body.String(), "Ut enim ad minim")
+				assert.NotContains(t, resp.Body.String(), "veniam")
+			})
+		})
+	})
+}

tests/integration/user_profile_test.go

@@ -11,6 +11,8 @@ import (
 
 	"forgejo.org/models/unittest"
 	user_model "forgejo.org/models/user"
+	"forgejo.org/modules/setting"
+	"forgejo.org/modules/test"
 	files_service "forgejo.org/services/repository/files"
 	"forgejo.org/tests"
 
@@ -63,5 +65,44 @@ func TestUserProfile(t *testing.T) {
 		checkReadme(t, "readme.md", "readme.md", 1)
 		checkReadme(t, "ReadMe.mD", "ReadMe.mD", 1)
 		checkReadme(t, "readme.org does not render", "README.org", 0)
+
+		t.Run("readme-size", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			// Prepare the test repository
+			user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+			_, _, f := tests.CreateDeclarativeRepo(t, user2, ".profile", nil, nil, []*files_service.ChangeRepoFile{
+				{
+					Operation: "update",
+					TreePath:  "README.md",
+					ContentReader: strings.NewReader(`## Lorem ipsum
+dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
+## Ut enim ad minim veniam
+quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum`),
+				},
+			})
+			defer f()
+
+			t.Run("full", func(t *testing.T) {
+				defer tests.PrintCurrentTest(t)()
+				defer test.MockVariableValue(&setting.UI.MaxDisplayFileSize, 500)()
+
+				req := NewRequest(t, "GET", "/user2")
+				resp := MakeRequest(t, req, http.StatusOK)
+				assert.Contains(t, resp.Body.String(), "Ut enim ad minim veniam")
+				assert.Contains(t, resp.Body.String(), "mollit anim id est laborum")
+			})
+
+			t.Run("truncated", func(t *testing.T) {
+				defer tests.PrintCurrentTest(t)()
+				defer test.MockVariableValue(&setting.UI.MaxDisplayFileSize, 146)()
+
+				req := NewRequest(t, "GET", "/user2")
+				resp := MakeRequest(t, req, http.StatusOK)
+				assert.Contains(t, resp.Body.String(), "Ut enim ad minim")
+				assert.NotContains(t, resp.Body.String(), "veniam")
+			})
+		})
 	})
 }