Merge pull request #921 from go-kivik/putFailure

Fix a PUT bug, and consistently handle updates to non-winning leaf revs
go-kivik · Apr 3, 2024 · a773c05 · a773c05
2 parents f1783a9 + 33b15a0
commit a773c05
Show file tree

Hide file tree

Showing 10 changed files with 251 additions and 106 deletions.
diff --git a/x/sqlite/delete.go b/x/sqlite/delete.go
@@ -21,17 +21,13 @@ import (
 )
 
 func (d *db) Delete(ctx context.Context, docID string, options driver.Options) (string, error) {
- opts := map[string]interface{}{}
+ opts := newOpts(options)
  options.Apply(opts)
- optRev, ok := opts["rev"].(string)
- if !ok {
+ optRev := opts.rev()
+ if optRev == "" {
  // Special case: No rev for DELETE is always a conflict, since you can't
  // delete a doc without a rev.
- return "", &internal.Error{Status: http.StatusConflict, Message: "conflict"}
- }
- delRev, err := parseRev(optRev)
- if err != nil {
- return "", err
+ return "", &internal.Error{Status: http.StatusConflict, Message: "document update conflict"}
  }
 
  data, err := prepareDoc(docID, map[string]interface{}{"_deleted": true})
@@ -45,28 +41,19 @@ func (d *db) Delete(ctx context.Context, docID string, options driver.Options) (
  }
  defer tx.Rollback()
 
- found, err := d.docRevExists(ctx, tx, docID, delRev)
+ curRev, err := parseRev(opts.rev())
  if err != nil {
  return "", err
  }
- if !found {
- return "", &internal.Error{Status: http.StatusConflict, Message: "conflict"}
- }
- var r revision
- err = tx.QueryRowContext(ctx, d.query(`
- INSERT INTO {{ .Revs }} (id, rev, rev_id, parent_rev, parent_rev_id)
- SELECT $1, COALESCE(MAX(rev),0) + 1, $2, $3, $4
- FROM {{ .Revs }}
- WHERE id = $1
- RETURNING rev, rev_id
- `), data.ID, data.RevID(), delRev.rev, delRev.id).Scan(&r.rev, &r.id)
+
+ data.MD5sum, err = d.isLeafRev(ctx, tx, docID, curRev.rev, curRev.id)
  if err != nil {
  return "", err
  }
- _, err = tx.ExecContext(ctx, d.query(`
-  INSERT INTO {{ .Docs }} (id, rev, rev_id, doc, md5sum, deleted)
- VALUES ($1, $2, $3, $4, $5, TRUE)
- `), data.ID, r.rev, r.id, data.Doc, data.MD5sum)
+ data.Deleted = true
+ data.Doc = []byte("{}")
+
+ r, err := d.createRev(ctx, tx, data, curRev)
  if err != nil {
  return "", err
  }

diff --git a/x/sqlite/delete_test.go b/x/sqlite/delete_test.go
@@ -83,7 +83,7 @@ func TestDBDelete(t *testing.T) {
  id: "foo",
  options: kivik.Rev(rev),
  wantStatus: http.StatusConflict,
- wantErr: "conflict",
+ wantErr: "document update conflict",
  }
  })
  tests.Add("delete deleted doc should succeed", func(t *testing.T) interface{} {
@@ -106,7 +106,7 @@ func TestDBDelete(t *testing.T) {
  db: db,
  id: "foo",
  wantStatus: http.StatusConflict,
- wantErr: "conflict",
+ wantErr: "document update conflict",
  }
  })
  tests.Add("delete losing rev for conflict should succeed", func(t *testing.T) interface{} {

diff --git a/x/sqlite/deleteattachment.go b/x/sqlite/deleteattachment.go
@@ -23,7 +23,9 @@ import (
 func (d *db) DeleteAttachment(ctx context.Context, docID, filename string, options driver.Options) (string, error) {
  opts := newOpts(options)
  if opts.rev() == "" {
- return "", &internal.Error{Status: http.StatusConflict, Message: "conflict"}
+ // Special case: No rev for DELETE is always a conflict, since you can't
+ // delete a doc without a rev.
+ return "", &internal.Error{Status: http.StatusConflict, Message: "document update conflict"}
  }
  tx, err := d.db.BeginTx(ctx, nil)
  if err != nil {

diff --git a/x/sqlite/deleteattachment_test.go b/x/sqlite/deleteattachment_test.go
@@ -59,7 +59,7 @@ func TestDBDeleteAttachment(t *testing.T) {
  db: db,
  docID: "foo",
  filename: "foo.txt",
- wantErr: "conflict",
+ wantErr: "document update conflict",
  wantStatus: http.StatusConflict,
  }
  })
@@ -72,7 +72,7 @@ func TestDBDeleteAttachment(t *testing.T) {
  docID: "foo",
  filename: "foo.txt",
  options: kivik.Rev("1-wrong"),
- wantErr: "Document update conflict",
+ wantErr: "document update conflict",
  wantStatus: http.StatusConflict,
  }
  })
@@ -214,14 +214,14 @@ func TestDBDeleteAttachment(t *testing.T) {
  {
  ID: "foo",
  Rev: 2,
- RevID: "def",
+ RevID: r2.id,
  ParentRev: &[]int{1}[0],
  ParentRevID: &r1.id,
  },
  {
  ID: "foo",
  Rev: 2,
- RevID: r2.id,
+ RevID: "def",
  ParentRev: &[]int{1}[0],
  ParentRevID: &r1.id,
  },

diff --git a/x/sqlite/options.go b/x/sqlite/options.go
@@ -51,3 +51,11 @@ func (o optsMap) rev() string {
  rev, _ := o["rev"].(string)
  return rev
 }
+
+func (o optsMap) newEdits() bool {
+ newEdits, ok := o["new_edits"].(bool)
+ if !ok {
+ return true
+ }
+ return newEdits
+}
diff --git a/x/sqlite/put.go b/x/sqlite/put.go
@@ -18,6 +18,7 @@ import (
  "errors"
  "net/http"
 
+ "github.com/go-kivik/kivik/v4"
  "github.com/go-kivik/kivik/v4/driver"
  "github.com/go-kivik/kivik/v4/internal"
 )
@@ -27,12 +28,9 @@ func (d *db) Put(ctx context.Context, docID string, doc interface{}, options dri
  if err != nil {
  return "", err
  }
- opts := map[string]interface{}{
- "new_edits": true,
- }
- options.Apply(opts)
- optsRev, _ := opts["rev"].(string)
- newEdits, _ := opts["new_edits"].(bool)
+ opts := newOpts(options)
+ optsRev := opts.rev()
+ newEdits := opts.newEdits()
  data, err := prepareDoc(docID, doc)
  if err != nil {
  return "", err
@@ -67,14 +65,14 @@ func (d *db) Put(ctx context.Context, docID string, doc interface{}, options dri
  return "", err
  }
  if !exists {
- return "", &internal.Error{Status: http.StatusConflict, Message: "conflict"}
+ return "", &internal.Error{Status: http.StatusConflict, Message: "document update conflict"}
  }
  }
  }
  docRev = data.Revisions.leaf().String()
  }
  if optsRev != "" && docRev != "" && optsRev != docRev {
- return "", &internal.Error{Status: http.StatusBadRequest, Message: "Document rev and option have different values"}
+ return "", &internal.Error{Status: http.StatusBadRequest, Message: "document rev and option have different values"}
  }
  if docRev == "" && optsRev != "" {
  docRev = optsRev
@@ -143,13 +141,24 @@ func (d *db) Put(ctx context.Context, docID string, doc interface{}, options dri
  return newRev, tx.Commit()
  }
 
- curRev, _, err := d.winningRev(ctx, tx, docID)
- if err != nil && !errors.Is(err, sql.ErrNoRows) {
- return "", err
+ var curRev revision
+ if docRev != "" {
+ curRev, err = parseRev(docRev)
+ if err != nil {
+ return "", err
+ }
  }
- if curRev.String() != docRev {
- return "", &internal.Error{Status: http.StatusConflict, Message: "conflict"}
+
+ data.MD5sum, err = d.isLeafRev(ctx, tx, docID, curRev.rev, curRev.id)
+ switch {
+ case kivik.HTTPStatus(err) == http.StatusNotFound:
+ if docRev != "" {
+ return "", &internal.Error{Status: http.StatusConflict, Message: "document update conflict"}
+ }
+ case err != nil:
+ return "", err
  }
+
  r, err := d.createRev(ctx, tx, data, curRev)
  if err != nil {
  return "", err