refactor: replace WithAvailable with WithVisible when fetching runners (#11657)

When fetching runners, the option `WithAvailable` can be enabled to fetch all runners that can be used by a repository, user, or organization, not only those that are owned by the respective repository, user, or organization. In the instance scope, `WithAvailable` has no meaning. You always get _all_ runners. This means it is impossible to only fetch runners that are owned by the instance, but no others.

This PR replaces `WithAvailable` with `WithVisible`. For repositories, users, and organizations, it has the same semantics as `WithAvailable`. For the instance scope, `WithVisible=true` equals today's default behaviour (i.e., return _all_ runners), whereas `WithVisible=false` is new and would only return the runners owned by the instance itself.

The advantage of `WithVisible` is that it has a consistent meaning across all scopes. This also lays the groundwork for the introduction of a `with-visible` (tentative name) flag in the HTTP API.

## 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.
  - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
  - [x] `make pr-go` before pushing

### Tests for JavaScript changes

(can be removed for Go changes)

- 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

- [ ] 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.

*The decision if the pull request will be shown in the release notes is up to the mergers / release team.*

The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11657
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Co-authored-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>
Co-committed-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>

models/actions/TestRunner_FindRunnerOptionsToConds/action_runner.yml

@@ -0,0 +1,45 @@
+- id: 719931
+  uuid: "9e0eb762-cbdf-4725-9c90-4298568a2a77"
+  name: "runner-1"
+  version: "dev"
+  owner_id: 3  # Owned by org3
+  repo_id: 0
+  description: "A superb runner"
+  agent_labels: ["debian", "gpu"]
+  deleted: 0
+- id: 719932
+  uuid: "b0a0a168-1b08-4365-95dd-e65040ca384d"
+  name: "runner-2"
+  version: "11.3.1"
+  owner_id: 2  # Owned by user2
+  repo_id: 0
+  description: "An exclusive runner"
+  agent_labels: ["docker"]
+  deleted: 0
+- id: 719933
+  uuid: "974f9caf-ee64-4022-a56b-67b28ea06a0d"
+  name: "runner-3"
+  version: "12.2.0"
+  owner_id: 0
+  repo_id: 0
+  description: "A runner for everyone"
+  agent_labels: ["docker"]
+  deleted: 0
+- id: 719934
+  uuid: "022650a8-e999-4a3d-afb0-21f75233798b"
+  name: "runner-4"
+  version: "12.1.0"
+  owner_id: 0
+  repo_id: 32  # owned by org3
+  description: ""
+  agent_labels: ["debian"]
+  deleted: 0
+- id: 719935
+  uuid: "f3031695-87ea-41cf-8d48-21948e83ee17"
+  name: "runner-5"
+  version: "12.7.0"
+  owner_id: 0
+  repo_id: 36  # owned by user2
+  description: ""
+  agent_labels: ["arch"]
+  deleted: 0

models/actions/runner.go

@@ -196,12 +196,12 @@ func init() {
 
 type FindRunnerOptions struct {
 	db.ListOptions
-	RepoID        int64
+	RepoID      int64
-	OwnerID       int64 // it will be ignored if RepoID is set
+	OwnerID     int64 // it will be ignored if RepoID is set
-	Sort          string
+	Sort        string
-	Filter        string
+	Filter      string
-	IsOnline      optional.Option[bool]
+	IsOnline    optional.Option[bool]
-	WithAvailable bool // not only runners belong to, but also runners can be used
+	WithVisible bool // include all runners that are visible to the repository, owner, or instance
 }
 
 func (opts FindRunnerOptions) ToConds() builder.Cond {
@@ -209,17 +209,19 @@ func (opts FindRunnerOptions) ToConds() builder.Cond {
 
 	if opts.RepoID > 0 {
 		c := builder.NewCond().And(builder.Eq{"repo_id": opts.RepoID})
-		if opts.WithAvailable {
+		if opts.WithVisible {
 			c = c.Or(builder.Eq{"owner_id": builder.Select("owner_id").From("repository").Where(builder.Eq{"id": opts.RepoID})})
 			c = c.Or(builder.Eq{"repo_id": 0, "owner_id": 0})
 		}
 		cond = cond.And(c)
 	} else if opts.OwnerID > 0 { // OwnerID is ignored if RepoID is set
 		c := builder.NewCond().And(builder.Eq{"owner_id": opts.OwnerID})
-		if opts.WithAvailable {
+		if opts.WithVisible {
 			c = c.Or(builder.Eq{"repo_id": 0, "owner_id": 0})
 		}
 		cond = cond.And(c)
+	} else if !opts.WithVisible {
+		cond = cond.And(builder.Eq{"repo_id": 0, "owner_id": 0})
 	}
 
 	if opts.Filter != "" {
@@ -278,9 +280,9 @@ func GetRunnerByID(ctx context.Context, id int64) (*ActionRunner, error) {
 	return &runner, nil
 }
 
-// GetAvailableRunnerByID is like GetRunnerByID, but it only finds the runner if it is accessible to the given owner or
+// GetVisibleRunnerByID is like GetRunnerByID, but it only finds the runner if it is visible to the given owner or
 // repository. If it is not, util.ErrNotExist will be returned even if the runner exists.
-func GetAvailableRunnerByID(ctx context.Context, id, ownerID, repoID int64) (*ActionRunner, error) {
+func GetVisibleRunnerByID(ctx context.Context, id, ownerID, repoID int64) (*ActionRunner, error) {
 	query := db.GetEngine(ctx).Where("id=?", id)
 
 	if repoID > 0 {

models/actions/runner_test.go

@@ -237,8 +237,8 @@ func TestRunnerEditable(t *testing.T) {
 	}
 }
 
-func TestRunner_GetAvailableRunnerByID(t *testing.T) {
+func TestRunner_GetVisibleRunnerByID(t *testing.T) {
-	defer unittest.OverrideFixtures("models/actions/TestRunner_GetAvailableRunnerByID")()
+	defer unittest.OverrideFixtures("models/actions/TestRunner_GetVisibleRunnerByID")()
 	require.NoError(t, unittest.PrepareTestDatabase())
 
 	repository32 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 32, OwnerID: 3})
@@ -365,7 +365,7 @@ func TestRunner_GetAvailableRunnerByID(t *testing.T) {
 
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			_, err := GetAvailableRunnerByID(t.Context(), testCase.runner.ID, testCase.ownerID, testCase.repoID)
+			_, err := GetVisibleRunnerByID(t.Context(), testCase.runner.ID, testCase.ownerID, testCase.repoID)
 			if testCase.expectedError == "" {
 				require.NoError(t, err)
 			} else {
@@ -374,3 +374,96 @@ func TestRunner_GetAvailableRunnerByID(t *testing.T) {
 		})
 	}
 }
+
+func TestRunner_FindRunnerOptionsToConds(t *testing.T) {
+	defer unittest.OverrideFixtures("models/actions/TestRunner_FindRunnerOptionsToConds")()
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	runner1 := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: 719931, OwnerID: 3, RepoID: 0}) // Owned by org3
+	runner2 := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: 719932, OwnerID: 2, RepoID: 0}) // Owned by user2
+	runner3 := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: 719933, OwnerID: 0, RepoID: 0})
+	runner4 := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: 719934, OwnerID: 0, RepoID: 32})
+	runner5 := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: 719935, OwnerID: 0, RepoID: 36})
+
+	testCases := []struct {
+		name              string
+		opts              FindRunnerOptions
+		expectedRunners   RunnerList
+		unexpectedRunners RunnerList
+	}{
+		{
+			name:              "Only runners owned by instance",
+			opts:              FindRunnerOptions{OwnerID: 0, RepoID: 0, WithVisible: false},
+			expectedRunners:   RunnerList{runner3},
+			unexpectedRunners: RunnerList{runner1, runner2, runner4, runner5},
+		},
+		{
+			name:              "All runners on instance",
+			opts:              FindRunnerOptions{OwnerID: 0, RepoID: 0, WithVisible: true},
+			expectedRunners:   RunnerList{runner1, runner2, runner3, runner4, runner5},
+			unexpectedRunners: RunnerList{},
+		},
+		{
+			name:              "Only runners owned by organization",
+			opts:              FindRunnerOptions{OwnerID: 3, RepoID: 0, WithVisible: false},
+			expectedRunners:   RunnerList{runner1},
+			unexpectedRunners: RunnerList{runner2, runner3, runner4, runner5},
+		},
+		{
+			name:              "Runners available to organization",
+			opts:              FindRunnerOptions{OwnerID: 3, RepoID: 0, WithVisible: true},
+			expectedRunners:   RunnerList{runner1, runner3},
+			unexpectedRunners: RunnerList{runner2, runner4, runner5},
+		},
+		{
+			name:              "Only runners owned by user",
+			opts:              FindRunnerOptions{OwnerID: 2, RepoID: 0, WithVisible: false},
+			expectedRunners:   RunnerList{runner2},
+			unexpectedRunners: RunnerList{runner1, runner3, runner4, runner5},
+		},
+		{
+			name:              "Runners available to user",
+			opts:              FindRunnerOptions{OwnerID: 2, RepoID: 0, WithVisible: true},
+			expectedRunners:   RunnerList{runner2, runner3},
+			unexpectedRunners: RunnerList{runner1, runner4, runner5},
+		},
+		{
+			name:              "Only runners owned by organization repository",
+			opts:              FindRunnerOptions{OwnerID: 0, RepoID: 32, WithVisible: false},
+			expectedRunners:   RunnerList{runner4},
+			unexpectedRunners: RunnerList{runner1, runner2, runner3, runner5},
+		},
+		{
+			name:              "Runners available to organization repository",
+			opts:              FindRunnerOptions{OwnerID: 0, RepoID: 32, WithVisible: true},
+			expectedRunners:   RunnerList{runner1, runner3, runner4},
+			unexpectedRunners: RunnerList{runner2, runner5},
+		},
+		{
+			name:              "Only runners owned by user repository",
+			opts:              FindRunnerOptions{OwnerID: 0, RepoID: 36, WithVisible: false},
+			expectedRunners:   RunnerList{runner5},
+			unexpectedRunners: RunnerList{runner1, runner2, runner3, runner4},
+		},
+		{
+			name:              "Runners available to user repository",
+			opts:              FindRunnerOptions{OwnerID: 0, RepoID: 36, WithVisible: true},
+			expectedRunners:   RunnerList{runner2, runner3, runner5},
+			unexpectedRunners: RunnerList{runner1, runner4},
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			runners, err := db.Find[ActionRunner](t.Context(), testCase.opts)
+			require.NoError(t, err)
+
+			for _, expectedRunner := range testCase.expectedRunners {
+				assert.Contains(t, runners, expectedRunner)
+			}
+			for _, unexpectedRunner := range testCase.unexpectedRunners {
+				assert.NotContains(t, runners, unexpectedRunner)
+			}
+		})
+	}
+}

routers/api/v1/shared/runners.go

@@ -106,6 +106,7 @@ func ListRunners(ctx *context.APIContext, ownerID, repoID int64) {
 		OwnerID:     ownerID,
 		RepoID:      repoID,
 		ListOptions: listOptions,
+		WithVisible: ownerID == 0 && repoID == 0,
 	})
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "FindCountRunners", map[string]string{})

routers/web/repo/actions/actions.go

@@ -88,9 +88,9 @@ func List(ctx *context.Context) {
 
 		// Get all runner labels
 		runners, err := db.Find[actions_model.ActionRunner](ctx, actions_model.FindRunnerOptions{
-			RepoID:        ctx.Repo.Repository.ID,
+			RepoID:      ctx.Repo.Repository.ID,
-			IsOnline:      optional.Some(true),
+			IsOnline:    optional.Some(true),
-			WithAvailable: true,
+			WithVisible: true,
 		})
 		if err != nil {
 			ctx.ServerError("FindRunners", err)

routers/web/repo/setting/runners.go

@@ -137,15 +137,14 @@ func Runners(ctx *context.Context) {
 			Page:     page,
 			PageSize: 100,
 		},
-		Sort:   ctx.Req.URL.Query().Get("sort"),
+		WithVisible: true,
-		Filter: ctx.Req.URL.Query().Get("q"),
+		Sort:        ctx.Req.URL.Query().Get("sort"),
+		Filter:      ctx.Req.URL.Query().Get("q"),
 	}
 	if rCtx.IsRepo {
 		opts.RepoID = rCtx.RepoID
-		opts.WithAvailable = true
 	} else if rCtx.IsOrg || rCtx.IsUser {
 		opts.OwnerID = rCtx.OwnerID
-		opts.WithAvailable = true
 	}
 
 	ctx.Data["RunnersListLink"] = rCtx.RedirectLink

routers/web/shared/actions/runners.go

@@ -77,12 +77,12 @@ func RunnersList(ctx *context.Context, template base.TplName, opts actions_model
 // RunnerDetails displays detail information about each runner. The page is purely informational and visible to everyone
 // who is allowed to use a runner.
 func RunnerDetails(ctx *context.Context, runnerID, ownerID, repoID int64, template base.TplName, page int) {
-	runner, err := actions_model.GetAvailableRunnerByID(ctx, runnerID, ownerID, repoID)
+	runner, err := actions_model.GetVisibleRunnerByID(ctx, runnerID, ownerID, repoID)
 	if errors.Is(err, util.ErrNotExist) {
-		ctx.NotFound("GetAvailableRunnerByID", err)
+		ctx.NotFound("GetVisibleRunnerByID", err)
 		return
 	} else if err != nil {
-		ctx.ServerError("GetAvailableRunnerByID", err)
+		ctx.ServerError("GetVisibleRunnerByID", err)
 		return
 	}
 	if err := runner.LoadAttributes(ctx); err != nil {
@@ -166,12 +166,12 @@ func RunnerCreatePost(ctx *context.Context, ownerID, repoID int64, template, suc
 
 // RunnerEdit displays a form to modify the given runner.
 func RunnerEdit(ctx *context.Context, runnerID, ownerID, repoID int64, template base.TplName) {
-	runner, err := actions_model.GetAvailableRunnerByID(ctx, runnerID, ownerID, repoID)
+	runner, err := actions_model.GetVisibleRunnerByID(ctx, runnerID, ownerID, repoID)
 	if errors.Is(err, util.ErrNotExist) {
-		ctx.NotFound("GetAvailableRunnerByID", err)
+		ctx.NotFound("GetVisibleRunnerByID", err)
 		return
 	} else if err != nil {
-		ctx.ServerError("GetAvailableRunnerByID", err)
+		ctx.ServerError("GetVisibleRunnerByID", err)
 		return
 	}
 	if err := runner.LoadAttributes(ctx); err != nil {
@@ -194,12 +194,12 @@ func RunnerEdit(ctx *context.Context, runnerID, ownerID, repoID int64, template
 
 // RunnerEditPost handles the form submitted by RunnerEdit.
 func RunnerEditPost(ctx *context.Context, runnerID, ownerID, repoID int64, template, successTemplate base.TplName, redirectTo string) {
-	runner, err := actions_model.GetAvailableRunnerByID(ctx, runnerID, ownerID, repoID)
+	runner, err := actions_model.GetVisibleRunnerByID(ctx, runnerID, ownerID, repoID)
 	if errors.Is(err, util.ErrNotExist) {
-		ctx.NotFound("GetAvailableRunnerByID", err)
+		ctx.NotFound("GetVisibleRunnerByID", err)
 		return
 	} else if err != nil {
-		ctx.ServerError("GetAvailableRunnerByID", err)
+		ctx.ServerError("GetVisibleRunnerByID", err)
 		return
 	}
 	if !runner.Editable(ownerID, repoID) {