Add support for Spaces #4404

glpatcern · 2023-12-13T09:18:28Z

All credits to @gmgigi96, I reopened this PR so we can keep working on it.

Missing parts:

Expose permission to share (currently, the frontend believes the user can't share anything and hides the "shared by me" view)
OCS API to support the new storageid format
Trashbin support
Share hiding/accepting
~~A way to enable/disable this format by config, such that we can have the functionality in the code and enable it once ready~~

And from the last merge:

Look into all replaces in go.mod, in particular for the cs3apis

glpatcern

I went for a full review of this branch. Some new code chunks implement the logic required by the LibreGraph API and I considered them "good", trusting the tests we're doing with the web UI. There are ~~a few trivial fixes~~ (already merged) and some other larger changes to be done/discussed.

internal/grpc/services/gateway/spaces.go

internal/grpc/services/spacesregistry/spacesregistry.go

internal/grpc/services/storageprovider/storageprovider.go

pkg/spaces/utils.go

glpatcern · 2025-02-03T09:36:24Z

pkg/projects/manager/sql/sql.go

+
+func (m *mgr) ListProjects(ctx context.Context, user *userpb.User) ([]*provider.StorageSpace, error) {
+	// TODO: for the time being we load everything in memory. We may find a better
+	// solution in future when the number of projects will grow.


Loading in memory is not a big issue per se, but how are we refreshing this cache as new project spaces get created?

Is the impact that great to keep getting the data from the DB instead of loading it? Can't we put a cache in front?

Jesse (because for some reason I cannot comment): I created a cache in a new PR: #5094

glpatcern · 2025-02-03T09:40:57Z

internal/http/services/owncloud/ocgraph/shares.go

+		drive, err := s.cs3ReceivedShareToDriveItem(ctx, share)
+		if err != nil {
+			log.Error().Err(err).Msg("error getting received shares")
+			w.WriteHeader(http.StatusInternalServerError)


Let's not accumulate further technical debt with those HTTP 500:

On a loop, we should skip and return the rest

In the other cases, we should return something relevant for the frontend to show (and help us in debugging afterwards)

internal/http/services/owncloud/ocgraph/unifiedrole.go

Co-authored-by: Jesse Geens <[email protected]>

…5053) Bumps google.golang.org/protobuf from 1.36.1 to 1.36.4. --- updated-dependencies: - dependency-name: google.golang.org/protobuf dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…#5049) Bumps [github.com/coreos/go-oidc/v3](https://github.com/coreos/go-oidc) from 3.11.0 to 3.12.0. - [Release notes](https://github.com/coreos/go-oidc/releases) - [Commits](coreos/go-oidc@v3.11.0...v3.12.0) --- updated-dependencies: - dependency-name: github.com/coreos/go-oidc/v3 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [go.step.sm/crypto](https://github.com/smallstep/crypto) from 0.55.0 to 0.57.0. - [Release notes](https://github.com/smallstep/crypto/releases) - [Commits](smallstep/crypto@v0.55.0...v0.57.0) --- updated-dependencies: - dependency-name: go.step.sm/crypto dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

diocas

This code could still benefit from (much) more detailed documentation.....
And some functions could be unified in a single place (e.g. things related to converting IDs to strings and reverse).
I would also take the time to go full orm with it.

pkg/projects/manager/sql/init.sql

pkg/spaces/utils.go

diocas · 2025-02-07T12:59:01Z

pkg/projects/manager/sql/sql.go

+
+func (m *mgr) ListProjects(ctx context.Context, user *userpb.User) ([]*provider.StorageSpace, error) {
+	// TODO: for the time being we load everything in memory. We may find a better
+	// solution in future when the number of projects will grow.


Is the impact that great to keep getting the data from the DB instead of loading it? Can't we put a cache in front?

Jesse (because for some reason I cannot comment): I created a cache in a new PR: #5094

diocas · 2025-02-07T14:47:09Z

internal/http/services/owncloud/ocgraph/drives.go

+
+	spacesRes := make([]*libregraph.Drive, 0, len(res.ShareInfos))
+	for _, share := range res.ShareInfos {
+		spacesRes = append(spacesRes, s.convertShareToSpace(share))


ListExistingReceivedShares checks if a share is valid (by doing a stat), right? I wonder if we find performance issues if that is necessary. After all, the real shares listing are requested separately, so even if we sent data that is no longer accessible, it wouldn't be a problem.
Either way, to me these mountpoint spaces don't make much sense. It should be 1 space per person that shared, and not by share (since we consider the users' homes the sole spaces). But alas, this would need changes from web.

internal/http/services/owncloud/ocgraph/drives.go

diocas · 2025-02-07T14:57:20Z

internal/http/services/owncloud/ocgraph/drives.go

+
+// For the moment, not supported
+func isShareJail(spaceID string) bool {
+	return false // TODO


Isn't this the hardcoded share jail id we set above?

I guess it would, but afaik we decided not to support sharejails for now

diocas · 2025-02-07T16:44:56Z

internal/http/services/owncloud/ocgraph/shares.go

+
+	d := &libregraph.DriveItem{
+		UIHidden:          libregraph.PtrBool(rsi.ReceivedShare.Hidden),
+		ClientSynchronize: libregraph.PtrBool(true),


Isn't this what we now added in the new share schema?

Yes, and this gets propagated now to the ReceivedShare.Hidden field :)

internal/http/services/owncloud/ocgraph/shares.go

diocas · 2025-02-07T17:00:08Z

internal/http/services/owncloud/ocgraph/shares.go

+
+func groupByResourceID(shares []*gateway.ShareResourceInfo, publicShares []*gateway.PublicShareResourceInfo) (map[string][]*share, map[string]*provider.ResourceInfo) {
+	grouped := make(map[string][]*share, len(shares)+len(publicShares)) // at most we have the sum of both lists
+	infos := make(map[string]*provider.ResourceInfo, len(shares)+len(publicShares))


Is it for performance reasons that we are allocating the space now?

I guess yes, otherwise you get a backing array with a small capacity and have to continually increase the backing array capacity as the map fills up. So if you know the size beforehand it's better to already pass it

internal/http/services/owncloud/ocgraph/shares.go

…lls to the database

gmgigi96 added 30 commits October 27, 2023 16:42

removed unused config variable

2bf2c87

removed old code of spaces

cce7bbc

add boilerplate for spaces

04a4243

add literals for space types

64602c6

add memory driver for spaces

adb7778

removed sql file

f9dcee2

add spaces loader

82c6f51

load spaces manager drivers

0caea57

add function to get spaces client

3411bf0

redirect spaces request to spaces server

1f6e508

add ocgraph service

025f6ab

add graph spec

505c67d

add permissions in the response

c451ac5

read and parse graph api filters to cs3 filters

fb5b43f

add filters when getting spaces

319b9eb

temporary use my fork for go-cs3apis

8616525

fixes

464b42a

load spacesregistry grpc service

de140a7

require user_space field in memory spaces driver

b066815

fix urls

b5f1788

fix permissions

bd5467e

fix role names

32132e4

fixes in permissions

fa2ee5f

add missing fields

6df8b37

use path as drive alias

e83bada

add get me endpoint

014070d

fix pattern

2d085e9

mock api endpoint

6380bec

register ocapi svc

7bee0a1

make all api unprotected

ca1e813

jessegeens force-pushed the spaces branch from 1e3b5c4 to 6534abc Compare January 28, 2025 15:23

Incorporate cache for ListExistingShares

6045b4d

jessegeens force-pushed the spaces branch from 6534abc to 6045b4d Compare January 28, 2025 15:39

glpatcern commented Feb 3, 2025

View reviewed changes

jessegeens and others added 5 commits February 3, 2025 10:59

Return an error when EOS returns an error in List (#5044)

34147b4

Co-authored-by: Jesse Geens <[email protected]>

Pseudo-transactionalize sharing (#5029)

e1b4805

Co-authored-by: Jesse Geens <[email protected]>

glpatcern force-pushed the spaces branch from 212f451 to fdc49ec Compare February 3, 2025 10:00

Minor fixes

a784b5e

glpatcern force-pushed the spaces branch from fdc49ec to a784b5e Compare February 4, 2025 08:58

Merge branch 'upsmaster' into spaces

e42fec0

glpatcern removed the request for review from a team February 4, 2025 15:18

embed mocked values through separate json files

209ccfc

jessegeens force-pushed the spaces branch from 3b2ab14 to 209ccfc Compare February 6, 2025 13:57

jessegeens added 2 commits February 6, 2025 15:16

Return 400 BadRequest instead of panic in decoding spaceHref

91a4961

remove hardcoded placeholder signing key

4655321

jessegeens force-pushed the spaces branch from a96a162 to 4655321 Compare February 6, 2025 14:33

Merge remote-tracking branch 'origin/master' into spaces

f99c24c

diocas reviewed Feb 7, 2025

View reviewed changes

jessegeens and others added 9 commits February 12, 2025 11:36

Fix possible nilpointer when stat gives an error

c758ce8

Remove named return values

02f339f

Removed duplication of relativePathToSpaceID

87693ac

ORMify projects table (#5081)

363220f

propagate link name

7ca7f21

Merge branch 'master' into spaces

213ca5c

go mod tidy

c94721b

Merge remote-tracking branch 'origin/master' into spaces

b88da4a

introduced a cache in front of ListProjects to limit the number of ca…

5f42d38

…lls to the database

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Spaces #4404

Add support for Spaces #4404

glpatcern commented Dec 13, 2023 •

edited

Loading

glpatcern left a comment •

edited

Loading

glpatcern Feb 3, 2025

diocas Feb 7, 2025 •

edited by jessegeens

Loading

glpatcern Feb 3, 2025

diocas left a comment

diocas Feb 7, 2025 •

edited by jessegeens

Loading

diocas Feb 7, 2025

diocas Feb 7, 2025

jessegeens Feb 24, 2025

diocas Feb 7, 2025

jessegeens Feb 20, 2025

diocas Feb 7, 2025

jessegeens Feb 20, 2025

Add support for Spaces #4404

Are you sure you want to change the base?

Add support for Spaces #4404

Conversation

glpatcern commented Dec 13, 2023 • edited Loading

glpatcern left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diocas Feb 7, 2025 • edited by jessegeens Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diocas left a comment

Choose a reason for hiding this comment

diocas Feb 7, 2025 • edited by jessegeens Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glpatcern commented Dec 13, 2023 •

edited

Loading

glpatcern left a comment •

edited

Loading

diocas Feb 7, 2025 •

edited by jessegeens

Loading

diocas Feb 7, 2025 •

edited by jessegeens

Loading