From b047a60a09bc2a2d298dd476449532cd97d2e22d Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Mon, 25 Aug 2025 19:33:40 +0200 Subject: [PATCH] fix!: use run ID instead of run Index in artifacts download web views - the run ID used to download artifacts is absolute (ID) instead of being relative to the repository (Index) for compatibility with the url built and returned as `artifact-url` by the the upload-artifact@v4 action. - this is a breaking change because URLs to download artifacts previous saved/bookmarked and not yet expired expired are no longer working, they need to be looked up again by visiting the job web page. - add unit tests for getRunByID(). - RepoActionView.test.js verifies the download URL is built using the run ID. - lAdd integration tests to verify the RunID is set as expected in the template used by RepoActionView.vue. Refs https://code.forgejo.org/forgejo/runner/issues/187 --- models/actions/run.go | 17 +++- release-notes/9023.md | 1 + routers/web/repo/actions/view.go | 39 +++++-- routers/web/repo/actions/view_test.go | 42 ++++++++ templates/repo/actions/view.tmpl | 1 + tests/integration/actions_view_test.go | 36 ++++++- tests/integration/html_helper.go | 11 ++ web_src/js/components/RepoActionView.test.js | 101 +++++++++++++++++++ web_src/js/components/RepoActionView.vue | 4 +- 9 files changed, 233 insertions(+), 19 deletions(-) create mode 100644 release-notes/9023.md diff --git a/models/actions/run.go b/models/actions/run.go index 6e0c70f8cf..dc5f522317 100644 --- a/models/actions/run.go +++ b/models/actions/run.go @@ -309,15 +309,26 @@ func GetLatestRunForBranchAndWorkflow(ctx context.Context, repoID int64, branch, } func GetRunByID(ctx context.Context, id int64) (*ActionRun, error) { - var run ActionRun - has, err := db.GetEngine(ctx).Where("id=?", id).Get(&run) + run, has, err := GetRunByIDWithHas(ctx, id) if err != nil { return nil, err } else if !has { return nil, fmt.Errorf("run with id %d: %w", id, util.ErrNotExist) } - return &run, nil + return run, nil +} + +func GetRunByIDWithHas(ctx context.Context, id int64) (*ActionRun, bool, error) { + var run ActionRun + has, err := db.GetEngine(ctx).Where("id=?", id).Get(&run) + if err != nil { + return nil, false, err + } else if !has { + return nil, false, nil + } + + return &run, true, nil } func GetRunByIndex(ctx context.Context, repoID, index int64) (*ActionRun, error) { diff --git a/release-notes/9023.md b/release-notes/9023.md new file mode 100644 index 0000000000..47db1abc0b --- /dev/null +++ b/release-notes/9023.md @@ -0,0 +1 @@ +The `artifact-url` ouput [returned by the upload-artifact@v4 action](https://code.forgejo.org/actions/upload-artifact#outputs) can be used to download the artifact. It was previously 404. To implement this compatibility fix, the web UI URL to download artifacts (i.e. `/{owner}/{repo}/actions/runs/{run_id}/artifacts/{artifact_name}`) now relies on an identifier that is unique accross the instance. URLs to download artifacts that were bookmarked or copied prior to this change use an id relative to the repository and will no longer work. It previously was `/{owner}/{repo}/actions/runs/{run_index}/artifacts/{artifact_name}`, note the difference between `{run_id}` and `{run_index}`. The new URL can be obtained again by visiting the parent page, which still uses the relative id (`/{owner}/{repo}/actions/runs/{run_index}`). diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index a325c51afe..b0d9346247 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -53,6 +53,7 @@ func View(ctx *context_module.Context) { workflowName := job.Run.WorkflowID ctx.Data["RunIndex"] = runIndex + ctx.Data["RunID"] = job.Run.ID ctx.Data["JobIndex"] = jobIndex ctx.Data["ActionsURL"] = ctx.Repo.RepoLink + "/actions" ctx.Data["WorkflowName"] = workflowName @@ -668,6 +669,31 @@ func ArtifactsDeleteView(ctx *context_module.Context) { ctx.JSON(http.StatusOK, struct{}{}) } +func getRunByID(ctx *context_module.Context, runID int64) *actions_model.ActionRun { + if runID == 0 { + log.Debug("Requested runID is zero.") + ctx.Error(http.StatusNotFound, "zero is not a valid run ID") + return nil + } + + run, has, err := actions_model.GetRunByIDWithHas(ctx, runID) + if err != nil { + ctx.Error(http.StatusInternalServerError, err.Error()) + return nil + } + if !has { + log.Debug("Requested runID[%d] not found.", runID) + ctx.Error(http.StatusNotFound, fmt.Sprintf("no such run %d", runID)) + return nil + } + if run.RepoID != ctx.Repo.Repository.ID { + log.Debug("Requested runID[%d] does not belong to repo[%-v].", runID, ctx.Repo.Repository) + ctx.Error(http.StatusNotFound, "no such run") + return nil + } + return run +} + func artifactsFind(ctx *context_module.Context, opts actions_model.FindArtifactsOptions) []*actions_model.ActionArtifact { artifacts, err := db.Find[actions_model.ActionArtifact](ctx, opts) if err != nil { @@ -711,18 +737,11 @@ func artifactsFindByNameOrID(ctx *context_module.Context, runID int64, nameOrID } func ArtifactsDownloadView(ctx *context_module.Context) { - runIndex := ctx.ParamsInt64("run") - artifactNameOrID := ctx.Params("artifact_name_or_id") - - run, err := actions_model.GetRunByIndex(ctx, ctx.Repo.Repository.ID, runIndex) - if err != nil { - if errors.Is(err, util.ErrNotExist) { - ctx.Error(http.StatusNotFound, err.Error()) - return - } - ctx.Error(http.StatusInternalServerError, err.Error()) + run := getRunByID(ctx, ctx.ParamsInt64("run")) + if ctx.Written() { return } + artifactNameOrID := ctx.Params("artifact_name_or_id") artifacts := artifactsFindByNameOrID(ctx, run.ID, artifactNameOrID) if ctx.Written() { diff --git a/routers/web/repo/actions/view_test.go b/routers/web/repo/actions/view_test.go index d6a390befc..dae1b47e6e 100644 --- a/routers/web/repo/actions/view_test.go +++ b/routers/web/repo/actions/view_test.go @@ -8,12 +8,54 @@ import ( "testing" actions_model "forgejo.org/models/actions" + repo_model "forgejo.org/models/repo" unittest "forgejo.org/models/unittest" "forgejo.org/services/contexttest" "github.com/stretchr/testify/assert" ) +func Test_getRunByID(t *testing.T) { + unittest.PrepareTestEnv(t) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: 5, ID: 4}) + + for _, testCase := range []struct { + name string + runID int64 + err string + }{ + { + name: "Found", + runID: 792, + }, + { + name: "NotFound", + runID: 24344, + err: "no such run", + }, + { + name: "ZeroNotFound", + runID: 0, + err: "zero is not a valid run ID", + }, + } { + t.Run(testCase.name, func(t *testing.T) { + ctx, resp := contexttest.MockContext(t, fmt.Sprintf("user5/repo4/actions/runs/%v/artifacts/some-name", testCase.runID)) + ctx.Repo.Repository = repo + run := getRunByID(ctx, testCase.runID) + if testCase.err == "" { + assert.NotNil(t, run) + assert.False(t, ctx.Written(), resp.Body.String()) + } else { + assert.Nil(t, run) + assert.True(t, ctx.Written()) + assert.Contains(t, resp.Body.String(), testCase.err) + } + }) + } +} + func Test_artifactsFind(t *testing.T) { unittest.PrepareTestEnv(t) diff --git a/templates/repo/actions/view.tmpl b/templates/repo/actions/view.tmpl index b787762719..abd3c5c764 100644 --- a/templates/repo/actions/view.tmpl +++ b/templates/repo/actions/view.tmpl @@ -4,6 +4,7 @@ {{template "repo/header" .}}
{ expect(wrapper.get('.job-step-section:nth-of-type(2) .job-log-line:nth-of-type(2) .log-msg').text()).toEqual('Step #2 Log #2'); expect(wrapper.get('.job-step-section:nth-of-type(2) .job-log-line:nth-of-type(3) .log-msg').text()).toEqual('Step #2 Log #3'); }); + +test('artifacts download links', async () => { + Object.defineProperty(document.documentElement, 'lang', {value: 'en'}); + vi.spyOn(global, 'fetch').mockImplementation((url, opts) => { + if (url.endsWith('/artifacts')) { + return Promise.resolve({ + ok: true, + json: vi.fn().mockResolvedValue( + { + artifacts: [ + {name: 'artifactname1', size: 111, status: 'completed'}, + {name: 'artifactname2', size: 222, status: 'expired'}, + ], + }, + ), + }); + } + + const postBody = JSON.parse(opts.body); + const stepsLog_value = []; + for (const cursor of postBody.logCursors) { + if (cursor.expanded) { + stepsLog_value.push( + { + step: cursor.step, + cursor: 0, + lines: [ + {index: 1, message: `Step #${cursor.step + 1} Log #1`, timestamp: 0}, + ], + }, + ); + } + } + const jobs_value = { + state: { + run: { + status: 'success', + commit: { + pusher: {}, + }, + }, + currentJob: { + steps: [ + { + summary: 'Test Step #1', + duration: '1s', + status: 'success', + }, + ], + }, + }, + logs: { + stepsLog: opts.body?.includes('"cursor":null') ? stepsLog_value : [], + }, + }; + + return Promise.resolve({ + ok: true, + json: vi.fn().mockResolvedValue( + jobs_value, + ), + }); + }); + + const wrapper = mount(RepoActionView, { + props: { + actionsURL: 'https://example.com/example-org/example-repo/actions', + runIndex: '10', + runID: '1001', + jobIndex: '2', + locale: { + approve: '', + cancel: '', + rerun: '', + artifactsTitle: 'artifactTitleHere', + areYouSure: '', + confirmDeleteArtifact: '', + rerun_all: '', + showTimeStamps: '', + showLogSeconds: '', + showFullScreen: '', + downloadLogs: '', + status: { + unknown: '', + waiting: '', + running: '', + success: '', + failure: '', + cancelled: '', + skipped: '', + blocked: '', + }, + }, + }, + }); + await flushPromises(); + + expect(wrapper.get('.job-artifacts .job-artifacts-title').text()).toEqual('artifactTitleHere'); + expect(wrapper.get('.job-artifacts .job-artifacts-item:nth-of-type(1) .job-artifacts-link').attributes('href')).toEqual('https://example.com/example-org/example-repo/actions/runs/1001/artifacts/artifactname1'); + expect(wrapper.get('.job-artifacts .job-artifacts-item:nth-of-type(2) .job-artifacts-link').attributes('href')).toEqual('https://example.com/example-org/example-repo/actions/runs/1001/artifacts/artifactname2'); +}); diff --git a/web_src/js/components/RepoActionView.vue b/web_src/js/components/RepoActionView.vue index 4d34960e46..3f89bc9806 100644 --- a/web_src/js/components/RepoActionView.vue +++ b/web_src/js/components/RepoActionView.vue @@ -15,6 +15,7 @@ const sfc = { }, props: { runIndex: String, + runID: String, jobIndex: String, actionsURL: String, workflowName: String, @@ -386,6 +387,7 @@ export function initRepositoryActionView() { const view = createApp(sfc, { runIndex: el.getAttribute('data-run-index'), + runID: el.getAttribute('data-run-id'), jobIndex: el.getAttribute('data-job-index'), actionsURL: el.getAttribute('data-actions-url'), workflowName: el.getAttribute('data-workflow-name'), @@ -473,7 +475,7 @@ export function initRepositoryActionView() {