-
Notifications
You must be signed in to change notification settings - Fork 158
Add File Move Operation to PATCH Endpoint (only the relevant commits) #191
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
base: main
Are you sure you want to change the base?
Conversation
- Add move operation to PATCH endpoint for files - Use Operation: move, Target-Type: file, Target: path - Automatically creates parent directories if needed - Preserves all internal links using FileManager.renameFile() - Validates paths and prevents overwrites - Add tests for edge cases
- Add new error codes for clearer validation messages - Use consistent returnCannedResponse pattern throughout - Reorder validation checks for better flow - Separate file operations from applyPatch operations early - Maintain upstream coding style and patterns
…ile move - Add path traversal protection to prevent access outside vault - Use returnCannedResponse consistently throughout handleMoveOperation - Add proper error codes to ErrorCode enum and ERROR_CODE_MESSAGES - Validate paths early to prevent directory path destinations - Check parent directory existence before creating - Add comprehensive security tests for path traversal attempts - Normalize paths to handle backslashes and multiple slashes
- Add move operation to PATCH endpoint enum - Update Target parameter description for move operations - Include example for file move functionality - Documents Operation: move, Target-Type: file usage
Forgive me, I might have misremembered my conversation, but I wonder if you could consider adding that support via an API extension instead? That funcionality is documented here: https://github.com/coddingtonbear/obsidian-local-rest-api/wiki/Adding-your-own-API-Routes-via-an-Extension ; maybe you ran into an obstacle when trying to go that direction? |
Note that I am a different person. :-D You discussed that with @duquesnay; I just took his work from PR #177 that I needed but was not being worked on (and that's fine, people have lives), so I figured I'd do the commit reshuffling you had requested. I didn't know about the API extensions; they might be a good way to dial in new operations, but maybe Move would be such an essential operation that it would belong in the API proper? |
I really appreciate you taking the time to do this! There’s an open discussion about adding a WebDAV-style MOVE method here, and that feels like a more natural direction to me right now. Part of the hesitation is that REST itself doesn’t really have a concept of MOVE — you can already achieve the same thing with a combination of GET, PUT, and DELETE. So while I do think it’s helpful to have a clean way to express “move,” I’m leaning toward the WebDAV-style approach over extending PATCH in this way. |
No worries! Using a custom verb would definitely work, and I agree that MOVE would be more logical endpoint than PATCH. Actually, PATCH probably isn't a great method for MOVE because the original resource gets lost in the operation. In the case of the Obsidian API, GET-PUT-DELETE is not a viable alternative to a Move operation because Move also handles internal links (and backlinks, I believe). Otherwise my LLM repo organizer would've been easier to implement on the file system level. |
You know, I'm not sure how it never occurred to me, but I honestly hadn't at all considered that one of the side-effects of having a dedicated way of moving a file would be re-writing links as appropriate. Still, definitely feeling like adding a In any case, would you forgive me if we moved this conversation over to #190 for the time being so both approaches can be considered side-by-side? |
I would. :-D |
I implemented a custom MOVE operation (with the kind assistance of Claude Code). I'll provide my opinion in Discussion #190. |
First, a disclaimer: I needed the Move operation for my own vault scripting and began working on the topic a couple of weeks ago. I didn't notice that Adam closed PR #177 meanwhile based on discussion with Guillaume.
Basically, I've just done the commit-wrangling requested by Adam in PR #177; there are no functional changes to any of @duquesnay's code. Despite the resolution of the original PR, I decided to publish the PR nevertheless, mainly for discussion or as an alternative solution. No ill will or passive-aggressiveness intended. :-)