Skip to content

Commit 0eb0179

Browse files
mfenniakMathieu Fenniak
mfenniak
authored and
Mathieu Fenniak
committed
feat: add foreign keys to the action_runner_token table (#10756)
In support of adding foreign keys to the `action_runner_token` table, this PR also had to: - Add detection and error if a table with "soft delete" is used with a foreign key, because it causes a tricky to track down foreign key violation - Remove unused xorm "soft delete" capability on the `action_runner_token` table - Change the `RepoID` and `OwnerID` fields to use the value `NULL` to indicate that this scope wasn't valid for the token, rather than the value `0` ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10756 Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org> Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
1 parent 2cd58c0 commit 0eb0179

21 files changed

Lines changed: 336 additions & 31 deletions

File tree

‎models/actions/runner_token.go‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import (
1212
user_model "forgejo.org/models/user"
1313
"forgejo.org/modules/timeutil"
1414
"forgejo.org/modules/util"
15+
16+
"xorm.io/builder"
1517
)
1618

1719
// ActionRunnerToken represents runner tokens
@@ -29,15 +31,14 @@ import (
2931
type ActionRunnerToken struct {
3032
ID int64
3133
Token string `xorm:"UNIQUE"`
32-
OwnerID int64 `xorm:"index"`
34+
OwnerID int64 `xorm:"index REFERENCES(user, id)"`
3335
Owner *user_model.User `xorm:"-"`
34-
RepoID int64 `xorm:"index"`
36+
RepoID int64 `xorm:"index REFERENCES(repository, id)"`
3537
Repo *repo_model.Repository `xorm:"-"`
3638
IsActive bool // true means it can be used
3739

3840
Created timeutil.TimeStamp `xorm:"created"`
3941
Updated timeutil.TimeStamp `xorm:"updated"`
40-
Deleted timeutil.TimeStamp `xorm:"deleted"`
4142
}
4243

4344
func init() {
@@ -77,6 +78,16 @@ func NewRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerTo
7778
ownerID = 0
7879
}
7980

81+
// To ensure that NULL values are used for the unused columns, rather than attempting to insert 0 values which will
82+
// cause FK violation, manage the list of columns that xorm will insert.
83+
cols := []string{"is_active", "token"}
84+
if ownerID != 0 {
85+
cols = append(cols, "owner_id")
86+
}
87+
if repoID != 0 {
88+
cols = append(cols, "repo_id")
89+
}
90+
8091
token := util.CryptoRandomString(util.RandomStringHigh)
8192
runnerToken := &ActionRunnerToken{
8293
OwnerID: ownerID,
@@ -86,17 +97,33 @@ func NewRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerTo
8697
}
8798

8899
return runnerToken, db.WithTx(ctx, func(ctx context.Context) error {
89-
if _, err := db.GetEngine(ctx).Where("owner_id =? AND repo_id = ?", ownerID, repoID).Cols("is_active").Update(&ActionRunnerToken{
100+
if _, err := db.GetEngine(ctx).Where(runnerTokenCond(ownerID, repoID)).Cols("is_active").Update(&ActionRunnerToken{
90101
IsActive: false,
91102
}); err != nil {
92103
return err
93104
}
94105

95-
_, err := db.GetEngine(ctx).Insert(runnerToken)
106+
_, err := db.GetEngine(ctx).Cols(cols...).Insert(runnerToken)
96107
return err
97108
})
98109
}
99110

111+
func runnerTokenCond(ownerID, repoID int64) builder.Cond {
112+
var condOwnerID builder.Cond
113+
if ownerID == 0 {
114+
condOwnerID = builder.IsNull{"owner_id"}
115+
} else {
116+
condOwnerID = builder.Eq{"owner_id": ownerID}
117+
}
118+
var condRepoID builder.Cond
119+
if repoID == 0 {
120+
condRepoID = builder.IsNull{"repo_id"}
121+
} else {
122+
condRepoID = builder.Eq{"repo_id": repoID}
123+
}
124+
return builder.And(condOwnerID, condRepoID)
125+
}
126+
100127
// GetLatestRunnerToken returns the latest runner token
101128
func GetLatestRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerToken, error) {
102129
if ownerID != 0 && repoID != 0 {
@@ -106,7 +133,7 @@ func GetLatestRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRu
106133
}
107134

108135
var runnerToken ActionRunnerToken
109-
has, err := db.GetEngine(ctx).Where("owner_id=? AND repo_id=?", ownerID, repoID).
136+
has, err := db.GetEngine(ctx).Where(runnerTokenCond(ownerID, repoID)).
110137
OrderBy("id DESC").Get(&runnerToken)
111138
if err != nil {
112139
return nil, err

‎models/actions/runner_token_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ func TestUpdateRunnerToken(t *testing.T) {
3434
require.NoError(t, unittest.PrepareTestDatabase())
3535
token := unittest.AssertExistsAndLoadBean(t, &ActionRunnerToken{ID: 3})
3636
token.IsActive = true
37-
require.NoError(t, UpdateRunnerToken(db.DefaultContext, token))
37+
require.NoError(t, UpdateRunnerToken(db.DefaultContext, token, "is_active"))
3838
expectedToken, err := GetLatestRunnerToken(db.DefaultContext, 1, 0)
3939
require.NoError(t, err)
4040
assert.Equal(t, expectedToken, token)

‎models/db/foreign_keys.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,13 @@ func extendBeansForCascade(beans []any) ([]any, error) {
256256
if deduplicateTables.Contains(schema.Name) {
257257
continue
258258
}
259+
260+
for _, column := range schema.Columns() {
261+
if column.IsDeleted {
262+
return nil, fmt.Errorf("unable to use table %q in a cascade operation, as it has a soft-delete column %q", schema.Name, column.FieldName)
263+
}
264+
}
265+
259266
deduplicateTables.Add(schema.Name)
260267
for _, referencingTable := range referencedTables[schema.Name] {
261268
table := tableMap[referencingTable]

‎models/fixtures/action_runner_token.yml‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
-
22
id: 1 # instance scope
33
token: xeiWBL5kuTYxGPynHCqQdoeYmJAeG3IzGXCYTrDX
4-
owner_id: 0
5-
repo_id: 0
4+
owner_id: null
5+
repo_id: null
66
is_active: true
77
created: 1695617748
88
updated: 1695617748
@@ -11,7 +11,7 @@
1111
id: 2 # user scope and can't be used
1212
token: vohJB9QcZuSv1gAXESTk2uqpSjHhsKT9j4zYF84x
1313
owner_id: 1
14-
repo_id: 0
14+
repo_id: null
1515
is_active: false
1616
created: 1695617749
1717
updated: 1695617749
@@ -20,15 +20,15 @@
2020
id: 3 # user scope and can be used
2121
token: gjItAeJ3CA74hNPmPPo0Zco8I1eMaNcP1jVifjOE
2222
owner_id: 1
23-
repo_id: 0
23+
repo_id: null
2424
is_active: true
2525
created: 1695617750
2626
updated: 1695617750
2727

2828
-
2929
id: 4 # repo scope
3030
token: NOjLubxzFxPGhPXflZknys0gjVvQNhomFbAYuhbH
31-
owner_id: 0
31+
owner_id: null
3232
repo_id: 1
3333
is_active: true
3434
created: 1695617751

‎models/forgejo_migrations/v15a_remove-softdelete-action_runner_token.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// Copyright 2026 The Forgejo Authors. All rights reserved.
2+
// SPDX-License-Identifier: GPL-3.0-or-later
3+
4+
package forgejo_migrations
5+
6+
import (
7+
"xorm.io/builder"
8+
"xorm.io/xorm"
9+
)
10+
11+
func init() {
12+
registerMigration(&Migration{
13+
Description: "remove soft-delete capability from action_runner_token",
14+
Upgrade: removeSoftDeleteActionRunnerToken,
15+
})
16+
}
17+
18+
func removeSoftDeleteActionRunnerToken(x *xorm.Engine) error {
19+
// ActionRunnerToken was implemented with a column: "Deleted timeutil.TimeStamp `xorm:"deleted"``", which invokes
20+
// xorm's soft-delete capability -- that is, if a record is deleted from the table, then it is just marked with a
21+
// delete timestamp which causes it to be automatically excluded from future queries. This functionality is not
22+
// used on `ActionRunnerToken` and it stands in the way of foreign key implementation -- if you can't actually
23+
// delete the record in the table, then you can't remove foreign key references and therefore can't delete contents
24+
// of the target tables, repository and user.
25+
//
26+
// This migration removes that column and deletes any records that were soft-deleted.
27+
28+
// Before dropping the 'deleted' column, hard-delete any soft-deleted records.
29+
if _, err := x.Table("action_runner_token").Where(builder.NotNull{"deleted"}).Delete(); err != nil {
30+
return err
31+
}
32+
33+
_, err := x.Exec("ALTER TABLE action_runner_token DROP COLUMN `deleted`")
34+
return err
35+
}

‎models/forgejo_migrations/v15a_remove-softdelete-action_runner_token_test.go‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// Copyright 2026 The Forgejo Authors.
2+
// SPDX-License-Identifier: GPL-3.0-or-later
3+
4+
package forgejo_migrations
5+
6+
import (
7+
"testing"
8+
9+
"forgejo.org/models/db"
10+
migration_tests "forgejo.org/models/gitea_migrations/test"
11+
"forgejo.org/modules/timeutil"
12+
13+
"github.com/stretchr/testify/assert"
14+
"github.com/stretchr/testify/require"
15+
)
16+
17+
func Test_removeSoftDeleteActionRunnerToken(t *testing.T) {
18+
type ActionRunnerToken struct {
19+
ID int64
20+
Token string `xorm:"UNIQUE"`
21+
OwnerID int64 `xorm:"index"`
22+
RepoID int64 `xorm:"index"`
23+
IsActive bool
24+
Created timeutil.TimeStamp `xorm:"created"`
25+
Updated timeutil.TimeStamp `xorm:"updated"`
26+
Deleted timeutil.TimeStamp `xorm:"deleted"`
27+
}
28+
x, deferable := migration_tests.PrepareTestEnv(t, 0, new(ActionRunnerToken))
29+
defer deferable()
30+
if x == nil || t.Failed() {
31+
return
32+
}
33+
34+
require.NoError(t, removeSoftDeleteActionRunnerToken(x))
35+
36+
var remainingRecords []*ActionRunnerToken
37+
require.NoError(t,
38+
db.GetEngine(t.Context()).
39+
Table("action_runner_token").
40+
Select("`id`, `owner_id`, `repo_id`").
41+
OrderBy("`id`").
42+
Unscoped(). // `Deleted` column doesn't exist anymore, so don't include in query
43+
Find(&remainingRecords))
44+
assert.Equal(t,
45+
[]*ActionRunnerToken{
46+
{ID: 4},
47+
{ID: 5, OwnerID: 1},
48+
{ID: 6, RepoID: 1},
49+
},
50+
remainingRecords)
51+
}

‎models/forgejo_migrations/v15b_add-foreign-keys-action_runner_token.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
// Copyright 2026 The Forgejo Authors. All rights reserved.
2+
// SPDX-License-Identifier: GPL-3.0-or-later
3+
4+
package forgejo_migrations
5+
6+
import (
7+
"xorm.io/builder"
8+
"xorm.io/xorm"
9+
)
10+
11+
func init() {
12+
registerMigration(&Migration{
13+
Description: "add foreign keys to action_runner_token",
14+
Upgrade: addForeignKeysActionRunnerToken,
15+
})
16+
}
17+
18+
func addForeignKeysActionRunnerToken(x *xorm.Engine) error {
19+
type ActionRunnerToken struct {
20+
OwnerID int64 `xorm:"index REFERENCES(user, id)"`
21+
RepoID int64 `xorm:"index REFERENCES(repository, id)"`
22+
}
23+
24+
// With the introduction of a foreign key, owner_id & repo_id cannot be set to "0". Runners can be registered as
25+
// global (owner_id = NULL, repo_id = NULL), user/org (repo_id = NULL), or repo (owner_id = NULL) and NULL values
26+
// now replace the '0' values.
27+
_, err := x.Table(&ActionRunnerToken{}).Where("owner_id = 0").Update(map[string]any{"owner_id": nil})
28+
if err != nil {
29+
return err
30+
}
31+
_, err = x.Table(&ActionRunnerToken{}).Where("repo_id = 0").Update(map[string]any{"repo_id": nil})
32+
if err != nil {
33+
return err
34+
}
35+
36+
return syncForeignKeyWithDelete(x,
37+
new(ActionRunnerToken),
38+
builder.Or(
39+
builder.And(
40+
builder.Expr("owner_id IS NOT NULL"),
41+
builder.Expr("NOT EXISTS (SELECT id FROM `user` WHERE `user`.id = action_runner_token.owner_id)"),
42+
),
43+
builder.And(
44+
builder.Expr("repo_id IS NOT NULL"),
45+
builder.Expr("NOT EXISTS (SELECT id FROM repository WHERE repository.id = action_runner_token.repo_id)"),
46+
),
47+
),
48+
)
49+
}

‎models/forgejo_migrations/v15b_add-foreign-keys-action_runner_token_test.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// Copyright 2026 The Forgejo Authors.
2+
// SPDX-License-Identifier: GPL-3.0-or-later
3+
4+
package forgejo_migrations
5+
6+
import (
7+
"testing"
8+
9+
"forgejo.org/models/db"
10+
migration_tests "forgejo.org/models/gitea_migrations/test"
11+
"forgejo.org/modules/timeutil"
12+
13+
"github.com/stretchr/testify/assert"
14+
"github.com/stretchr/testify/require"
15+
)
16+
17+
func Test_addForeignKeysActionRunnerToken(t *testing.T) {
18+
type ActionRunnerToken struct {
19+
ID int64
20+
Token string `xorm:"UNIQUE"`
21+
OwnerID int64 `xorm:"index"`
22+
RepoID int64 `xorm:"index"`
23+
IsActive bool
24+
Created timeutil.TimeStamp `xorm:"created"`
25+
Updated timeutil.TimeStamp `xorm:"updated"`
26+
}
27+
type User struct {
28+
ID int64 `xorm:"pk autoincr"`
29+
}
30+
type Repository struct {
31+
ID int64 `xorm:"pk autoincr"`
32+
}
33+
x, deferable := migration_tests.PrepareTestEnv(t, 0, new(User), new(Repository), new(ActionRunnerToken))
34+
defer deferable()
35+
if x == nil || t.Failed() {
36+
return
37+
}
38+
39+
require.NoError(t, addForeignKeysActionRunnerToken(x))
40+
41+
var remainingRecords []*ActionRunnerToken
42+
require.NoError(t,
43+
db.GetEngine(t.Context()).
44+
Table("action_runner_token").
45+
Select("`id`, `owner_id`, `repo_id`").
46+
OrderBy("`id`").
47+
Find(&remainingRecords))
48+
assert.Equal(t,
49+
[]*ActionRunnerToken{
50+
{ID: 1},
51+
{ID: 2, OwnerID: 1},
52+
{ID: 3, RepoID: 1},
53+
},
54+
remainingRecords)
55+
}

‎models/gitea_migrations/fixtures/Test_addForeignKeysActionRunnerToken/action_runner_token.yml‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
-
2+
id: 1
3+
owner_id: null
4+
repo_id: null
5+
6+
-
7+
id: 2
8+
owner_id: 1
9+
repo_id: null
10+
11+
-
12+
id: 3
13+
owner_id: null
14+
repo_id: 1
15+
16+
# Expected to be deleted due to invalid owner_id foreign key
17+
-
18+
id: 4
19+
owner_id: 100
20+
21+
# Expected to be deleted due to invalid repo_id foreign key
22+
-
23+
id: 5
24+
repo_id: 100

‎models/gitea_migrations/fixtures/Test_addForeignKeysActionRunnerToken/repository.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
-
2+
id: 1

0 commit comments

Comments
 (0)