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

Add an optional lock_id argument to storage methods #163

Merged
merged 5 commits into from
Feb 25, 2022

Conversation

glpatcern
Copy link
Member

@glpatcern glpatcern commented Feb 9, 2022

Follow up of #162: the storage methods that modify a resource now have an optional lock_id argument, such that storage provider implementations can enforce the lock validation.

In addition, also the InitiateDownloadRequest has this extra argument, in order to honor exclusive locks. The assumption then is that stat and metadata read operations are always allowed (when access rights are met), such that e.g. the web UI can show users that "File important_stuff.txt is locked in exclusive mode by user bob".

@glpatcern glpatcern force-pushed the lockid-args branch 3 times, most recently from 673979f to c01842c Compare February 9, 2022 11:39
Copy link
Contributor

@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.

I think these messages also need a lock_id:

  • CreateContainer
  • TouchFile
  • RestoreFileVersion
  • RestoreRecycleItem

they all might end up writing into a locked resource

@glpatcern
Copy link
Member Author

We looked at those too:

  • CreateContainer and TouchFile expect to create a resource and must fail if it exists
  • Restore actions indeed may need the lock_id argument, going to add it

@glpatcern glpatcern force-pushed the lockid-args branch 3 times, most recently from 4d00f64 to 0729f43 Compare February 10, 2022 09:17
@glpatcern
Copy link
Member Author

I ended up adding the lock_id argument also to OpenInApp, though this is far-fetching: it would be needed when exclusive locks are implemented.

@glpatcern
Copy link
Member Author

@butonic not sure why I can't ask you to review again, anyway could you have another look? I'd go ahead and merge this as is, in case we can keep adding further lock_id arguments if we deem it needed.

@ishank011
Copy link
Contributor

In addition, also the InitiateDownloadRequest has this extra argument, in order to honor exclusive locks

Should we also enforce it for List, since it is equivalent in case the exclusive lock is for a directory?

@ishank011
Copy link
Contributor

Can we also rename Unlock to ReleaseLock?

Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

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

For RestoreFileVersion and RestoreRecycleItem, we should specify that the lock is for the restore path

cs3/storage/provider/v1beta1/provider_api.proto Outdated Show resolved Hide resolved
@glpatcern
Copy link
Member Author

Can we also rename Unlock to ReleaseLock?

As discussed, Unlock came from the WOPI terminology, so we leave it as is.

Otherwise, I expanded the spec for RestoreRecycleItem.

@glpatcern
Copy link
Member Author

Should we also enforce it for List, since it is equivalent in case the exclusive lock is for a directory?

As a matter of fact locking a directory was not taken into account so far. I think we should put a comment that although SetLock supports any reference, the semantics of locked containers are not specified at this stage.

@ishank011 ishank011 merged commit 8a2c7a3 into cs3org:main Feb 25, 2022
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.

3 participants