Check primary keys for all tables and drop ForeignReference (#21721)
Some dbs require that all tables have primary keys, see - #16802 - #21086 We can add a test to keep it from being broken again. Edit: ~Added missing primary key for `ForeignReference`~ Dropped the `ForeignReference` table to satisfy the check, so it closes #21086. More context can be found in comments. Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: zeripath <art27@cantab.net>
This commit is contained in:
parent
41f0668da8
commit
71ca3067bc
11 changed files with 55 additions and 203 deletions
|
@ -12,6 +12,8 @@ import (
|
|||
"code.gitea.io/gitea/models/unittest"
|
||||
"code.gitea.io/gitea/modules/setting"
|
||||
|
||||
_ "code.gitea.io/gitea/cmd" // for TestPrimaryKeys
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
)
|
||||
|
||||
|
@ -51,3 +53,34 @@ func TestDeleteOrphanedObjects(t *testing.T) {
|
|||
assert.NoError(t, err)
|
||||
assert.EqualValues(t, countBefore, countAfter)
|
||||
}
|
||||
|
||||
func TestPrimaryKeys(t *testing.T) {
|
||||
// Some dbs require that all tables have primary keys, see
|
||||
// https://github.com/go-gitea/gitea/issues/21086
|
||||
// https://github.com/go-gitea/gitea/issues/16802
|
||||
// To avoid creating tables without primary key again, this test will check them.
|
||||
// Import "code.gitea.io/gitea/cmd" to make sure each db.RegisterModel in init functions has been called.
|
||||
|
||||
beans, err := db.NamesToBean()
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
whitelist := map[string]string{
|
||||
"the_table_name_to_skip_checking": "Write a note here to explain why",
|
||||
}
|
||||
|
||||
for _, bean := range beans {
|
||||
table, err := db.TableInfo(bean)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if why, ok := whitelist[table.Name]; ok {
|
||||
t.Logf("ignore %q because %q", table.Name, why)
|
||||
continue
|
||||
}
|
||||
if len(table.PrimaryKeys) == 0 {
|
||||
t.Errorf("table %q has no primary key", table.Name)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
@ -1 +0,0 @@
|
|||
[] # empty
|
|
@ -1,52 +0,0 @@
|
|||
// Copyright 2022 Gitea. All rights reserved.
|
||||
// SPDX-License-Identifier: MIT
|
||||
|
||||
package foreignreference
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
|
||||
"code.gitea.io/gitea/modules/util"
|
||||
)
|
||||
|
||||
// ErrLocalIndexNotExist represents a "LocalIndexNotExist" kind of error.
|
||||
type ErrLocalIndexNotExist struct {
|
||||
RepoID int64
|
||||
ForeignIndex int64
|
||||
Type string
|
||||
}
|
||||
|
||||
// ErrLocalIndexNotExist checks if an error is a ErrLocalIndexNotExist.
|
||||
func IsErrLocalIndexNotExist(err error) bool {
|
||||
_, ok := err.(ErrLocalIndexNotExist)
|
||||
return ok
|
||||
}
|
||||
|
||||
func (err ErrLocalIndexNotExist) Error() string {
|
||||
return fmt.Sprintf("repository %d has no LocalIndex for ForeignIndex %d of type %s", err.RepoID, err.ForeignIndex, err.Type)
|
||||
}
|
||||
|
||||
func (err ErrLocalIndexNotExist) Unwrap() error {
|
||||
return util.ErrNotExist
|
||||
}
|
||||
|
||||
// ErrForeignIndexNotExist represents a "ForeignIndexNotExist" kind of error.
|
||||
type ErrForeignIndexNotExist struct {
|
||||
RepoID int64
|
||||
LocalIndex int64
|
||||
Type string
|
||||
}
|
||||
|
||||
// ErrForeignIndexNotExist checks if an error is a ErrForeignIndexNotExist.
|
||||
func IsErrForeignIndexNotExist(err error) bool {
|
||||
_, ok := err.(ErrForeignIndexNotExist)
|
||||
return ok
|
||||
}
|
||||
|
||||
func (err ErrForeignIndexNotExist) Error() string {
|
||||
return fmt.Sprintf("repository %d has no ForeignIndex for LocalIndex %d of type %s", err.RepoID, err.LocalIndex, err.Type)
|
||||
}
|
||||
|
||||
func (err ErrForeignIndexNotExist) Unwrap() error {
|
||||
return util.ErrNotExist
|
||||
}
|
|
@ -1,31 +0,0 @@
|
|||
// Copyright 2022 Gitea. All rights reserved.
|
||||
// SPDX-License-Identifier: MIT
|
||||
|
||||
package foreignreference
|
||||
|
||||
import (
|
||||
"code.gitea.io/gitea/models/db"
|
||||
)
|
||||
|
||||
// Type* are valid values for the Type field of ForeignReference
|
||||
const (
|
||||
TypeIssue = "issue"
|
||||
TypePullRequest = "pull_request"
|
||||
TypeComment = "comment"
|
||||
TypeReview = "review"
|
||||
TypeReviewComment = "review_comment"
|
||||
TypeRelease = "release"
|
||||
)
|
||||
|
||||
// ForeignReference represents external references
|
||||
type ForeignReference struct {
|
||||
// RepoID is the first column in all indices. now we only need 2 indices: (repo, local) and (repo, foreign, type)
|
||||
RepoID int64 `xorm:"UNIQUE(repo_foreign_type) INDEX(repo_local)" `
|
||||
LocalIndex int64 `xorm:"INDEX(repo_local)"` // the resource key inside Gitea, it can be IssueIndex, or some model ID.
|
||||
ForeignIndex string `xorm:"INDEX UNIQUE(repo_foreign_type)"`
|
||||
Type string `xorm:"VARCHAR(16) INDEX UNIQUE(repo_foreign_type)"`
|
||||
}
|
||||
|
||||
func init() {
|
||||
db.RegisterModel(new(ForeignReference))
|
||||
}
|
|
@ -9,11 +9,9 @@ import (
|
|||
"fmt"
|
||||
"regexp"
|
||||
"sort"
|
||||
"strconv"
|
||||
"strings"
|
||||
|
||||
"code.gitea.io/gitea/models/db"
|
||||
"code.gitea.io/gitea/models/foreignreference"
|
||||
"code.gitea.io/gitea/models/organization"
|
||||
"code.gitea.io/gitea/models/perm"
|
||||
access_model "code.gitea.io/gitea/models/perm/access"
|
||||
|
@ -136,12 +134,11 @@ type Issue struct {
|
|||
UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
|
||||
ClosedUnix timeutil.TimeStamp `xorm:"INDEX"`
|
||||
|
||||
Attachments []*repo_model.Attachment `xorm:"-"`
|
||||
Comments []*Comment `xorm:"-"`
|
||||
Reactions ReactionList `xorm:"-"`
|
||||
TotalTrackedTime int64 `xorm:"-"`
|
||||
Assignees []*user_model.User `xorm:"-"`
|
||||
ForeignReference *foreignreference.ForeignReference `xorm:"-"`
|
||||
Attachments []*repo_model.Attachment `xorm:"-"`
|
||||
Comments []*Comment `xorm:"-"`
|
||||
Reactions ReactionList `xorm:"-"`
|
||||
TotalTrackedTime int64 `xorm:"-"`
|
||||
Assignees []*user_model.User `xorm:"-"`
|
||||
|
||||
// IsLocked limits commenting abilities to users on an issue
|
||||
// with write access
|
||||
|
@ -321,29 +318,6 @@ func (issue *Issue) loadReactions(ctx context.Context) (err error) {
|
|||
return nil
|
||||
}
|
||||
|
||||
func (issue *Issue) loadForeignReference(ctx context.Context) (err error) {
|
||||
if issue.ForeignReference != nil {
|
||||
return nil
|
||||
}
|
||||
reference := &foreignreference.ForeignReference{
|
||||
RepoID: issue.RepoID,
|
||||
LocalIndex: issue.Index,
|
||||
Type: foreignreference.TypeIssue,
|
||||
}
|
||||
has, err := db.GetEngine(ctx).Get(reference)
|
||||
if err != nil {
|
||||
return err
|
||||
} else if !has {
|
||||
return foreignreference.ErrForeignIndexNotExist{
|
||||
RepoID: issue.RepoID,
|
||||
LocalIndex: issue.Index,
|
||||
Type: foreignreference.TypeIssue,
|
||||
}
|
||||
}
|
||||
issue.ForeignReference = reference
|
||||
return nil
|
||||
}
|
||||
|
||||
// LoadMilestone load milestone of this issue.
|
||||
func (issue *Issue) LoadMilestone(ctx context.Context) (err error) {
|
||||
if (issue.Milestone == nil || issue.Milestone.ID != issue.MilestoneID) && issue.MilestoneID > 0 {
|
||||
|
@ -406,10 +380,6 @@ func (issue *Issue) LoadAttributes(ctx context.Context) (err error) {
|
|||
}
|
||||
}
|
||||
|
||||
if err = issue.loadForeignReference(ctx); err != nil && !foreignreference.IsErrForeignIndexNotExist(err) {
|
||||
return err
|
||||
}
|
||||
|
||||
return issue.loadReactions(ctx)
|
||||
}
|
||||
|
||||
|
@ -1097,26 +1067,6 @@ func GetIssueByIndex(repoID, index int64) (*Issue, error) {
|
|||
return issue, nil
|
||||
}
|
||||
|
||||
// GetIssueByForeignIndex returns raw issue by foreign ID
|
||||
func GetIssueByForeignIndex(ctx context.Context, repoID, foreignIndex int64) (*Issue, error) {
|
||||
reference := &foreignreference.ForeignReference{
|
||||
RepoID: repoID,
|
||||
ForeignIndex: strconv.FormatInt(foreignIndex, 10),
|
||||
Type: foreignreference.TypeIssue,
|
||||
}
|
||||
has, err := db.GetEngine(ctx).Get(reference)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
} else if !has {
|
||||
return nil, foreignreference.ErrLocalIndexNotExist{
|
||||
RepoID: repoID,
|
||||
ForeignIndex: foreignIndex,
|
||||
Type: foreignreference.TypeIssue,
|
||||
}
|
||||
}
|
||||
return GetIssueByIndex(repoID, reference.LocalIndex)
|
||||
}
|
||||
|
||||
// GetIssueWithAttrsByIndex returns issue by index in a repository.
|
||||
func GetIssueWithAttrsByIndex(repoID, index int64) (*Issue, error) {
|
||||
issue, err := GetIssueByIndex(repoID, index)
|
||||
|
|
|
@ -7,13 +7,11 @@ import (
|
|||
"context"
|
||||
"fmt"
|
||||
"sort"
|
||||
"strconv"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"code.gitea.io/gitea/models/db"
|
||||
"code.gitea.io/gitea/models/foreignreference"
|
||||
issues_model "code.gitea.io/gitea/models/issues"
|
||||
"code.gitea.io/gitea/models/organization"
|
||||
repo_model "code.gitea.io/gitea/models/repo"
|
||||
|
@ -501,38 +499,6 @@ func TestCorrectIssueStats(t *testing.T) {
|
|||
assert.EqualValues(t, issueStats.OpenCount, issueAmount)
|
||||
}
|
||||
|
||||
func TestIssueForeignReference(t *testing.T) {
|
||||
assert.NoError(t, unittest.PrepareTestDatabase())
|
||||
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 4})
|
||||
assert.NotEqualValues(t, issue.Index, issue.ID) // make sure they are different to avoid false positive
|
||||
|
||||
// it is fine for an issue to not have a foreign reference
|
||||
err := issue.LoadAttributes(db.DefaultContext)
|
||||
assert.NoError(t, err)
|
||||
assert.Nil(t, issue.ForeignReference)
|
||||
|
||||
var foreignIndex int64 = 12345
|
||||
_, err = issues_model.GetIssueByForeignIndex(context.Background(), issue.RepoID, foreignIndex)
|
||||
assert.True(t, foreignreference.IsErrLocalIndexNotExist(err))
|
||||
|
||||
err = db.Insert(db.DefaultContext, &foreignreference.ForeignReference{
|
||||
LocalIndex: issue.Index,
|
||||
ForeignIndex: strconv.FormatInt(foreignIndex, 10),
|
||||
RepoID: issue.RepoID,
|
||||
Type: foreignreference.TypeIssue,
|
||||
})
|
||||
assert.NoError(t, err)
|
||||
|
||||
err = issue.LoadAttributes(db.DefaultContext)
|
||||
assert.NoError(t, err)
|
||||
|
||||
assert.EqualValues(t, issue.ForeignReference.ForeignIndex, strconv.FormatInt(foreignIndex, 10))
|
||||
|
||||
found, err := issues_model.GetIssueByForeignIndex(context.Background(), issue.RepoID, foreignIndex)
|
||||
assert.NoError(t, err)
|
||||
assert.EqualValues(t, found.Index, issue.Index)
|
||||
}
|
||||
|
||||
func TestMilestoneList_LoadTotalTrackedTimes(t *testing.T) {
|
||||
assert.NoError(t, unittest.PrepareTestDatabase())
|
||||
miles := issues_model.MilestoneList{
|
||||
|
|
|
@ -83,13 +83,6 @@ func insertIssue(ctx context.Context, issue *issues_model.Issue) error {
|
|||
}
|
||||
}
|
||||
|
||||
if issue.ForeignReference != nil {
|
||||
issue.ForeignReference.LocalIndex = issue.Index
|
||||
if _, err := sess.Insert(issue.ForeignReference); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
|
|
|
@ -4,11 +4,9 @@
|
|||
package models
|
||||
|
||||
import (
|
||||
"strconv"
|
||||
"testing"
|
||||
|
||||
"code.gitea.io/gitea/models/db"
|
||||
"code.gitea.io/gitea/models/foreignreference"
|
||||
issues_model "code.gitea.io/gitea/models/issues"
|
||||
repo_model "code.gitea.io/gitea/models/repo"
|
||||
"code.gitea.io/gitea/models/unittest"
|
||||
|
@ -48,7 +46,6 @@ func assertCreateIssues(t *testing.T, isPull bool) {
|
|||
UserID: owner.ID,
|
||||
}
|
||||
|
||||
foreignIndex := int64(12345)
|
||||
title := "issuetitle1"
|
||||
is := &issues_model.Issue{
|
||||
RepoID: repo.ID,
|
||||
|
@ -62,20 +59,11 @@ func assertCreateIssues(t *testing.T, isPull bool) {
|
|||
IsClosed: true,
|
||||
Labels: []*issues_model.Label{label},
|
||||
Reactions: []*issues_model.Reaction{reaction},
|
||||
ForeignReference: &foreignreference.ForeignReference{
|
||||
ForeignIndex: strconv.FormatInt(foreignIndex, 10),
|
||||
RepoID: repo.ID,
|
||||
Type: foreignreference.TypeIssue,
|
||||
},
|
||||
}
|
||||
err := InsertIssues(is)
|
||||
assert.NoError(t, err)
|
||||
|
||||
i := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: title})
|
||||
assert.Nil(t, i.ForeignReference)
|
||||
err = i.LoadAttributes(db.DefaultContext)
|
||||
assert.NoError(t, err)
|
||||
assert.EqualValues(t, strconv.FormatInt(foreignIndex, 10), i.ForeignReference.ForeignIndex)
|
||||
unittest.AssertExistsAndLoadBean(t, &issues_model.Reaction{Type: "heart", UserID: owner.ID, IssueID: i.ID})
|
||||
}
|
||||
|
||||
|
|
|
@ -444,6 +444,8 @@ var migrations = []Migration{
|
|||
NewMigration("Add index for access_token", v1_19.AddIndexForAccessToken),
|
||||
// v236 -> v237
|
||||
NewMigration("Create secrets table", v1_19.CreateSecretsTable),
|
||||
// v237 -> v238
|
||||
NewMigration("Drop ForeignReference table", v1_19.DropForeignReferenceTable),
|
||||
}
|
||||
|
||||
// GetCurrentDBVersion returns the current db version
|
||||
|
|
15
models/migrations/v1_19/v237.go
Normal file
15
models/migrations/v1_19/v237.go
Normal file
|
@ -0,0 +1,15 @@
|
|||
// Copyright 2022 The Gitea Authors. All rights reserved.
|
||||
// SPDX-License-Identifier: MIT
|
||||
|
||||
package v1_19 //nolint
|
||||
|
||||
import (
|
||||
"xorm.io/xorm"
|
||||
)
|
||||
|
||||
func DropForeignReferenceTable(x *xorm.Engine) error {
|
||||
// Drop the table introduced in `v211`, it's considered badly designed and doesn't look like to be used.
|
||||
// See: https://github.com/go-gitea/gitea/issues/21086#issuecomment-1318217453
|
||||
type ForeignReference struct{}
|
||||
return x.DropTables(new(ForeignReference))
|
||||
}
|
|
@ -17,7 +17,6 @@ import (
|
|||
|
||||
"code.gitea.io/gitea/models"
|
||||
"code.gitea.io/gitea/models/db"
|
||||
"code.gitea.io/gitea/models/foreignreference"
|
||||
issues_model "code.gitea.io/gitea/models/issues"
|
||||
repo_model "code.gitea.io/gitea/models/repo"
|
||||
user_model "code.gitea.io/gitea/models/user"
|
||||
|
@ -403,16 +402,6 @@ func (g *GiteaLocalUploader) CreateIssues(issues ...*base.Issue) error {
|
|||
Labels: labels,
|
||||
CreatedUnix: timeutil.TimeStamp(issue.Created.Unix()),
|
||||
UpdatedUnix: timeutil.TimeStamp(issue.Updated.Unix()),
|
||||
ForeignReference: &foreignreference.ForeignReference{
|
||||
LocalIndex: issue.GetLocalIndex(),
|
||||
ForeignIndex: strconv.FormatInt(issue.GetForeignIndex(), 10),
|
||||
RepoID: g.repo.ID,
|
||||
Type: foreignreference.TypeIssue,
|
||||
},
|
||||
}
|
||||
|
||||
if is.ForeignReference.ForeignIndex == "0" {
|
||||
is.ForeignReference.ForeignIndex = strconv.FormatInt(is.Index, 10)
|
||||
}
|
||||
|
||||
if err := g.remapUser(issue, &is); err != nil {
|
||||
|
|
Loading…
Reference in a new issue