Address trash moves and deletes by path, not fileid
Trashing or emptying the trash failed on every scanned image with "operation unsupported by this backend: fetch by fileid requires a path". RemoteId::Stable(oc:fileid) is an identity — it answers whether a file is the same one after a move, and keys the thumbnail shards. It is not an address: WebDAV exposes no fileid-addressable endpoint, so the Nextcloud backend serves get/delete/move_to by path and rejects a bare Stable. Both trash workers preferred the fileid whenever the catalog knew one, so the unreachable Path fallback was the only arm that would have worked, and the failure hit every properly-scanned image rather than some edge case. Address by path at both sites, and keep the fileid for what it is for: the identity MOVE preserves, and the key the thumbnail cleanup uses. Move::file_id was documented as "the id the MOVE addresses", which is the wrong claim that seeded this; corrected, along with a note on RemoteId itself so the distinction is stated where the type is defined. Only the backend rejection was covered by a test. Added the positive case, since that contract is what the call sites now depend on. The workers build their own NextcloudBackend, so no test can reach the call sites directly — closing that would mean injecting the backend, which is left alone here. Verified by build and test; not exercised against a live server. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -530,6 +530,20 @@ mod tests {
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn addressing_by_path_is_what_works() {
|
||||||
|
// The counterpart to the rejection above, and the reason trash and
|
||||||
|
// purge pass `RemoteId::Path` even when the catalog knows the fileid:
|
||||||
|
// every id-taking method routes through here, so a `Stable` turns a
|
||||||
|
// trash into "operation unsupported by this backend".
|
||||||
|
let b = backend();
|
||||||
|
assert_eq!(
|
||||||
|
b.url_for_id(&RemoteId::Path(RemotePath::new("Photos/_MG_8154.dng")))
|
||||||
|
.unwrap(),
|
||||||
|
"https://cloud.example/remote.php/dav/files/duncan/Photos/_MG_8154.dng"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn statuses_map_to_actionable_errors() {
|
fn statuses_map_to_actionable_errors() {
|
||||||
use reqwest::StatusCode;
|
use reqwest::StatusCode;
|
||||||
|
|||||||
@@ -66,6 +66,16 @@ impl std::fmt::Display for RemotePath {
|
|||||||
/// survives server-side rename and move, so a move is detected as a move
|
/// survives server-side rename and move, so a move is detected as a move
|
||||||
/// rather than a delete plus a re-download of a large file (FR-NC-5).
|
/// rather than a delete plus a re-download of a large file (FR-NC-5).
|
||||||
/// Otherwise it falls back to the path, and moves cost a transfer.
|
/// Otherwise it falls back to the path, and moves cost a transfer.
|
||||||
|
///
|
||||||
|
/// **Identity is not an address.** `Stable` answers "is this the same object
|
||||||
|
/// as before?"; it does not necessarily name one the backend can address.
|
||||||
|
/// Nextcloud, the only backend, exposes no fileid-addressable endpoint, so it
|
||||||
|
/// serves [`RemoteBackend::get`], [`delete`](RemoteBackend::delete) and
|
||||||
|
/// [`move_to`](RemoteBackend::move_to) by path alone and rejects a `Stable`
|
||||||
|
/// with [`RemoteError::Unsupported`]. Callers holding both — the catalog keeps
|
||||||
|
/// the fileid beside `remote_path` — must pass the path to those three and
|
||||||
|
/// keep the fileid for what it is good for: matching moves across scans, and
|
||||||
|
/// keying the thumbnail shards.
|
||||||
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
|
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
|
||||||
pub enum RemoteId {
|
pub enum RemoteId {
|
||||||
/// A server-assigned identifier, e.g. Nextcloud's `oc:fileid`.
|
/// A server-assigned identifier, e.g. Nextcloud's `oc:fileid`.
|
||||||
|
|||||||
+28
-14
@@ -65,14 +65,20 @@ pub enum Direction {
|
|||||||
|
|
||||||
/// One image to move, resolved before the worker starts.
|
/// One image to move, resolved before the worker starts.
|
||||||
///
|
///
|
||||||
/// Carries the id the `MOVE` addresses and the destination path, so the worker
|
/// Carries the paths the `MOVE` runs between, so the worker needs no catalog
|
||||||
/// needs no catalog access to do its half — the catalog is not `Send`, and the
|
/// access to do its half — the catalog is not `Send`, and the worker owns a
|
||||||
/// worker owns a separate connection only for the write-back.
|
/// separate connection only for the write-back.
|
||||||
#[derive(Debug, Clone)]
|
#[derive(Debug, Clone)]
|
||||||
pub struct Move {
|
pub struct Move {
|
||||||
pub image_id: ImageId,
|
pub image_id: ImageId,
|
||||||
/// `oc:fileid` where known, else the path. A stable id survives the move and
|
/// `oc:fileid` where the catalog knows one — the identity `MOVE` preserves,
|
||||||
/// keeps the thumbnail and sidecar mapping attached.
|
/// which is what keeps the thumbnail and sidecar mapping attached across a
|
||||||
|
/// trash and restore.
|
||||||
|
///
|
||||||
|
/// Not how the file is addressed: the `MOVE` goes from `from` to `to`,
|
||||||
|
/// because WebDAV exposes no fileid-addressable endpoint and the backend
|
||||||
|
/// rejects a bare `RemoteId::Stable`. Carried so the plan records the
|
||||||
|
/// identity it expects to survive.
|
||||||
pub file_id: Option<u64>,
|
pub file_id: Option<u64>,
|
||||||
pub from: String,
|
pub from: String,
|
||||||
pub to: String,
|
pub to: String,
|
||||||
@@ -199,13 +205,22 @@ pub fn spawn_move(
|
|||||||
let mut failed: Vec<String> = Vec::new();
|
let mut failed: Vec<String> = Vec::new();
|
||||||
|
|
||||||
for (i, mv) in moves.iter().enumerate() {
|
for (i, mv) in moves.iter().enumerate() {
|
||||||
let id = match mv.file_id {
|
// Addressed by path, not by `mv.file_id`: WebDAV has no
|
||||||
Some(f) => RemoteId::Stable(f),
|
// fileid-addressable endpoint, so a `RemoteId::Stable` here is
|
||||||
None => RemoteId::Path(RemotePath::new(&mv.from)),
|
// rejected by the backend. The fileid is an identity that the
|
||||||
};
|
// MOVE preserves, not a way to name the source.
|
||||||
|
let id = RemoteId::Path(RemotePath::new(&mv.from));
|
||||||
|
|
||||||
match backend.move_to(&id, &RemotePath::new(&mv.to)).await {
|
match backend.move_to(&id, &RemotePath::new(&mv.to)).await {
|
||||||
Ok(()) => succeeded.push((mv.image_id, mv.to.clone())),
|
Ok(()) => {
|
||||||
|
// The fileid is logged, not sent: if a restore later
|
||||||
|
// shows a missing thumbnail, this is the record of which
|
||||||
|
// identity the MOVE was supposed to carry across.
|
||||||
|
if let Some(f) = mv.file_id {
|
||||||
|
log::debug!("moved {} to {} as fileid {f}", mv.from, mv.to);
|
||||||
|
}
|
||||||
|
succeeded.push((mv.image_id, mv.to.clone()));
|
||||||
|
}
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
// Named by file, not by id: the user recognises the
|
// Named by file, not by id: the user recognises the
|
||||||
// filename and cannot do anything with a row number.
|
// filename and cannot do anything with a row number.
|
||||||
@@ -307,10 +322,9 @@ pub fn spawn_purge(
|
|||||||
let mut failed: Vec<String> = Vec::new();
|
let mut failed: Vec<String> = Vec::new();
|
||||||
|
|
||||||
for (i, (image, file_id, path)) in paths.iter().enumerate() {
|
for (i, (image, file_id, path)) in paths.iter().enumerate() {
|
||||||
let id = match file_id {
|
// By path — see `spawn_move`. `file_id` still matters below, as
|
||||||
Some(f) => RemoteId::Stable(*f),
|
// the key the thumbnail shards are stored under.
|
||||||
None => RemoteId::Path(RemotePath::new(path)),
|
let id = RemoteId::Path(RemotePath::new(path));
|
||||||
};
|
|
||||||
|
|
||||||
match backend.delete(&id, None).await {
|
match backend.delete(&id, None).await {
|
||||||
Ok(()) => {
|
Ok(()) => {
|
||||||
|
|||||||
Reference in New Issue
Block a user