fix minio storage iterator path #24691

fuxiaohei · 2023-05-13T00:34:26Z

minio storage iterator shows different behavior with local fs iterator.

in local fs storage:

s.IterateObjects("prefix", func(path,obj)
     println(path) // show "prefix/xxx.file"
})

in minio storage:

s.IterateObjects("prefix", func(path,obj)
     println(path) // show "xxx.file"
})

I think local fs is correct, minio use wrong basePath to trim storage path prefix.

lunny · 2023-05-13T01:41:17Z

Are there any features affected by this?

wxiaoguang · 2023-05-13T01:47:59Z

Interfaces should have the same behaviors.

wxiaoguang · 2023-05-13T01:49:10Z

ps: I guess we need some common test code for different storages, to make sure they behave consistently

fuxiaohei · 2023-05-13T01:57:44Z

Are there any features affected by this?

I think there are no affects. All are s.IterateObjects("")

lunny · 2023-05-13T06:30:14Z

There is a test TestLocalStorageIterator but only for local disk, maybe we can have another test for minio with the same test data.

lunny

tests needed.

wxiaoguang · 2023-05-13T12:09:20Z

How about using a common function to test all storages? Then it could guarantee that they have the consistent behavior.

ps: when rewriting the queue, I introduced a general test function, then all queues are tested by it, so all queues should have the same behavior now

wxiaoguang · 2023-05-13T13:34:18Z

modules/storage/minio.go

+	p = strings.TrimPrefix(p, "/") // url path prefix should not start with /
 	return util.PathJoinRelX(m.basePath, p)


I think util.PathJoinRelX already removes the prefix slash. no need to trim it ahead?

If p = "/" and m.base = "", it generates "./" which is invalid to minio api, shows Received unexpected error: Object name cannot be empty

Nope, it never generates "./" here.

See the comment of "PathJoinRel" family functions, it only outputs "", "." or "foo/bar" path

Even if you trim "/" here, if the input p is "./" , you still get "." and later your code append another suffix slash.

So ideally, I think it should be:

p = util.PathJoinRelX(m.basePath, p) if p == "." { p = "" } // minio doesn't use dot as relative path return p

I guess there could be a test case for it.

if p == "." { p = "" } is ok

GiteaBot · 2023-05-13T22:33:31Z

I was unable to create a backport for 1.19. @fuxiaohei, please send one manually. 🍵

go run ./contrib/backport 24691
...  // fix git conflicts if any
go run ./contrib/backport --continue

lunny · 2023-05-14T01:23:11Z

Since v1.19 hasn't prefix, it's unnecessary to backport.

* upstream/main: (30 commits) Don't filter action runs based on state (go-gitea#24711) Add Go package registry (go-gitea#24687) Fix flash of unstyled content in action view page (go-gitea#24712) Clean up various avatar dimensions (go-gitea#24701) Remove the parallelizing when loading repo for dashboard (go-gitea#24705) Optimize actions list by removing an unnecessary `git` call (go-gitea#24710) Update cron-translations.yml (go-gitea#24708) Fix run list broken when trigger user deleted (go-gitea#24706) Remove Fomantic comment module (go-gitea#24703) Update to Alpine 3.18 (go-gitea#24700) fix minio storage iterator path (go-gitea#24691) Add status indicator on main home screen for each repo (go-gitea#24638) Add test for api team orgnization (go-gitea#24699) Improve button-ghost, remove tertiary button (go-gitea#24692) Add icon support for safari (go-gitea#24697) Improve avatar uploading / resizing / compressing, remove Fomantic card module (go-gitea#24653) Fix docs documenting invalid `@every` for `OLDER_THAN` cron settings (go-gitea#24695) Fix `organization` field being `null` in `GET /api/v1/teams/{id}` (go-gitea#24694) Use standard HTTP library to serve files (go-gitea#24693) Add `eslint-plugin-eslint-comments` (go-gitea#24690) ...

DmitryFrolovTri · 2023-05-17T08:38:29Z

@fuxiaohei @lunny I think this broke local tests at least for me - when they are run locally here is what I get consistently when I run make test tried with go1.19, go1.19.9, go 1.20.2
Does anything in gitea actually start the http://127.0.0.1:9000/gitea/ for those tests?

Can we modify the standard testing that it avoids running minio test if no minio? May be to run this test
we define an env variable minio sort of like with sqlite? What is your view? Or may be nominio
or add some code that verifies that minio is actually running and if not simply skips the test?

2023/05/17 08:27:10 ...les/storage/minio.go:101:NewMinioStorage() [I] Creating Minio storage at 127.0.0.1:9000:gitea with base path 
--- FAIL: TestMinioStorageIterator (4.98s)
    storage_test.go:15: 
        	Error Trace:	/home/puni/sources/mine/gitea-sapk-fork-test/modules/storage/storage_test.go:15
        	            				/home/puni/sources/mine/gitea-sapk-fork-test/modules/storage/minio_test.go:11
        	Error:      	Received unexpected error:
        	            	Put "http://127.0.0.1:9000/gitea/": dial tcp 127.0.0.1:9000: connect: connection refused
        	Test:       	TestMinioStorageIterator
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0xa35537]

goroutine 47 [running]:
testing.tRunner.func1.2({0xb7f5e0, 0x10d6330})
	/home/puni/.gvm/gos/go1.20.2/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	/home/puni/.gvm/gos/go1.20.2/src/testing/testing.go:1529 +0x39f
panic({0xb7f5e0, 0x10d6330})
	/home/puni/.gvm/gos/go1.20.2/src/runtime/panic.go:884 +0x213
code.gitea.io/gitea/modules/storage.testStorageIterator(0xc00036c4e0, {0xc16a8a?, 0x5?}, {0xbec960?, 0xc00036e080?})
	/home/puni/sources/mine/gitea-sapk-fork-test/modules/storage/storage_test.go:27 +0x3d7
code.gitea.io/gitea/modules/storage.TestMinioStorageIterator(0x0?)
	/home/puni/sources/mine/gitea-sapk-fork-test/modules/storage/minio_test.go:11 +0x87
testing.tRunner(0xc00036c4e0, 0xc739d8)
	/home/puni/.gvm/gos/go1.20.2/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
	/home/puni/.gvm/gos/go1.20.2/src/testing/testing.go:1629 +0x3ea
FAIL	code.gitea.io/gitea/modules/storage	5.635s

to run tests I clone a fresh copy of repo and then:

TAGS="bindata sqlite sqlite_unlock_notify" make build
TAGS="bindata sqlite sqlite_unlock_notify" make test

Fix #24691 (comment) --------- Co-authored-by: silverwind <me@silverwind.io>

fix minio storage iterator path

e161de0

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 13, 2023

pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 13, 2023

Merge branch 'main' into fix/storage-iterator

61a7492

lunny added type/bug outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels May 13, 2023

lunny added this to the 1.20.0 milestone May 13, 2023

lunny approved these changes May 13, 2023

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 13, 2023

add minio storage iterator tests

3c1ca3d

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 13, 2023

fuxiaohei added 2 commits May 13, 2023 19:03

add tests cases for storage iterator

23f7faf

storege iterator prefix should be dirName

ddba317

use test helper in storage iterator tests

4f3c992

pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 13, 2023

wxiaoguang reviewed May 13, 2023

View reviewed changes

fuxiaohei added 2 commits May 13, 2023 22:31

correct trim for minio storage path dirName

72b27a2

Merge branch 'main' into fix/storage-iterator

2d98bac

wxiaoguang approved these changes May 13, 2023

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 13, 2023

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 13, 2023

GiteaBot added 6 commits May 13, 2023 10:48

Merge branch 'main' into fix/storage-iterator

47fb6f5

Merge branch 'main' into fix/storage-iterator

6a677f2

Merge branch 'main' into fix/storage-iterator

ffb3b39

Merge branch 'main' into fix/storage-iterator

2d1c374

Merge branch 'main' into fix/storage-iterator

ebc60ca

Merge branch 'main' into fix/storage-iterator

04bcb41

silverwind enabled auto-merge (squash) May 13, 2023 21:33

Merge branch 'main' into fix/storage-iterator

d79f321

silverwind merged commit 61ad4c6 into go-gitea:main May 13, 2023

GiteaBot added backport/manual No power to the bots! Create your backport yourself! and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels May 13, 2023

fuxiaohei deleted the fix/storage-iterator branch May 13, 2023 23:07

lunny removed the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label May 14, 2023

lunny mentioned this pull request May 17, 2023

Skip TestMinioStorageIterator on non-CI env #24762

Merged

silverwind added a commit that referenced this pull request May 17, 2023

Skip TestMinioStorageIterator on CI (#24762)

6c8235d

Fix #24691 (comment) --------- Co-authored-by: silverwind <me@silverwind.io>

go-gitea locked as resolved and limited conversation to collaborators Aug 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix minio storage iterator path #24691

fix minio storage iterator path #24691

fuxiaohei commented May 13, 2023

lunny commented May 13, 2023

wxiaoguang commented May 13, 2023

wxiaoguang commented May 13, 2023

fuxiaohei commented May 13, 2023

lunny commented May 13, 2023

lunny left a comment

wxiaoguang commented May 13, 2023

wxiaoguang May 13, 2023

fuxiaohei May 13, 2023

wxiaoguang May 13, 2023 •

edited

Loading

fuxiaohei May 13, 2023

GiteaBot commented May 13, 2023

lunny commented May 14, 2023

DmitryFrolovTri commented May 17, 2023 •

edited

Loading

		p = strings.TrimPrefix(p, "/") // url path prefix should not start with /
		return util.PathJoinRelX(m.basePath, p)

fix minio storage iterator path #24691

fix minio storage iterator path #24691

Conversation

fuxiaohei commented May 13, 2023

lunny commented May 13, 2023

wxiaoguang commented May 13, 2023

wxiaoguang commented May 13, 2023

fuxiaohei commented May 13, 2023

lunny commented May 13, 2023

lunny left a comment

Choose a reason for hiding this comment

wxiaoguang commented May 13, 2023

wxiaoguang May 13, 2023

Choose a reason for hiding this comment

fuxiaohei May 13, 2023

Choose a reason for hiding this comment

wxiaoguang May 13, 2023 • edited Loading

Choose a reason for hiding this comment

fuxiaohei May 13, 2023

Choose a reason for hiding this comment

GiteaBot commented May 13, 2023

lunny commented May 14, 2023

DmitryFrolovTri commented May 17, 2023 • edited Loading

wxiaoguang May 13, 2023 •

edited

Loading

DmitryFrolovTri commented May 17, 2023 •

edited

Loading