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

Extend InitiateFileUpload to support exclusive uploads and etags-based checks #122

Merged
merged 3 commits into from
Jun 11, 2021

Conversation

glpatcern
Copy link
Member

This is a revamp of #87, which got closed because the master branch got renamed to main...

Pasting here the relevant parts:

This extension is motivated by cs3org/wopiserver#25. Discussing with @ishank011 it looks reasonable to extend the InitiateFileUploadRequest to support exclusive operations: I may want to upload a file "no matter what", overwriting the destination - which is the current behavior. Or I may want to atomically check-and-upload, which is the scope of this new flag. Both actions are legitimate and should be supported.

The PR proposes a minimal extension of the protocol. May we need to support other POSIX(-like) flags and have rather an enum instead of just a bool? I guess by now we're settled, yet for reference the years-old open() call has plenty of options.

Related to this, there's been quite some discussion around locking within ownCloud, that's why I'm involving @butonic too.

Comments/opinions welcome of course, but I'd like to move forward so to start testing the WOPI server locking logic with oCIS.

@glpatcern glpatcern requested review from butonic and ishank011 April 22, 2021 09:45
@glpatcern glpatcern requested a review from labkode as a code owner April 22, 2021 09:45
@glpatcern glpatcern force-pushed the glp-openinapp-changes branch from a7f1694 to bf25a81 Compare April 22, 2021 09:49
@glpatcern
Copy link
Member Author

Following discussions, a quick summary. We should follow the if-match etags preconditions used in HTTP headers and implement:

  • A more explicit upload-if-absent flag (rename exclusive)
  • A upload-if-match option to handle the etag use-case from sync clients

We agree that the default behavior without options is to overwrite the destination without further checks. Name of the optional arguments to be agreed.

Also, the response MUST contain the current etag when upload-if-match is used and the precondition fails.

@glpatcern glpatcern changed the title Extend InitiateFileUpload to support exclusive (atomic) uploads WIP: Extend InitiateFileUpload to support preconditions for atomic uploads and etags-based checks Apr 28, 2021
@glpatcern glpatcern changed the title WIP: Extend InitiateFileUpload to support preconditions for atomic uploads and etags-based checks WIP: Extend InitiateFileUpload to support atomic uploads and etags-based checks Apr 28, 2021
@glpatcern glpatcern force-pushed the glp-openinapp-changes branch from 8ee2c5b to 844ccd8 Compare April 30, 2021 13:10
@glpatcern glpatcern changed the title WIP: Extend InitiateFileUpload to support atomic uploads and etags-based checks Extend InitiateFileUpload to support exclusive uploads and etags-based checks Apr 30, 2021
Copy link
Member Author

@glpatcern glpatcern left a comment

Choose a reason for hiding this comment

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

Reviewing myself, two comments:

  1. Shall we use a oneof grup for if_not_exist and if_match, i.e. make them alternative? I'm using a oneof for both options, please have a look
  2. In case of etag mismatch, do we want the current etag to be returned as opaque or do we give a specific (again optional) field in the InitiateFileUploadResponse?

@glpatcern glpatcern force-pushed the glp-openinapp-changes branch from 8dd3b14 to cf474e1 Compare June 11, 2021 13:06
@ishank011 ishank011 merged commit 72517b1 into cs3org:main Jun 11, 2021
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