webdav-client icon indicating copy to clipboard operation
webdav-client copied to clipboard

`moveFile` operation type has missing optional `options` parameter

Open ahaltindis opened this issue 3 years ago • 3 comments

Hello, first of all thanks for this great webdav client, it is really solid!

I have a minor issue though; it seems the types for some of the operations (i.e. moveFile [1]) are not supporting optional options parameter even though the implementation supports [2] it and documentation [3] says they do.

I am creating this issue very specific to moveFile because passing a header for Overwrite flag is part of the standard [4], and by default it overwrites (using Nextcloud), which is not safe in my use-case (My current and probably only workaround is ignoring ts check at the moment).

So, as the possible solutions: 1 - types.ts can be updated to reflect the implementation. 2 - Overwrite parameter can be supported natively by moveFile operation as an optional parameter.

I'd be more than happy to implement both or one of them if you could tell me your preference (and the target version v4 or v5).

Thanks!

[1] - https://github.com/perry-mitchell/webdav-client/blob/f4403c449b5cf648e678a64306ba35d27f49053b/source/types.ts#L233 [2] - https://github.com/perry-mitchell/webdav-client/blob/f4403c449b5cf648e678a64306ba35d27f49053b/source/factory.ts#L96 [3] - https://github.com/perry-mitchell/webdav-client/blob/master/README.md#movefile [4] - http://www.webdav.org/specs/rfc4918.html#rfc.section.9.9.3

ahaltindis avatar Dec 30 '22 20:12 ahaltindis

Thanks for the detailed issue! I'd actually not realised that such an option wasn't available on moveFile. I thought it had already been implemented.

I'd accept a PR for this, though I'd probably get to this at some stage. What would the default be though? Need to test current functionality without it specified and it's safer that the default mirrors current functionality i think.

perry-mitchell avatar Dec 31 '22 12:12 perry-mitchell

Thanks for really quick response!

I understand that the goal of this package is not to follow any standard but the one I am checking rfc4918 clearly explains the default behavior for move and copy operations:

If the overwrite header is not included in a COPY or MOVE request, then the resource MUST treat the request as if it has an overwrite header of value "T".

That means, lack of overwrite behavior does overwrite the target resource if exists. I also tested against Nextcloud, which aligns with this. Though, I am not sure if there are any other standards which contradict with this, or any implementation that doesn't follow.

My suggestion would be:

  1. Have an optional boolean parameter overwrite for moveFile and copyFile (why not also fix for copying..) implementations.
  2. a. If the overwrite parameter is not provided, do not pass Overwrite header at all.
  3. If the overwrite parameter is provided as true, then pass T to Overwrite header.
  4. If the overwrite parameter is provided as false, then pass F to Overwrite header.
  5. Update types.ts to support new overwrite, and existing options parameters for moveFile and copyFile operations.

I think this is the safest approach for backward compatibility without following any standard strictly.

Alternatively, step 2. b. would be, passing also T if the overwrite parameter is not provided but there is a risk of breaking some current functionality. Equally, might be less confusing in the perspective of API design..

Let me know what do you think about it.

ahaltindis avatar Jan 01 '23 19:01 ahaltindis

Hi @ahaltindis - I'm sorry for the long delay. I completely agree with your suggestion.. this sounds like the best way to implement Overwrite.

perry-mitchell avatar Jan 17 '23 20:01 perry-mitchell