mirror of
https://github.com/go-gitea/gitea.git
synced 2026-07-02 23:53:28 +00:00
This PR replaces a set of struct-based `Get` lookups with explicit `db.Get` / `db.Exist` conditions in places where zero-value fields can lead to ambiguous matches or incorrect records being returned. The main goal is to make read paths deterministic and avoid accidentally matching the wrong row when only part of a struct is populated. ### What changed - replace many `db.GetEngine(ctx).Get(bean)` calls with explicit `builder.Eq` conditions across models such as actions, admin tasks, issues, pull requests, repositories, users, packages, redirects, watches, stars, and follows - use quoted column names where needed for reserved fields like `index`, `type`, and `name` - add dedicated user lookup helpers for: - primary email - OAuth login source / login name - update sign-in and OAuth-related flows to use explicit individual-user lookups instead of partially populated `User` structs - tighten package property and Terraform lock lookups to avoid ambiguous reads and updates - keep existing fallback behavior where needed, while removing reliance on zero-value struct matching ### User-facing impact These changes primarily affect authentication and account lookup paths: - email/username sign-in now re-fetches users through explicit keys - OAuth2 auto-linking now resolves users by name or primary email explicitly - OAuth2 login/sync now looks up users by login source, login type, and login name explicitly - non-individual accounts are no longer implicitly matched through partial user lookups in these flows This should reduce the risk of incorrect account matches and make query behavior more predictable across the codebase. --------- Co-authored-by: bircni <bircni@icloud.com>
159 lines
6.1 KiB
Go
159 lines
6.1 KiB
Go
// Copyright 2022 The Gitea Authors. All rights reserved.
|
|
// SPDX-License-Identifier: MIT
|
|
|
|
package pull
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"maps"
|
|
|
|
"gitea.dev/models/db"
|
|
"gitea.dev/modules/log"
|
|
"gitea.dev/modules/timeutil"
|
|
|
|
"xorm.io/builder"
|
|
)
|
|
|
|
// ViewedState stores for a file in which state it is currently viewed
|
|
type ViewedState uint8
|
|
|
|
const (
|
|
Unviewed ViewedState = iota
|
|
HasChanged // cannot be set from the UI/ API, only internally
|
|
Viewed
|
|
)
|
|
|
|
func (viewedState ViewedState) String() string {
|
|
switch viewedState {
|
|
case Unviewed:
|
|
return "unviewed"
|
|
case HasChanged:
|
|
return "has-changed"
|
|
case Viewed:
|
|
return "viewed"
|
|
default:
|
|
return fmt.Sprintf("unknown(value=%d)", viewedState)
|
|
}
|
|
}
|
|
|
|
// ReviewState stores for a user-PR-commit combination which files the user has already viewed
|
|
type ReviewState struct {
|
|
ID int64 `xorm:"pk autoincr"`
|
|
UserID int64 `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
|
|
PullID int64 `xorm:"NOT NULL INDEX UNIQUE(pull_commit_user) DEFAULT 0"` // Which PR was the review on?
|
|
CommitSHA string `xorm:"NOT NULL VARCHAR(64) UNIQUE(pull_commit_user)"` // Which commit was the head commit for the review?
|
|
UpdatedFiles map[string]ViewedState `xorm:"NOT NULL LONGTEXT JSON"` // Stores for each of the changed files of a PR whether they have been viewed, changed since last viewed, or not viewed
|
|
UpdatedUnix timeutil.TimeStamp `xorm:"updated"` // Is an accurate indicator of the order of commits as we do not expect it to be possible to make reviews on previous commits
|
|
}
|
|
|
|
func init() {
|
|
db.RegisterModel(new(ReviewState))
|
|
}
|
|
|
|
func (rs *ReviewState) GetViewedFileCount() int {
|
|
if len(rs.UpdatedFiles) == 0 {
|
|
return 0
|
|
}
|
|
var numViewedFiles int
|
|
for _, state := range rs.UpdatedFiles {
|
|
if state == Viewed {
|
|
numViewedFiles++
|
|
}
|
|
}
|
|
return numViewedFiles
|
|
}
|
|
|
|
// GetReviewState returns the ReviewState with all given values prefilled, whether or not it exists in the database.
|
|
// If the review didn't exist before in the database, it won't afterwards either.
|
|
// The returned boolean shows whether the review exists in the database
|
|
func GetReviewState(ctx context.Context, userID, pullID int64, commitSHA string) (*ReviewState, bool, error) {
|
|
review, has, err := db.Get[ReviewState](ctx, builder.Eq{"user_id": userID, "pull_id": pullID, "commit_sha": commitSHA})
|
|
if err != nil {
|
|
return nil, false, err
|
|
}
|
|
if review == nil {
|
|
review = &ReviewState{UserID: userID, PullID: pullID, CommitSHA: commitSHA}
|
|
}
|
|
return review, has, nil
|
|
}
|
|
|
|
// UpdateReviewState updates the given review inside the database, regardless of whether it existed before or not
|
|
// The given map of files with their viewed state will be merged with the previous review, if present
|
|
func UpdateReviewState(ctx context.Context, userID, pullID int64, commitSHA string, updatedFiles map[string]ViewedState) (*ReviewState, error) {
|
|
log.Trace("Updating review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, updatedFiles)
|
|
|
|
review, exists, err := GetReviewState(ctx, userID, pullID, commitSHA)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
if exists {
|
|
review.UpdatedFiles = mergeFiles(review.UpdatedFiles, updatedFiles)
|
|
} else if previousReview, err := getNewestReviewStateApartFrom(ctx, userID, pullID, commitSHA); err != nil {
|
|
return nil, err
|
|
|
|
// Overwrite the viewed files of the previous review if present
|
|
} else if previousReview != nil {
|
|
review.UpdatedFiles = mergeFiles(previousReview.UpdatedFiles, updatedFiles)
|
|
} else {
|
|
review.UpdatedFiles = updatedFiles
|
|
}
|
|
|
|
// Insert or Update review
|
|
engine := db.GetEngine(ctx)
|
|
if !exists {
|
|
log.Trace("Inserting new review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, review.UpdatedFiles)
|
|
_, err := engine.Insert(review)
|
|
return nil, err
|
|
}
|
|
log.Trace("Updating already existing review with ID %d (user %d, repo %d, commit %s) with the updated files %v.", review.ID, userID, pullID, commitSHA, review.UpdatedFiles)
|
|
_, err = engine.ID(review.ID).Cols("updated_files").Update(review)
|
|
return review, err
|
|
}
|
|
|
|
// mergeFiles merges the given maps of files with their viewing state into one map.
|
|
// Values from oldFiles will be overridden with values from newFiles
|
|
func mergeFiles(oldFiles, newFiles map[string]ViewedState) map[string]ViewedState {
|
|
if oldFiles == nil {
|
|
return newFiles
|
|
} else if newFiles == nil {
|
|
return oldFiles
|
|
}
|
|
|
|
maps.Copy(oldFiles, newFiles)
|
|
return oldFiles
|
|
}
|
|
|
|
// GetNewestReviewState gets the newest review of the current user in the current PR.
|
|
// The returned PR Review will be nil if the user has not yet reviewed this PR.
|
|
func GetNewestReviewState(ctx context.Context, userID, pullID int64) (*ReviewState, error) {
|
|
var review ReviewState
|
|
has, err := db.GetEngine(ctx).Where("user_id = ?", userID).And("pull_id = ?", pullID).OrderBy("updated_unix DESC").Get(&review)
|
|
if err != nil || !has {
|
|
return nil, err
|
|
}
|
|
return &review, err
|
|
}
|
|
|
|
// getNewestReviewStateApartFrom is like GetNewestReview, except that the second newest review will be returned if the newest review points at the given commit.
|
|
// The returned PR Review will be nil if the user has not yet reviewed this PR.
|
|
func getNewestReviewStateApartFrom(ctx context.Context, userID, pullID int64, commitSHA string) (*ReviewState, error) {
|
|
var reviews []ReviewState
|
|
err := db.GetEngine(ctx).Where("user_id = ?", userID).And("pull_id = ?", pullID).OrderBy("updated_unix DESC").Limit(2).Find(&reviews)
|
|
// It would also be possible to use ".And("commit_sha != ?", commitSHA)" instead of the error handling below
|
|
// However, benchmarks show drastically improved performance by not doing that
|
|
|
|
// Error cases in which no review should be returned
|
|
if err != nil || len(reviews) == 0 || (len(reviews) == 1 && reviews[0].CommitSHA == commitSHA) {
|
|
return nil, err
|
|
|
|
// The first review points at the commit to exclude, hence skip to the second review
|
|
} else if len(reviews) >= 2 && reviews[0].CommitSHA == commitSHA {
|
|
return &reviews[1], nil
|
|
}
|
|
|
|
// As we have no error cases left, the result must be the first element in the list
|
|
return &reviews[0], nil
|
|
}
|