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" {
+	if plan.UpdateID != "" {
-		t.Fatalf("UpdateID = %q, want 3799 (update the saved .xlsx in place)", plan.UpdateID)
+		t.Fatalf("UpdateID = %q, want empty (do not update across conversion)", plan.UpdateID)
 	}
-	if len(plan.DeleteIDs) != 0 {
+	if len(plan.DeleteIDs) != 1 || plan.DeleteIDs[0] != 3799 {
-		t.Fatalf("DeleteIDs = %v, want none", plan.DeleteIDs)
+		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
+// TestPlanUploadReplacementDeletesConvertedDuplicates: with a converted match
-// upload of the same .xls must find the server-converted .xlsx and update it,
+// every stale copy is deleted (there is no keeper — the fresh upload replaces
-// not append a second file.
+// 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" {
+	if plan.UpdateID != "" {
-		t.Fatalf("second upload plan = %+v, want in-place update of 3799 (no duplicate)", plan)
+		t.Fatalf("second upload plan = %+v, want delete+upload (not UpdateFile)", plan)
 	}
-	if len(plan.DeleteIDs) != 0 {
+	if len(plan.DeleteIDs) != 1 || plan.DeleteIDs[0] != 3799 {
-		t.Fatalf("second upload would delete %v, want none", plan.DeleteIDs)
+		t.Fatalf("second upload DeleteIDs = %v, want [3799]", plan.DeleteIDs)
 	}
 }

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
+	// UpdateID is the existing file to overwrite in place; set only when the
-	// matches the upload stem and (converted) extension.
+	// 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 {
+	if FileEntryExt(keep) == normalizeExt(ext) {
-		plan.UpdateID = strconv.FormatInt(id, 10)
+		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
+// accounted for: a local .xls is stored as .xlsx, so a repeated upload replaces
-// overwrites the saved document instead of appending a duplicate. When a
+// the saved document instead of appending a duplicate. An exact-extension
-// counterpart exists it is updated in place (stable id, no window without the
+// counterpart is updated in place (stable id, no window without the file); a
-// file) and any extra duplicates are removed; portals that reject an in-place
+// conversion-equivalent one is deleted and uploaded afresh so the server
-// update fall back to delete + fresh upload, which still leaves one file.
+// 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)
+	if plan.UpdateID != "" {
-		return ent, nil, err
+		ent, err := c.UpdateFile(ctx, plan.UpdateID, localPath)
-	}
+		if err == nil {
-	ent, err := c.UpdateFile(ctx, plan.UpdateID, localPath)
+			deleted, derr := c.deleteReplacementStale(ctx, plan.DeleteIDs)
-	if err != nil {
+			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
+// deleteReplacementStale removes the ids collected by planUploadReplacement:
-// planUploadReplacement after the surviving file was updated in place.
+// duplicate files after an in-place update, or every stale counterpart before a
-func (c *Client) deleteReplacementExtras(ctx context.Context, ids []int) ([]int, error) {
+// converted reupload.
+func (c *Client) deleteReplacementStale(ctx context.Context, ids []int) ([]int, error) {
 	if len(ids) == 0 {
 		return nil, nil
 	}