From bf728b83069884b7867418d8357cb408fcb1fd44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Dachary?= Date: Tue, 3 May 2022 23:25:15 +0100 Subject: [PATCH 1/4] GetFeeds must always discard actions with dangling repo_id See https://discourse.gitea.io/t/blank-page-after-login/5051/12 for a panic in 1.16.6. --- models/action.go | 10 +++++----- models/action_test.go | 17 +++++++++++++++++ models/fixtures/action.yml | 8 ++++++++ models/unittest/consistency.go | 6 ++++-- 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/models/action.go b/models/action.go index 5177616497514..efbe243bed3b9 100644 --- a/models/action.go +++ b/models/action.go @@ -340,14 +340,14 @@ func GetFeeds(ctx context.Context, opts GetFeedsOptions) (ActionList, error) { } e := db.GetEngine(ctx) - sess := e.Where(cond) + sess := e.Where(cond).Join("INNER", "repository", "`repository`.id = `action`.repo_id") opts.SetDefaultValues() sess = db.SetSessionPagination(sess, &opts) actions := make([]*Action, 0, opts.PageSize) - if err := sess.Desc("created_unix").Find(&actions); err != nil { + if err := sess.Desc("`action`.created_unix").Find(&actions); err != nil { return nil, fmt.Errorf("Find: %v", err) } @@ -417,7 +417,7 @@ func activityQueryCondition(opts GetFeedsOptions) (builder.Cond, error) { } if !opts.IncludePrivate { - cond = cond.And(builder.Eq{"is_private": false}) + cond = cond.And(builder.Eq{"`action`.is_private": false}) } if !opts.IncludeDeleted { cond = cond.And(builder.Eq{"is_deleted": false}) @@ -430,8 +430,8 @@ func activityQueryCondition(opts GetFeedsOptions) (builder.Cond, error) { } else { dateHigh := dateLow.Add(86399000000000) // 23h59m59s - cond = cond.And(builder.Gte{"created_unix": dateLow.Unix()}) - cond = cond.And(builder.Lte{"created_unix": dateHigh.Unix()}) + cond = cond.And(builder.Gte{"`action`.created_unix": dateLow.Unix()}) + cond = cond.And(builder.Lte{"`action`.created_unix": dateHigh.Unix()}) } } diff --git a/models/action_test.go b/models/action_test.go index e247a5ec29c80..fb8a6c2686f12 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -211,3 +211,20 @@ func TestNotifyWatchers(t *testing.T) { OpType: action.OpType, }) } + +func TestGetFeedsCorrupted(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}).(*user_model.User) + unittest.AssertExistsAndLoadBean(t, &Action{ + ID: 8, + RepoID: 1700, + }) + + actions, err := GetFeeds(db.DefaultContext, GetFeedsOptions{ + RequestedUser: user, + Actor: user, + IncludePrivate: true, + }) + assert.NoError(t, err) + assert.Len(t, actions, 0) +} diff --git a/models/fixtures/action.yml b/models/fixtures/action.yml index e283b01db29fc..ef9b55177c0f5 100644 --- a/models/fixtures/action.yml +++ b/models/fixtures/action.yml @@ -56,3 +56,11 @@ repo_id: 8 # public is_private: false created_unix: 1603011540 # grouped with id:7 + +- id: 8 + user_id: 1 + op_type: 12 # close issue + act_user_id: 1 + repo_id: 1700 + is_private: false + created_unix: 1603011541 diff --git a/models/unittest/consistency.go b/models/unittest/consistency.go index 2645084d3ede3..0597f9d5008d2 100644 --- a/models/unittest/consistency.go +++ b/models/unittest/consistency.go @@ -175,8 +175,10 @@ func init() { checkForActionConsistency := func(t assert.TestingT, bean interface{}) { action := reflectionWrap(bean) - repoRow := AssertExistsAndLoadMap(t, "repository", builder.Eq{"id": action.int("RepoID")}) - assert.Equal(t, parseBool(repoRow["is_private"]), action.bool("IsPrivate"), "action: %+v", action) + if action.int("RepoID") != 1700 { + repoRow := AssertExistsAndLoadMap(t, "repository", builder.Eq{"id": action.int("RepoID")}) + assert.Equal(t, parseBool(repoRow["is_private"]), action.bool("IsPrivate"), "action: %+v", action) + } } consistencyCheckMap["user"] = checkForUserConsistency From 4b4da6bacdda7ab0fe7bf2b8e2481cfbd2a4b33f Mon Sep 17 00:00:00 2001 From: singuliere Date: Wed, 4 May 2022 01:23:02 +0100 Subject: [PATCH 2/4] add comment to explain the dangling ID in the fixture --- models/fixtures/action.yml | 2 +- models/unittest/consistency.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/models/fixtures/action.yml b/models/fixtures/action.yml index ef9b55177c0f5..a75092feb0ecc 100644 --- a/models/fixtures/action.yml +++ b/models/fixtures/action.yml @@ -61,6 +61,6 @@ user_id: 1 op_type: 12 # close issue act_user_id: 1 - repo_id: 1700 + repo_id: 1700 # dangling intentional is_private: false created_unix: 1603011541 diff --git a/models/unittest/consistency.go b/models/unittest/consistency.go index 0597f9d5008d2..af05348868339 100644 --- a/models/unittest/consistency.go +++ b/models/unittest/consistency.go @@ -175,7 +175,7 @@ func init() { checkForActionConsistency := func(t assert.TestingT, bean interface{}) { action := reflectionWrap(bean) - if action.int("RepoID") != 1700 { + if action.int("RepoID") != 1700 { // dangling intentional repoRow := AssertExistsAndLoadMap(t, "repository", builder.Eq{"id": action.int("RepoID")}) assert.Equal(t, parseBool(repoRow["is_private"]), action.bool("IsPrivate"), "action: %+v", action) } From 3fc19d85b1038a6209dbfd06a4c6700259812d51 Mon Sep 17 00:00:00 2001 From: singuliere Date: Wed, 4 May 2022 01:35:34 +0100 Subject: [PATCH 3/4] loadRepoOwner must not attempt to use a nil action.Repo --- models/action_list.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/models/action_list.go b/models/action_list.go index c180a82552c51..609a8fc791211 100644 --- a/models/action_list.go +++ b/models/action_list.go @@ -80,6 +80,9 @@ func (actions ActionList) loadRepoOwner(e db.Engine, userMap map[int64]*user_mod } for _, action := range actions { + if action.Repo == nil { + continue + } repoOwner, ok := userMap[action.Repo.OwnerID] if !ok { repoOwner, err = user_model.GetUserByID(action.Repo.OwnerID) From f0ca51276d73dff36f8c2bd3197f14cf73d4162c Mon Sep 17 00:00:00 2001 From: singuliere Date: Wed, 4 May 2022 07:07:15 +0100 Subject: [PATCH 4/4] make fmt --- models/action_list.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/models/action_list.go b/models/action_list.go index 609a8fc791211..5f7b17b9de1ef 100644 --- a/models/action_list.go +++ b/models/action_list.go @@ -80,9 +80,9 @@ func (actions ActionList) loadRepoOwner(e db.Engine, userMap map[int64]*user_mod } for _, action := range actions { - if action.Repo == nil { - continue - } + if action.Repo == nil { + continue + } repoOwner, ok := userMap[action.Repo.OwnerID] if !ok { repoOwner, err = user_model.GetUserByID(action.Repo.OwnerID)