nextcloud / files_lock

GNU Affero General Public License v3.0
25 stars 8 forks source link

Lock and Unlock does not provide the new etag value for a file in question #162

Closed allexzander closed 1 year ago

allexzander commented 1 year ago

When we lock or unlock a file its etag gets modified and is later synced by the desktop client. The problem is, in case we also modify a file locally after we lock it, the etag we are storing in the desktop client is still the one before calling lock/unlock, and, since we've modified a file as well as its etag got changed on the server - we are getting a false positive file conflict. The etag is used by desktop client to detect changes made to the file on the server.

Possible solutions (not sure why the etag is chosen to mark file lock state change, could be that we need a more exclusive way of handling this not involving the etag):

Example of XML response I am getting:

<?xml version="1.0"?>
<d:prop xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns" xmlns:nc="http://nextcloud.org/ns">
 <nc:lock>1</nc:lock>
 <nc:lock-owner-type>0</nc:lock-owner-type>
 <nc:lock-owner>admin</nc:lock-owner>
 <nc:lock-owner-displayname>admin</nc:lock-owner-displayname>
 <nc:lock-owner-editor>admin</nc:lock-owner-editor>
 <nc:lock-time>1693899473</nc:lock-time>
 <nc:lock-timeout>0</nc:lock-timeout>
 <nc:lock-token>files_lock/cf140436-7691-4cc9-b4d5-33ad1daf52cb</nc:lock-token>
</d:prop>
jospoortvliet commented 1 year ago

FYI this can cause a ton of issues, especially with larger files. If your workflow is:

  1. Lock file
  2. edit file
  3. save
  4. unlock

and the file is, say, 100mb or more, you risk causing conflicts, besides the 3x inflation of network traffic.

AndyScherzinger commented 1 year ago

looping in @juliushaertl

juliusknorr commented 1 year ago

We can expose the changed etag with the lock response, however having a separate one is not really feasible as we would need that separation for a metadata tag as described in https://github.com/nextcloud/files_lock/issues/147 / https://github.com/nextcloud/server/issues/8477

A quick implementation draft for returning the etag property on the user LOCK/UNLOCK webdav requests, so maybe that already helps for this specific case https://github.com/nextcloud/files_lock/pull/163

allexzander commented 1 year ago

@juliushaertl

A quick implementation draft for returning the etag property on the user LOCK/UNLOCK webdav requests, so maybe that already helps for this specific case https://github.com/nextcloud/files_lock/pull/163

This will help, I would appreciate it if this gets merged/deployed (at least to one of try.nextcloud.com instances).

allexzander commented 1 year ago

@juliushaertl So I tested it and the etag is returned but is still the old one

max-nextcloud commented 1 year ago

@allexzander could you try again? Julius updated the PR.

allexzander commented 1 year ago

@max-nextcloud Yes, changes on desktop client's side already merged