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"` }