From 75eb6cc9c1efde75d2b34d85d4a3e4f8484e6ab4 Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 10 Mar 2022 10:09:48 +0000 Subject: [PATCH 1/7] Improve SyncMirrors logging (#19045) Yet another issue has come up where the logging from SyncMirrors does not provide enough context. This PR adds more context to these logging events. Related #19038 Signed-off-by: Andrew Thornton --- models/lfs.go | 5 ++-- modules/lfs/pointer.go | 13 +++++++++++ modules/repository/repo.go | 40 ++++++++++++++++---------------- services/mirror/mirror_pull.go | 42 +++++++++++++++++----------------- 4 files changed, 57 insertions(+), 43 deletions(-) diff --git a/models/lfs.go b/models/lfs.go index e0c16767f71e6..037ed8556b758 100644 --- a/models/lfs.go +++ b/models/lfs.go @@ -193,12 +193,13 @@ func LFSAutoAssociate(metas []*LFSMetaObject, user *user_model.User, repoID int6 // admin can associate any LFS object to any repository, and we do not care about errors (eg: duplicated unique key), // even if error occurs, it won't hurt users and won't make things worse for i := range metas { + p := lfs.Pointer{Oid: metas[i].Oid, Size: metas[i].Size} _, err = sess.Insert(&LFSMetaObject{ - Pointer: lfs.Pointer{Oid: metas[i].Oid, Size: metas[i].Size}, + Pointer: p, RepositoryID: repoID, }) if err != nil { - log.Warn("failed to insert LFS meta object into database, err=%v", err) + log.Warn("failed to insert LFS meta object %-v for repo_id: %d into database, err=%v", p, repoID, err) } } } diff --git a/modules/lfs/pointer.go b/modules/lfs/pointer.go index 975b5e7dc6d7e..2a3a2116b4138 100644 --- a/modules/lfs/pointer.go +++ b/modules/lfs/pointer.go @@ -14,6 +14,8 @@ import ( "regexp" "strconv" "strings" + + "code.gitea.io/gitea/modules/log" ) const ( @@ -111,6 +113,17 @@ func (p Pointer) RelativePath() string { return path.Join(p.Oid[0:2], p.Oid[2:4], p.Oid[4:]) } +// ColorFormat provides a basic color format for a Team +func (p Pointer) ColorFormat(s fmt.State) { + if p.Oid == "" && p.Size == 0 { + log.ColorFprintf(s, "") + return + } + log.ColorFprintf(s, "%s:%d", + log.NewColoredIDValue(p.Oid), + p.Size) +} + // GeneratePointer generates a pointer for arbitrary content func GeneratePointer(content io.Reader) (Pointer, error) { h := sha256.New() diff --git a/modules/repository/repo.go b/modules/repository/repo.go index ff022f0aeddf6..ed9ed508be4c6 100644 --- a/modules/repository/repo.go +++ b/modules/repository/repo.go @@ -254,7 +254,7 @@ func SyncReleasesWithTags(repo *repo_model.Repository, gitRepo *git.Repository) opts.Page = page rels, err := models.GetReleasesByRepoID(repo.ID, opts) if err != nil { - return fmt.Errorf("GetReleasesByRepoID: %v", err) + return fmt.Errorf("unable to GetReleasesByRepoID in Repo[%d:%s/%s]: %w", repo.ID, repo.OwnerName, repo.Name, err) } if len(rels) == 0 { break @@ -265,11 +265,11 @@ func SyncReleasesWithTags(repo *repo_model.Repository, gitRepo *git.Repository) } commitID, err := gitRepo.GetTagCommitID(rel.TagName) if err != nil && !git.IsErrNotExist(err) { - return fmt.Errorf("GetTagCommitID: %s: %v", rel.TagName, err) + return fmt.Errorf("unable to GetTagCommitID for %q in Repo[%d:%s/%s]: %w", rel.TagName, repo.ID, repo.OwnerName, repo.Name, err) } if git.IsErrNotExist(err) || commitID != rel.Sha1 { if err := models.PushUpdateDeleteTag(repo, rel.TagName); err != nil { - return fmt.Errorf("PushUpdateDeleteTag: %s: %v", rel.TagName, err) + return fmt.Errorf("unable to PushUpdateDeleteTag: %q in Repo[%d:%s/%s]: %w", rel.TagName, repo.ID, repo.OwnerName, repo.Name, err) } } else { existingRelTags[strings.ToLower(rel.TagName)] = struct{}{} @@ -278,12 +278,12 @@ func SyncReleasesWithTags(repo *repo_model.Repository, gitRepo *git.Repository) } tags, err := gitRepo.GetTags(0, 0) if err != nil { - return fmt.Errorf("GetTags: %v", err) + return fmt.Errorf("unable to GetTags in Repo[%d:%s/%s]: %w", repo.ID, repo.OwnerName, repo.Name, err) } for _, tagName := range tags { if _, ok := existingRelTags[strings.ToLower(tagName)]; !ok { if err := PushUpdateAddTag(repo, gitRepo, tagName); err != nil { - return fmt.Errorf("pushUpdateAddTag: %v", err) + return fmt.Errorf("unable to PushUpdateAddTag: %q to Repo[%d:%s/%s]: %w", tagName, repo.ID, repo.OwnerName, repo.Name, err) } } } @@ -294,11 +294,11 @@ func SyncReleasesWithTags(repo *repo_model.Repository, gitRepo *git.Repository) func PushUpdateAddTag(repo *repo_model.Repository, gitRepo *git.Repository, tagName string) error { tag, err := gitRepo.GetTag(tagName) if err != nil { - return fmt.Errorf("GetTag: %v", err) + return fmt.Errorf("unable to GetTag: %w", err) } commit, err := tag.Commit(gitRepo) if err != nil { - return fmt.Errorf("Commit: %v", err) + return fmt.Errorf("unable to get tag Commit: %w", err) } sig := tag.Tagger @@ -315,14 +315,14 @@ func PushUpdateAddTag(repo *repo_model.Repository, gitRepo *git.Repository, tagN if sig != nil { author, err = user_model.GetUserByEmail(sig.Email) if err != nil && !user_model.IsErrUserNotExist(err) { - return fmt.Errorf("GetUserByEmail: %v", err) + return fmt.Errorf("unable to GetUserByEmail for %q: %w", sig.Email, err) } createdAt = sig.When } commitsCount, err := commit.CommitsCount() if err != nil { - return fmt.Errorf("CommitsCount: %v", err) + return fmt.Errorf("unable to get CommitsCount: %w", err) } rel := models.Release{ @@ -359,14 +359,14 @@ func StoreMissingLfsObjectsInRepository(ctx context.Context, repo *repo_model.Re _, err := models.NewLFSMetaObject(&models.LFSMetaObject{Pointer: p, RepositoryID: repo.ID}) if err != nil { - log.Error("Error creating LFS meta object %v: %v", p, err) + log.Error("Repo[%-v]: Error creating LFS meta object %-v: %v", repo, p, err) return err } if err := contentStore.Put(p, content); err != nil { - log.Error("Error storing content for LFS meta object %v: %v", p, err) + log.Error("Repo[%-v]: Error storing content for LFS meta object %-v: %v", repo, p, err) if _, err2 := models.RemoveLFSMetaObjectByOid(repo.ID, p.Oid); err2 != nil { - log.Error("Error removing LFS meta object %v: %v", p, err2) + log.Error("Repo[%-v]: Error removing LFS meta object %-v: %v", repo, p, err2) } return err } @@ -386,32 +386,32 @@ func StoreMissingLfsObjectsInRepository(ctx context.Context, repo *repo_model.Re for pointerBlob := range pointerChan { meta, err := models.GetLFSMetaObjectByOid(repo.ID, pointerBlob.Oid) if err != nil && err != models.ErrLFSObjectNotExist { - log.Error("Error querying LFS meta object %v: %v", pointerBlob.Pointer, err) + log.Error("Repo[%-v]: Error querying LFS meta object %-v: %v", repo, pointerBlob.Pointer, err) return err } if meta != nil { - log.Trace("Skipping unknown LFS meta object %v", pointerBlob.Pointer) + log.Trace("Repo[%-v]: Skipping unknown LFS meta object %-v", repo, pointerBlob.Pointer) continue } - log.Trace("LFS object %v not present in repository %s", pointerBlob.Pointer, repo.FullName()) + log.Trace("Repo[%-v]: LFS object %-v not present in repository", repo, pointerBlob.Pointer) exist, err := contentStore.Exists(pointerBlob.Pointer) if err != nil { - log.Error("Error checking if LFS object %v exists: %v", pointerBlob.Pointer, err) + log.Error("Repo[%-v]: Error checking if LFS object %-v exists: %v", repo, pointerBlob.Pointer, err) return err } if exist { - log.Trace("LFS object %v already present; creating meta object", pointerBlob.Pointer) + log.Trace("Repo[%-v]: LFS object %-v already present; creating meta object", repo, pointerBlob.Pointer) _, err := models.NewLFSMetaObject(&models.LFSMetaObject{Pointer: pointerBlob.Pointer, RepositoryID: repo.ID}) if err != nil { - log.Error("Error creating LFS meta object %v: %v", pointerBlob.Pointer, err) + log.Error("Repo[%-v]: Error creating LFS meta object %-v: %v", repo, pointerBlob.Pointer, err) return err } } else { if setting.LFS.MaxFileSize > 0 && pointerBlob.Size > setting.LFS.MaxFileSize { - log.Info("LFS object %v download denied because of LFS_MAX_FILE_SIZE=%d < size %d", pointerBlob.Pointer, setting.LFS.MaxFileSize, pointerBlob.Size) + log.Info("Repo[%-v]: LFS object %-v download denied because of LFS_MAX_FILE_SIZE=%d < size %d", repo, pointerBlob.Pointer, setting.LFS.MaxFileSize, pointerBlob.Size) continue } @@ -432,7 +432,7 @@ func StoreMissingLfsObjectsInRepository(ctx context.Context, repo *repo_model.Re err, has := <-errChan if has { - log.Error("Error enumerating LFS objects for repository: %v", err) + log.Error("Repo[%-v]: Error enumerating LFS objects for repository: %v", repo, err) return err } diff --git a/services/mirror/mirror_pull.go b/services/mirror/mirror_pull.go index c86f15efc0210..6c9c4a04838d2 100644 --- a/services/mirror/mirror_pull.go +++ b/services/mirror/mirror_pull.go @@ -201,7 +201,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo remoteAddr, remoteErr := git.GetRemoteAddress(ctx, repoPath, m.GetRemoteName()) if remoteErr != nil { - log.Error("GetRemoteAddress Error %v", remoteErr) + log.Error("SyncMirrors [repo: %-v]: GetRemoteAddress Error %v", m.Repo, remoteErr) } stdoutBuilder := strings.Builder{} @@ -225,7 +225,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo // Now check if the error is a resolve reference due to broken reference if strings.Contains(stderr, "unable to resolve reference") && strings.Contains(stderr, "reference broken") { - log.Warn("Failed to update mirror repository %-v due to broken references:\nStdout: %s\nStderr: %s\nErr: %v\nAttempting Prune", m.Repo, stdoutMessage, stderrMessage, err) + log.Warn("SyncMirrors [repo: %-v]: failed to update mirror repository due to broken references:\nStdout: %s\nStderr: %s\nErr: %v\nAttempting Prune", m.Repo, stdoutMessage, stderrMessage, err) err = nil // Attempt prune @@ -255,7 +255,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo // If there is still an error (or there always was an error) if err != nil { - log.Error("Failed to update mirror repository %-v:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err) + log.Error("SyncMirrors [repo: %-v]: failed to update mirror repository:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err) desc := fmt.Sprintf("Failed to update mirror repository '%s': %s", repoPath, stderrMessage) if err = admin_model.CreateRepositoryNotice(desc); err != nil { log.Error("CreateRepositoryNotice: %v", err) @@ -267,13 +267,13 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo gitRepo, err := git.OpenRepositoryCtx(ctx, repoPath) if err != nil { - log.Error("OpenRepository: %v", err) + log.Error("SyncMirrors [repo: %-v]: failed to OpenRepository: %v", m.Repo, err) return nil, false } log.Trace("SyncMirrors [repo: %-v]: syncing releases with tags...", m.Repo) if err = repo_module.SyncReleasesWithTags(m.Repo, gitRepo); err != nil { - log.Error("Failed to synchronize tags to releases for repository: %v", err) + log.Error("SyncMirrors [repo: %-v]: failed to synchronize tags to releases: %v", m.Repo, err) } if m.LFS && setting.LFS.StartServer { @@ -281,14 +281,14 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo endpoint := lfs.DetermineEndpoint(remoteAddr.String(), m.LFSEndpoint) lfsClient := lfs.NewClient(endpoint, nil) if err = repo_module.StoreMissingLfsObjectsInRepository(ctx, m.Repo, gitRepo, lfsClient); err != nil { - log.Error("Failed to synchronize LFS objects for repository: %v", err) + log.Error("SyncMirrors [repo: %-v]: failed to synchronize LFS objects for repository: %v", m.Repo, err) } } gitRepo.Close() log.Trace("SyncMirrors [repo: %-v]: updating size of repository", m.Repo) if err := models.UpdateRepoSize(db.DefaultContext, m.Repo); err != nil { - log.Error("Failed to update size for mirror repository: %v", err) + log.Error("SyncMirrors [repo: %-v]: failed to update size for mirror repository: %v", m.Repo, err) } if m.Repo.HasWiki() { @@ -311,7 +311,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo remoteAddr, remoteErr := git.GetRemoteAddress(ctx, wikiPath, m.GetRemoteName()) if remoteErr != nil { - log.Error("GetRemoteAddress Error %v", remoteErr) + log.Error("SyncMirrors [repo: %-v Wiki]: unable to get GetRemoteAddress Error %v", m.Repo, remoteErr) } // sanitize the output, since it may contain the remote address, which may @@ -322,7 +322,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo // Now check if the error is a resolve reference due to broken reference if strings.Contains(stderrMessage, "unable to resolve reference") && strings.Contains(stderrMessage, "reference broken") { - log.Warn("Failed to update mirror wiki repository %-v due to broken references:\nStdout: %s\nStderr: %s\nErr: %v\nAttempting Prune", m.Repo, stdoutMessage, stderrMessage, err) + log.Warn("SyncMirrors [repo: %-v Wiki]: failed to update mirror wiki repository due to broken references:\nStdout: %s\nStderr: %s\nErr: %v\nAttempting Prune", m.Repo, stdoutMessage, stderrMessage, err) err = nil // Attempt prune @@ -350,7 +350,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo // If there is still an error (or there always was an error) if err != nil { - log.Error("Failed to update mirror repository wiki %-v:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err) + log.Error("SyncMirrors [repo: %-v Wiki]: failed to update mirror repository wiki:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err) desc := fmt.Sprintf("Failed to update mirror repository wiki '%s': %s", wikiPath, stderrMessage) if err = admin_model.CreateRepositoryNotice(desc); err != nil { log.Error("CreateRepositoryNotice: %v", err) @@ -364,7 +364,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo log.Trace("SyncMirrors [repo: %-v]: invalidating mirror branch caches...", m.Repo) branches, _, err := git.GetBranchesByPath(ctx, m.Repo.RepoPath(), 0, 0) if err != nil { - log.Error("GetBranches: %v", err) + log.Error("SyncMirrors [repo: %-v]: failed to GetBranches: %v", m.Repo, err) return nil, false } @@ -385,12 +385,12 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool { return } // There was a panic whilst syncMirrors... - log.Error("PANIC whilst syncMirrors[%d] Panic: %v\nStacktrace: %s", repoID, err, log.Stack(2)) + log.Error("PANIC whilst SyncMirrors[repo_id: %d] Panic: %v\nStacktrace: %s", repoID, err, log.Stack(2)) }() m, err := repo_model.GetMirrorByRepoID(repoID) if err != nil { - log.Error("GetMirrorByRepoID [%d]: %v", repoID, err) + log.Error("SyncMirrors [repo_id: %v]: unable to GetMirrorByRepoID: %v", repoID, err) return false } @@ -406,7 +406,7 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool { log.Trace("SyncMirrors [repo: %-v]: Scheduling next update", m.Repo) m.ScheduleNextUpdate() if err = repo_model.UpdateMirror(m); err != nil { - log.Error("UpdateMirror [%d]: %v", m.RepoID, err) + log.Error("SyncMirrors [repo: %-v]: failed to UpdateMirror with next update date: %v", m.Repo, err) return false } @@ -417,7 +417,7 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool { log.Trace("SyncMirrors [repo: %-v]: %d branches updated", m.Repo, len(results)) gitRepo, err = git.OpenRepositoryCtx(ctx, m.Repo.RepoPath()) if err != nil { - log.Error("OpenRepository [%d]: %v", m.RepoID, err) + log.Error("SyncMirrors [repo: %-v]: unable to OpenRepository: %v", m.Repo, err) return false } defer gitRepo.Close() @@ -444,7 +444,7 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool { } commitID, err := gitRepo.GetRefCommitID(result.refName) if err != nil { - log.Error("gitRepo.GetRefCommitID [repo_id: %d, ref_name: %s]: %v", m.RepoID, result.refName, err) + log.Error("SyncMirrors [repo: %-v]: unable to GetRefCommitID [ref_name: %s]: %v", m.Repo, result.refName, err) continue } notification.NotifySyncPushCommits(m.Repo.MustOwner(), m.Repo, &repo_module.PushUpdateOptions{ @@ -465,17 +465,17 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool { // Push commits oldCommitID, err := git.GetFullCommitID(gitRepo.Ctx, gitRepo.Path, result.oldCommitID) if err != nil { - log.Error("GetFullCommitID [%d]: %v", m.RepoID, err) + log.Error("SyncMirrors [repo: %-v]: unable to get GetFullCommitID[%s]: %v", m.Repo, result.oldCommitID, err) continue } newCommitID, err := git.GetFullCommitID(gitRepo.Ctx, gitRepo.Path, result.newCommitID) if err != nil { - log.Error("GetFullCommitID [%d]: %v", m.RepoID, err) + log.Error("SyncMirrors [repo: %-v]: unable to get GetFullCommitID [%s]: %v", m.Repo, result.newCommitID, err) continue } commits, err := gitRepo.CommitsBetweenIDs(newCommitID, oldCommitID) if err != nil { - log.Error("CommitsBetweenIDs [repo_id: %d, new_commit_id: %s, old_commit_id: %s]: %v", m.RepoID, newCommitID, oldCommitID, err) + log.Error("SyncMirrors [repo: %-v]: unable to get CommitsBetweenIDs [new_commit_id: %s, old_commit_id: %s]: %v", m.Repo, newCommitID, oldCommitID, err) continue } @@ -497,12 +497,12 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool { // Get latest commit date and update to current repository updated time commitDate, err := git.GetLatestCommitTime(ctx, m.Repo.RepoPath()) if err != nil { - log.Error("GetLatestCommitDate [%d]: %v", m.RepoID, err) + log.Error("SyncMirrors [repo: %-v]: unable to GetLatestCommitDate: %v", m.Repo, err) return false } if err = repo_model.UpdateRepositoryUpdatedTime(m.RepoID, commitDate); err != nil { - log.Error("Update repository 'updated_unix' [%d]: %v", m.RepoID, err) + log.Error("SyncMirrors [repo: %-v]: unable to update repository 'updated_unix': %v", m.Repo, err) return false } From 5fdd30423eac1f9de8546461e11d5fde26bc7d7c Mon Sep 17 00:00:00 2001 From: Norwin Date: Thu, 10 Mar 2022 11:11:26 +0100 Subject: [PATCH 2/7] Fix flag validation (#19046) Regression from #5785 --- cmd/cmd.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index fd713b9affd95..3846a8690020f 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -31,7 +31,7 @@ func argsSet(c *cli.Context, args ...string) error { return errors.New(a + " is not set") } - if util.IsEmptyString(a) { + if util.IsEmptyString(c.String(a)) { return errors.New(a + " is required") } } From cc98737ca81d9552f20c277e6ad0031927f9b757 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 10 Mar 2022 15:54:51 +0100 Subject: [PATCH 3/7] RSS/Atom support for Orgs (#17714) part of #569 --- models/action.go | 94 +++++++++++++++++++++---------------- models/action_test.go | 40 ++++++++++++++++ models/user_heatmap_test.go | 47 ++++++++++++------- routers/web/feed/profile.go | 45 +++++++++--------- routers/web/user/profile.go | 13 ++--- 5 files changed, 154 insertions(+), 85 deletions(-) diff --git a/models/action.go b/models/action.go index 26d05730c5f6d..f2723a20147cc 100644 --- a/models/action.go +++ b/models/action.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" @@ -315,19 +316,21 @@ func (a *Action) GetIssueContent() string { // GetFeedsOptions options for retrieving feeds type GetFeedsOptions struct { - RequestedUser *user_model.User // the user we want activity for - RequestedTeam *Team // the team we want activity for - Actor *user_model.User // the user viewing the activity - IncludePrivate bool // include private actions - OnlyPerformedBy bool // only actions performed by requested user - IncludeDeleted bool // include deleted actions - Date string // the day we want activity for: YYYY-MM-DD + db.ListOptions + RequestedUser *user_model.User // the user we want activity for + RequestedTeam *Team // the team we want activity for + RequestedRepo *repo_model.Repository // the repo we want activity for + Actor *user_model.User // the user viewing the activity + IncludePrivate bool // include private actions + OnlyPerformedBy bool // only actions performed by requested user + IncludeDeleted bool // include deleted actions + Date string // the day we want activity for: YYYY-MM-DD } // GetFeeds returns actions according to the provided options func GetFeeds(opts GetFeedsOptions) ([]*Action, error) { - if !activityReadable(opts.RequestedUser, opts.Actor) { - return make([]*Action, 0), nil + if opts.RequestedUser == nil && opts.RequestedTeam == nil && opts.RequestedRepo == nil { + return nil, fmt.Errorf("need at least one of these filters: RequestedUser, RequestedTeam, RequestedRepo") } cond, err := activityQueryCondition(opts) @@ -335,9 +338,14 @@ func GetFeeds(opts GetFeedsOptions) ([]*Action, error) { return nil, err } - actions := make([]*Action, 0, setting.UI.FeedPagingNum) + sess := db.GetEngine(db.DefaultContext).Where(cond) - if err := db.GetEngine(db.DefaultContext).Limit(setting.UI.FeedPagingNum).Desc("created_unix").Where(cond).Find(&actions); err != nil { + opts.SetDefaultValues() + sess = db.SetSessionPagination(sess, &opts) + + actions := make([]*Action, 0, opts.PageSize) + + if err := sess.Desc("created_unix").Find(&actions); err != nil { return nil, fmt.Errorf("Find: %v", err) } @@ -349,41 +357,44 @@ func GetFeeds(opts GetFeedsOptions) ([]*Action, error) { } func activityReadable(user, doer *user_model.User) bool { - var doerID int64 - if doer != nil { - doerID = doer.ID - } - if doer == nil || !doer.IsAdmin { - if user.KeepActivityPrivate && doerID != user.ID { - return false - } - } - return true + return !user.KeepActivityPrivate || + doer != nil && (doer.IsAdmin || user.ID == doer.ID) } func activityQueryCondition(opts GetFeedsOptions) (builder.Cond, error) { cond := builder.NewCond() - var repoIDs []int64 - var actorID int64 - if opts.Actor != nil { - actorID = opts.Actor.ID + if opts.RequestedTeam != nil && opts.RequestedUser == nil { + org, err := user_model.GetUserByID(opts.RequestedTeam.OrgID) + if err != nil { + return nil, err + } + opts.RequestedUser = org + } + + // check activity visibility for actor ( similar to activityReadable() ) + if opts.Actor == nil { + cond = cond.And(builder.In("act_user_id", + builder.Select("`user`.id").Where( + builder.Eq{"keep_activity_private": false, "visibility": structs.VisibleTypePublic}, + ).From("`user`"), + )) + } else if !opts.Actor.IsAdmin { + cond = cond.And(builder.In("act_user_id", + builder.Select("`user`.id").Where( + builder.Eq{"keep_activity_private": false}. + And(builder.In("visibility", structs.VisibleTypePublic, structs.VisibleTypeLimited))). + Or(builder.Eq{"id": opts.Actor.ID}).From("`user`"), + )) } // check readable repositories by doer/actor if opts.Actor == nil || !opts.Actor.IsAdmin { - if opts.RequestedUser.IsOrganization() { - env, err := OrgFromUser(opts.RequestedUser).AccessibleReposEnv(actorID) - if err != nil { - return nil, fmt.Errorf("AccessibleReposEnv: %v", err) - } - if repoIDs, err = env.RepoIDs(1, opts.RequestedUser.NumRepos); err != nil { - return nil, fmt.Errorf("GetUserRepositories: %v", err) - } - cond = cond.And(builder.In("repo_id", repoIDs)) - } else { - cond = cond.And(builder.In("repo_id", AccessibleRepoIDsQuery(opts.Actor))) - } + cond = cond.And(builder.In("repo_id", AccessibleRepoIDsQuery(opts.Actor))) + } + + if opts.RequestedRepo != nil { + cond = cond.And(builder.Eq{"repo_id": opts.RequestedRepo.ID}) } if opts.RequestedTeam != nil { @@ -395,11 +406,14 @@ func activityQueryCondition(opts GetFeedsOptions) (builder.Cond, error) { cond = cond.And(builder.In("repo_id", teamRepoIDs)) } - cond = cond.And(builder.Eq{"user_id": opts.RequestedUser.ID}) + if opts.RequestedUser != nil { + cond = cond.And(builder.Eq{"user_id": opts.RequestedUser.ID}) - if opts.OnlyPerformedBy { - cond = cond.And(builder.Eq{"act_user_id": opts.RequestedUser.ID}) + if opts.OnlyPerformedBy { + cond = cond.And(builder.Eq{"act_user_id": opts.RequestedUser.ID}) + } } + if !opts.IncludePrivate { cond = cond.And(builder.Eq{"is_private": false}) } diff --git a/models/action_test.go b/models/action_test.go index 306d382364c44..0ce9183b9658e 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -93,6 +93,46 @@ func TestGetFeeds2(t *testing.T) { assert.Len(t, actions, 0) } +func TestActivityReadable(t *testing.T) { + tt := []struct { + desc string + user *user_model.User + doer *user_model.User + result bool + }{{ + desc: "user should see own activity", + user: &user_model.User{ID: 1}, + doer: &user_model.User{ID: 1}, + result: true, + }, { + desc: "anon should see activity if public", + user: &user_model.User{ID: 1}, + result: true, + }, { + desc: "anon should NOT see activity", + user: &user_model.User{ID: 1, KeepActivityPrivate: true}, + result: false, + }, { + desc: "user should see own activity if private too", + user: &user_model.User{ID: 1, KeepActivityPrivate: true}, + doer: &user_model.User{ID: 1}, + result: true, + }, { + desc: "other user should NOT see activity", + user: &user_model.User{ID: 1, KeepActivityPrivate: true}, + doer: &user_model.User{ID: 2}, + result: false, + }, { + desc: "admin should see activity", + user: &user_model.User{ID: 1, KeepActivityPrivate: true}, + doer: &user_model.User{ID: 2, IsAdmin: true}, + result: true, + }} + for _, test := range tt { + assert.Equal(t, test.result, activityReadable(test.user, test.doer), test.desc) + } +} + func TestNotifyWatchers(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/models/user_heatmap_test.go b/models/user_heatmap_test.go index 7d2997648df4a..7915363d9574a 100644 --- a/models/user_heatmap_test.go +++ b/models/user_heatmap_test.go @@ -19,25 +19,40 @@ import ( func TestGetUserHeatmapDataByUser(t *testing.T) { testCases := []struct { + desc string userID int64 doerID int64 CountResult int JSONResult string }{ - // self looks at action in private repo - {2, 2, 1, `[{"timestamp":1603227600,"contributions":1}]`}, - // admin looks at action in private repo - {2, 1, 1, `[{"timestamp":1603227600,"contributions":1}]`}, - // other user looks at action in private repo - {2, 3, 0, `[]`}, - // nobody looks at action in private repo - {2, 0, 0, `[]`}, - // collaborator looks at action in private repo - {16, 15, 1, `[{"timestamp":1603267200,"contributions":1}]`}, - // no action action not performed by target user - {3, 3, 0, `[]`}, - // multiple actions performed with two grouped together - {10, 10, 3, `[{"timestamp":1603009800,"contributions":1},{"timestamp":1603010700,"contributions":2}]`}, + { + "self looks at action in private repo", + 2, 2, 1, `[{"timestamp":1603227600,"contributions":1}]`, + }, + { + "admin looks at action in private repo", + 2, 1, 1, `[{"timestamp":1603227600,"contributions":1}]`, + }, + { + "other user looks at action in private repo", + 2, 3, 0, `[]`, + }, + { + "nobody looks at action in private repo", + 2, 0, 0, `[]`, + }, + { + "collaborator looks at action in private repo", + 16, 15, 1, `[{"timestamp":1603267200,"contributions":1}]`, + }, + { + "no action action not performed by target user", + 3, 3, 0, `[]`, + }, + { + "multiple actions performed with two grouped together", + 10, 10, 3, `[{"timestamp":1603009800,"contributions":1},{"timestamp":1603010700,"contributions":2}]`, + }, } // Prepare assert.NoError(t, unittest.PrepareTestDatabase()) @@ -46,7 +61,7 @@ func TestGetUserHeatmapDataByUser(t *testing.T) { timeutil.Set(time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC)) defer timeutil.Unset() - for i, tc := range testCases { + for _, tc := range testCases { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: tc.userID}).(*user_model.User) doer := &user_model.User{ID: tc.doerID} @@ -74,7 +89,7 @@ func TestGetUserHeatmapDataByUser(t *testing.T) { } assert.NoError(t, err) assert.Len(t, actions, contributions, "invalid action count: did the test data became too old?") - assert.Equal(t, tc.CountResult, contributions, fmt.Sprintf("testcase %d", i)) + assert.Equal(t, tc.CountResult, contributions, fmt.Sprintf("testcase '%s'", tc.desc)) // Test JSON rendering jsonData, err := json.Marshal(heatmap) diff --git a/routers/web/feed/profile.go b/routers/web/feed/profile.go index 1a7f4ad24b21e..4c1eff04a9d94 100644 --- a/routers/web/feed/profile.go +++ b/routers/web/feed/profile.go @@ -23,31 +23,34 @@ func RetrieveFeeds(ctx *context.Context, options models.GetFeedsOptions) []*mode return nil } - userCache := map[int64]*user_model.User{options.RequestedUser.ID: options.RequestedUser} - if ctx.User != nil { - userCache[ctx.User.ID] = ctx.User - } - for _, act := range actions { - if act.ActUser != nil { - userCache[act.ActUserID] = act.ActUser + // TODO: move load repoOwner of act.Repo into models.GetFeeds->loadAttributes() + { + userCache := map[int64]*user_model.User{options.RequestedUser.ID: options.RequestedUser} + if ctx.User != nil { + userCache[ctx.User.ID] = ctx.User } - } - - for _, act := range actions { - repoOwner, ok := userCache[act.Repo.OwnerID] - if !ok { - repoOwner, err = user_model.GetUserByID(act.Repo.OwnerID) - if err != nil { - if user_model.IsErrUserNotExist(err) { - continue + for _, act := range actions { + if act.ActUser != nil { + userCache[act.ActUserID] = act.ActUser + } + } + for _, act := range actions { + repoOwner, ok := userCache[act.Repo.OwnerID] + if !ok { + repoOwner, err = user_model.GetUserByID(act.Repo.OwnerID) + if err != nil { + if user_model.IsErrUserNotExist(err) { + continue + } + ctx.ServerError("GetUserByID", err) + return nil } - ctx.ServerError("GetUserByID", err) - return nil + userCache[repoOwner.ID] = repoOwner } - userCache[repoOwner.ID] = repoOwner + act.Repo.Owner = repoOwner } - act.Repo.Owner = repoOwner } + return actions } @@ -57,7 +60,7 @@ func ShowUserFeed(ctx *context.Context, ctxUser *user_model.User, formatType str RequestedUser: ctxUser, Actor: ctx.User, IncludePrivate: false, - OnlyPerformedBy: true, + OnlyPerformedBy: !ctxUser.IsOrganization(), IncludeDeleted: false, Date: ctx.FormString("date"), }) diff --git a/routers/web/user/profile.go b/routers/web/user/profile.go index 9c0ce10dae6e3..b4198ef8fd8b8 100644 --- a/routers/web/user/profile.go +++ b/routers/web/user/profile.go @@ -94,14 +94,11 @@ func Profile(ctx *context.Context) { } if ctxUser.IsOrganization() { - /* - // TODO: enable after rss.RetrieveFeeds() do handle org correctly - // Show Org RSS feed - if len(showFeedType) != 0 { - rss.ShowUserFeed(ctx, ctxUser, showFeedType) - return - } - */ + // Show Org RSS feed + if len(showFeedType) != 0 { + feed.ShowUserFeed(ctx, ctxUser, showFeedType) + return + } org.Home(ctx) return From ba470a85dd657c586aa07bd5bb4010b93aa8dfa9 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 10 Mar 2022 19:12:10 +0100 Subject: [PATCH 4/7] use xorm builder for models.getReviewers() (#19033) * xorm builder * dedup code --- models/repo.go | 62 +++++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/models/repo.go b/models/repo.go index 53199bcca355f..d20e5f81d31be 100644 --- a/models/repo.go +++ b/models/repo.go @@ -218,50 +218,44 @@ func getReviewers(ctx context.Context, repo *repo_model.Repository, doerID, post return nil, err } - var users []*user_model.User - e := db.GetEngine(ctx) + cond := builder.And(builder.Neq{"`user`.id": posterID}) if repo.IsPrivate || repo.Owner.Visibility == api.VisibleTypePrivate { // This a private repository: // Anyone who can read the repository is a requestable reviewer - if err := e. - SQL("SELECT * FROM `user` WHERE id in ("+ - "SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id != ?"+ // private org repos - ") ORDER BY name", - repo.ID, perm.AccessModeRead, - posterID). - Find(&users); err != nil { - return nil, err - } + + cond = cond.And(builder.In("`user`.id", + builder.Select("user_id").From("access").Where( + builder.Eq{"repo_id": repo.ID}. + And(builder.Gte{"mode": perm.AccessModeRead}), + ), + )) if repo.Owner.Type == user_model.UserTypeIndividual && repo.Owner.ID != posterID { // as private *user* repos don't generate an entry in the `access` table, // the owner of a private repo needs to be explicitly added. - users = append(users, repo.Owner) + cond = cond.Or(builder.Eq{"`user`.id": repo.Owner.ID}) } - return users, nil - } - - // This is a "public" repository: - // Any user that has read access, is a watcher or organization member can be requested to review - if err := e. - SQL("SELECT * FROM `user` WHERE id IN ( "+ - "SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? "+ - "UNION "+ - "SELECT user_id FROM `watch` WHERE repo_id = ? AND mode IN (?, ?) "+ - "UNION "+ - "SELECT uid AS user_id FROM `org_user` WHERE org_id = ? "+ - ") AND id != ? ORDER BY name", - repo.ID, perm.AccessModeRead, - repo.ID, repo_model.WatchModeNormal, repo_model.WatchModeAuto, - repo.OwnerID, - posterID). - Find(&users); err != nil { - return nil, err - } - - return users, nil + } else { + // This is a "public" repository: + // Any user that has read access, is a watcher or organization member can be requested to review + cond = cond.And(builder.And(builder.In("`user`.id", + builder.Select("user_id").From("access"). + Where(builder.Eq{"repo_id": repo.ID}. + And(builder.Gte{"mode": perm.AccessModeRead})), + ).Or(builder.In("`user`.id", + builder.Select("user_id").From("watch"). + Where(builder.Eq{"repo_id": repo.ID}. + And(builder.In("mode", repo_model.WatchModeNormal, repo_model.WatchModeAuto))), + ).Or(builder.In("`user`.id", + builder.Select("uid").From("org_user"). + Where(builder.Eq{"org_id": repo.OwnerID}), + ))))) + } + + users := make([]*user_model.User, 0, 8) + return users, db.GetEngine(ctx).Where(cond).OrderBy("name").Find(&users) } // GetReviewers get all users can be requested to review: From a0db075f21e961b687221a8b9ed9defb26d2624e Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 10 Mar 2022 20:23:15 +0000 Subject: [PATCH 5/7] If rendering has failed due to a net.OpError stop rendering (attempt 2) (#19049) Unfortunately #18642 does not work because a `*net.OpError` does not implement the `Is` interface to make `errors.Is` work correctly - thus leading to the irritating conclusion that a `*net.OpError` is not a `*net.OpError`. Here we keep the `errors.Is` because presumably this will be fixed at some point in the golang main source code but also we add a simply type cast to also check. Fix #18629 Signed-off-by: Andrew Thornton --- modules/context/context.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/context/context.go b/modules/context/context.go index 6aeeb9e694676..8e50e154a14d5 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -269,7 +269,7 @@ func (ctx *Context) ServerError(logMsg string, logErr error) { func (ctx *Context) serverErrorInternal(logMsg string, logErr error) { if logErr != nil { log.ErrorWithSkip(2, "%s: %v", logMsg, logErr) - if errors.Is(logErr, &net.OpError{}) { + if _, ok := logErr.(*net.OpError); ok || errors.Is(logErr, &net.OpError{}) { // This is an error within the underlying connection // and further rendering will not work so just return return From 886b1de94914186a502ed68a6a281a15f1d8d9a3 Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 10 Mar 2022 22:04:55 +0000 Subject: [PATCH 6/7] Update the webauthn_credential_id_sequence in Postgres (#19048) * Update the webauthn_credential_id_sequence in Postgres There is (yet) another problem with v210 in that Postgres will silently allow preset ID insertions ... but it will not update the sequence value. This PR simply adds a little step to the end of the v210 migration to update the sequence number. Users who have already migrated who find that they cannot insert new webauthn_credentials into the DB can either run: ```bash gitea doctor recreate-table webauthn_credential ``` or ```bash ./gitea doctor --run=check-db-consistency --fix ``` which will fix the bad sequence. Fix #19012 Signed-off-by: Andrew Thornton --- docs/content/doc/developers/guidelines-backend.md | 14 ++++++++++++-- models/migrations/v210.go | 6 ++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/docs/content/doc/developers/guidelines-backend.md b/docs/content/doc/developers/guidelines-backend.md index d249465453f61..1248d4143233f 100644 --- a/docs/content/doc/developers/guidelines-backend.md +++ b/docs/content/doc/developers/guidelines-backend.md @@ -42,7 +42,7 @@ To maintain understandable code and avoid circular dependencies it is important - `modules/setting`: Store all system configurations read from ini files and has been referenced by everywhere. But they should be used as function parameters when possible. - `modules/git`: Package to interactive with `Git` command line or Gogit package. - `public`: Compiled frontend files (javascript, images, css, etc.) -- `routers`: Handling of server requests. As it uses other Gitea packages to serve the request, other packages (models, modules or services) shall not depend on routers. +- `routers`: Handling of server requests. As it uses other Gitea packages to serve the request, other packages (models, modules or services) must not depend on routers. - `routers/api` Contains routers for `/api/v1` aims to handle RESTful API requests. - `routers/install` Could only respond when system is in INSTALL mode (INSTALL_LOCK=false). - `routers/private` will only be invoked by internal sub commands, especially `serv` and `hooks`. @@ -106,10 +106,20 @@ i.e. `servcies/user`, `models/repository`. Since there are some packages which use the same package name, it is possible that you find packages like `modules/user`, `models/user`, and `services/user`. When these packages are imported in one Go file, it's difficult to know which package we are using and if it's a variable name or an import name. So, we always recommend to use import aliases. To differ from package variables which are commonly in camelCase, just use **snake_case** for import aliases. i.e. `import user_service "code.gitea.io/gitea/services/user"` +### Important Gotchas + +- Never write `x.Update(exemplar)` without an explicit `WHERE` clause: + - This will cause all rows in the table to be updated with the non-zero values of the exemplar - including IDs. + - You should usually write `x.ID(id).Update(exemplar)`. +- If during a migration you are inserting into a table using `x.Insert(exemplar)` where the ID is preset: + - You will need to ``SET IDENTITY_INSERT `table` ON`` for the MSSQL variant (the migration will fail otherwise) + - However, you will also need to update the id sequence for postgres - the migration will silently pass here but later insertions will fail: + ``SELECT setval('table_name_id_seq', COALESCE((SELECT MAX(id)+1 FROM `table_name`), 1), false)`` + ### Future Tasks Currently, we are creating some refactors to do the following things: - Correct that codes which doesn't follow the rules. - There are too many files in `models`, so we are moving some of them into a sub package `models/xxx`. -- Some `modules` sub packages should be moved to `services` because they depends on `models`. \ No newline at end of file +- Some `modules` sub packages should be moved to `services` because they depend on `models`. diff --git a/models/migrations/v210.go b/models/migrations/v210.go index dafe355fe211a..f32fae77bacc9 100644 --- a/models/migrations/v210.go +++ b/models/migrations/v210.go @@ -174,5 +174,11 @@ func remigrateU2FCredentials(x *xorm.Engine) error { regs = regs[:0] } + if x.Dialect().URI().DBType == schemas.POSTGRES { + if _, err := x.Exec("SELECT setval('webauthn_credential_id_seq', COALESCE((SELECT MAX(id)+1 FROM `webauthn_credential`), 1), false)"); err != nil { + return err + } + } + return nil } From a223bc8765f18e1efc59d5291412d0076dfeaa9b Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 10 Mar 2022 22:40:43 +0000 Subject: [PATCH 7/7] Prevent 500 when there is an error during new auth source post (#19041) Fix #19036 Signed-off-by: Andrew Thornton --- routers/web/admin/auths.go | 7 ++----- templates/admin/auth/new.tmpl | 2 +- templates/admin/auth/source/oauth.tmpl | 4 ++-- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/routers/web/admin/auths.go b/routers/web/admin/auths.go index a8e0cd37b6a62..4c77a169ae1c6 100644 --- a/routers/web/admin/auths.go +++ b/routers/web/admin/auths.go @@ -93,7 +93,7 @@ func NewAuthSource(ctx *context.Context) { ctx.Data["PageIsAdmin"] = true ctx.Data["PageIsAdminAuthentications"] = true - ctx.Data["type"] = auth.LDAP + ctx.Data["type"] = auth.LDAP.Int() ctx.Data["CurrentTypeName"] = auth.Names[auth.LDAP] ctx.Data["CurrentSecurityProtocol"] = ldap.SecurityProtocolNames[ldap.SecurityProtocolUnencrypted] ctx.Data["smtp_auth"] = "PLAIN" @@ -112,7 +112,7 @@ func NewAuthSource(ctx *context.Context) { ctx.Data["SSPIDefaultLanguage"] = "" // only the first as default - ctx.Data["oauth2_provider"] = oauth2providers[0] + ctx.Data["oauth2_provider"] = oauth2providers[0].Name ctx.HTML(http.StatusOK, tplAuthNew) } @@ -253,9 +253,6 @@ func NewAuthSourcePost(ctx *context.Context) { ctx.Data["SSPISeparatorReplacement"] = "_" ctx.Data["SSPIDefaultLanguage"] = "" - // FIXME: most error path to render tplAuthNew will fail and result in 500 - // * template: admin/auth/new:17:68: executing "admin/auth/new" at <.type.Int>: can't evaluate field Int in type interface {} - // * template: admin/auth/source/oauth:5:93: executing "admin/auth/source/oauth" at <.oauth2_provider.Name>: can't evaluate field Name in type interface {} hasTLS := false var config convert.Conversion switch auth.Type(form.Type) { diff --git a/templates/admin/auth/new.tmpl b/templates/admin/auth/new.tmpl index b8e80dbcaaa01..9882cde03bcd4 100644 --- a/templates/admin/auth/new.tmpl +++ b/templates/admin/auth/new.tmpl @@ -14,7 +14,7 @@
- +
{{.CurrentTypeName}}
{{svg "octicon-triangle-down" 14 "dropdown icon"}}
diff --git a/templates/admin/auth/source/oauth.tmpl b/templates/admin/auth/source/oauth.tmpl index 85c7cb6166d3c..3991dc2170792 100644 --- a/templates/admin/auth/source/oauth.tmpl +++ b/templates/admin/auth/source/oauth.tmpl @@ -2,8 +2,8 @@
- -
{{.oauth2_provider.Name}}
+ +
{{.oauth2_provider}}
{{svg "octicon-triangle-down" 14 "dropdown icon"}}
{{range .OAuth2Providers}}