From a012b8bf369c29bca8edc6bc9bdcffd1f9ddd181 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Tue, 10 Mar 2026 03:19:16 +0100 Subject: [PATCH] refactor: replace ActionRunnerToken.OwnerID & RepoID with optional.Option[int64] (#11601) Currently: - In the database, `NULL` is used in `action_runner_token.owner_id` & `.repo_id` to represent an absent value, as required by the foreign key - In the code, `0` is used in `ActionRunnerToken.OwnerID` and `.RepoID` to represent an absent value This PR replaces the `int64` fields with `optional.Option[int64]` which allows a single data type to be used for both cases, and removes the usage of the value `0` as a placeholder. This change has a limited scope -- although `ActionRunnerToken` uses `NULL` values in the database, the related table `ActionRunner` still uses zero-values for `OwnerID` and `RepoID`. This means a lot of code interacting with both of these tables still uses `0` values, such as the UI. The changes here were stopped at a reasonable point to avoid cascading into all places that use the `ActionRunner` table. (I'll continue this work in the future to enable foreign keys on `ActionRunner`, but likely after #11516 is completed to avoid serious conflict resolution problems.) ## 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 for Go changes (can be removed for JavaScript changes) - 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 ran... - [x] `make pr-go` before pushing ### 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 - [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11601 Reviewed-by: Gusted Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- models/actions/runner_token.go | 41 +++++++++++---------------- models/actions/runner_token_test.go | 9 +++--- models/organization/org.go | 3 +- routers/api/actions/runner/runner.go | 12 ++++---- routers/api/v1/shared/runners.go | 14 +++++++-- routers/private/actions.go | 14 +++++++-- routers/web/shared/actions/runners.go | 25 ++++++++++++++-- services/org/org_test.go | 4 +-- services/repository/delete.go | 3 +- services/user/delete.go | 3 +- 10 files changed, 81 insertions(+), 47 deletions(-) diff --git a/models/actions/runner_token.go b/models/actions/runner_token.go index 848330c954..f789f30ddf 100644 --- a/models/actions/runner_token.go +++ b/models/actions/runner_token.go @@ -10,6 +10,7 @@ import ( "forgejo.org/models/db" repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" + "forgejo.org/modules/optional" "forgejo.org/modules/timeutil" "forgejo.org/modules/util" @@ -31,9 +32,9 @@ import ( type ActionRunnerToken struct { ID int64 Token string `xorm:"UNIQUE"` - OwnerID int64 `xorm:"index REFERENCES(user, id)"` + OwnerID optional.Option[int64] `xorm:"index REFERENCES(user, id)"` Owner *user_model.User `xorm:"-"` - RepoID int64 `xorm:"index REFERENCES(repository, id)"` + RepoID optional.Option[int64] `xorm:"index REFERENCES(repository, id)"` Repo *repo_model.Repository `xorm:"-"` IsActive bool // true means it can be used @@ -71,21 +72,11 @@ func UpdateRunnerToken(ctx context.Context, r *ActionRunnerToken, cols ...string // NewRunnerToken creates a new active runner token and invalidate all old tokens // ownerID will be ignored and treated as 0 if repoID is non-zero. -func NewRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerToken, error) { - if ownerID != 0 && repoID != 0 { +func NewRunnerToken(ctx context.Context, ownerID, repoID optional.Option[int64]) (*ActionRunnerToken, error) { + if ownerID.Has() && repoID.Has() { // It's trying to create a runner token that belongs to a repository, but OwnerID has been set accidentally. // Remove OwnerID to avoid confusion; it's not worth returning an error here. - ownerID = 0 - } - - // To ensure that NULL values are used for the unused columns, rather than attempting to insert 0 values which will - // cause FK violation, manage the list of columns that xorm will insert. - cols := []string{"is_active", "token"} - if ownerID != 0 { - cols = append(cols, "owner_id") - } - if repoID != 0 { - cols = append(cols, "repo_id") + ownerID = optional.None[int64]() } token := util.CryptoRandomString(util.RandomStringHigh) @@ -103,33 +94,33 @@ func NewRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerTo return err } - _, err := db.GetEngine(ctx).Cols(cols...).Insert(runnerToken) + _, err := db.GetEngine(ctx).Insert(runnerToken) return err }) } -func runnerTokenCond(ownerID, repoID int64) builder.Cond { +func runnerTokenCond(ownerID, repoID optional.Option[int64]) builder.Cond { var condOwnerID builder.Cond - if ownerID == 0 { + if has, value := ownerID.Get(); !has { condOwnerID = builder.IsNull{"owner_id"} } else { - condOwnerID = builder.Eq{"owner_id": ownerID} + condOwnerID = builder.Eq{"owner_id": value} } var condRepoID builder.Cond - if repoID == 0 { + if has, value := repoID.Get(); !has { condRepoID = builder.IsNull{"repo_id"} } else { - condRepoID = builder.Eq{"repo_id": repoID} + condRepoID = builder.Eq{"repo_id": value} } return builder.And(condOwnerID, condRepoID) } // GetLatestRunnerToken returns the latest runner token -func GetLatestRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerToken, error) { - if ownerID != 0 && repoID != 0 { - // It's trying to get a runner token that belongs to a repository, but OwnerID has been set accidentally. +func GetLatestRunnerToken(ctx context.Context, ownerID, repoID optional.Option[int64]) (*ActionRunnerToken, error) { + if ownerID.Has() && repoID.Has() { + // It's trying to create a runner token that belongs to a repository, but OwnerID has been set accidentally. // Remove OwnerID to avoid confusion; it's not worth returning an error here. - ownerID = 0 + ownerID = optional.None[int64]() } var runnerToken ActionRunnerToken diff --git a/models/actions/runner_token_test.go b/models/actions/runner_token_test.go index 73320077a6..4b69f5488f 100644 --- a/models/actions/runner_token_test.go +++ b/models/actions/runner_token_test.go @@ -8,6 +8,7 @@ import ( "forgejo.org/models/db" "forgejo.org/models/unittest" + "forgejo.org/modules/optional" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -16,16 +17,16 @@ import ( func TestGetLatestRunnerToken(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) token := unittest.AssertExistsAndLoadBean(t, &ActionRunnerToken{ID: 3}) - expectedToken, err := GetLatestRunnerToken(db.DefaultContext, 1, 0) + expectedToken, err := GetLatestRunnerToken(db.DefaultContext, optional.Some[int64](1), optional.None[int64]()) require.NoError(t, err) assert.Equal(t, expectedToken, token) } func TestNewRunnerToken(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) - token, err := NewRunnerToken(db.DefaultContext, 1, 0) + token, err := NewRunnerToken(db.DefaultContext, optional.Some[int64](1), optional.None[int64]()) require.NoError(t, err) - expectedToken, err := GetLatestRunnerToken(db.DefaultContext, 1, 0) + expectedToken, err := GetLatestRunnerToken(db.DefaultContext, optional.Some[int64](1), optional.None[int64]()) require.NoError(t, err) assert.Equal(t, expectedToken, token) } @@ -35,7 +36,7 @@ func TestUpdateRunnerToken(t *testing.T) { token := unittest.AssertExistsAndLoadBean(t, &ActionRunnerToken{ID: 3}) token.IsActive = true require.NoError(t, UpdateRunnerToken(db.DefaultContext, token, "is_active")) - expectedToken, err := GetLatestRunnerToken(db.DefaultContext, 1, 0) + expectedToken, err := GetLatestRunnerToken(db.DefaultContext, optional.Some[int64](1), optional.None[int64]()) require.NoError(t, err) assert.Equal(t, expectedToken, token) } diff --git a/models/organization/org.go b/models/organization/org.go index b6a7e8d1a0..7dcb78cb9b 100644 --- a/models/organization/org.go +++ b/models/organization/org.go @@ -17,6 +17,7 @@ import ( "forgejo.org/models/unit" user_model "forgejo.org/models/user" "forgejo.org/modules/log" + "forgejo.org/modules/optional" "forgejo.org/modules/setting" "forgejo.org/modules/structs" "forgejo.org/modules/util" @@ -404,7 +405,7 @@ func DeleteOrganization(ctx context.Context, org *Organization) error { &TeamInvite{OrgID: org.ID}, &secret_model.Secret{OwnerID: org.ID}, &actions_model.ActionRunner{OwnerID: org.ID}, - &actions_model.ActionRunnerToken{OwnerID: org.ID}, + &actions_model.ActionRunnerToken{OwnerID: optional.Some(org.ID)}, ); err != nil { return fmt.Errorf("DeleteBeans: %w", err) } diff --git a/routers/api/actions/runner/runner.go b/routers/api/actions/runner/runner.go index 5a29dd7ab7..41e33fc476 100644 --- a/routers/api/actions/runner/runner.go +++ b/routers/api/actions/runner/runner.go @@ -56,14 +56,14 @@ func (s *Service) Register( return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("runner registration token has been invalidated, please use the latest one")) } - if runnerToken.OwnerID != 0 { - if _, err := user_model.GetUserByID(ctx, runnerToken.OwnerID); err != nil { + if has, ownerID := runnerToken.OwnerID.Get(); has { + if _, err := user_model.GetUserByID(ctx, ownerID); err != nil { return nil, connect.NewError(connect.CodeInternal, errors.New("owner of the token not found")) } } - if runnerToken.RepoID != 0 { - if _, err := repo_model.GetRepositoryByID(ctx, runnerToken.RepoID); err != nil { + if has, repoID := runnerToken.RepoID.Get(); has { + if _, err := repo_model.GetRepositoryByID(ctx, repoID); err != nil { return nil, connect.NewError(connect.CodeInternal, errors.New("repository of the token not found")) } } @@ -75,8 +75,8 @@ func (s *Service) Register( runner := &actions_model.ActionRunner{ UUID: gouuid.New().String(), Name: name, - OwnerID: runnerToken.OwnerID, - RepoID: runnerToken.RepoID, + OwnerID: runnerToken.OwnerID.ValueOrDefault(0), + RepoID: runnerToken.RepoID.ValueOrDefault(0), Version: req.Msg.Version, AgentLabels: labels, Ephemeral: req.Msg.Ephemeral, diff --git a/routers/api/v1/shared/runners.go b/routers/api/v1/shared/runners.go index a96a10ad30..5fadbd92e0 100644 --- a/routers/api/v1/shared/runners.go +++ b/routers/api/v1/shared/runners.go @@ -11,6 +11,7 @@ import ( actions_model "forgejo.org/models/actions" "forgejo.org/models/db" + "forgejo.org/modules/optional" "forgejo.org/modules/structs" "forgejo.org/modules/util" "forgejo.org/modules/web" @@ -27,9 +28,18 @@ type RegistrationToken struct { } func GetRegistrationToken(ctx *context.APIContext, ownerID, repoID int64) { - token, err := actions_model.GetLatestRunnerToken(ctx, ownerID, repoID) + optOwnerID := optional.None[int64]() + if ownerID != 0 { + optOwnerID = optional.Some(ownerID) + } + optRepoID := optional.None[int64]() + if repoID != 0 { + optRepoID = optional.Some(repoID) + } + + token, err := actions_model.GetLatestRunnerToken(ctx, optOwnerID, optRepoID) if errors.Is(err, util.ErrNotExist) || (token != nil && !token.IsActive) { - token, err = actions_model.NewRunnerToken(ctx, ownerID, repoID) + token, err = actions_model.NewRunnerToken(ctx, optOwnerID, optRepoID) } if err != nil { ctx.InternalServerError(err) diff --git a/routers/private/actions.go b/routers/private/actions.go index 441fb881ed..2b22c0f964 100644 --- a/routers/private/actions.go +++ b/routers/private/actions.go @@ -15,6 +15,7 @@ import ( user_model "forgejo.org/models/user" "forgejo.org/modules/json" "forgejo.org/modules/log" + "forgejo.org/modules/optional" "forgejo.org/modules/private" "forgejo.org/modules/util" "forgejo.org/services/context" @@ -42,9 +43,18 @@ func GenerateActionsRunnerToken(ctx *context.PrivateContext) { }) } - token, err := actions_model.GetLatestRunnerToken(ctx, owner, repo) + ownerID := optional.None[int64]() + if owner != 0 { + ownerID = optional.Some(owner) + } + repoID := optional.None[int64]() + if repo != 0 { + repoID = optional.Some(repo) + } + + token, err := actions_model.GetLatestRunnerToken(ctx, ownerID, repoID) if errors.Is(err, util.ErrNotExist) || (token != nil && !token.IsActive) { - token, err = actions_model.NewRunnerToken(ctx, owner, repo) + token, err = actions_model.NewRunnerToken(ctx, ownerID, repoID) if err != nil { errMsg := fmt.Sprintf("error while creating runner token: %v", err) log.Error("NewRunnerToken failed: %v", errMsg) diff --git a/routers/web/shared/actions/runners.go b/routers/web/shared/actions/runners.go index 1b77fb1597..9d0ad3b0cd 100644 --- a/routers/web/shared/actions/runners.go +++ b/routers/web/shared/actions/runners.go @@ -9,6 +9,7 @@ import ( actions_model "forgejo.org/models/actions" "forgejo.org/models/db" "forgejo.org/modules/log" + "forgejo.org/modules/optional" "forgejo.org/modules/util" "forgejo.org/modules/web" "forgejo.org/services/context" @@ -29,10 +30,19 @@ func RunnersList(ctx *context.Context, opts actions_model.FindRunnerOptions) { } // ownid=0,repo_id=0,means this token is used for global + ownerID := optional.None[int64]() + if opts.OwnerID != 0 { + ownerID = optional.Some(opts.OwnerID) + } + repoID := optional.None[int64]() + if opts.RepoID != 0 { + repoID = optional.Some(opts.RepoID) + } + var token *actions_model.ActionRunnerToken - token, err = actions_model.GetLatestRunnerToken(ctx, opts.OwnerID, opts.RepoID) + token, err = actions_model.GetLatestRunnerToken(ctx, ownerID, repoID) if errors.Is(err, util.ErrNotExist) || (token != nil && !token.IsActive) { - token, err = actions_model.NewRunnerToken(ctx, opts.OwnerID, opts.RepoID) + token, err = actions_model.NewRunnerToken(ctx, ownerID, repoID) if err != nil { ctx.ServerError("CreateRunnerToken", err) return @@ -130,7 +140,16 @@ func RunnerDetailsEditPost(ctx *context.Context, runnerID, ownerID, repoID int64 // RunnerResetRegistrationToken reset registration token func RunnerResetRegistrationToken(ctx *context.Context, ownerID, repoID int64, redirectTo string) { - _, err := actions_model.NewRunnerToken(ctx, ownerID, repoID) + optOwnerID := optional.None[int64]() + if ownerID != 0 { + optOwnerID = optional.Some(ownerID) + } + optRepoID := optional.None[int64]() + if repoID != 0 { + optRepoID = optional.Some(repoID) + } + + _, err := actions_model.NewRunnerToken(ctx, optOwnerID, optRepoID) if err != nil { ctx.ServerError("ResetRunnerRegistrationToken", err) return diff --git a/services/org/org_test.go b/services/org/org_test.go index 10f753acd6..38736286f4 100644 --- a/services/org/org_test.go +++ b/services/org/org_test.go @@ -12,6 +12,7 @@ import ( "forgejo.org/models/organization" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" + "forgejo.org/modules/optional" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -30,8 +31,7 @@ func TestDeleteOrganization(t *testing.T) { unittest.AssertNotExistsBean(t, &organization.Organization{ID: 6}) unittest.AssertNotExistsBean(t, &organization.OrgUser{OrgID: 6}) unittest.AssertNotExistsBean(t, &organization.Team{OrgID: 6}) - orgID := int64(6) - unittest.AssertNotExistsBean(t, &actions.ActionRunnerToken{OwnerID: orgID}) + unittest.AssertNotExistsBean(t, &actions.ActionRunnerToken{OwnerID: optional.Some[int64](6)}) org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}) err := DeleteOrganization(db.DefaultContext, org, false) diff --git a/services/repository/delete.go b/services/repository/delete.go index ebebdef3f1..3ea7e51a9b 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -28,6 +28,7 @@ import ( actions_module "forgejo.org/modules/actions" "forgejo.org/modules/lfs" "forgejo.org/modules/log" + "forgejo.org/modules/optional" "forgejo.org/modules/setting" "forgejo.org/modules/storage" actions_service "forgejo.org/services/actions" @@ -187,7 +188,7 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID &actions_model.ActionArtifact{RepoID: repoID}, &actions_model.ActionUser{RepoID: repoID}, &repo_model.RepoArchiveDownloadCount{RepoID: repoID}, - &actions_model.ActionRunnerToken{RepoID: repoID}, + &actions_model.ActionRunnerToken{RepoID: optional.Some(repoID)}, ); err != nil { return fmt.Errorf("deleteBeans: %w", err) } diff --git a/services/user/delete.go b/services/user/delete.go index 0c2a0b63ed..bcd465ce9d 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -23,6 +23,7 @@ import ( pull_model "forgejo.org/models/pull" repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" + "forgejo.org/modules/optional" "forgejo.org/modules/setting" issue_service "forgejo.org/services/issue" @@ -97,7 +98,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) &actions_model.ActionUser{UserID: u.ID}, &user_model.BlockedUser{BlockID: u.ID}, &user_model.BlockedUser{UserID: u.ID}, - &actions_model.ActionRunnerToken{OwnerID: u.ID}, + &actions_model.ActionRunnerToken{OwnerID: optional.Some(u.ID)}, &auth_model.AuthorizationToken{UID: u.ID}, ); err != nil { return fmt.Errorf("deleteBeans: %w", err)