From b10914c722444afb79228accd4e5e3d80c07647e Mon Sep 17 00:00:00 2001 From: Amen <2668333+amenophis1er@users.noreply.github.com> Date: Sun, 13 Sep 2026 13:19:53 -0400 Subject: [PATCH] fix(backup): store media under media// so same-named files cannot overwrite each other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'trokky backup' wrote every media file to the archive as media/. A zip tolerates duplicate entry names, so the archive appeared complete, but 'trokky restore' built a filename -> entry map where the last entry won and uploaded that one for every media id sharing the name. On the Cour des Comptes instance 94 of 662 filenames are shared between media ids. 86 are byte-identical copies, harmless; 8 names cover 84 ids that are different photos (Photo-de-famille.jpg x17, Vue-partielle-des-participants.jpg x24). A restore reported 942/942 with 0 failed and ~76 photos came back as the wrong image. Entries are now scoped by media id, and the manifest records each item's archivePath. Restore resolves the manifest path first, then the id-scoped layout, then the flat legacy layout — and when it has to fall back to a legacy entry that several ids share, it warns that the bytes may be wrong and that a fresh backup is needed. Old archives with collisions cannot be repaired: the overwritten bytes were never written. --- cmd/backup.go | 13 ++-- cmd/backup_test.go | 119 +++++++++++++++++++++++++++++++++ cmd/restore.go | 43 ++++++++++-- cmd/restore_test.go | 141 +++++++++++++++++++++++++++++++++++++++ internal/backup/types.go | 47 +++++++++---- 5 files changed, 338 insertions(+), 25 deletions(-) create mode 100644 cmd/backup_test.go diff --git a/cmd/backup.go b/cmd/backup.go index e9f7424..8078da1 100644 --- a/cmd/backup.go +++ b/cmd/backup.go @@ -179,8 +179,10 @@ Example: filename = item.ID } - // Stream directly to zip - w, err := zw.Create("media/" + filename) + // Stream directly to zip, under an id-scoped path so files that + // share an original filename cannot overwrite each other. + archivePath := backup.MediaArchivePath(item.ID, filename) + w, err := zw.Create(archivePath) if err != nil { resp.Body.Close() continue @@ -194,9 +196,10 @@ Example: } mediaIndex[item.ID] = backup.MediaFileInfo{ - Filename: filename, - MimeType: item.MimeType, - Size: written, + Filename: filename, + MimeType: item.MimeType, + ArchivePath: archivePath, + Size: written, } mediaCount++ } diff --git a/cmd/backup_test.go b/cmd/backup_test.go new file mode 100644 index 0000000..1854f80 --- /dev/null +++ b/cmd/backup_test.go @@ -0,0 +1,119 @@ +package cmd + +import ( + "archive/zip" + "bytes" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/trokky/cli/internal/backup" +) + +func runBackup(t *testing.T, args ...string) (string, string, error) { + t.Helper() + t.Setenv("HOME", t.TempDir()) + t.Setenv("TROKKY_URL", "") + t.Setenv("TROKKY_TOKEN", "") + t.Setenv("TROKKY_INSTANCE", "") + for name, def := range map[string]string{"collections": "", "skip-media": "false", "description": "", "output": ""} { + if err := backupCmd.Flags().Set(name, def); err != nil { + t.Fatal(err) + } + } + var stdout, stderr bytes.Buffer + rootCmd.SetOut(&stdout) + rootCmd.SetErr(&stderr) + defer func() { + rootCmd.SetOut(nil) + rootCmd.SetErr(nil) + }() + rootCmd.SetArgs(append([]string{"backup"}, args...)) + var err error + rawOut, rawErr := captureProcessOutput(t, func() { + err = rootCmd.Execute() + }) + return stdout.String() + rawOut, stderr.String() + rawErr, err +} + +// TestBackupKeepsSameNamedMediaApart covers the archive layout: two media items +// whose original filename is identical must land in distinct zip entries with +// their own bytes, and the manifest must say where each one is. +func TestBackupKeepsSameNamedMediaApart(t *testing.T) { + files := map[string]string{"media-1": "bytesA", "media-2": "bytesB"} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + p := apiPath(r) + switch { + case p == "/collections": + w.Header().Set("Content-Type", "application/json") + io.WriteString(w, testCollectionsResponse) + case strings.HasPrefix(p, "/collections/posts"): + w.Header().Set("Content-Type", "application/json") + io.WriteString(w, `{"success":true,"data":{"documents":[{"id":"doc-1","title":"One"}]}}`) + case p == "/media": + w.Header().Set("Content-Type", "application/json") + io.WriteString(w, `{"success":true,"data":[{"id":"media-1","filename":"photo.png","mimeType":"image/png","size":6},{"id":"media-2","filename":"photo.png","mimeType":"image/png","size":6}]}`) + case strings.HasPrefix(p, "/media/") && strings.HasSuffix(p, "/file"): + id := strings.TrimSuffix(strings.TrimPrefix(p, "/media/"), "/file") + io.WriteString(w, files[id]) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + out := filepath.Join(t.TempDir(), "b.zip") + stdout, stderr, err := runBackup(t, "--output", out, "--url", server.URL+"/api", "--token", "t") + if err != nil { + t.Fatalf("backup failed: %v\nstdout:\n%s\nstderr:\n%s", err, stdout, stderr) + } + + data, err := os.ReadFile(out) + if err != nil { + t.Fatal(err) + } + zr, err := zip.NewReader(bytes.NewReader(data), int64(len(data))) + if err != nil { + t.Fatal(err) + } + + got := map[string]string{} + var manifest backup.BackupManifest + for _, f := range zr.File { + rc, err := f.Open() + if err != nil { + t.Fatal(err) + } + b, _ := io.ReadAll(rc) + rc.Close() + if f.Name == "manifest.json" { + if err := json.Unmarshal(b, &manifest); err != nil { + t.Fatal(err) + } + continue + } + if strings.HasPrefix(f.Name, "media/") { + got[f.Name] = string(b) + } + } + + want := map[string]string{"media/media-1/photo.png": "bytesA", "media/media-2/photo.png": "bytesB"} + for name, body := range want { + if got[name] != body { + t.Errorf("entry %s = %q, want %q (all media entries: %v)", name, got[name], body, got) + } + } + if len(got) != 2 { + t.Errorf("expected exactly 2 media entries, got %d: %v", len(got), got) + } + for id, path := range map[string]string{"media-1": "media/media-1/photo.png", "media-2": "media/media-2/photo.png"} { + if manifest.MediaIndex[id].ArchivePath != path { + t.Errorf("manifest archivePath for %s = %q, want %q", id, manifest.MediaIndex[id].ArchivePath, path) + } + } +} diff --git a/cmd/restore.go b/cmd/restore.go index fc891da..c5c32c7 100644 --- a/cmd/restore.go +++ b/cmd/restore.go @@ -448,26 +448,36 @@ Example: } else { fmt.Fprint(out, " Restoring media... ") - // Build zip file lookup for media + // Build zip file lookup for media, keyed by full entry name. Current + // archives store each file under media//; older ones + // used the flat media/, where same-named files overwrote + // each other and only one survives. mediaZipFiles := make(map[string]*zip.File) + legacyDuplicates := make(map[string]int) for _, f := range zr.File { - if strings.HasPrefix(f.Name, "media/") { - // Strip "media/" prefix to get filename - name := f.Name[6:] - mediaZipFiles[name] = f + if !strings.HasPrefix(f.Name, "media/") { + continue + } + if _, seen := mediaZipFiles[f.Name]; seen { + legacyDuplicates[f.Name]++ } + mediaZipFiles[f.Name] = f } mediaTotal := len(manifest.MediaIndex) mediaFailed := 0 for oldID, mediaInfo := range manifest.MediaIndex { - zipFile, ok := mediaZipFiles[mediaInfo.Filename] - if !ok { + entry, legacy := findMediaEntry(mediaZipFiles, oldID, mediaInfo) + if entry == "" { fmt.Fprintf(errOut, "\n Warning: media file %s not found in archive\n", mediaInfo.Filename) mediaFailed++ continue } + if legacy && legacyDuplicates[entry] > 0 { + fmt.Fprintf(errOut, "\n Warning: %s shares its filename with %d other file(s) in this old-format archive; only one set of bytes survived, so %s may be restored with the wrong content. Take a fresh backup with this CLI version.\n", oldID, legacyDuplicates[entry], oldID) + } + zipFile := mediaZipFiles[entry] // Retry up to 3 times with backoff var result map[string]interface{} @@ -728,3 +738,22 @@ func init() { restoreCmd.Flags().Bool("dry-run", false, "preview changes without applying") rootCmd.AddCommand(restoreCmd) } + +// findMediaEntry resolves the zip entry holding a media item's bytes. It prefers +// the path the manifest recorded, then the id-scoped layout, and finally the flat +// legacy layout. The second return value reports whether the legacy layout was +// used, because same-named files in that layout have already collided. +func findMediaEntry(entries map[string]*zip.File, id string, info backup.MediaFileInfo) (string, bool) { + if info.ArchivePath != "" { + if _, ok := entries[info.ArchivePath]; ok { + return info.ArchivePath, false + } + } + if p := backup.MediaArchivePath(id, info.Filename); entries[p] != nil { + return p, false + } + if p := backup.LegacyMediaArchivePath(info.Filename); entries[p] != nil { + return p, true + } + return "", false +} diff --git a/cmd/restore_test.go b/cmd/restore_test.go index b1e35f7..ea04168 100644 --- a/cmd/restore_test.go +++ b/cmd/restore_test.go @@ -5,6 +5,7 @@ import ( "bytes" "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -465,3 +466,143 @@ func TestIsPermissionDenied(t *testing.T) { }) } } + +// writeCollidingBackupZip builds an archive holding two media items that share +// an original filename but carry different bytes. With idScoped the entries use +// the current media// layout; otherwise the flat legacy layout, +// where the second entry overwrites the first inside the zip reader's lookup. +func writeCollidingBackupZip(t *testing.T, idScoped bool) string { + t.Helper() + + entryFor := func(id string) string { + if idScoped { + return "media/" + id + "/photo.png" + } + return "media/photo.png" + } + manifest := map[string]interface{}{ + "version": "2.0", + "timestamp": "2026-01-01T00:00:00Z", + "source": map[string]interface{}{"url": "https://cms.example.com/api"}, + "schemas": []map[string]interface{}{{"name": "posts", "title": "Posts", "fields": []map[string]interface{}{{"name": "title", "type": "string"}}}}, + "dependencyGraph": map[string][]string{}, + "restoreOrder": []string{"posts"}, + "mediaIndex": map[string]interface{}{ + "media-old-a": map[string]interface{}{"filename": "photo.png", "mimeType": "image/png", "size": 5}, + "media-old-b": map[string]interface{}{"filename": "photo.png", "mimeType": "image/png", "size": 5}, + }, + "statistics": map[string]interface{}{"totalDocuments": 1, "totalMedia": 2}, + } + manifestJSON, err := json.Marshal(manifest) + if err != nil { + t.Fatal(err) + } + docJSON, err := json.Marshal(map[string]interface{}{"id": "doc-old-1", "title": "Example post"}) + if err != nil { + t.Fatal(err) + } + + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + for _, e := range []struct { + name string + data []byte + }{ + {"manifest.json", manifestJSON}, + {"collections/posts/doc-old-1.json", docJSON}, + {entryFor("media-old-a"), []byte("bytesA")}, + {entryFor("media-old-b"), []byte("bytesB")}, + } { + w, err := zw.Create(e.name) + if err != nil { + t.Fatal(err) + } + if _, err := w.Write(e.data); err != nil { + t.Fatal(err) + } + } + if err := zw.Close(); err != nil { + t.Fatal(err) + } + path := filepath.Join(t.TempDir(), "backup.zip") + if err := os.WriteFile(path, buf.Bytes(), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +// uploadedBodies returns the multipart bodies of every /media/upload request. +func uploadedBodies(rec *recorder) []string { + var out []string + for _, r := range rec.all() { + if r.Path == "/media/upload" { + out = append(out, r.Body) + } + } + return out +} + +func newRestoreServer(rec *recorder) *httptest.Server { + uploads := 0 + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + rec.record(r) + w.Header().Set("Content-Type", "application/json") + switch { + case r.Method == http.MethodGet && apiPath(r) == "/collections": + io.WriteString(w, testCollectionsResponse) + case r.Method == http.MethodPost && apiPath(r) == "/collections/posts": + io.WriteString(w, `{"success":true,"data":{"id":"`+testNewDocID+`"}}`) + case r.Method == http.MethodPost && apiPath(r) == "/media/upload": + uploads++ + fmt.Fprintf(w, `{"success":true,"data":{"files":[{"id":"media-new-%d"}]}}`, uploads) + default: + io.WriteString(w, `{"success":true,"data":{}}`) + } + })) +} + +func TestRestoreUploadsEachMediaItemsOwnBytesWhenFilenamesCollide(t *testing.T) { + rec := &recorder{} + server := newRestoreServer(rec) + defer server.Close() + + stdout, stderr, err := runRestore(t, "--input", writeCollidingBackupZip(t, true), "--url", server.URL+"/api", "--token", "t", "-q") + if err != nil { + t.Fatalf("restore failed: %v\nstdout:\n%s\nstderr:\n%s", err, stdout, stderr) + } + + bodies := uploadedBodies(rec) + if len(bodies) != 2 { + t.Fatalf("expected 2 uploads, got %d", len(bodies)) + } + joined := strings.Join(bodies, "\n") + for _, want := range []string{"bytesA", "bytesB"} { + if !strings.Contains(joined, want) { + t.Errorf("no upload carried %q; the two same-named files collapsed into one:\n%s", want, joined) + } + } + if strings.Contains(stderr, "shares its filename") { + t.Errorf("id-scoped archive must not trigger the legacy collision warning:\n%s", stderr) + } +} + +func TestRestoreReadsLegacyFlatLayoutAndWarnsOnCollisions(t *testing.T) { + rec := &recorder{} + server := newRestoreServer(rec) + defer server.Close() + + stdout, stderr, err := runRestore(t, "--input", writeCollidingBackupZip(t, false), "--url", server.URL+"/api", "--token", "t", "-q") + if err != nil { + t.Fatalf("restore failed: %v\nstdout:\n%s\nstderr:\n%s", err, stdout, stderr) + } + + if n := len(uploadedBodies(rec)); n != 2 { + t.Fatalf("legacy archives must still restore every indexed item; got %d uploads", n) + } + if !strings.Contains(stderr, "shares its filename with 1 other file(s)") { + t.Errorf("expected a collision warning for the old-format archive, got:\n%s", stderr) + } + if !strings.Contains(stderr, "Take a fresh backup") { + t.Errorf("warning should tell the user how to get an uncorrupted archive:\n%s", stderr) + } +} diff --git a/internal/backup/types.go b/internal/backup/types.go index 873dc6a..0a0df27 100644 --- a/internal/backup/types.go +++ b/internal/backup/types.go @@ -61,15 +61,15 @@ func (s *SchemaDefinition) UnmarshalFields() { // FieldDefinition describes a field in a schema. type FieldDefinition struct { - Name string `json:"name"` - Type string `json:"type"` - Title string `json:"title,omitempty"` - Required bool `json:"required,omitempty"` - To json.RawMessage `json:"to,omitempty"` // string or []string for references - Of *FieldDefinition `json:"of,omitempty"` // for array fields - Fields []FieldDefinition `json:"fields,omitempty"` // for object fields - Hidden bool `json:"hidden,omitempty"` - ReadOnly bool `json:"readOnly,omitempty"` + Name string `json:"name"` + Type string `json:"type"` + Title string `json:"title,omitempty"` + Required bool `json:"required,omitempty"` + To json.RawMessage `json:"to,omitempty"` // string or []string for references + Of *FieldDefinition `json:"of,omitempty"` // for array fields + Fields []FieldDefinition `json:"fields,omitempty"` // for object fields + Hidden bool `json:"hidden,omitempty"` + ReadOnly bool `json:"readOnly,omitempty"` } // FieldTargets returns the reference target collections for a field. @@ -99,12 +99,33 @@ type MediaFileInfo struct { MimeType string `json:"mimeType"` Size int64 `json:"size"` Metadata map[string]interface{} `json:"metadata,omitempty"` + // ArchivePath is where the bytes live inside the zip. Archives written + // before this field existed stored every file as "media/", which + // let two media items with the same original filename overwrite each other. + ArchivePath string `json:"archivePath,omitempty"` +} + +// MediaArchivePath is the zip entry for a media item: scoped by the item's id so +// two uploads that share an original filename — "Photo-de-famille.jpg" appears +// 17 times in one production instance — never collide inside the archive. +func MediaArchivePath(id, filename string) string { + if filename == "" { + filename = id + } + return "media/" + id + "/" + filename +} + +// LegacyMediaArchivePath is the flat layout older archives used. A restore +// still reads it, but same-named files in such an archive have already +// overwritten each other and cannot be told apart. +func LegacyMediaArchivePath(filename string) string { + return "media/" + filename } // BackupStatistics holds summary stats for a backup. type BackupStatistics struct { - TotalDocuments int `json:"totalDocuments"` - TotalMedia int `json:"totalMedia"` - Collections map[string]int `json:"collections"` - BackupSizeBytes int64 `json:"backupSizeBytes"` + TotalDocuments int `json:"totalDocuments"` + TotalMedia int `json:"totalMedia"` + Collections map[string]int `json:"collections"` + BackupSizeBytes int64 `json:"backupSizeBytes"` }