From d54cf4790964deaf22c804d50a741115e8341ee0 Mon Sep 17 00:00:00 2001 From: FuXiaoHei Date: Wed, 17 Jan 2024 11:21:16 +0800 Subject: [PATCH 1/2] Fix uploaded artifacts should be overwritten (#28726) Fix `Uploaded artifacts should be overwritten` https://github.com/go-gitea/gitea/issues/28549 When upload different content to uploaded artifact, it checks that content size is not match in db record with previous artifact size, then the new artifact is refused. Now if it finds uploading content size is not matching db record when receiving chunks, it updates db records to follow the latest size value. --- routers/api/actions/artifacts.go | 9 +- routers/api/actions/artifacts_chunks.go | 9 +- .../integration/api_actions_artifact_test.go | 89 +++++++++++++++++++ 3 files changed, 104 insertions(+), 3 deletions(-) diff --git a/routers/api/actions/artifacts.go b/routers/api/actions/artifacts.go index c45dc667afcdb..9da48cc5a6248 100644 --- a/routers/api/actions/artifacts.go +++ b/routers/api/actions/artifacts.go @@ -257,8 +257,11 @@ func (ar artifactRoutes) uploadArtifact(ctx *ArtifactContext) { return } - // update artifact size if zero - if artifact.FileSize == 0 || artifact.FileCompressedSize == 0 { + // update artifact size if zero or not match, over write artifact size + if artifact.FileSize == 0 || + artifact.FileCompressedSize == 0 || + artifact.FileSize != fileRealTotalSize || + artifact.FileCompressedSize != chunksTotalSize { artifact.FileSize = fileRealTotalSize artifact.FileCompressedSize = chunksTotalSize artifact.ContentEncoding = ctx.Req.Header.Get("Content-Encoding") @@ -267,6 +270,8 @@ func (ar artifactRoutes) uploadArtifact(ctx *ArtifactContext) { ctx.Error(http.StatusInternalServerError, "Error update artifact") return } + log.Debug("[artifact] update artifact size, artifact_id: %d, size: %d, compressed size: %d", + artifact.ID, artifact.FileSize, artifact.FileCompressedSize) } ctx.JSON(http.StatusOK, map[string]string{ diff --git a/routers/api/actions/artifacts_chunks.go b/routers/api/actions/artifacts_chunks.go index 0e0cf8a3aa6e5..69801df787f18 100644 --- a/routers/api/actions/artifacts_chunks.go +++ b/routers/api/actions/artifacts_chunks.go @@ -182,7 +182,14 @@ func mergeChunksForArtifact(ctx *ArtifactContext, chunks []*chunkFileItem, st st }() // save storage path to artifact - log.Debug("[artifact] merge chunks to artifact: %d, %s", artifact.ID, storagePath) + log.Debug("[artifact] merge chunks to artifact: %d, %s, old:%s", artifact.ID, storagePath, artifact.StoragePath) + // if artifact is already uploaded, delete the old file + if artifact.StoragePath != "" { + if err := st.Delete(artifact.StoragePath); err != nil { + log.Warn("Error deleting old artifact: %s, %v", artifact.StoragePath, err) + } + } + artifact.StoragePath = storagePath artifact.Status = int64(actions.ArtifactStatusUploadConfirmed) if err := actions.UpdateArtifactByID(ctx, artifact.ID, artifact); err != nil { diff --git a/tests/integration/api_actions_artifact_test.go b/tests/integration/api_actions_artifact_test.go index 101bedde02788..6a07650004a59 100644 --- a/tests/integration/api_actions_artifact_test.go +++ b/tests/integration/api_actions_artifact_test.go @@ -290,3 +290,92 @@ func TestActionsArtifactUploadWithRetentionDays(t *testing.T) { req = addTokenAuthHeader(req, "Bearer 8061e833a55f6fc0157c98b883e91fcfeeb1a71a") MakeRequest(t, req, http.StatusOK) } + +func TestActionsArtifactOverwrite(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + { + // download old artifact uploaded by tests above, it should 1024 A + req := NewRequest(t, "GET", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts"). + AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + resp := MakeRequest(t, req, http.StatusOK) + var listResp listArtifactsResponse + DecodeJSON(t, resp, &listResp) + + idx := strings.Index(listResp.Value[0].FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") + url := listResp.Value[0].FileContainerResourceURL[idx+1:] + "?itemPath=artifact" + req = NewRequest(t, "GET", url). + AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + resp = MakeRequest(t, req, http.StatusOK) + var downloadResp downloadArtifactResponse + DecodeJSON(t, resp, &downloadResp) + + idx = strings.Index(downloadResp.Value[0].ContentLocation, "/api/actions_pipeline/_apis/pipelines/") + url = downloadResp.Value[0].ContentLocation[idx:] + req = NewRequest(t, "GET", url). + AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + resp = MakeRequest(t, req, http.StatusOK) + body := strings.Repeat("A", 1024) + assert.Equal(t, resp.Body.String(), body) + } + + { + // upload same artifact, it uses 4096 B + req := NewRequestWithJSON(t, "POST", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts", getUploadArtifactRequest{ + Type: "actions_storage", + Name: "artifact", + }).AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + resp := MakeRequest(t, req, http.StatusOK) + var uploadResp uploadArtifactResponse + DecodeJSON(t, resp, &uploadResp) + + idx := strings.Index(uploadResp.FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") + url := uploadResp.FileContainerResourceURL[idx:] + "?itemPath=artifact/abc.txt" + body := strings.Repeat("B", 4096) + req = NewRequestWithBody(t, "PUT", url, strings.NewReader(body)). + AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a"). + SetHeader("Content-Range", "bytes 0-4095/4096"). + SetHeader("x-tfs-filelength", "4096"). + SetHeader("x-actions-results-md5", "wUypcJFeZCK5T6r4lfqzqg==") // base64(md5(body)) + MakeRequest(t, req, http.StatusOK) + + // confirm artifact upload + req = NewRequest(t, "PATCH", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts?artifactName=artifact"). + AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + MakeRequest(t, req, http.StatusOK) + } + + { + // download artifact again, it should 4096 B + req := NewRequest(t, "GET", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts"). + AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + resp := MakeRequest(t, req, http.StatusOK) + var listResp listArtifactsResponse + DecodeJSON(t, resp, &listResp) + + var uploadedItem listArtifactsResponseItem + for _, item := range listResp.Value { + if item.Name == "artifact" { + uploadedItem = item + break + } + } + assert.Equal(t, uploadedItem.Name, "artifact") + + idx := strings.Index(uploadedItem.FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") + url := uploadedItem.FileContainerResourceURL[idx+1:] + "?itemPath=artifact" + req = NewRequest(t, "GET", url). + AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + resp = MakeRequest(t, req, http.StatusOK) + var downloadResp downloadArtifactResponse + DecodeJSON(t, resp, &downloadResp) + + idx = strings.Index(downloadResp.Value[0].ContentLocation, "/api/actions_pipeline/_apis/pipelines/") + url = downloadResp.Value[0].ContentLocation[idx:] + req = NewRequest(t, "GET", url). + AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + resp = MakeRequest(t, req, http.StatusOK) + body := strings.Repeat("B", 4096) + assert.Equal(t, resp.Body.String(), body) + } +} From 57b070b53c4aeb7e86de453251c79c1d0e0478d7 Mon Sep 17 00:00:00 2001 From: fuxiaohei Date: Wed, 17 Jan 2024 22:12:37 +0800 Subject: [PATCH 2/2] fix: use old tests codes to backport artifact overwrite test --- .../integration/api_actions_artifact_test.go | 41 ++++++++++--------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/tests/integration/api_actions_artifact_test.go b/tests/integration/api_actions_artifact_test.go index 6a07650004a59..1b579c268e10a 100644 --- a/tests/integration/api_actions_artifact_test.go +++ b/tests/integration/api_actions_artifact_test.go @@ -296,24 +296,24 @@ func TestActionsArtifactOverwrite(t *testing.T) { { // download old artifact uploaded by tests above, it should 1024 A - req := NewRequest(t, "GET", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts"). - AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + req := NewRequest(t, "GET", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts") + req = addTokenAuthHeader(req, "Bearer 8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp := MakeRequest(t, req, http.StatusOK) var listResp listArtifactsResponse DecodeJSON(t, resp, &listResp) idx := strings.Index(listResp.Value[0].FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") url := listResp.Value[0].FileContainerResourceURL[idx+1:] + "?itemPath=artifact" - req = NewRequest(t, "GET", url). - AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + req = NewRequest(t, "GET", url) + req = addTokenAuthHeader(req, "Bearer 8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp = MakeRequest(t, req, http.StatusOK) var downloadResp downloadArtifactResponse DecodeJSON(t, resp, &downloadResp) idx = strings.Index(downloadResp.Value[0].ContentLocation, "/api/actions_pipeline/_apis/pipelines/") url = downloadResp.Value[0].ContentLocation[idx:] - req = NewRequest(t, "GET", url). - AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + req = NewRequest(t, "GET", url) + req = addTokenAuthHeader(req, "Bearer 8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp = MakeRequest(t, req, http.StatusOK) body := strings.Repeat("A", 1024) assert.Equal(t, resp.Body.String(), body) @@ -324,7 +324,8 @@ func TestActionsArtifactOverwrite(t *testing.T) { req := NewRequestWithJSON(t, "POST", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts", getUploadArtifactRequest{ Type: "actions_storage", Name: "artifact", - }).AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + }) + req = addTokenAuthHeader(req, "Bearer 8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp := MakeRequest(t, req, http.StatusOK) var uploadResp uploadArtifactResponse DecodeJSON(t, resp, &uploadResp) @@ -332,23 +333,23 @@ func TestActionsArtifactOverwrite(t *testing.T) { idx := strings.Index(uploadResp.FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") url := uploadResp.FileContainerResourceURL[idx:] + "?itemPath=artifact/abc.txt" body := strings.Repeat("B", 4096) - req = NewRequestWithBody(t, "PUT", url, strings.NewReader(body)). - AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a"). - SetHeader("Content-Range", "bytes 0-4095/4096"). - SetHeader("x-tfs-filelength", "4096"). - SetHeader("x-actions-results-md5", "wUypcJFeZCK5T6r4lfqzqg==") // base64(md5(body)) + req = NewRequestWithBody(t, "PUT", url, strings.NewReader(body)) + req = addTokenAuthHeader(req, "Bearer 8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + req.Header.Add("Content-Range", "bytes 0-4095/4096") + req.Header.Add("x-tfs-filelength", "4096") + req.Header.Add("x-actions-results-md5", "wUypcJFeZCK5T6r4lfqzqg==") // base64(md5(body)) MakeRequest(t, req, http.StatusOK) // confirm artifact upload - req = NewRequest(t, "PATCH", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts?artifactName=artifact"). - AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + req = NewRequest(t, "PATCH", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts?artifactName=artifact") + req = addTokenAuthHeader(req, "Bearer 8061e833a55f6fc0157c98b883e91fcfeeb1a71a") MakeRequest(t, req, http.StatusOK) } { // download artifact again, it should 4096 B - req := NewRequest(t, "GET", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts"). - AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + req := NewRequest(t, "GET", "/api/actions_pipeline/_apis/pipelines/workflows/791/artifacts") + req = addTokenAuthHeader(req, "Bearer 8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp := MakeRequest(t, req, http.StatusOK) var listResp listArtifactsResponse DecodeJSON(t, resp, &listResp) @@ -364,16 +365,16 @@ func TestActionsArtifactOverwrite(t *testing.T) { idx := strings.Index(uploadedItem.FileContainerResourceURL, "/api/actions_pipeline/_apis/pipelines/") url := uploadedItem.FileContainerResourceURL[idx+1:] + "?itemPath=artifact" - req = NewRequest(t, "GET", url). - AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + req = NewRequest(t, "GET", url) + req = addTokenAuthHeader(req, "Bearer 8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp = MakeRequest(t, req, http.StatusOK) var downloadResp downloadArtifactResponse DecodeJSON(t, resp, &downloadResp) idx = strings.Index(downloadResp.Value[0].ContentLocation, "/api/actions_pipeline/_apis/pipelines/") url = downloadResp.Value[0].ContentLocation[idx:] - req = NewRequest(t, "GET", url). - AddTokenAuth("8061e833a55f6fc0157c98b883e91fcfeeb1a71a") + req = NewRequest(t, "GET", url) + req = addTokenAuthHeader(req, "Bearer 8061e833a55f6fc0157c98b883e91fcfeeb1a71a") resp = MakeRequest(t, req, http.StatusOK) body := strings.Repeat("B", 4096) assert.Equal(t, resp.Body.String(), body)