From 74e72d0e061e076b1a35c7dea3de965cb9578301 Mon Sep 17 00:00:00 2001 From: Andriy Oblivantsev Date: Sun, 27 Sep 2026 15:05:13 +0100 Subject: [PATCH] =?UTF-8?q?fix(dav):=20upsert=20upload=20matches=20server-?= =?UTF-8?q?converted=20ext=20(xls=E2=86=92xlsx)=20(#82)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OnlyOffice converts legacy binary Office uploads (.xls/.doc/.ppt) into OOXML (.xlsx/.docx/.pptx) on the server. UploadToFolderReplacing matched by the exact stem|ext via FindFilesByDedupKey, so a repeated `oo dav upload FOLDER f.xls --replace` never found the stored f.xlsx and appended a second file (live: ids 3799+3887, 3800+3888). - EquivalentUploadExt / FindFilesByStemExt: match by stem with a legacy↔OOXML extension equivalence, so .xls finds the saved .xlsx. - planUploadReplacement: pick the surviving file and the redundant duplicate ids for a replacing upload. - UploadToFolderReplacing updates the existing file in place (UpdateFile, stable id, no delete window), removes extra duplicates, and falls back to conversion-aware delete + upload when the portal rejects the update. - AssertNoFileConflict (--no-replace) uses the same conversion-aware matching so a .xls upload conflicts with an existing .xlsx. - Offline tests cover the matcher, the plan and the repeated-.xls regression; pdf/xlsx behaviour unchanged. --- AGENTS.md | 2 +- README.md | 2 +- cmd/oo/dav.go | 9 +-- cmd/oo/projects_files.go | 8 ++- files.go | 3 +- files_dedupe.go | 79 ++++++++++++++++++++++++++ files_replace_test.go | 120 +++++++++++++++++++++++++++++++++++++++ files_stem.go | 84 ++++++++++++++++++++++++--- 8 files changed, 290 insertions(+), 17 deletions(-) create mode 100644 files_replace_test.go diff --git a/AGENTS.md b/AGENTS.md index 1eb0610..e916480 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ Canonical Go client for OnlyOffice Workspace (Projects + Calendar + CRM) and the - Prefer `ResponseObject` / `postFormObject` / `putFormObject` / `deleteObject` over hand-rolled `json.Unmarshal(responseField(...))` blocks — they exist for DRY, use them. - Domain split is by file, **not** by subpackage. Don't introduce `internal/` or `pkg/*` subpackages inside the library — it flattens the `*Client` call surface for a reason. - CLI commands follow **subject → verb** structure (`oo `), never `oo -`. Add new commands to the existing subject file if one fits; create a new `cmd/oo/.go` for a genuinely new domain. The subject→verb tree in `cmd/oo/main.go` and the README table are documentation — update them with the code. -- **Documents for agents:** prefer Markdown in git; OnlyOffice UI is weak for `.md`/`.txt`. Use `oo docs put-md` (md→docx) and `oo docs put-txt` (txt→docx, preserves line breaks). All upload paths default to **upsert** by `stem|ext` (`--replace`, default true); `--no-replace` fails on conflict; `--allow-duplicate` opts into raw OO append. `oo projects files dedupe PROJECT_ID` reports/removes duplicate stem|ext copies (`--apply`, `--cross`; includes project root folder). +- **Documents for agents:** prefer Markdown in git; OnlyOffice UI is weak for `.md`/`.txt`. Use `oo docs put-md` (md→docx) and `oo docs put-txt` (txt→docx, preserves line breaks). All upload paths default to **upsert** by `stem` with the extension matched conversion-aware (legacy `.xls/.doc/.ppt` ↔ OOXML `.xlsx/.docx/.pptx`, since OnlyOffice converts them on upload), so a repeated `.xls` upload updates the saved `.xlsx` instead of appending a duplicate (`--replace`, default true); `--no-replace` fails on conflict; `--allow-duplicate` opts into raw OO append. `oo projects files dedupe PROJECT_ID` reports/removes duplicate stem|ext copies (`--apply`, `--cross`; includes project root folder). - Every table output goes through `printTable(headers, rows)`; every single-object through `printObject(v)`. Do not `fmt.Println` rows ad-hoc or the `--output json` flag breaks for that command. - No secrets in the repo; use `.env` (gitignored). Commit `.env.example` only. - **No host/client specifics in the tree.** Endpoints, IPs/ports, client mail diff --git a/README.md b/README.md index 2cfc62b..a47eafd 100644 --- a/README.md +++ b/README.md @@ -743,7 +743,7 @@ oo dav ls @root # virtual sections (Documents, Projects, oo dav mkdir 659 "2026 inbox" oo dav ensure-path "Banks/Caixa" # resolve-or-create; idempotent, prints the folder id oo dav ensure-path "Banks/Caixa" --under 659 # start under an explicit folder id -oo dav upload 659 ./historico.xlsx # multipart upload, upsert by stem|ext +oo dav upload 659 ./historico.xlsx # multipart upload, upsert by stem (legacy↔OOXML ext aware) oo dav upload 659 ./a.xlsx ./b.pdf --replace=false # fail on name conflict instead of replacing oo dav move 659 22881 22882 # DEST_FOLDER_ID FILE_ID… oo dav move 659 22881 --folders 670 # move folders along with files diff --git a/cmd/oo/dav.go b/cmd/oo/dav.go index 690d2de..0b15053 100644 --- a/cmd/oo/dav.go +++ b/cmd/oo/dav.go @@ -253,7 +253,7 @@ func davUploadCmd() *cobra.Command { return nil }, } - cmd.Flags().BoolVar(&replace, "replace", true, "replace same stem|ext before upload (default); false = fail if name taken") + cmd.Flags().BoolVar(&replace, "replace", true, "replace the same logical file (stem, legacy↔OOXML ext) before upload (default); false = fail if name taken") return cmd } @@ -271,9 +271,10 @@ type uploader interface { UploadToFolder(ctx context.Context, folderID, localPath string) (*onlyoffice.FileEntry, error) } -// uploadLocal uploads each local file into folderID. With replace a same -// stem|ext file is removed first (the UploadToFolderReplacing upsert); without -// replace a name clash fails with ErrFileExists before anything is sent. +// uploadLocal uploads each local file into folderID. With replace the existing +// same logical file is overwritten in place (the UploadToFolderReplacing +// upsert, which matches the server-converted extension too); without replace a +// name clash fails with ErrFileExists before anything is sent. func uploadLocal(ctx context.Context, up uploader, folderID string, paths []string, replace bool) ([]uploadedFile, error) { results := make([]uploadedFile, 0, len(paths)) for _, path := range paths { diff --git a/cmd/oo/projects_files.go b/cmd/oo/projects_files.go index e44912e..f6c4ccf 100644 --- a/cmd/oo/projects_files.go +++ b/cmd/oo/projects_files.go @@ -113,9 +113,11 @@ func prjFilesUploadCmd() *cobra.Command { var replace, allowDuplicate bool cmd := &cobra.Command{ Use: "upload PROJECT_ID LOCAL_PATH [LOCAL_PATH...]", - Short: "Upload file(s) into the project's Documents folder (upsert by stem|ext)", - Long: `Default: replace an existing file with the same logical name (stem|ext), like cp overwrite. -Pass --no-replace to fail when the name is taken; --allow-duplicate to always create a new file id.`, + Short: "Upload file(s) into the project's Documents folder (upsert by stem, legacy↔OOXML ext)", + Long: `Default: replace an existing file with the same logical name (stem, treating +legacy .xls/.doc/.ppt and their OOXML .xlsx/.docx/.pptx as one file), like cp +overwrite. Pass --no-replace to fail when the name is taken; --allow-duplicate +to always create a new file id.`, Args: cobra.MinimumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { c, err := newOO(cmd) diff --git a/files.go b/files.go index 737612b..c60791d 100644 --- a/files.go +++ b/files.go @@ -237,7 +237,8 @@ func (c *Client) UploadProjectFile(ctx context.Context, projectID, localPath str return decodeResponseFileEntry(raw) } -// UploadProjectFileReplacing upserts by stem|ext in the project Documents folder. +// UploadProjectFileReplacing upserts by stem and (server-converted) extension +// in the project Documents folder. func (c *Client) UploadProjectFileReplacing(ctx context.Context, projectID, localPath string) (*FileEntry, []int, error) { folderID, err := c.projectFolderID(ctx, projectID) if err != nil { diff --git a/files_dedupe.go b/files_dedupe.go index bd3a3bf..808cc7a 100644 --- a/files_dedupe.go +++ b/files_dedupe.go @@ -83,6 +83,85 @@ func UploadExtFromLocal(localPath string) string { return strings.ToLower(ext) } +// legacyToOOXMLExt maps the legacy binary Office extensions OnlyOffice accepts +// on upload to the OOXML extension the server converts them into. +var legacyToOOXMLExt = map[string]string{ + ".xls": ".xlsx", + ".doc": ".docx", + ".ppt": ".pptx", +} + +// normalizeExt lowercases an extension and ensures a leading dot. +func normalizeExt(ext string) string { + ext = strings.ToLower(strings.TrimSpace(ext)) + if ext == "" { + return "" + } + if !strings.HasPrefix(ext, ".") { + ext = "." + ext + } + return ext +} + +// EquivalentUploadExt reports whether two extensions designate the same +// document once the server-side conversion is taken into account: equal +// extensions, or a legacy binary Office format and its OOXML equivalent +// (.xls/.xlsx, .doc/.docx, .ppt/.pptx). Empty extensions only match each other. +func EquivalentUploadExt(a, b string) bool { + na, nb := normalizeExt(a), normalizeExt(b) + if na == nb { + return true + } + if na == "" || nb == "" { + return false + } + return legacyToOOXMLExt[na] == nb || legacyToOOXMLExt[nb] == na +} + +// FindFilesByStemExt returns folder files matching stem and a same-or-converted +// extension (see EquivalentUploadExt). Unlike FindFilesByDedupKey, foo.xls and +// foo.xlsx are one logical file, so a replacing upload after OnlyOffice's +// legacy→OOXML conversion finds the saved file instead of appending a copy. +func FindFilesByStemExt(files []*FileEntry, stem, ext string) []*FileEntry { + stem = strings.TrimSpace(stem) + if stem == "" { + return nil + } + var out []*FileEntry + for _, f := range files { + if FileEntryStem(f) != stem { + continue + } + if EquivalentUploadExt(FileEntryExt(f), ext) { + out = append(out, f) + } + } + return out +} + +// DeleteFilesByStemExt removes every folder file matching stem and a +// same-or-converted extension (legacy ↔ OOXML). +func (c *Client) DeleteFilesByStemExt(ctx context.Context, folderID, stem, ext string) ([]int, error) { + files, err := c.FolderFiles(ctx, folderID) + if err != nil { + return nil, err + } + matches := FindFilesByStemExt(files, stem, ext) + ids := make([]int, 0, len(matches)) + for _, f := range matches { + if n := int(FileEntryNumericID(f)); n != 0 { + ids = append(ids, n) + } + } + if len(ids) == 0 { + return nil, nil + } + if err := c.DeleteFiles(ctx, ids); err != nil { + return nil, err + } + return ids, nil +} + // IsTrashFolderTitle reports staging/trash folders (e.g. _trash-md). func IsTrashFolderTitle(title string) bool { t := strings.ToLower(strings.TrimSpace(title)) diff --git a/files_replace_test.go b/files_replace_test.go new file mode 100644 index 0000000..80019e9 --- /dev/null +++ b/files_replace_test.go @@ -0,0 +1,120 @@ +package onlyoffice + +import ( + "testing" + "time" +) + +func TestEquivalentUploadExt(t *testing.T) { + tests := []struct { + a, b string + want bool + }{ + {".xls", ".xlsx", true}, + {".XLS", ".xlsx", true}, + {"xls", "xlsx", true}, + {".doc", ".docx", true}, + {".ppt", ".pptx", true}, + {".pdf", ".pdf", true}, + {"", "", true}, + {".xls", ".docx", false}, + {".xlsx", "", false}, + {".csv", ".xlsx", false}, + } + for _, tc := range tests { + if got := EquivalentUploadExt(tc.a, tc.b); got != tc.want { + t.Errorf("EquivalentUploadExt(%q,%q)=%v want %v", tc.a, tc.b, got, tc.want) + } + if got := EquivalentUploadExt(tc.b, tc.a); got != tc.want { + t.Errorf("EquivalentUploadExt(%q,%q)=%v want %v (symmetric)", tc.b, tc.a, got, tc.want) + } + } +} + +func TestFindFilesByStemExtMatchesConvertedXLS(t *testing.T) { + xlsx := &FileEntry{ID: jsonNum("3799"), Title: strPtr("ES29-extracto.xlsx"), FileExst: strPtr(".xlsx")} + pdf := &FileEntry{ID: jsonNum("5"), Title: strPtr("ES29-extracto.pdf"), FileExst: strPtr(".pdf")} + other := &FileEntry{ID: jsonNum("3887"), Title: strPtr("ES87-extracto.xlsx"), FileExst: strPtr(".xlsx")} + files := []*FileEntry{xlsx, pdf, other} + + got := FindFilesByStemExt(files, "ES29-extracto", ".xls") + if len(got) != 1 || got[0] != xlsx { + t.Fatalf("converted .xls match = %+v, want the saved .xlsx only", got) + } +} + +func TestFindFilesByStemExtKeepsExactMatch(t *testing.T) { + xlsx := &FileEntry{ID: jsonNum("3799"), Title: strPtr("foo.xlsx"), FileExst: strPtr(".xlsx")} + pdf := &FileEntry{ID: jsonNum("5"), Title: strPtr("foo.pdf"), FileExst: strPtr(".pdf")} + files := []*FileEntry{xlsx, pdf} + + if got := FindFilesByStemExt(files, "foo", ".xlsx"); len(got) != 1 || got[0] != xlsx { + t.Fatalf("exact .xlsx match = %+v, want only xlsx", got) + } + if got := FindFilesByStemExt(files, "foo", ".pdf"); len(got) != 1 || got[0] != pdf { + t.Fatalf("exact .pdf match = %+v, want only pdf", got) + } + if got := FindFilesByStemExt(files, "foo", ".docx"); len(got) != 0 { + t.Fatalf("unrelated ext matched %+v, want none", got) + } +} + +func TestFindFilesByStemExtEmptyStem(t *testing.T) { + f := &FileEntry{ID: jsonNum("1"), Title: strPtr("foo.xlsx"), FileExst: strPtr(".xlsx")} + if got := FindFilesByStemExt([]*FileEntry{f}, "", ".xlsx"); len(got) != 0 { + t.Fatalf("empty stem matched %+v, want none", got) + } +} + +func TestPlanUploadReplacementFreshUpload(t *testing.T) { + plan := planUploadReplacement(nil, "ES29-extracto", ".xls") + if plan.UpdateID != "" || len(plan.DeleteIDs) != 0 { + t.Fatalf("empty folder plan = %+v, want a fresh upload", plan) + } +} + +func TestPlanUploadReplacementUpdatesConvertedFile(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 len(plan.DeleteIDs) != 0 { + t.Fatalf("DeleteIDs = %v, want none", plan.DeleteIDs) + } +} + +func TestPlanUploadReplacementCollapsesDuplicates(t *testing.T) { + older := time.Date(2026, 9, 1, 10, 0, 0, 0, time.UTC) + 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") + if plan.UpdateID != "3887" { + t.Fatalf("UpdateID = %q, want the newest duplicate 3887", plan.UpdateID) + } + if len(plan.DeleteIDs) != 1 || plan.DeleteIDs[0] != 3799 { + t.Fatalf("DeleteIDs = %v, want [3799]", plan.DeleteIDs) + } +} + +// 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. +func TestPlanUploadReplacementRepeatedXLS(t *testing.T) { + const stem = "ES29-extracto" + + // First upload: nothing in the folder. + if plan := planUploadReplacement(nil, stem, ".xls"); plan.UpdateID != "" { + t.Fatalf("first upload plan = %+v, want create", plan) + } + // OnlyOffice converts .xls -> .xlsx on upload; second upload must match 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 len(plan.DeleteIDs) != 0 { + t.Fatalf("second upload would delete %v, want none", plan.DeleteIDs) + } +} diff --git a/files_stem.go b/files_stem.go index 02b2b53..dbccc3b 100644 --- a/files_stem.go +++ b/files_stem.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "path/filepath" + "strconv" "strings" ) @@ -121,7 +122,9 @@ func (c *Client) DeleteFilesByStem(ctx context.Context, folderID, stem string) ( return ids, nil } -// AssertNoFileConflict reports ErrFileExists when localPath stem|ext is already in folderID. +// AssertNoFileConflict reports ErrFileExists when localPath logical name is +// already in folderID. Matching is conversion-aware, so a .xls upload also +// conflicts with an existing .xlsx and does not create a hidden duplicate. func (c *Client) AssertNoFileConflict(ctx context.Context, folderID, localPath string) error { files, err := c.FolderFiles(ctx, folderID) if err != nil { @@ -129,7 +132,7 @@ func (c *Client) AssertNoFileConflict(ctx context.Context, folderID, localPath s } stem := UploadStemFromLocal(localPath) ext := UploadExtFromLocal(localPath) - matches := FindFilesByDedupKey(files, stem, ext) + matches := FindFilesByStemExt(files, stem, ext) if len(matches) == 0 { return nil } @@ -153,14 +156,81 @@ func (c *Client) UploadProjectFileNoClobber(ctx context.Context, projectID, loca return c.UploadProjectFile(ctx, projectID, localPath) } -// UploadToFolderReplacing deletes same stem+ext files then uploads localPath. +// 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 string + // DeleteIDs are redundant duplicate ids to remove after the update. + 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 +// OnlyOffice performs on upload, so a repeated upload of f.xls finds the saved +// f.xlsx instead of creating a second file. +func planUploadReplacement(files []*FileEntry, stem, ext string) uploadReplacementPlan { + matches := FindFilesByStemExt(files, stem, ext) + if len(matches) == 0 { + return uploadReplacementPlan{} + } + keep, remove := pickDuplicateKeeper(matches, false) + plan := uploadReplacementPlan{} + if id := FileEntryNumericID(keep); id != 0 { + plan.UpdateID = strconv.FormatInt(id, 10) + } + for _, f := range remove { + if id := int(FileEntryNumericID(f)); id != 0 { + plan.DeleteIDs = append(plan.DeleteIDs, id) + } + } + return plan +} + +// 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. func (c *Client) UploadToFolderReplacing(ctx context.Context, folderID, localPath string) (*FileEntry, []int, error) { stem := UploadStemFromLocal(localPath) ext := UploadExtFromLocal(localPath) - deleted, err := c.DeleteFilesByDedupKey(ctx, folderID, stem, ext) + files, err := c.FolderFiles(ctx, folderID) if err != nil { - return nil, deleted, err + return nil, nil, err } - ent, err := c.UploadToFolder(ctx, folderID, localPath) - return ent, deleted, 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 { + deleted, derr := c.DeleteFilesByStemExt(ctx, folderID, stem, ext) + if derr != nil { + return nil, deleted, derr + } + ent, uerr := c.UploadToFolder(ctx, folderID, localPath) + return ent, deleted, uerr + } + deleted, derr := c.deleteReplacementExtras(ctx, plan.DeleteIDs) + if derr != nil { + return ent, deleted, derr + } + return ent, deleted, nil +} + +// 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) { + if len(ids) == 0 { + return nil, nil + } + if err := c.DeleteFiles(ctx, ids); err != nil { + return ids, err + } + return ids, nil }