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 <gusted@noreply.codeberg.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>

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

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)
 }

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)
 	}