From aad6315731c4e1ed2a88f5d7bf7d87386bdb742e Mon Sep 17 00:00:00 2001 From: Andriy Oblivantsev Date: Sun, 27 Sep 2026 15:12:13 +0100 Subject: [PATCH] fix(dav): re-upload converted ext instead of UpdateFile (#84) UploadToFolderReplacing matched legacy->OOXML counterparts (f.xls vs the saved f.xlsx) and updated them in place. UpdateFile replaces the body without re-running OnlyOffice's server-side conversion, so the raw OLE2 .xls landed under the stored .xlsx name and the file would not open (#84). UpdateFile now runs only when the stored and local extensions are equal. A conversion-equivalent match is deleted (all duplicates collapsed) and uploaded afresh, so the server converts again. AssertNoFileConflict stays conversion-aware for --no-replace and never had an UpdateFile path. --- files_replace_test.go | 90 +++++++++++++++++++++++++++++++++++-------- files_stem.go | 66 +++++++++++++++++++------------ 2 files changed, 117 insertions(+), 39 deletions(-) diff --git a/files_replace_test.go b/files_replace_test.go index 80019e9..74611ff 100644 --- a/files_replace_test.go +++ b/files_replace_test.go @@ -73,14 +73,58 @@ func TestPlanUploadReplacementFreshUpload(t *testing.T) { } } -func TestPlanUploadReplacementUpdatesConvertedFile(t *testing.T) { +// TestPlanUploadReplacementUpdatesExactExt covers the in-place path: only a +// stored file with the same extension is updated via UpdateFile. PDF over PDF +// and OOXML over OOXML keep the id and rewrite the body. +func TestPlanUploadReplacementUpdatesExactExt(t *testing.T) { + tests := []struct { + name string + file *FileEntry + stem string + ext string + id string + }{ + { + name: "xlsx over xlsx", + file: &FileEntry{ID: jsonNum("3799"), Title: strPtr("ES29-extracto.xlsx"), FileExst: strPtr(".xlsx")}, + stem: "ES29-extracto", ext: ".xlsx", id: "3799", + }, + { + name: "pdf over pdf", + file: &FileEntry{ID: jsonNum("42"), Title: strPtr("extracto.pdf"), FileExst: strPtr(".pdf")}, + stem: "extracto", ext: ".pdf", id: "42", + }, + { + name: "legacy xls over xls", + file: &FileEntry{ID: jsonNum("11"), Title: strPtr("legacy.xls"), FileExst: strPtr(".xls")}, + stem: "legacy", ext: ".xls", id: "11", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + plan := planUploadReplacement([]*FileEntry{tc.file}, tc.stem, tc.ext) + if plan.UpdateID != tc.id { + t.Fatalf("UpdateID = %q, want %q (same ext updates in place)", plan.UpdateID, tc.id) + } + if len(plan.DeleteIDs) != 0 { + t.Fatalf("DeleteIDs = %v, want none", plan.DeleteIDs) + } + }) + } +} + +// TestPlanUploadReplacementConvertedExtDeletesThenUploads is the regression for +// #84: UpdateFile does not re-run the server-side legacy→OOXML conversion, so a +// .xls upload must not overwrite a stored .xlsx in place (raw OLE2 under an +// .xlsx name). The stale counterpart is deleted and the file uploaded afresh. +func TestPlanUploadReplacementConvertedExtDeletesThenUploads(t *testing.T) { xlsx := &FileEntry{ID: jsonNum("3799"), Title: strPtr("ES29-extracto.xlsx"), FileExst: strPtr(".xlsx")} plan := planUploadReplacement([]*FileEntry{xlsx}, "ES29-extracto", ".xls") - if plan.UpdateID != "3799" { - t.Fatalf("UpdateID = %q, want 3799 (update the saved .xlsx in place)", plan.UpdateID) + if plan.UpdateID != "" { + t.Fatalf("UpdateID = %q, want empty (do not update across conversion)", plan.UpdateID) } - if len(plan.DeleteIDs) != 0 { - t.Fatalf("DeleteIDs = %v, want none", plan.DeleteIDs) + if len(plan.DeleteIDs) != 1 || plan.DeleteIDs[0] != 3799 { + t.Fatalf("DeleteIDs = %v, want [3799] (delete the stale .xlsx before upload)", plan.DeleteIDs) } } @@ -89,7 +133,7 @@ func TestPlanUploadReplacementCollapsesDuplicates(t *testing.T) { newer := older.Add(time.Hour) first := &FileEntry{ID: jsonNum("3799"), Title: strPtr("ES29-extracto.xlsx"), FileExst: strPtr(".xlsx"), Updated: &older} second := &FileEntry{ID: jsonNum("3887"), Title: strPtr("ES29-extracto.xlsx"), FileExst: strPtr(".xlsx"), Updated: &newer} - plan := planUploadReplacement([]*FileEntry{first, second}, "ES29-extracto", ".xls") + plan := planUploadReplacement([]*FileEntry{first, second}, "ES29-extracto", ".xlsx") if plan.UpdateID != "3887" { t.Fatalf("UpdateID = %q, want the newest duplicate 3887", plan.UpdateID) } @@ -98,23 +142,39 @@ func TestPlanUploadReplacementCollapsesDuplicates(t *testing.T) { } } -// TestPlanUploadReplacementRepeatedXLS is the regression for #82: the second -// upload of the same .xls must find the server-converted .xlsx and update it, -// not append a second file. +// TestPlanUploadReplacementDeletesConvertedDuplicates: with a converted match +// every stale copy is deleted (there is no keeper — the fresh upload replaces +// them all). +func TestPlanUploadReplacementDeletesConvertedDuplicates(t *testing.T) { + first := &FileEntry{ID: jsonNum("3799"), Title: strPtr("ES29-extracto.xlsx"), FileExst: strPtr(".xlsx")} + second := &FileEntry{ID: jsonNum("3887"), Title: strPtr("ES29-extracto.xlsx"), FileExst: strPtr(".xlsx")} + plan := planUploadReplacement([]*FileEntry{first, second}, "ES29-extracto", ".xls") + if plan.UpdateID != "" { + t.Fatalf("UpdateID = %q, want empty", plan.UpdateID) + } + if len(plan.DeleteIDs) != 2 { + t.Fatalf("DeleteIDs = %v, want both stale .xlsx ids", plan.DeleteIDs) + } +} + +// TestPlanUploadReplacementRepeatedXLS is the regression for #84: with the +// server-converted .xlsx already present, the second .xls upload deletes it and +// uploads anew so OnlyOffice converts again — UpdateFile would corrupt it. func TestPlanUploadReplacementRepeatedXLS(t *testing.T) { const stem = "ES29-extracto" // First upload: nothing in the folder. - if plan := planUploadReplacement(nil, stem, ".xls"); plan.UpdateID != "" { + if plan := planUploadReplacement(nil, stem, ".xls"); plan.UpdateID != "" || len(plan.DeleteIDs) != 0 { t.Fatalf("first upload plan = %+v, want create", plan) } - // OnlyOffice converts .xls -> .xlsx on upload; second upload must match it. + // OnlyOffice converts .xls -> .xlsx on upload; the second upload must + // delete it and upload fresh, never UpdateFile it. saved := &FileEntry{ID: jsonNum("3799"), Title: strPtr(stem + ".xlsx"), FileExst: strPtr(".xlsx")} plan := planUploadReplacement([]*FileEntry{saved}, stem, ".xls") - if plan.UpdateID != "3799" { - t.Fatalf("second upload plan = %+v, want in-place update of 3799 (no duplicate)", plan) + if plan.UpdateID != "" { + t.Fatalf("second upload plan = %+v, want delete+upload (not UpdateFile)", plan) } - if len(plan.DeleteIDs) != 0 { - t.Fatalf("second upload would delete %v, want none", plan.DeleteIDs) + if len(plan.DeleteIDs) != 1 || plan.DeleteIDs[0] != 3799 { + t.Fatalf("second upload DeleteIDs = %v, want [3799]", plan.DeleteIDs) } } diff --git a/files_stem.go b/files_stem.go index dbccc3b..87ff7b1 100644 --- a/files_stem.go +++ b/files_stem.go @@ -158,18 +158,24 @@ func (c *Client) UploadProjectFileNoClobber(ctx context.Context, projectID, loca // uploadReplacementPlan is how a replacing upload reconciles with a folder. type uploadReplacementPlan struct { - // UpdateID is the existing file to overwrite in place; empty when no file - // matches the upload stem and (converted) extension. + // UpdateID is the existing file to overwrite in place; set only when the + // stored extension equals the local one. UpdateFile keeps the stored name, + // so overwriting across extensions would leave an unconverted body under a + // mismatched name. UpdateID string - // DeleteIDs are redundant duplicate ids to remove after the update. + // DeleteIDs are the ids to remove before uploading. They are the redundant + // duplicates of an in-place update, or every conversion-equivalent + // counterpart when the body must be converted again by a fresh upload. DeleteIDs []int } // planUploadReplacement matches an incoming local upload (stem + ext) against // the files already in a folder and decides between a fresh upload, an in-place -// update and duplicate cleanup. Matching tolerates the legacy→OOXML conversion +// update and delete + reupload. Matching tolerates the legacy→OOXML conversion // OnlyOffice performs on upload, so a repeated upload of f.xls finds the saved -// f.xlsx instead of creating a second file. +// f.xlsx instead of creating a second file. Because UpdateFile replaces the body +// without re-running that conversion, an equivalent-but-different extension is +// deleted and re-uploaded rather than updated in place (#84). func planUploadReplacement(files []*FileEntry, stem, ext string) uploadReplacementPlan { matches := FindFilesByStemExt(files, stem, ext) if len(matches) == 0 { @@ -177,8 +183,12 @@ func planUploadReplacement(files []*FileEntry, stem, ext string) uploadReplaceme } keep, remove := pickDuplicateKeeper(matches, false) plan := uploadReplacementPlan{} - if id := FileEntryNumericID(keep); id != 0 { - plan.UpdateID = strconv.FormatInt(id, 10) + if FileEntryExt(keep) == normalizeExt(ext) { + if id := FileEntryNumericID(keep); id != 0 { + plan.UpdateID = strconv.FormatInt(id, 10) + } + } else if id := int(FileEntryNumericID(keep)); id != 0 { + plan.DeleteIDs = append(plan.DeleteIDs, id) } for _, f := range remove { if id := int(FileEntryNumericID(f)); id != 0 { @@ -190,11 +200,11 @@ func planUploadReplacement(files []*FileEntry, stem, ext string) uploadReplaceme // UploadToFolderReplacing upserts localPath into folderID by logical name. // Matching is stem + extension with OnlyOffice's server-side conversion -// accounted for: a local .xls is stored as .xlsx, so a repeated upload -// overwrites the saved document instead of appending a duplicate. When a -// counterpart exists it is updated in place (stable id, no window without the -// file) and any extra duplicates are removed; portals that reject an in-place -// update fall back to delete + fresh upload, which still leaves one file. +// accounted for: a local .xls is stored as .xlsx, so a repeated upload replaces +// the saved document instead of appending a duplicate. An exact-extension +// counterpart is updated in place (stable id, no window without the file); a +// conversion-equivalent one is deleted and uploaded afresh so the server +// converts the body again. Extra duplicates are collapsed either way. func (c *Client) UploadToFolderReplacing(ctx context.Context, folderID, localPath string) (*FileEntry, []int, error) { stem := UploadStemFromLocal(localPath) ext := UploadExtFromLocal(localPath) @@ -203,12 +213,15 @@ func (c *Client) UploadToFolderReplacing(ctx context.Context, folderID, localPat return nil, nil, err } plan := planUploadReplacement(files, stem, ext) - if plan.UpdateID == "" { - ent, err := c.UploadToFolder(ctx, folderID, localPath) - return ent, nil, err - } - ent, err := c.UpdateFile(ctx, plan.UpdateID, localPath) - if err != nil { + + if plan.UpdateID != "" { + ent, err := c.UpdateFile(ctx, plan.UpdateID, localPath) + if err == nil { + deleted, derr := c.deleteReplacementStale(ctx, plan.DeleteIDs) + return ent, deleted, derr + } + // Portal rejected the in-place update: delete + fresh upload still + // leaves one file (server-converted). deleted, derr := c.DeleteFilesByStemExt(ctx, folderID, stem, ext) if derr != nil { return nil, deleted, derr @@ -216,16 +229,21 @@ func (c *Client) UploadToFolderReplacing(ctx context.Context, folderID, localPat ent, uerr := c.UploadToFolder(ctx, folderID, localPath) return ent, deleted, uerr } - deleted, derr := c.deleteReplacementExtras(ctx, plan.DeleteIDs) + + // Conversion-equivalent (or no counterpart): remove the stale files first so + // the fresh upload is converted and exactly one file remains. + deleted, derr := c.deleteReplacementStale(ctx, plan.DeleteIDs) if derr != nil { - return ent, deleted, derr + return nil, deleted, derr } - return ent, deleted, nil + ent, uerr := c.UploadToFolder(ctx, folderID, localPath) + return ent, deleted, uerr } -// deleteReplacementExtras removes redundant duplicate ids collected by -// planUploadReplacement after the surviving file was updated in place. -func (c *Client) deleteReplacementExtras(ctx context.Context, ids []int) ([]int, error) { +// deleteReplacementStale removes the ids collected by planUploadReplacement: +// duplicate files after an in-place update, or every stale counterpart before a +// converted reupload. +func (c *Client) deleteReplacementStale(ctx context.Context, ids []int) ([]int, error) { if len(ids) == 0 { return nil, nil } -- 2.54.0