From 3de4b351a2cccb0a2b1ba6018410c6c3f5ef9358 Mon Sep 17 00:00:00 2001 From: oliverpool Date: Tue, 26 Aug 2025 10:20:00 +0200 Subject: [PATCH] [v11.0/forgejo] storage test: reader should not be closed on save (#9031) Backport of the fixes for #8529 (v11 is currently not affected because minio-client is an older version. Porting the tests should help in preventing future breakages). - #8541: storage test (reader should not get closed) - #8527 #8816: defer uploader.Close (most robust way) - #8166: enable storage tests The test can be run locally: ``` docker run --rm -e MINIO_DOMAIN=minio -e MINIO_ROOT_USER=123456 -e MINIO_ROOT_PASSWORD=12345678 -p 9000:9000 data.forgejo.org/oci/bitnami/minio:2024.8.17 ``` ``` TEST_MINIO_ENDPOINT=localhost:9000 go test -v -run ^TestMinioStorageIterator$ ./modules/storage ``` ### Release notes - [x] I do not want this change to show in the release notes. - [ ] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Co-authored-by: Earl Warren Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9031 Reviewed-by: Michael Kriese Co-authored-by: oliverpool Co-committed-by: oliverpool --- .forgejo/workflows/testing.yml | 1 + .../elasticsearch/elasticsearch_test.go | 14 +++----- modules/storage/minio_test.go | 21 ++++++----- modules/storage/storage_test.go | 36 ++++++++++++++----- routers/api/packages/container/container.go | 13 +------ 5 files changed, 45 insertions(+), 40 deletions(-) diff --git a/.forgejo/workflows/testing.yml b/.forgejo/workflows/testing.yml index 62136b1b28..567a62d94d 100644 --- a/.forgejo/workflows/testing.yml +++ b/.forgejo/workflows/testing.yml @@ -91,6 +91,7 @@ jobs: RACE_ENABLED: 'true' TAGS: bindata TEST_ELASTICSEARCH_URL: http://elasticsearch:9200 + TEST_MINIO_ENDPOINT: minio:9000 test-e2e: if: vars.ROLE == 'forgejo-coding' || vars.ROLE == 'forgejo-testing' runs-on: docker diff --git a/modules/indexer/issues/elasticsearch/elasticsearch_test.go b/modules/indexer/issues/elasticsearch/elasticsearch_test.go index f8cd4e02f6..e428495705 100644 --- a/modules/indexer/issues/elasticsearch/elasticsearch_test.go +++ b/modules/indexer/issues/elasticsearch/elasticsearch_test.go @@ -14,16 +14,10 @@ import ( ) func TestElasticsearchIndexer(t *testing.T) { - // The elasticsearch instance started by testing.yml > test-unit > services > elasticsearch - url := "http://elastic:changeme@elasticsearch:9200" - - if os.Getenv("CI") == "" { - // Make it possible to run tests against a local elasticsearch instance - url = os.Getenv("TEST_ELASTICSEARCH_URL") - if url == "" { - t.Skip("TEST_ELASTICSEARCH_URL not set and not running in CI") - return - } + url := os.Getenv("TEST_ELASTICSEARCH_URL") + if url == "" { + t.Skip("TEST_ELASTICSEARCH_URL not set") + return } ok := false diff --git a/modules/storage/minio_test.go b/modules/storage/minio_test.go index 99f70c4565..4a5d3f1d30 100644 --- a/modules/storage/minio_test.go +++ b/modules/storage/minio_test.go @@ -18,13 +18,14 @@ import ( ) func TestMinioStorageIterator(t *testing.T) { - if os.Getenv("CI") == "" { - t.Skip("minioStorage not present outside of CI") + endpoint := os.Getenv("TEST_MINIO_ENDPOINT") + if endpoint == "" { + t.Skip("TEST_MINIO_ENDPOINT not set") return } testStorageIterator(t, setting.MinioStorageType, &setting.Storage{ MinioConfig: setting.MinioStorageConfig{ - Endpoint: "minio:9000", + Endpoint: endpoint, AccessKeyID: "123456", SecretAccessKey: "12345678", Bucket: "gitea", @@ -34,13 +35,14 @@ func TestMinioStorageIterator(t *testing.T) { } func TestVirtualHostMinioStorage(t *testing.T) { - if os.Getenv("CI") == "" { - t.Skip("minioStorage not present outside of CI") + endpoint := os.Getenv("TEST_MINIO_ENDPOINT") + if endpoint == "" { + t.Skip("TEST_MINIO_ENDPOINT not set") return } testStorageIterator(t, setting.MinioStorageType, &setting.Storage{ MinioConfig: setting.MinioStorageConfig{ - Endpoint: "minio:9000", + Endpoint: endpoint, AccessKeyID: "123456", SecretAccessKey: "12345678", Bucket: "gitea", @@ -85,13 +87,14 @@ func TestMinioStoragePath(t *testing.T) { } func TestS3StorageBadRequest(t *testing.T) { - if os.Getenv("CI") == "" { - t.Skip("S3Storage not present outside of CI") + endpoint := os.Getenv("TEST_MINIO_ENDPOINT") + if endpoint == "" { + t.Skip("TEST_MINIO_ENDPOINT not set") return } cfg := &setting.Storage{ MinioConfig: setting.MinioStorageConfig{ - Endpoint: "minio:9000", + Endpoint: endpoint, AccessKeyID: "123456", SecretAccessKey: "12345678", Bucket: "bucket", diff --git a/modules/storage/storage_test.go b/modules/storage/storage_test.go index af3dd9520e..76589d941a 100644 --- a/modules/storage/storage_test.go +++ b/modules/storage/storage_test.go @@ -5,6 +5,7 @@ package storage import ( "bytes" + "io" "testing" "forgejo.org/modules/setting" @@ -13,22 +14,39 @@ import ( "github.com/stretchr/testify/require" ) +type spyCloser struct { + io.Reader + closed int +} + +func (s *spyCloser) Close() error { + s.closed++ + return nil +} + +var _ io.ReadCloser = &spyCloser{} + func testStorageIterator(t *testing.T, typStr Type, cfg *setting.Storage) { l, err := NewStorage(typStr, cfg) require.NoError(t, err) - testFiles := [][]string{ - {"a/1.txt", "a1"}, - {"/a/1.txt", "aa1"}, // same as above, but with leading slash that will be trim - {"ab/1.txt", "ab1"}, - {"b/1.txt", "b1"}, - {"b/2.txt", "b2"}, - {"b/3.txt", "b3"}, - {"b/x 4.txt", "bx4"}, + testFiles := []struct { + path, content string + size int64 + }{ + {"a/1.txt", "a1", -1}, + {"/a/1.txt", "aa1", -1}, // same as above, but with leading slash that will be trim + {"ab/1.txt", "ab1", 3}, + {"b/1.txt", "b1", 2}, // minio closes when the size is set + {"b/2.txt", "b2", -1}, + {"b/3.txt", "b3", -1}, + {"b/x 4.txt", "bx4", -1}, } for _, f := range testFiles { - _, err = l.Save(f[0], bytes.NewBufferString(f[1]), -1) + sc := &spyCloser{bytes.NewBufferString(f.content), 0} + _, err = l.Save(f.path, sc, f.size) require.NoError(t, err) + assert.Equal(t, 0, sc.closed) } expectedList := map[string][]string{ diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index 5276dd5706..f3e69bab1e 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -389,12 +389,7 @@ func EndUploadBlob(ctx *context.Context) { } return } - doClose := true - defer func() { - if doClose { - uploader.Close() - } - }() + defer uploader.Close() if ctx.Req.Body != nil { if err := uploader.Append(ctx, ctx.Req.Body); err != nil { @@ -427,12 +422,6 @@ func EndUploadBlob(ctx *context.Context) { return } - if err := uploader.Close(); err != nil { - apiError(ctx, http.StatusInternalServerError, err) - return - } - doClose = false - if err := container_service.RemoveBlobUploadByID(ctx, uploader.ID); err != nil { apiError(ctx, http.StatusInternalServerError, err) return