From 7a340f486b924bfe2a89b89d3805225fb10e799f Mon Sep 17 00:00:00 2001 From: Artur Wierzbicki Date: Tue, 30 Aug 2022 15:23:16 +0200 Subject: [PATCH] Storage: add `WithContents` option to `storage.Get()` (#53105) * Storage: add `WithContents` option to `storage.Get()` * fix tests Co-authored-by: Ryan McKinley --- pkg/infra/filestorage/api.go | 7 +++- pkg/infra/filestorage/cdk_blob_filestorage.go | 32 ++++++++++++------- pkg/infra/filestorage/db_filestorage.go | 18 ++++++++--- pkg/infra/filestorage/file_storage_mock.go | 27 ++++++++++------ pkg/infra/filestorage/fs_integration_test.go | 13 ++++++++ pkg/infra/filestorage/test_utils.go | 8 +++-- pkg/infra/filestorage/wrapper.go | 28 ++++++++++++---- pkg/services/store/service.go | 2 +- pkg/services/store/service_test.go | 6 ++-- pkg/services/store/tree.go | 3 +- 10 files changed, 103 insertions(+), 41 deletions(-) diff --git a/pkg/infra/filestorage/api.go b/pkg/infra/filestorage/api.go index e9c3c1f344c..184881a5b7d 100644 --- a/pkg/infra/filestorage/api.go +++ b/pkg/infra/filestorage/api.go @@ -166,9 +166,14 @@ type DeleteFolderOptions struct { AccessFilter PathFilter } +type GetFileOptions struct { + // WithContents if set to false, the `Get` operation will return just the file metadata. Default is `true` + WithContents bool +} + //go:generate mockery --name FileStorage --structname MockFileStorage --inpackage --filename file_storage_mock.go type FileStorage interface { - Get(ctx context.Context, path string) (*File, error) + Get(ctx context.Context, path string, options *GetFileOptions) (*File, bool, error) Delete(ctx context.Context, path string) error Upsert(ctx context.Context, command *UpsertFileCommand) error diff --git a/pkg/infra/filestorage/cdk_blob_filestorage.go b/pkg/infra/filestorage/cdk_blob_filestorage.go index 566aa90ad57..55b821572f3 100644 --- a/pkg/infra/filestorage/cdk_blob_filestorage.go +++ b/pkg/infra/filestorage/cdk_blob_filestorage.go @@ -30,17 +30,27 @@ func NewCdkBlobStorage(log log.Logger, bucket *blob.Bucket, rootFolder string, f }, filter, rootFolder) } -func (c cdkBlobStorage) Get(ctx context.Context, filePath string) (*File, error) { - contents, err := c.bucket.ReadAll(ctx, strings.ToLower(filePath)) +func (c cdkBlobStorage) Get(ctx context.Context, path string, options *GetFileOptions) (*File, bool, error) { + var err error + var contents []byte + if options.WithContents { + contents, err = c.bucket.ReadAll(ctx, strings.ToLower(path)) + if err != nil { + if gcerrors.Code(err) == gcerrors.NotFound { + return nil, false, nil + } + return nil, false, err + } + } else { + contents = make([]byte, 0) + } + + attributes, err := c.bucket.Attributes(ctx, strings.ToLower(path)) if err != nil { if gcerrors.Code(err) == gcerrors.NotFound { - return nil, nil + return nil, false, nil } - return nil, err - } - attributes, err := c.bucket.Attributes(ctx, strings.ToLower(filePath)) - if err != nil { - return nil, err + return nil, false, err } var originalPath string @@ -53,7 +63,7 @@ func (c cdkBlobStorage) Get(ctx context.Context, filePath string) (*File, error) } } else { props = make(map[string]string) - originalPath = filePath + originalPath = path } return &File{ @@ -67,7 +77,7 @@ func (c cdkBlobStorage) Get(ctx context.Context, filePath string) (*File, error) Size: attributes.Size, MimeType: detectContentType(originalPath, attributes.ContentType), }, - }, nil + }, true, nil } func (c cdkBlobStorage) Delete(ctx context.Context, filePath string) error { @@ -85,7 +95,7 @@ func (c cdkBlobStorage) Delete(ctx context.Context, filePath string) error { } func (c cdkBlobStorage) Upsert(ctx context.Context, command *UpsertFileCommand) error { - existing, err := c.Get(ctx, command.Path) + existing, _, err := c.Get(ctx, command.Path, &GetFileOptions{WithContents: true}) if err != nil { return err } diff --git a/pkg/infra/filestorage/db_filestorage.go b/pkg/infra/filestorage/db_filestorage.go index c2044835fe7..e477227b652 100644 --- a/pkg/infra/filestorage/db_filestorage.go +++ b/pkg/infra/filestorage/db_filestorage.go @@ -89,16 +89,24 @@ func (s dbFileStorage) getProperties(sess *sqlstore.DBSession, pathHashes []stri return attributesByPath, nil } -func (s dbFileStorage) Get(ctx context.Context, filePath string) (*File, error) { +func (s dbFileStorage) Get(ctx context.Context, path string, options *GetFileOptions) (*File, bool, error) { var result *File - pathHash, err := createPathHash(filePath) + pathHash, err := createPathHash(path) if err != nil { - return nil, err + return nil, false, err } err = s.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { table := &file{} - exists, err := sess.Table("file").Where("path_hash = ?", pathHash).Get(table) + + sess.Table("file") + if options.WithContents { + sess.Cols(allFileCols...) + } else { + sess.Cols(fileColsNoContents...) + } + + exists, err := sess.Where("path_hash = ?", pathHash).Get(table) if !exists { return nil } @@ -134,7 +142,7 @@ func (s dbFileStorage) Get(ctx context.Context, filePath string) (*File, error) return err }) - return result, err + return result, result != nil, err } func (s dbFileStorage) Delete(ctx context.Context, filePath string) error { diff --git a/pkg/infra/filestorage/file_storage_mock.go b/pkg/infra/filestorage/file_storage_mock.go index c4b6bf70488..daa8eeaaec2 100644 --- a/pkg/infra/filestorage/file_storage_mock.go +++ b/pkg/infra/filestorage/file_storage_mock.go @@ -55,27 +55,34 @@ func (_m *MockFileStorage) DeleteFolder(ctx context.Context, path string, option return r0 } -// Get provides a mock function with given fields: ctx, path -func (_m *MockFileStorage) Get(ctx context.Context, path string) (*File, error) { - ret := _m.Called(ctx, path) +// Get provides a mock function with given fields: ctx, path, options +func (_m *MockFileStorage) Get(ctx context.Context, path string, options *GetFileOptions) (*File, bool, error) { + ret := _m.Called(ctx, path, options) var r0 *File - if rf, ok := ret.Get(0).(func(context.Context, string) *File); ok { - r0 = rf(ctx, path) + if rf, ok := ret.Get(0).(func(context.Context, string, *GetFileOptions) *File); ok { + r0 = rf(ctx, path, options) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*File) } } - var r1 error - if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { - r1 = rf(ctx, path) + var r1 bool + if rf, ok := ret.Get(1).(func(context.Context, string, *GetFileOptions) bool); ok { + r1 = rf(ctx, path, options) } else { - r1 = ret.Error(1) + r1 = ret.Get(1).(bool) } - return r0, r1 + var r2 error + if rf, ok := ret.Get(2).(func(context.Context, string, *GetFileOptions) error); ok { + r2 = rf(ctx, path, options) + } else { + r2 = ret.Error(2) + } + + return r0, r1, r2 } // List provides a mock function with given fields: ctx, folderPath, paging, options diff --git a/pkg/infra/filestorage/fs_integration_test.go b/pkg/infra/filestorage/fs_integration_test.go index 39719d1aba7..c0f743b0408 100644 --- a/pkg/infra/filestorage/fs_integration_test.go +++ b/pkg/infra/filestorage/fs_integration_test.go @@ -859,6 +859,19 @@ func TestIntegrationFsStorage(t *testing.T) { fContents(pngImage), ), }, + queryGet{ + input: queryGetInput{ + path: "/file.png", + options: &GetFileOptions{WithContents: false}, + }, + checks: checks( + fName("FILE.png"), + fMimeType("image/png"), + fProperties(map[string]string{"a": "av", "b": "bv"}), + fSize(pngImageSize), + fContents(emptyContents), + ), + }, }, }, { diff --git a/pkg/infra/filestorage/test_utils.go b/pkg/infra/filestorage/test_utils.go index 7ca19b6a631..f63cba35caf 100644 --- a/pkg/infra/filestorage/test_utils.go +++ b/pkg/infra/filestorage/test_utils.go @@ -38,7 +38,8 @@ type cmdDeleteFolder struct { } type queryGetInput struct { - path string + path string + options *GetFileOptions } type fileNameCheck struct { @@ -270,15 +271,18 @@ func handleQuery(t *testing.T, ctx context.Context, query interface{}, queryName switch q := query.(type) { case queryGet: inputPath := q.input.path - file, err := fs.Get(ctx, inputPath) + options := q.input.options + file, fileFound, err := fs.Get(ctx, inputPath, options) require.NoError(t, err, "%s: should be able to get file %s", queryName, inputPath) if q.checks != nil && len(q.checks) > 0 { require.NotNil(t, file, "%s %s", queryName, inputPath) + require.True(t, fileFound, "%s %s", queryName, inputPath) require.Equal(t, strings.ToLower(inputPath), strings.ToLower(file.FullPath), "%s %s", queryName, inputPath) runChecks(t, queryName, inputPath, file, q.checks) } else { require.Nil(t, file, "%s %s", queryName, inputPath) + require.False(t, fileFound, "%s %s", queryName, inputPath) } case queryListFiles: inputPath := q.input.path diff --git a/pkg/infra/filestorage/wrapper.go b/pkg/infra/filestorage/wrapper.go index cfdb344091e..75303dbf8f1 100644 --- a/pkg/infra/filestorage/wrapper.go +++ b/pkg/infra/filestorage/wrapper.go @@ -114,25 +114,35 @@ func (b wrapper) removeRoot(path string) string { return Join(Delimiter, strings.TrimPrefix(path, b.rootFolder)) } -func (b wrapper) Get(ctx context.Context, path string) (*File, error) { +func (b wrapper) getOptionsWithDefaults(options *GetFileOptions) *GetFileOptions { + if options == nil { + return &GetFileOptions{WithContents: true} + } + + return options +} + +func (b wrapper) Get(ctx context.Context, path string, options *GetFileOptions) (*File, bool, error) { if err := b.validatePath(path); err != nil { - return nil, err + return nil, false, err } rootedPath := b.addRoot(path) if !b.filter.IsAllowed(rootedPath) { - return nil, nil + return nil, false, nil } + optionsWithDefaults := b.getOptionsWithDefaults(options) + if b.rootFolder == rootedPath { - return nil, nil + return nil, false, nil } - file, err := b.wrapped.Get(ctx, rootedPath) + file, _, err := b.wrapped.Get(ctx, rootedPath, optionsWithDefaults) if file != nil { file.FullPath = b.removeRoot(file.FullPath) } - return file, err + return file, file != nil, err } func (b wrapper) Delete(ctx context.Context, path string) error { @@ -323,7 +333,11 @@ func (b wrapper) List(ctx context.Context, folderPath string, paging *Paging, op go func() { if options.WithFiles { - if f, err := b.Get(fileRetrievalCtx, folderPath); err == nil { + var getOptions *GetFileOptions + if options.WithContents { + getOptions = &GetFileOptions{WithContents: true} + } + if f, _, err := b.Get(fileRetrievalCtx, folderPath, getOptions); err == nil { fileChan <- f return } diff --git a/pkg/services/store/service.go b/pkg/services/store/service.go index 7a0ba400e36..b92532e3af7 100644 --- a/pkg/services/store/service.go +++ b/pkg/services/store/service.go @@ -334,7 +334,7 @@ func (s *standardStorageService) Upload(ctx context.Context, user *user.SignedIn grafanaStorageLogger.Info("uploading a file", "path", req.Path) if !req.OverwriteExistingFile { - file, err := root.Store().Get(ctx, storagePath) + file, _, err := root.Store().Get(ctx, storagePath, &filestorage.GetFileOptions{WithContents: false}) if err != nil { grafanaStorageLogger.Error("failed while checking file existence", "err", err, "path", req.Path) return ErrUploadInternalError diff --git a/pkg/services/store/service_test.go b/pkg/services/store/service_test.go index ff8124160b8..89b81d5b63c 100644 --- a/pkg/services/store/service_test.go +++ b/pkg/services/store/service_test.go @@ -128,7 +128,7 @@ func TestShouldUploadWhenNoFileAlreadyExists(t *testing.T) { service, mockStorage, storageName := setupUploadStore(t, nil) fileName := "/myFile.jpg" - mockStorage.On("Get", mock.Anything, fileName).Return(nil, nil) + mockStorage.On("Get", mock.Anything, fileName, &filestorage.GetFileOptions{WithContents: false}).Return(nil, false, nil) mockStorage.On("Upsert", mock.Anything, &filestorage.UpsertFileCommand{ Path: fileName, MimeType: "image/jpeg", @@ -157,7 +157,7 @@ func TestShouldFailUploadWithoutAccess(t *testing.T) { func TestShouldFailUploadWhenFileAlreadyExists(t *testing.T) { service, mockStorage, storageName := setupUploadStore(t, nil) - mockStorage.On("Get", mock.Anything, "/myFile.jpg").Return(&filestorage.File{Contents: make([]byte, 0)}, nil) + mockStorage.On("Get", mock.Anything, "/myFile.jpg", &filestorage.GetFileOptions{WithContents: false}).Return(&filestorage.File{Contents: make([]byte, 0)}, true, nil) err := service.Upload(context.Background(), dummyUser, &UploadRequest{ EntityType: EntityTypeImage, @@ -213,7 +213,7 @@ func TestShouldUploadSvg(t *testing.T) { service, mockStorage, storageName := setupUploadStore(t, nil) fileName := "/myFile.svg" - mockStorage.On("Get", mock.Anything, fileName).Return(nil, nil) + mockStorage.On("Get", mock.Anything, fileName, &filestorage.GetFileOptions{WithContents: false}).Return(nil, false, nil) mockStorage.On("Upsert", mock.Anything, &filestorage.UpsertFileCommand{ Path: fileName, MimeType: "image/svg+xml", diff --git a/pkg/services/store/tree.go b/pkg/services/store/tree.go index ae569b67759..2d5ce1fd5ca 100644 --- a/pkg/services/store/tree.go +++ b/pkg/services/store/tree.go @@ -86,7 +86,8 @@ func (t *nestedTree) GetFile(ctx context.Context, orgId int64, path string) (*fi if store == nil { return nil, fmt.Errorf("store not ready") } - return store.Get(ctx, path) + file, _, err := store.Get(ctx, path, nil) + return file, err } func (t *nestedTree) getStorages(orgId int64) []storageRuntime {