CBG-2055 Do not evaluate revpos during storeAttachments (#5557)

* CBG-2055 Do not evaluate revpos during storeAttachments

Revpos shouldn't be used as a criteria for attachment persistence for incoming writes. In particular during storeAttachments, a variety of circumstances can result in changes to the incoming revpos, and we don't want to fail to persist or remove attachments based on revpos.

In addition to removing the revpos check in storeAttachments, adds some additional handling in blip_handler for different flavors of revpos being sent by CBL to ensure it's being set to the incoming revID when the attachment doesn't exist.

* Update rest/blip_api_test.go

Co-authored-by: Ben Brooks <ben.brooks@couchbase.com>

* Ensure version is accurate for new attachments with shared digests

* Add test for legacy attachment name change

* Additional test case for legacy attachment w/out name change

Co-authored-by: Ben Brooks <ben.brooks@couchbase.com>

db/attachment.go

@@ -104,23 +104,18 @@ func (db *Database) storeAttachments(doc *Document, newAttachmentsMeta Attachmen
 			if meta["stub"] != true {
 				return nil, base.HTTPErrorf(400, "Missing data of attachment %q", name)
 			}
-
-			revpos, ok := base.ToInt64(meta["revpos"])
-			if !ok || revpos < 1 {
-				return nil, base.HTTPErrorf(400, "Missing/invalid revpos in stub attachment %q", name)
-			}
 			// Try to look up the attachment in ancestor attachments
 			if parentAttachments == nil {
 				parentAttachments = db.retrieveAncestorAttachments(doc, parentRev, docHistory)
 			}
 
+			// Note: in a non-conflict CAS retry, parentAttachments may be nil, because the attachment
+			//  data was persisted prior to the CAS failure writing the doc.  In this scenario the
+			//  incoming doc attachment metadata has already been updated to stub=true to avoid attempting to
+			//  persist the attachment again, even though there is not an attachment on an ancestor.
 			if parentAttachments != nil {
 				if parentAttachment := parentAttachments[name]; parentAttachment != nil {
-					parentrevpos, ok := base.ToInt64(parentAttachment.(map[string]interface{})["revpos"])
-
-					if ok && revpos <= parentrevpos {
-						atts[name] = parentAttachment
-					}
+					atts[name] = parentAttachment
 				}
 			} else if meta["digest"] == nil {
 				return nil, base.HTTPErrorf(400, "Missing digest in stub attachment %q", name)
@@ -295,12 +290,18 @@ func (db *Database) ForEachStubAttachment(body Body, minRevpos int, docID string
 			if err != nil && !base.IsDocNotFoundError(err) {
 				return err
 			}
-			if newData, err := callback(name, digest, data, meta); err != nil {
+			newData, err := callback(name, digest, data, meta)
+			if err != nil {
 				return err
-			} else if newData != nil {
+			}
+			if newData != nil {
 				meta["data"] = newData
 				delete(meta, "stub")
 				delete(meta, "follows")
+			} else {
+				// Update version in the case where this is a new attachment on the doc sharing a V2 digest with
+				// an existing attachment
+				meta["ver"] = AttVersion2
 			}
 		}
 	}

db/attachment_test.go

@@ -757,8 +757,8 @@ func TestStoreAttachments(t *testing.T) {
 	revId, doc, err = db.Put("doc5", revBody)
 	assert.Empty(t, revId, "The revId should be empty since revpos is not included in attachment")
 	assert.Empty(t, doc, "The doc should be empty since revpos is not included in attachment")
-	assert.Error(t, err, "It should throw 400 Missing/invalid revpos in stub attachment error")
-	assert.Contains(t, err.Error(), "400 Missing/invalid revpos in stub attachment")
+	assert.Error(t, err, "It should throw 400 Missing digest in stub attachment")
+	assert.Contains(t, err.Error(), "400 Missing digest in stub attachment")
 }
 
 // TestMigrateBodyAttachments will set up a document with an attachment in pre-2.5 metadata format, and test various upgrade scenarios.

db/blip_handler.go

@@ -951,6 +951,9 @@ func (bh *blipHandler) handleRev(rq *blip.Message) (err error) {
 				// Check if we have this attachment name already, if we do, continue check
 				currentAttachment, ok := currentBucketDoc.Attachments[name]
 				if !ok {
+					// If we don't have this attachment already, ensure incoming revpos is greater than minRevPos, otherwise
+					// update to ensure it's fetched and uploaded
+					bodyAtts[name].(map[string]interface{})["revpos"], _ = ParseRevID(revID)
 					continue
 				}
 
@@ -987,10 +990,9 @@ func (bh *blipHandler) handleRev(rq *blip.Message) (err error) {
 				}
 
 				// Compare the revpos and attachment digest. If incoming revpos is less than or equal to minRevPos and
-				// digest is different we need to override the revpos and set it to the current revision as the incoming
-				// revpos must be invalid and we need to request it.
+				// digest is different we need to override the revpos and set it to the current revision to ensure
+				// the attachment is requested and stored
 				if int(incomingAttachmentRevpos) <= minRevpos && currentAttachmentDigest != incomingAttachmentDigest {
-					minRevpos, _ = ParseRevID(history[len(history)-1])
 					bodyAtts[name].(map[string]interface{})["revpos"], _ = ParseRevID(revID)
 				}
 			}

rest/api_test.go

@@ -8034,57 +8034,8 @@ func TestBasicAttachmentRemoval(t *testing.T) {
 		assert.Equal(t, attBodyExpected, attBodyActual)
 	}
 
-	rawDocWithAttachmentAndSyncMeta := func() []byte {
-		return []byte(`{
-   "_sync": {
-      "rev": "1-5fc93bd36377008f96fdae2719c174ed",
-      "sequence": 2,
-      "recent_sequences": [
-         2
-      ],
-      "history": {
-         "revs": [
-            "1-5fc93bd36377008f96fdae2719c174ed"
-         ],
-         "parents": [
-            -1
-         ],
-         "channels": [
-            null
-         ]
-      },
-      "cas": "",
-      "attachments": {
-         "hi.txt": {
-            "revpos": 1,
-            "content_type": "text/plain",
-            "length": 2,
-            "stub": true,
-            "digest": "sha1-witfkXg0JglCjW9RssWvTAveakI="
-         }
-      },
-      "time_saved": "2021-09-01T17:33:03.054227821Z"
-   },
-  "key": "value"
-}`)
-	}
-
 	createDocWithLegacyAttachment := func(docID string, rawDoc []byte, attKey string, attBody []byte) {
-		// Write attachment directly to the bucket.
-		_, err := rt.Bucket().Add(attKey, 0, attBody)
-		require.NoError(t, err)
-
-		body := db.Body{}
-		err = body.Unmarshal(rawDoc)
-		require.NoError(t, err, "Error unmarshalling body")
-
-		// Write raw document to the bucket.
-		_, err = rt.Bucket().Add(docID, 0, rawDoc)
-		require.NoError(t, err)
-
-		// Migrate document metadata from document body to system xattr.
-		attachments := retrieveAttachmentMeta(docID)
-		require.Len(t, attachments, 1)
+		createDocWithLegacyAttachment(t, rt, docID, rawDoc, attKey, attBody)
 	}
 
 	t.Run("single attachment removal upon document update", func(t *testing.T) {
@@ -9393,3 +9344,63 @@ func TestAttachmentDeleteOnExpiry(t *testing.T) {
 	}
 
 }
+
+func createDocWithLegacyAttachment(t *testing.T, rt *RestTester, docID string, rawDoc []byte, attKey string, attBody []byte) {
+	// Write attachment directly to the bucket.
+	_, err := rt.Bucket().Add(attKey, 0, attBody)
+	require.NoError(t, err)
+
+	body := db.Body{}
+	err = body.Unmarshal(rawDoc)
+	require.NoError(t, err, "Error unmarshalling body")
+
+	// Write raw document to the bucket.
+	_, err = rt.Bucket().Add(docID, 0, rawDoc)
+	require.NoError(t, err)
+
+	// Migrate document metadata from document body to system xattr.
+	attachments := retrieveAttachmentMeta(t, rt, docID)
+	require.Len(t, attachments, 1)
+}
+
+func retrieveAttachmentMeta(t *testing.T, rt *RestTester, docID string) (attMeta map[string]interface{}) {
+	body := rt.getDoc(docID)
+	attachments, ok := body["_attachments"].(map[string]interface{})
+	require.True(t, ok)
+	return attachments
+}
+
+func rawDocWithAttachmentAndSyncMeta() []byte {
+	return []byte(`{
+   "_sync": {
+      "rev": "1-5fc93bd36377008f96fdae2719c174ed",
+      "sequence": 2,
+      "recent_sequences": [
+         2
+      ],
+      "history": {
+         "revs": [
+            "1-5fc93bd36377008f96fdae2719c174ed"
+         ],
+         "parents": [
+            -1
+         ],
+         "channels": [
+            null
+         ]
+      },
+      "cas": "",
+      "attachments": {
+         "hi.txt": {
+            "revpos": 1,
+            "content_type": "text/plain",
+            "length": 2,
+            "stub": true,
+            "digest": "sha1-witfkXg0JglCjW9RssWvTAveakI="
+         }
+      },
+      "time_saved": "2021-09-01T17:33:03.054227821Z"
+   },
+  "key": "value"
+}`)
+}