Skip to content

Commit f1d60b9

Browse files
Crash--claude
Crash--
and
claude
committed
refactor: extract shared S3 helpers into pkg/s3util
Move duplicated S3 utility functions (DeleteObjects, DeletePrefixObjects, EnsureBucket, IsNotFound, WrapNotFound) from vfss3 and appfs into a shared pkg/s3util package. Includes integration tests with minio. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 10d471f commit f1d60b9

7 files changed

Lines changed: 239 additions & 140 deletions

File tree

‎model/vfs/vfss3/avatar.go‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@ import (
55
"fmt"
66
"io"
77
"net/http"
8-
"os"
98
"time"
109

1110
"github.com/cozy/cozy-stack/model/vfs"
11+
"github.com/cozy/cozy-stack/pkg/s3util"
1212
"github.com/minio/minio-go/v7"
1313
)
1414

@@ -83,13 +83,13 @@ func (a *avatarS3) DeleteAvatar() error {
8383
func (a *avatarS3) ServeAvatarContent(w http.ResponseWriter, req *http.Request) error {
8484
obj, err := a.client.GetObject(a.ctx, a.bucket, a.avatarKey(), minio.GetObjectOptions{})
8585
if err != nil {
86-
return wrapS3Err(err)
86+
return s3util.WrapNotFound(err)
8787
}
8888
defer obj.Close()
8989

9090
info, err := obj.Stat()
9191
if err != nil {
92-
return wrapS3Err(err)
92+
return s3util.WrapNotFound(err)
9393
}
9494

9595
t := time.Time{}
@@ -104,14 +104,3 @@ func (a *avatarS3) ServeAvatarContent(w http.ResponseWriter, req *http.Request)
104104
http.ServeContent(w, req, "avatar", t, obj)
105105
return nil
106106
}
107-
108-
func wrapS3Err(err error) error {
109-
if minio.ToErrorResponse(err).Code == "NoSuchKey" {
110-
return os.ErrNotExist
111-
}
112-
if minio.ToErrorResponse(err).Code == "NoSuchBucket" {
113-
return os.ErrNotExist
114-
}
115-
// Sanitize S3 errors to avoid leaking internal bucket/key details
116-
return fmt.Errorf("s3 storage error: %s", minio.ToErrorResponse(err).Code)
117-
}

‎model/vfs/vfss3/impl.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ import (
2121
"github.com/cozy/cozy-stack/pkg/couchdb"
2222
"github.com/cozy/cozy-stack/pkg/lock"
2323
"github.com/cozy/cozy-stack/pkg/logger"
24+
"github.com/cozy/cozy-stack/pkg/s3util"
2425
"github.com/cozy/cozy-stack/pkg/utils"
2526
"github.com/gofrs/uuid/v5"
26-
"github.com/hashicorp/go-multierror"
2727
"github.com/minio/minio-go/v7"
2828
)
2929

@@ -208,7 +208,7 @@ func (sfs *s3VFS) InitFs() error {
208208

209209
func (sfs *s3VFS) Delete() error {
210210
sfs.log.Infof("Deleting all objects with prefix %q in bucket %q", sfs.keyPrefix, sfs.bucket)
211-
return deletePrefixObjects(sfs.ctx, sfs.client, sfs.bucket, sfs.keyPrefix)
211+
return s3util.DeletePrefixObjects(sfs.ctx, sfs.client, sfs.bucket, sfs.keyPrefix)
212212
}
213213

214214
func (sfs *s3VFS) CreateDir(doc *vfs.DirDoc) error {
@@ -495,7 +495,7 @@ func (sfs *s3VFS) destroyFileLocked(doc *vfs.FileDoc) error {
495495
sfs.log.Warnf("DestroyFile failed on BatchDeleteVersions: %s", err)
496496
}
497497
}
498-
if err := deleteObjects(sfs.ctx, sfs.client, sfs.bucket, objNames); err != nil {
498+
if err := s3util.DeleteObjects(sfs.ctx, sfs.client, sfs.bucket, objNames); err != nil {
499499
sfs.log.Warnf("DestroyFile failed on deleteObjects: %s", err)
500500
}
501501
vfs.DiskQuotaAfterDestroy(sfs, diskUsage, destroyed)
@@ -513,7 +513,7 @@ func (sfs *s3VFS) EnsureErased(journal vfs.TrashJournal) error {
513513
if err != nil {
514514
if !couchdb.IsNoDatabaseError(err) {
515515
sfs.log.Warnf("EnsureErased failed on VersionsFor(%s): %s", fileID, err)
516-
errm = multierror.Append(errm, err)
516+
errm = errors.Join(errm, err)
517517
}
518518
continue
519519
}
@@ -529,11 +529,11 @@ func (sfs *s3VFS) EnsureErased(journal vfs.TrashJournal) error {
529529
}
530530
if err := sfs.Indexer.BatchDeleteVersions(allVersions); err != nil {
531531
sfs.log.Warnf("EnsureErased failed on BatchDeleteVersions: %s", err)
532-
errm = multierror.Append(errm, err)
532+
errm = errors.Join(errm, err)
533533
}
534-
if err := deleteObjects(sfs.ctx, sfs.client, sfs.bucket, objNames); err != nil {
534+
if err := s3util.DeleteObjects(sfs.ctx, sfs.client, sfs.bucket, objNames); err != nil {
535535
sfs.log.Warnf("EnsureErased failed on deleteObjects: %s", err)
536-
errm = multierror.Append(errm, err)
536+
errm = errors.Join(errm, err)
537537
}
538538
vfs.DiskQuotaAfterDestroy(sfs, diskUsage, destroyed)
539539
return errm
@@ -687,7 +687,7 @@ func (sfs *s3VFS) ClearOldVersions() error {
687687
return err
688688
}
689689
vfs.DiskQuotaAfterDestroy(sfs, diskUsage, destroyed)
690-
return deleteObjects(sfs.ctx, sfs.client, sfs.bucket, objNames)
690+
return s3util.DeleteObjects(sfs.ctx, sfs.client, sfs.bucket, objNames)
691691
}
692692

693693
func (sfs *s3VFS) CopyFileFromOtherFS(

‎model/vfs/vfss3/s3.go‎

Lines changed: 0 additions & 53 deletions
This file was deleted.

‎model/vfs/vfss3/thumbs.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"github.com/cozy/cozy-stack/model/vfs"
1414
"github.com/cozy/cozy-stack/pkg/consts"
1515
"github.com/cozy/cozy-stack/pkg/logger"
16+
"github.com/cozy/cozy-stack/pkg/s3util"
1617
"github.com/labstack/echo/v4"
1718
"github.com/minio/minio-go/v7"
1819
)
@@ -133,20 +134,20 @@ func (ts *thumbsS3) RemoveThumbs(img *vfs.FileDoc, formats []string) error {
133134
for i, format := range formats {
134135
objNames[i] = ts.makeName(img.ID(), format)
135136
}
136-
return deleteObjects(ts.ctx, ts.client, ts.bucket, objNames)
137+
return s3util.DeleteObjects(ts.ctx, ts.client, ts.bucket, objNames)
137138
}
138139

139140
func (ts *thumbsS3) ServeThumbContent(w http.ResponseWriter, req *http.Request, img *vfs.FileDoc, format string) error {
140141
name := ts.makeName(img.ID(), format)
141142
obj, err := ts.client.GetObject(ts.ctx, ts.bucket, name, minio.GetObjectOptions{})
142143
if err != nil {
143-
return wrapS3Err(err)
144+
return s3util.WrapNotFound(err)
144145
}
145146
defer obj.Close()
146147

147148
info, err := obj.Stat()
148149
if err != nil {
149-
return wrapS3Err(err)
150+
return s3util.WrapNotFound(err)
150151
}
151152

152153
if info.ContentType == echo.MIMEOctetStream {
@@ -176,7 +177,7 @@ func (ts *thumbsS3) OpenNoteThumb(id, format string) (io.ReadCloser, error) {
176177
name := ts.makeName(id, format)
177178
obj, err := ts.client.GetObject(ts.ctx, ts.bucket, name, minio.GetObjectOptions{})
178179
if err != nil {
179-
return nil, wrapS3Err(err)
180+
return nil, s3util.WrapNotFound(err)
180181
}
181182
// Stat to verify the object actually exists (GetObject doesn't fail on missing keys).
182183
if _, err := obj.Stat(); err != nil {
@@ -194,7 +195,7 @@ func (ts *thumbsS3) RemoveNoteThumb(id string, formats []string) error {
194195
for i, format := range formats {
195196
objNames[i] = ts.makeName(id, format)
196197
}
197-
err := deleteObjects(ts.ctx, ts.client, ts.bucket, objNames)
198+
err := s3util.DeleteObjects(ts.ctx, ts.client, ts.bucket, objNames)
198199
if err != nil {
199200
logger.WithNamespace("vfss3").Infof("Cannot remove note thumbs: %s", err)
200201
}
@@ -205,7 +206,7 @@ func (ts *thumbsS3) ServeNoteThumbContent(w http.ResponseWriter, req *http.Reque
205206
name := ts.makeName(id, consts.NoteImageThumbFormat)
206207
obj, err := ts.client.GetObject(ts.ctx, ts.bucket, name, minio.GetObjectOptions{})
207208
if err != nil {
208-
return wrapS3Err(err)
209+
return s3util.WrapNotFound(err)
209210
}
210211

211212
info, err := obj.Stat()
@@ -215,12 +216,12 @@ func (ts *thumbsS3) ServeNoteThumbContent(w http.ResponseWriter, req *http.Reque
215216
name = ts.makeName(id, consts.NoteImageOriginalFormat)
216217
obj, err = ts.client.GetObject(ts.ctx, ts.bucket, name, minio.GetObjectOptions{})
217218
if err != nil {
218-
return wrapS3Err(err)
219+
return s3util.WrapNotFound(err)
219220
}
220221
info, err = obj.Stat()
221222
if err != nil {
222223
obj.Close()
223-
return wrapS3Err(err)
224+
return s3util.WrapNotFound(err)
224225
}
225226
}
226227
defer obj.Close()

‎pkg/appfs/s3.go‎

Lines changed: 8 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"bytes"
55
"compress/gzip"
66
"context"
7-
"errors"
87
"fmt"
98
"io"
109
"mime"
@@ -17,6 +16,7 @@ import (
1716
"github.com/andybalholm/brotli"
1817
"github.com/cozy/cozy-stack/pkg/consts"
1918
"github.com/cozy/cozy-stack/pkg/filetype"
19+
"github.com/cozy/cozy-stack/pkg/s3util"
2020

2121
web_utils "github.com/cozy/cozy-stack/pkg/utils"
2222
"github.com/labstack/echo/v4"
@@ -51,7 +51,7 @@ func (f *s3Copier) Exist(slug, version, shasum string) (bool, error) {
5151
if err == nil {
5252
return true, nil
5353
}
54-
if isS3NotFound(err) {
54+
if s3util.IsNotFound(err) {
5555
return false, nil
5656
}
5757
return false, err
@@ -63,7 +63,7 @@ func (f *s3Copier) Start(slug, version, shasum string) (bool, error) {
6363
return exist, err
6464
}
6565

66-
if err := ensureS3Bucket(f.ctx, f.client, f.bucket); err != nil {
66+
if err := s3util.EnsureBucket(f.ctx, f.client, f.bucket, ""); err != nil {
6767
return false, err
6868
}
6969

@@ -116,7 +116,7 @@ func (f *s3Copier) Copy(stat os.FileInfo, src io.Reader) error {
116116
}
117117

118118
func (f *s3Copier) Abort() error {
119-
return s3DeleteObjects(f.ctx, f.client, f.bucket, f.objectNames)
119+
return s3util.DeleteObjects(f.ctx, f.client, f.bucket, f.objectNames)
120120
}
121121

122122
func (f *s3Copier) Commit() (err error) {
@@ -148,12 +148,12 @@ func (s *s3Server) Open(slug, version, shasum, file string) (io.ReadCloser, erro
148148
objName := s.makeObjectName(slug, version, shasum, file)
149149
obj, err := s.client.GetObject(s.ctx, s.bucket, objName, minio.GetObjectOptions{})
150150
if err != nil {
151-
return nil, wrapS3ErrNotExist(err)
151+
return nil, s3util.WrapNotFound(err)
152152
}
153153
info, err := obj.Stat()
154154
if err != nil {
155155
obj.Close()
156-
return nil, wrapS3ErrNotExist(err)
156+
return nil, s3util.WrapNotFound(err)
157157
}
158158
contentEncoding := info.UserMetadata["X-Content-Encoding"]
159159
if contentEncoding == "br" {
@@ -168,13 +168,13 @@ func (s *s3Server) ServeFileContent(w http.ResponseWriter, req *http.Request, sl
168168
objName := s.makeObjectName(slug, version, shasum, file)
169169
obj, err := s.client.GetObject(s.ctx, s.bucket, objName, minio.GetObjectOptions{})
170170
if err != nil {
171-
return wrapS3ErrNotExist(err)
171+
return s3util.WrapNotFound(err)
172172
}
173173
defer obj.Close()
174174

175175
info, err := obj.Stat()
176176
if err != nil {
177-
return wrapS3ErrNotExist(err)
177+
return s3util.WrapNotFound(err)
178178
}
179179

180180
if checkETag := req.Header.Get("Cache-Control") == ""; checkETag {
@@ -317,55 +317,6 @@ func S3AppsBucket(bucketPrefix string, appsType consts.AppType) string {
317317
panic("Unknown AppType")
318318
}
319319

320-
// ensureS3Bucket creates the bucket if it does not already exist.
321-
func ensureS3Bucket(ctx context.Context, client *minio.Client, bucket string) error {
322-
err := client.MakeBucket(ctx, bucket, minio.MakeBucketOptions{})
323-
if err != nil {
324-
code := minio.ToErrorResponse(err).Code
325-
if code == "BucketAlreadyOwnedByYou" || code == "BucketAlreadyExists" {
326-
return nil
327-
}
328-
return err
329-
}
330-
return nil
331-
}
332-
333-
// s3DeleteObjects deletes a list of named objects from a bucket.
334-
func s3DeleteObjects(ctx context.Context, client *minio.Client, bucket string, objNames []string) error {
335-
if len(objNames) == 0 {
336-
return nil
337-
}
338-
objectsCh := make(chan minio.ObjectInfo, len(objNames))
339-
for _, name := range objNames {
340-
objectsCh <- minio.ObjectInfo{Key: name}
341-
}
342-
close(objectsCh)
343-
var errm error
344-
for e := range client.RemoveObjects(ctx, bucket, objectsCh, minio.RemoveObjectsOptions{}) {
345-
errm = errors.Join(errm, e.Err)
346-
}
347-
return errm
348-
}
349-
350-
// isS3NotFound returns true when the error is an S3 "not found" response.
351-
func isS3NotFound(err error) bool {
352-
code := minio.ToErrorResponse(err).Code
353-
return code == "NoSuchKey" || code == "NoSuchBucket"
354-
}
355-
356-
// wrapS3ErrNotExist converts S3 not-found errors to os.ErrNotExist and
357-
// sanitizes other S3 errors to avoid leaking internal bucket/key details.
358-
func wrapS3ErrNotExist(err error) error {
359-
if isS3NotFound(err) {
360-
return os.ErrNotExist
361-
}
362-
code := minio.ToErrorResponse(err).Code
363-
if code != "" {
364-
return fmt.Errorf("s3 storage error: %s", code)
365-
}
366-
return err
367-
}
368-
369320
// prepareTarball is reused from server.go via the FileServer interface (it
370321
// calls Open and FilesList). The function is defined in server.go.
371322
// We reference it here to document that s3Server satisfies prepareTarball's

0 commit comments

Comments
 (0)