Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs-only] Add draft of adr for spaces API. #1827

Merged
merged 12 commits into from
Apr 30, 2021
Merged

Conversation

dragotin
Copy link
Contributor

Description

First draft of an API Spec for the spaces API that we want to implement on oCIS.

Drafted to give a get better understanding of client impact and implementation steps.

Related Issue

Jira OCIS-1683
Jira OCIS-1684

@update-docs
Copy link

update-docs bot commented Mar 19, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Copy link

@felix-schwarz felix-schwarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As promised, here are my notes:

  • semantically different/clearer API paths for "home" and "all spaces"?
    • /me/drive/home
    • /me/drive/all
    • also clear structure for additional ones in the future, i.e.
      • /me/drive/pending
  • webUrl -> davPath if always on the same host?
  • ability for users to organize/move spaces like OC10 shares? Add path attribute?
  • eTag / cTag? -> to detect changes to favorites, share status, etc…
  • mechanism to manage or at least identify space invites (pending/accepted attribute)?
  • per-user metadata for spaces

@butonic
Copy link
Member

butonic commented Mar 31, 2021

related cs3org/reva#1568

@micbar
Copy link
Contributor

micbar commented Apr 7, 2021

@labkode

@micbar
Copy link
Contributor

micbar commented Apr 19, 2021

Ci broken.

@micbar
Copy link
Contributor

micbar commented Apr 19, 2021

Needs a rebase to get the golang 1.16 images for the docs build.

@dragotin dragotin force-pushed the doc-adr-spaces-api branch from d0c6f71 to e6f8009 Compare April 19, 2021 15:28
@dragotin
Copy link
Contributor Author

rebase done

@dragotin
Copy link
Contributor Author

@felix-schwarz - thanks for your feedback

  • semantically clearer APIs: We try to implement the graph API as close as possible, so this feedback should go to upsteam ;-)
  • The webUrl must not neccessarily be on the same host... I think. At least I am not really ready to limit it ;-)
  • No moving of the spaces is supported any more.
  • eTag/cTag well... Yes, we could add it but semantically I'd say it is not really correct. This API reflects the "static" setup while the tags are highly dynamic. But the ETag is part of the original API, so granted ;-)
  • 'pending'- attribute: GREAT catch. Adding...
  • Per user metadata: Good point, but I like to push to postpone.

@butonic
Copy link
Member

butonic commented Apr 22, 2021

@micbar with regards to owners of a drive: https://docs.microsoft.com/en-us/graph/api/resources/drive?view=graph-rest-beta#properties lists:

Property Type Description
owner identitySet Optional. The user account that owns the drive. Read-only.

identitySet is a set, so it can actually contain multiple identity entries, just like @michaelstingl originally proposed. No it cant ... see below

Double checking with the graph explorer, the createdBy property also is an Identity set:

            "createdBy": {
                "application": {
                    "displayName": "OneDrive website",
                    "id": "44048800"
                },
                "user": {
                    "displayName": "Jörn Dreyer",
                    "id": "c12644a14b0a7750"
                }
            },

so they combine not only the user, but also the application and the device that was used into an identitySet ... 🤦‍♂️ well ... that means it cannot be used to add multiple owners ...

Anyway, in the list call with @micbar we found that a drilve with multiple owners would be represented by a group, because a group has a relationship with a (default) drive: https://docs.microsoft.com/en-us/graph/api/resources/group?view=graph-rest-1.0#relationships as well as multiple drives.

The same relationships (drive and drives) exist for users ... which makes me wonder if the drives reference contains all drives the user has access to or only all drives he is the owner of ... at least for my personal account I cannot see the drives of drive items that have been shared with me.

docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
@micbar micbar mentioned this pull request Apr 22, 2021
19 tasks
Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now.


The following *driveStatus* values are available:

* **accepted**: The user has accepted the space and uses it
Copy link
Member

@labkode labkode Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These fields represents only a perspective on how shares could work.
As a user when I'm receiving a share I may want to check the contents of it before accepting it. Accepts has collateral effect that the share will be synchronised in the clients.
As a user you may want to access the shares (from the web) but not necessarily sync it.

@refs
Copy link
Member

refs commented Apr 28, 2021

@dragotin since this was opened there were 2 more ADR merged, Could you update the title / index to 0007-api-for-spaces.md? :)

Copy link

@felix-schwarz felix-schwarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ability for users to organize/move spaces like OC10 shares? Add path attribute?

  • No moving of the spaces is supported any more.

In the light of discussing the "bonus problem" of how a single folder could be shared into multiple spaces, I'd like to bring this up again.

Assuming that each shared folder shows up as a space of its own, wouldn't a mountPaths attribute make sense here?

Like, f.ex. if I have one space C that I want to map into space A at /projectA/ and into space B at /contributions/felix/, the contents of mountPaths for space C could be {"[spaceA.id]:/projectA/", "[spaceB.id]:/contributions/felix/"}.

Benefits:

  • allows to map spaces into other spaces (incl. one space into multiple spaces)
  • give clients a way to resolve path in space to the actual/origin space and path in that space, which would be the same across all locations an item is shared into (i.e. [spaceA.id]:/projectA/README.md and [spaceB.id]:/contributions/felix/README.md would be directly mapped to [spaceC.id]:/README.md in the client)
  • clients don't have to enumerate the same folder multiple times in different spaces (or even through different spaces), but instead can enumerate the source space directly and have up-to-date, consistent data across all locations the space is mapped/mounted at
  • lightweight server implementation, since the server wouldn't need to return contents from other spaces within itself (or at all), propagate eTag/cTag changes, etc. … – the client would take care of mapping the space into the target space at the correct location

docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved

* **personal**: The users home space
* **projectSpaces**: The project spaces available for the user (*)
* **shares**: The share jail, contains all shares for the user (*)
Copy link

@felix-schwarz felix-schwarz Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it sound as if all shares are mapped into this space, but the existence of driveStatus suggests that each share would be represented by its own space.

If so, the value should probably use the singular share (and likewise projectSpace if one space is returned for each project space).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is now on the essentials about shares vs. spaces.

The SharesJail is just a collection of shares which does not include spaces (project folders)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the name SharesJail imply that files there cannot be moved outside the jail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, shares are always listed inside shares. That does not necessarily mean that shares is a directory. It is up to the clients to render that.

refs
refs previously requested changes Apr 30, 2021
Copy link
Member

@refs refs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the title to 0007-api-for-spaces.md? and also the filename? 2 more ADR were merged since this is open :)

docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved

### API to Get Info about Spaces

ownCloud servers provide an API to query for available spaces of an user.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ownCloud servers provide an API to query for available spaces of an user.
ownCloud servers provide an API to query for available spaces of a user.


* **personal**: The users home space
* **projectSpaces**: The project spaces available for the user (*)
* **shares**: The share jail, contains all shares for the user (*)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the name SharesJail imply that files there cannot be moved outside the jail?

docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
Copy link
Member

@butonic butonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felix-schwarz I think mount points should be managed by the clients. the could actually add an extended attribute to mark a folder as tho mount point for another storage space. no need for a dedicated api or special properties.

reva does have references that are used to implement shares. But the logic about 'plumbing together' a namespace out of a 'sea of storage space' is client specific. how you presend all incoming shares is up to the actual client. web might do it differently ...

I'd leave mount points out of this for now. storage spaces have types and we can aggregate them in lists for eg. 'shares', 'spaces', 'home' ...

docs/adr/0005-api-for-spaces.md Outdated Show resolved Hide resolved
@dragotin dragotin force-pushed the doc-adr-spaces-api branch from 4fd6674 to 3122239 Compare April 30, 2021 15:15
@butonic butonic requested a review from refs April 30, 2021 15:17
@dragotin dragotin dismissed refs’s stale review April 30, 2021 15:22

Incorperated the suggested changes.

@dragotin dragotin merged commit 14ac8e5 into master Apr 30, 2021
@delete-merged-branch delete-merged-branch bot deleted the doc-adr-spaces-api branch April 30, 2021 15:23
ownclouders pushed a commit that referenced this pull request Apr 30, 2021
Merge: 6826e57 3122239
Author: Klaas Freitag <[email protected]>
Date:   Fri Apr 30 17:23:44 2021 +0200

    Merge pull request #1827 from owncloud/doc-adr-spaces-api

    [docs-only] Add draft of adr for spaces API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants