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

moveFile operation type has missing optional options parameter #329

Closed
ahaltindis opened this issue Dec 30, 2022 · 3 comments · Fixed by #361
Closed

moveFile operation type has missing optional options parameter #329

ahaltindis opened this issue Dec 30, 2022 · 3 comments · Fixed by #361

Comments

@ahaltindis
Copy link

ahaltindis commented Dec 30, 2022

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] -

moveFile: (filename: string, destinationFilename: string) => Promise<void>;

[2] -
moveFile: (filename: string, destinationFilename: string, options?: WebDAVMethodOptions) =>

[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

@perry-mitchell
Copy link
Owner

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.

@ahaltindis
Copy link
Author

ahaltindis commented Jan 1, 2023

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.

@perry-mitchell
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants