Merge pull request #919 from go-kivik/deleteAttachmentsProgress

Delete attachments progress

x/sqlite/deleteattachment.go

@@ -14,8 +14,6 @@ package sqlite
 
 import (
  "context"
- "database/sql"
- "errors"
  "net/http"
 
  "github.com/go-kivik/kivik/v4/driver"
@@ -34,24 +32,57 @@ func (d *db) DeleteAttachment(ctx context.Context, docID, filename string, optio
  defer tx.Rollback()
 
  data := &docData{
- ID: docID,
+ ID: docID,
+ Attachments: map[string]attachment{},
  }
 
- curRev, hash, err := d.winningRev(ctx, tx, docID)
- switch {
- case errors.Is(err, sql.ErrNoRows):
- return "", &internal.Error{Status: http.StatusNotFound, Message: "document not found"}
- case err != nil:
+ curRev, err := parseRev(opts.rev())
+ if err != nil {
  return "", err
- default:
- data.MD5sum = hash
  }
 
- if rev := opts.rev(); rev != "" && rev != curRev.String() {
- return "", &internal.Error{Status: http.StatusConflict, Message: "conflict"}
+ data.MD5sum, err = d.isLeafRev(ctx, tx, docID, curRev.rev, curRev.id)
+ if err != nil {
+ return "", err
  }
 
- data.RemovedAttachments = []string{filename}
+ // Read list of current attachments, then remove the requested one
+
+ rows, err := tx.QueryContext(ctx, d.query(`
+ SELECT att.filename
+ FROM {{ .Attachments }} AS att
+ JOIN {{ .AttachmentsBridge }} AS bridge ON att.pk = bridge.pk
+ WHERE bridge.id = $1
+ AND bridge.rev = $2
+ AND bridge.rev_id = $3
+ `), docID, curRev.rev, curRev.id)
+ if err != nil {
+ return "", err
+ }
+ defer rows.Close()
+
+ var attachments []string
+ for rows.Next() {
+ var filename string
+ if err := rows.Scan(&filename); err != nil {
+ return "", err
+ }
+ attachments = append(attachments, filename)
+ }
+ if err := rows.Err(); err != nil {
+ return "", err
+ }
+
+ if !attachmentsContains(attachments, filename) {
+ return "", &internal.Error{Status: http.StatusNotFound, Message: "attachment not found"}
+ }
+
+ for _, att := range attachments {
+ if att == filename {
+ continue
+ }
+ data.Attachments[att] = attachment{Stub: true}
+ }
 
  r, err := d.createRev(ctx, tx, data, curRev)
  if err != nil {
@@ -60,3 +91,12 @@ func (d *db) DeleteAttachment(ctx context.Context, docID, filename string, optio
 
  return r.String(), tx.Commit()
 }
+
+func attachmentsContains(attachments []string, filename string) bool {
+ for _, att := range attachments {
+ if att == filename {
+ return true
+ }
+ }
+ return false
+}

x/sqlite/deleteattachment_test.go

@@ -72,7 +72,7 @@ func TestDBDeleteAttachment(t *testing.T) {
  docID: "foo",
  filename: "foo.txt",
  options: kivik.Rev("1-wrong"),
- wantErr: "conflict",
+ wantErr: "Document update conflict",
  wantStatus: http.StatusConflict,
  }
  })
@@ -116,11 +116,161 @@ func TestDBDeleteAttachment(t *testing.T) {
  },
  }
  })
+ tests.Add("when attempting to delete an attachment that does not exist, a 404 is returned", func(t *testing.T) interface{} {
+ d := newDB(t)
+ rev := d.tPut("foo", map[string]interface{}{
+ "cat": "meow",
+ })
+
+ return test{
+ db: d,
+ docID: "foo",
+ filename: "foo.txt",
+ options: kivik.Rev(rev),
+ wantErr: "attachment not found",
+ wantStatus: http.StatusNotFound,
+ wantRevs: []leaf{}, // skip checking leaves
+ }
+ })
+ tests.Add("when attempting to delete an attachment, unspecified attachments are unaltered", func(t *testing.T) interface{} {
+ d := newDB(t)
+ rev := d.tPut("foo", map[string]interface{}{
+ "cat": "meow",
+ "_attachments": map[string]interface{}{
+ "foo.txt": map[string]interface{}{
+ "content_type": "text/plain",
+ "data": "VGhpcyBpcyBhIGJhc2U2NCBlbmNvZGluZw==",
+ },
+ "bar.txt": map[string]interface{}{
+ "content_type": "text/plain",
+ "data": "VGhpcyBpcyBhIGJhc2U2NCBlbmNvZGluZw==",
+ },
+ },
+ })
+
+ return test{
+ db: d,
+ docID: "foo",
+ filename: "foo.txt",
+ options: kivik.Rev(rev),
+ wantRevs: []leaf{
+ {
+ ID: "foo",
+ Rev: 1,
+ },
+ {
+ ID: "foo",
+ Rev: 2,
+ ParentRev: &[]int{1}[0],
+ },
+ },
+ wantAttachments: []attachmentRow{
+ {
+ DocID: "foo",
+ RevPos: 1,
+ Rev: 1,
+ Filename: "bar.txt",
+ Digest: "md5-TmfHxaRgUrE9l3tkAn4s0Q==",
+ },
+ {
+ DocID: "foo",
+ RevPos: 1,
+ Rev: 1,
+ Filename: "foo.txt",
+ Digest: "md5-TmfHxaRgUrE9l3tkAn4s0Q==",
+ },
+ {
+ DocID: "foo",
+ RevPos: 1,
+ Rev: 2,
+ Filename: "bar.txt",
+ Digest: "md5-TmfHxaRgUrE9l3tkAn4s0Q==",
+ },
+ },
+ }
+ })
+ tests.Add("deleting attachments on non-winning leaf only alters that revision branch", func(t *testing.T) interface{} {
+ d := newDB(t)
+ rev1 := d.tPut("foo", map[string]interface{}{
+ "cat": "meow",
+ })
+ rev2 := d.tPut("foo", map[string]interface{}{
+ "dog": "woof",
+ "_attachments": map[string]interface{}{
+ "foo.txt": map[string]interface{}{
+ "content_type": "text/plain",
+ "data": "VGhpcyBpcyBhIGJhc2U2NCBlbmNvZGluZw==",
+ },
+ },
+ }, kivik.Rev(rev1))
+
+ r1, _ := parseRev(rev1)
+ r2, _ := parseRev(rev2)
+
+ // Create a conflict
+ _ = d.tPut("foo", map[string]interface{}{
+ "pig": "oink",
+ "_revisions": map[string]interface{}{
+ "start": 3,
+ "ids": []string{"abc", "def", r1.id},
+ },
+ }, kivik.Params(map[string]interface{}{"new_edits": false}))
+
+ return test{
+ db: d,
+ docID: "foo",
+ filename: "foo.txt",
+ options: kivik.Rev(rev2),
+ wantRev: "3-.*",
+ wantRevs: []leaf{
+ {
+ ID: "foo",
+ Rev: 1,
+ RevID: r1.id,
+ },
+ {
+ ID: "foo",
+ Rev: 2,
+ RevID: "def",
+ ParentRev: &[]int{1}[0],
+ ParentRevID: &r1.id,
+ },
+ {
+ ID: "foo",
+ Rev: 2,
+ RevID: r2.id,
+ ParentRev: &[]int{1}[0],
+ ParentRevID: &r1.id,
+ },
+ {
+ ID: "foo",
+ Rev: 3,
+ ParentRev: &[]int{2}[0],
+ ParentRevID: &r2.id,
+ },
+ {
+ ID: "foo",
+ Rev: 3,
+ RevID: "abc",
+ ParentRev: &[]int{2}[0],
+ ParentRevID: &[]string{"def"}[0],
+ },
+ },
+ wantAttachments: []attachmentRow{
+ {
+ DocID: "foo",
+ RevPos: 2,
+ Rev: 2,
+ Filename: "foo.txt",
+ Digest: "md5-TmfHxaRgUrE9l3tkAn4s0Q==",
+ },
+ },
+ }
+ })
 
  /*
  TODO:
  - db missing => db not found
- - file does not exist => file not found
  */
 
  tests.Run(t, func(t *testing.T, tt test) {
@@ -146,22 +296,26 @@ func TestDBDeleteAttachment(t *testing.T) {
  if !regexp.MustCompile(tt.wantRev).MatchString(rev) {
  t.Errorf("Unexpected rev: %s, want %s", rev, tt.wantRev)
  }
- if len(tt.wantRevs) == 0 {
+ switch {
+ case tt.wantRevs == nil:
  t.Errorf("No leaves to check")
- }
- leaves := readRevisions(t, dbc.underlying(), tt.docID)
- for i, r := range tt.wantRevs {
- // allow tests to omit RevID
- if r.RevID == "" {
- leaves[i].RevID = ""
+ case len(tt.wantRevs) == 0:
+ // Do nothing
+ default:
+ leaves := readRevisions(t, dbc.underlying(), tt.docID)
+ for i, r := range tt.wantRevs {
+ // allow tests to omit RevID
+ if r.RevID == "" {
+ leaves[i].RevID = ""
+ }
+ if r.ParentRevID == nil {
+ leaves[i].ParentRevID = nil
+ }
  }
- if r.ParentRevID == nil {
- leaves[i].ParentRevID = nil
+ if d := cmp.Diff(tt.wantRevs, leaves); d != "" {
+ t.Errorf("Unexpected leaves: %s", d)
  }
  }
- if d := cmp.Diff(tt.wantRevs, leaves); d != "" {
- t.Errorf("Unexpected leaves: %s", d)
- }
  checkAttachments(t, dbc.underlying(), tt.wantAttachments)
  })
 }

x/sqlite/schema.go

@@ -31,7 +31,7 @@ var schema = []string{
  rev INTEGER NOT NULL,
  rev_id TEXT NOT NULL,
  doc BLOB NOT NULL,
- md5sum TEXT NOT NULL,
+ md5sum BLOB NOT NULL,
  deleted BOOLEAN NOT NULL DEFAULT FALSE,
  FOREIGN KEY (id, rev, rev_id) REFERENCES {{ .Revs }} (id, rev, rev_id),
  UNIQUE(id, rev, rev_id)

x/sqlite/util.go

@@ -32,33 +32,48 @@ func placeholders(start, count int) string {
  return strings.Join(result, ", ")
 }
 
-// isLeafRev returns a nil error if the specified revision is a leaf revision.
-// If the revision is not a leaf revision, it returns a conflict error.
-// If the document is not found, it returns a not found error.
-func (d *db) isLeafRev(ctx context.Context, tx *sql.Tx, docID string, rev int, revID string) error {
- var exists bool
+// isLeafRev returns the leaf rev's hash and a nil error if the specified
+// revision is a leaf revision. If the revision is not a leaf revision, it
+// returns a conflict error. If the document is not found, it returns a not
+// found error.
+func (d *db) isLeafRev(ctx context.Context, tx *sql.Tx, docID string, rev int, revID string) (md5sum, error) {
+ var isLeaf bool
+ var hash md5sum
  err := tx.QueryRowContext(ctx, d.query(`
  SELECT
- parent.parent_rev IS NOT NULL
- FROM {{ .Revs }} AS r
- LEFT JOIN {{ .Revs }} AS parent
- ON r.id = parent.id
- AND r.parent_rev = parent.parent_rev
- AND r.parent_rev_id = parent.parent_rev_id
- WHERE r.id = $1
- AND r.rev = $2
- AND r.rev_id = $3
- `), docID, rev, revID).Scan(&exists)
+ doc.md5sum,
+ COALESCE(revs.is_leaf, FALSE) AS is_leaf
+ FROM (
+ SELECT
+ id,
+ rev,
+ rev_id,
+ md5sum
+ FROM {{ .Docs }}
+ WHERE id = $1
+ LIMIT 1
+ ) AS doc
+ LEFT JOIN (
+ SELECT
+ parent.id, parent.rev, parent.rev_id,
+ child.id IS NULL AS is_leaf
+ FROM {{ .Revs }} AS parent
+ LEFT JOIN {{ .Revs }} AS child ON parent.id = child.id AND parent.rev = child.parent_rev AND parent.rev_id = child.parent_rev_id
+ WHERE parent.id = $1
+ AND parent.rev = $2
+ AND parent.rev_id = $3
+ ) AS revs ON doc.id=revs.id
+ `), docID, rev, revID).Scan(&hash, &isLeaf)
  switch {
  case errors.Is(err, sql.ErrNoRows):
- return &internal.Error{Status: http.StatusNotFound, Message: "document not found"}
+ return md5sum{}, &internal.Error{Status: http.StatusNotFound, Message: "document not found"}
  case err != nil:
- return err
+ return md5sum{}, err
  }
- if !exists {
- return &internal.Error{Status: http.StatusConflict, Message: "Document update conflict"}
+ if !isLeaf {
+ return md5sum{}, &internal.Error{Status: http.StatusConflict, Message: "Document update conflict"}
  }
- return nil
+ return hash, nil
 }
 
 // winningRev returns the current winning revision, and MD5sum, for the specified document.
@@ -97,7 +112,6 @@ func (d *db) createRev(ctx context.Context, tx *sql.Tx, data *docData, curRev re
  if err != nil {
  return r, err
  }
-
  if len(data.Doc) == 0 {
  // No body can happen for example when calling PutAttachment, so we
  // create the new docs table entry by reading the previous one.