Skip to content

Batch-load related data in actions run, job, and task API endpoints#37032

Merged
wxiaoguang merged 25 commits intogo-gitea:mainfrom
myers:fix/actions-api-n-plus-1
Apr 29, 2026
Merged

Batch-load related data in actions run, job, and task API endpoints#37032
wxiaoguang merged 25 commits intogo-gitea:mainfrom
myers:fix/actions-api-n-plus-1

Conversation

@myers
Copy link
Copy Markdown
Contributor

@myers myers commented Mar 29, 2026

Avoid per-item DB queries in ListRuns, ListJobs, and ListActionTasks by batch-loading trigger users, repositories, and task attributes before the conversion loop. Remove ReferencesGitRepo from the /actions route group since no task/run endpoints use it.

I ran into this in another fork that will remain nameless, their action page wouldn't load for me.

Added tests for these endpoints as well.


This was built using Claude Code with Opus.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 29, 2026
@myers myers force-pushed the fix/actions-api-n-plus-1 branch from 15f60ab to 88787d9 Compare March 29, 2026 18:11
@silverwind silverwind requested a review from Copilot March 29, 2026 18:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes GitHub Actions API list endpoints by reducing per-item database queries through batch-loading related models (repos, trigger users, job/task attributes) before converting results to API structs, and adjusts routing middleware accordingly.

Changes:

  • Batch-load run trigger users and repositories in ListRuns, and job run/repo references in ListJobs.
  • Batch-load task/job/run references in ListActionTasks.
  • Remove context.ReferencesGitRepo(true) from the repo /actions route group and add integration tests for user-level run/job listing.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
tests/integration/api_actions_run_test.go Adds integration coverage for /api/v1/user/actions/runs and /api/v1/user/actions/jobs responses.
routers/api/v1/shared/action.go Batch-loads runs/jobs dependencies to avoid per-item repo/user queries in shared listing helpers.
routers/api/v1/repo/action.go Batch-loads task attributes prior to conversion in repo task listing endpoint.
routers/api/v1/api.go Removes Git repo reference middleware from repo /actions API routes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread routers/api/v1/shared/action.go
Comment thread routers/api/v1/shared/action.go
Comment thread routers/api/v1/shared/action.go
Comment thread routers/api/v1/repo/action.go
Comment thread tests/integration/api_actions_run_test.go Outdated
@myers myers force-pushed the fix/actions-api-n-plus-1 branch 3 times, most recently from b729742 to 7a87efe Compare March 29, 2026 20:59
myers added 2 commits March 29, 2026 21:04
ToActionWorkflowRun and ToActionTask both call LoadAttributes
unconditionally, which re-queries repos and loads unused task steps
even when the list handler has already batch-loaded everything.

Guard both with a nil check on the already-populated field: skip
LoadAttributes when TriggerUser (runs) or Job (tasks) is set.
@myers myers force-pushed the fix/actions-api-n-plus-1 branch from 7a87efe to 4c02528 Compare March 29, 2026 21:05
@myers
Copy link
Copy Markdown
Contributor Author

myers commented Mar 29, 2026

Sorry about the force push. I forced a squashed fix, then again to unsquash it.

Comment thread services/convert/convert.go Outdated
@silverwind
Copy link
Copy Markdown
Member

silverwind commented Mar 30, 2026

Please click "Resolve conversation" on all addressed comments so we know what is still pending.

Add ActionTask.LoadJobAttributes that loads Job and its attributes
(Run, Repo) without loading Steps. Use it in ToActionTask so the
task list endpoint no longer issues per-task step queries.

Co-Authored-By: Claude (Opus)
@myers myers requested a review from wxiaoguang April 12, 2026 01:24
Comment thread routers/api/v1/api.go
Comment thread services/convert/convert.go Outdated
wxiaoguang and others added 8 commits April 12, 2026 10:43
Assign the already-available repo parameter to run.Repo so
LoadAttributes skips the redundant repo lookup, and remove
the TriggerUser nil guard since LoadAttributes handles nil
checks internally.

Co-Authored-By: Claude (Opus)
…x/actions-api-n-plus-1

# Conflicts:
#	services/convert/convert.go
wxiaoguang
wxiaoguang previously approved these changes Apr 12, 2026
myers added 2 commits April 19, 2026 20:49
If a job references a run that was deleted between the job query and
the run query, the map lookup returns nil and the next line panics.
Nil-check before setting j.Run.Repo.

Co-Authored-By: Claude <noreply@anthropic.com> (claude-opus-4-7)
This branch indicates server-side data inconsistency (the batch loader
couldn't resolve the run or repo for a job we just listed), not a
missing resource the client asked for. APIErrorInternal returns 500
and logs the error detail server-side instead of echoing the job ID
in the response body.

Co-Authored-By: Claude <noreply@anthropic.com> (claude-opus-4-7)
@myers
Copy link
Copy Markdown
Contributor Author

myers commented Apr 19, 2026

@silverwind

Thanks for the review.

Finding 1 (wrong HTTP status in ListJobs): Fixed in 096ad618c8. Now returns 500 via APIErrorInternal(fmt.Errorf(...)) — the job ID stays in the server-side log but isn't echoed in the response body. Also pushed 79afaec19a defensively: ActionJobList.LoadRuns can leave j.Run nil if a run row is missing, so guarded j.Run.Repo = j.Repo against the nil case.

Finding 2 (Repository.APIURL semantics change): This change came in with bc61edad ("fix more") from @wxiaoguang during his review-pass on this branch, not the original PR. The observation is accurate — under PUBLIC_URL_DETECTION=auto, ~43 call sites will now return request-host-derived URLs instead of the configured AppURL. I don't think it should block this PR (it's a maintainer-initiated change), but happy to note it in the PR description, and wxiaoguang may want to weigh in on whether it belongs here or in a follow-up.


Comment authored by Claude.

silverwind and others added 2 commits April 19, 2026 23:18
When isRepoLevel, ctx.Repo.Repository (with Owner populated by
middleware) is already assigned to every run, so the subsequent
LoadRepos + LoadOwners queries are redundant and overwrite the
pre-assigned pointers.

Add a repo-level ListRuns integration test covering the branch.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@silverwind
Copy link
Copy Markdown
Member

Remaining findings fixed in 8da8495.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 19, 2026
@lunny lunny added the performance/speed performance issues with slow downs label Apr 19, 2026
@lunny lunny added this to the 1.27.0 milestone Apr 19, 2026
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 29, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread services/convert/convert.go Outdated
Comment thread services/convert/convert.go
@wxiaoguang wxiaoguang enabled auto-merge (squash) April 29, 2026 08:20
@wxiaoguang wxiaoguang merged commit 18762c7 into go-gitea:main Apr 29, 2026
26 checks passed
@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 29, 2026
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 30, 2026
* origin/main:
  Refactor CI workflows (go-gitea#37487)
  Allow multiple projects per issue and pull requests (go-gitea#36784)
  [skip ci] Updated translations via Crowdin
  Refactor compare diff/pull page (1) (go-gitea#37481)
  Fix review submission from single-commit PR view (go-gitea#37475)
  Refactor integration tests infrastructure (go-gitea#37462)
  Fix allow maintainer edit permission check (go-gitea#37479)
  Serve OpenAPI 3.0 spec at /openapi.v1.json (go-gitea#37038)
  Batch-load related data in actions run, job, and task API endpoints (go-gitea#37032)
  Add DEFAULT_TITLE_SOURCE setting for pull request title default behavior (go-gitea#37465)
  Fix compare dropdown for branches without common history (go-gitea#37470)
  FIX: URL sanitization to handle schemeless credentials (go-gitea#37440)
  Refactor pull request view (4) (go-gitea#37451)

# Conflicts:
#	modules/indexer/issues/elasticsearch/elasticsearch.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. performance/speed performance issues with slow downs type/refactoring Existing code has been cleaned up. There should be no new functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants