Add repo_id for attachment (#16958)

When create a new issue or comment and paste/upload an attachment/image, it will not assign an issue id before submit. So if user give up the creating, the attachments will lost key feature and become dirty content. We don't know if we need to delete the attachment even if the repository deleted.

This PR add a repo_id in attachment table so that even if a new upload attachment with no issue_id or release_id but should have repo_id. When deleting a repository, they could also be deleted.

Co-authored-by: 6543 <6543@obermui.de>

models/attachment.go

@@ -5,25 +5,23 @@
 package models
 
 import (
-	"bytes"
 	"fmt"
-	"io"
 	"path"
 
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/storage"
 	"code.gitea.io/gitea/modules/timeutil"
 
-	gouuid "github.com/google/uuid"
 	"xorm.io/xorm"
 )
 
 // Attachment represent a attachment of issue/comment/release.
 type Attachment struct {
 	ID            int64  `xorm:"pk autoincr"`
 	UUID          string `xorm:"uuid UNIQUE"`
-	IssueID       int64  `xorm:"INDEX"`
-	ReleaseID     int64  `xorm:"INDEX"`
+	RepoID        int64  `xorm:"INDEX"`           // this should not be zero
+	IssueID       int64  `xorm:"INDEX"`           // maybe zero when creating
+	ReleaseID     int64  `xorm:"INDEX"`           // maybe zero when creating
 	UploaderID    int64  `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added
 	CommentID     int64
 	Name          string
@@ -81,23 +79,6 @@ func (a *Attachment) LinkedRepository() (*Repository, UnitType, error) {
 	return nil, -1, nil
 }
 
-// NewAttachment creates a new attachment object.
-func NewAttachment(attach *Attachment, buf []byte, file io.Reader) (_ *Attachment, err error) {
-	attach.UUID = gouuid.New().String()
-
-	size, err := storage.Attachments.Save(attach.RelativePath(), io.MultiReader(bytes.NewReader(buf), file), -1)
-	if err != nil {
-		return nil, fmt.Errorf("Create: %v", err)
-	}
-	attach.Size = size
-
-	if _, err := x.Insert(attach); err != nil {
-		return nil, err
-	}
-
-	return attach, nil
-}
-
 // GetAttachmentByID returns attachment by given id
 func GetAttachmentByID(id int64) (*Attachment, error) {
 	return getAttachmentByID(x, id)

models/attachment_test.go

@@ -5,40 +5,11 @@
 package models
 
 import (
-	"os"
-	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
-func TestUploadAttachment(t *testing.T) {
-	assert.NoError(t, PrepareTestDatabase())
-
-	user := AssertExistsAndLoadBean(t, &User{ID: 1}).(*User)
-
-	fPath := "./attachment_test.go"
-	f, err := os.Open(fPath)
-	assert.NoError(t, err)
-	defer f.Close()
-
-	buf := make([]byte, 1024)
-	n, err := f.Read(buf)
-	assert.NoError(t, err)
-	buf = buf[:n]
-
-	attach, err := NewAttachment(&Attachment{
-		UploaderID: user.ID,
-		Name:       filepath.Base(fPath),
-	}, buf, f)
-	assert.NoError(t, err)
-
-	attachment, err := GetAttachmentByUUID(attach.UUID)
-	assert.NoError(t, err)
-	assert.EqualValues(t, user.ID, attachment.UploaderID)
-	assert.Equal(t, int64(0), attachment.DownloadCount)
-}
-
 func TestIncreaseDownloadCount(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 

models/context.go

@@ -66,3 +66,9 @@ func Iterate(ctx DBContext, tableBean interface{}, cond builder.Cond, fun func(i
 		BufferSize(setting.Database.IterateBufferSize).
 		Iterate(tableBean, fun)
 }
+
+// Insert inserts records into database
+func Insert(ctx DBContext, beans ...interface{}) error {
+	_, err := ctx.e.Insert(beans...)
+	return err
+}

models/fixtures/attachment.yml

@@ -1,6 +1,7 @@
 -
   id: 1
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11
+  repo_id: 1
   issue_id: 1
   comment_id: 0
   name: attach1
@@ -10,6 +11,7 @@
 -
   id: 2
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12
+  repo_id: 2
   issue_id: 4
   comment_id: 0
   name: attach2
@@ -19,6 +21,7 @@
 -
   id: 3
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a13
+  repo_id: 1
   issue_id: 2
   comment_id: 1
   name: attach1
@@ -28,6 +31,7 @@
 -
   id: 4
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14
+  repo_id: 1
   issue_id: 3
   comment_id: 1
   name: attach2
@@ -37,6 +41,7 @@
 -
   id: 5
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a15
+  repo_id: 2
   issue_id: 4
   comment_id: 0
   name: attach1
@@ -46,6 +51,7 @@
 -
   id: 6
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a16
+  repo_id: 1
   issue_id: 5
   comment_id: 2
   name: attach1
@@ -55,6 +61,7 @@
 -
   id: 7
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a17
+  repo_id: 1
   issue_id: 5
   comment_id: 2
   name: attach1
@@ -64,6 +71,7 @@
 -
   id: 8
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18
+  repo_id: 3
   issue_id: 6
   comment_id: 0
   name: attach1
@@ -73,6 +81,7 @@
 -
   id: 9
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a19
+  repo_id: 1
   release_id: 1
   name: attach1
   download_count: 0
@@ -81,6 +90,7 @@
 -
   id: 10
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a20
+  repo_id: 0 # TestGetAttachment/NotLinked
   uploader_id: 8
   name: attach1
   download_count: 0
@@ -89,6 +99,7 @@
 -
   id: 11
   uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a21
+  repo_id: 40
   release_id: 2
   name: attach1
   download_count: 0

models/migrations/migrations.go

@@ -338,6 +338,8 @@ var migrations = []Migration{
 	NewMigration("Alter issue/comment table TEXT fields to LONGTEXT", alterIssueAndCommentTextFieldsToLongText),
 	// v192 -> v193
 	NewMigration("RecreateIssueResourceIndexTable to have a primary key instead of an unique index", recreateIssueResourceIndexTable),
+	// v193 -> v194
+	NewMigration("Add repo id column for attachment table", addRepoIDForAttachment),
 }
 
 // GetCurrentDBVersion returns the current db version

models/migrations/v193.go

@@ -0,0 +1,33 @@
+// Copyright 2021 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"xorm.io/xorm"
+)
+
+func addRepoIDForAttachment(x *xorm.Engine) error {
+	type Attachment struct {
+		ID         int64  `xorm:"pk autoincr"`
+		UUID       string `xorm:"uuid UNIQUE"`
+		RepoID     int64  `xorm:"INDEX"` // this should not be zero
+		IssueID    int64  `xorm:"INDEX"` // maybe zero when creating
+		ReleaseID  int64  `xorm:"INDEX"` // maybe zero when creating
+		UploaderID int64  `xorm:"INDEX DEFAULT 0"`
+	}
+	if err := x.Sync2(new(Attachment)); err != nil {
+		return err
+	}
+
+	if _, err := x.Exec("UPDATE `attachment` set repo_id = (SELECT repo_id FROM `issue` WHERE `issue`.id = `attachment`.issue_id) WHERE `attachment`.issue_id > 0"); err != nil {
+		return err
+	}
+
+	if _, err := x.Exec("UPDATE `attachment` set repo_id = (SELECT repo_id FROM `release` WHERE `release`.id = `attachment`.release_id) WHERE `attachment`.release_id > 0"); err != nil {
+		return err
+	}
+
+	return nil
+}

models/migrations/v193_test.go

@@ -0,0 +1,71 @@
+// Copyright 2021 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_addRepoIDForAttachment(t *testing.T) {
+	type Attachment struct {
+		ID         int64  `xorm:"pk autoincr"`
+		UUID       string `xorm:"uuid UNIQUE"`
+		RepoID     int64  `xorm:"INDEX"` // this should not be zero
+		IssueID    int64  `xorm:"INDEX"` // maybe zero when creating
+		ReleaseID  int64  `xorm:"INDEX"` // maybe zero when creating
+		UploaderID int64  `xorm:"INDEX DEFAULT 0"`
+	}
+
+	type Issue struct {
+		ID     int64
+		RepoID int64
+	}
+
+	type Release struct {
+		ID     int64
+		RepoID int64
+	}
+
+	// Prepare and load the testing database
+	x, deferrable := prepareTestEnv(t, 0, new(Attachment), new(Issue), new(Release))
+	defer deferrable()
+	if x == nil || t.Failed() {
+		return
+	}
+
+	// Run the migration
+	if err := addRepoIDForAttachment(x); err != nil {
+		assert.NoError(t, err)
+		return
+	}
+
+	var issueAttachments []*Attachment
+	err := x.Where("issue_id > 0").Find(&issueAttachments)
+	assert.NoError(t, err)
+	for _, attach := range issueAttachments {
+		assert.Greater(t, attach.RepoID, 0)
+		assert.Greater(t, attach.IssueID, 0)
+		var issue Issue
+		has, err := x.ID(attach.IssueID).Get(&issue)
+		assert.NoError(t, err)
+		assert.True(t, has)
+		assert.EqualValues(t, attach.RepoID, issue.RepoID)
+	}
+
+	var releaseAttachments []*Attachment
+	err = x.Where("release_id > 0").Find(&releaseAttachments)
+	assert.NoError(t, err)
+	for _, attach := range releaseAttachments {
+		assert.Greater(t, attach.RepoID, 0)
+		assert.Greater(t, attach.IssueID, 0)
+		var release Release
+		has, err := x.ID(attach.ReleaseID).Get(&release)
+		assert.NoError(t, err)
+		assert.True(t, has)
+		assert.EqualValues(t, attach.RepoID, release.RepoID)
+	}
+}