chore: name the document holding a contested unique index value

Author: mennatnaga

internal/db/index.go

@@ -438,6 +438,8 @@ func saveUniqueKey(
 	}
 
 	if len(val) != 0 {
+		// This read puts the unique key in the transaction's read set, so two transactions
+		// writing the same index value conflict at commit rather than at this check.
 		existing, err := txn.Datastore().Get(ctx, &key)
 		if err != nil && !errors.Is(err, corekv.ErrNotFound) {
 			return NewErrCheckUniqueIndexConstraint(err)
@@ -446,7 +448,7 @@ func saveUniqueKey(
 			if tolerateSameDoc && string(existing) == string(val) {
 				return nil
 			}
-			return newUniqueIndexError(doc, fieldsDescs)
+			return newUniqueIndexError(ctx, doc, fieldsDescs, existing)
 		}
 	}
 
@@ -456,8 +458,33 @@ func saveUniqueKey(
 	return nil
 }
 
-func newUniqueIndexError(doc *client.Document, fieldsDescs []client.CollectionFieldDescription) error {
-	kvs := make([]errors.KV, 0, len(fieldsDescs))
+// incumbentDocID resolves the document already holding a unique index entry, from the
+// short ID the entry stores. Returns an empty string if it cannot be resolved, since this
+// only decorates an error that is being returned either way.
+func incumbentDocID(ctx context.Context, encodedShortID []byte) string {
+	shortID, err := keys.DecodeDocShortID(encodedShortID)
+	if err != nil {
+		return ""
+	}
+	docID, found, err := id.GetDocID(ctx, shortID)
+	if err != nil || !found {
+		return ""
+	}
+	return docID
+}
+
+func newUniqueIndexError(
+	ctx context.Context,
+	doc *client.Document,
+	fieldsDescs []client.CollectionFieldDescription,
+	existing []byte,
+) error {
+	kvs := make([]errors.KV, 0, len(fieldsDescs)+1)
+	// Naming the document that already holds the value is what makes the two comparable;
+	// without it the error only says which one lost.
+	if incumbent := incumbentDocID(ctx, existing); incumbent != "" {
+		kvs = append(kvs, errors.NewKV("HeldBy", incumbent))
+	}
 	for iter := range fieldsDescs {
 		fieldVal, err := doc.TryGetValue(fieldsDescs[iter].Name)
 		var val any

internal/db/index_backfill_test.go

@@ -370,3 +370,38 @@ func TestBackfillBatchTxn_ConflictsWhenReadDocIsModified(t *testing.T) {
 	require.True(t, errors.Is(commitErr, corekv.ErrTxnConflict),
 		"expected ErrTxnConflict but got: %v", commitErr)
 }
+
+// A unique violation names the document that lost. Naming the one that already holds the
+// value is what lets the two be compared, which is the only way to tell a genuine duplicate
+// from two documents that disagree on a field they do not share an index on.
+func TestSaveUniqueKey_ErrorNamesTheDocumentHoldingTheValue(t *testing.T) {
+	ctx := context.Background()
+
+	db, err := newBadgerDB(ctx)
+	require.NoError(t, err)
+	t.Cleanup(func() { db.Close() })
+
+	_, err = db.AddCollection(ctx, userSchema)
+	require.NoError(t, err)
+	col, err := db.GetCollectionByName(ctx, "User")
+	require.NoError(t, err)
+
+	_, err = col.NewIndex(ctx, client.NewIndexRequest{
+		Fields: []client.IndexedFieldDescription{{Name: "name"}},
+		Unique: true,
+	})
+	require.NoError(t, err)
+
+	incumbent, err := client.NewDocFromJSON(ctx, []byte(`{"name":"alice","age":1}`), col.Version())
+	require.NoError(t, err)
+	require.NoError(t, col.AddDocument(ctx, incumbent))
+
+	// Same indexed value, different age, so a different document that cannot have the slot.
+	duplicate, err := client.NewDocFromJSON(ctx, []byte(`{"name":"alice","age":2}`), col.Version())
+	require.NoError(t, err)
+	require.NotEqual(t, incumbent.ID().String(), duplicate.ID().String())
+
+	err = col.AddDocument(ctx, duplicate)
+	require.ErrorContains(t, err, "violates unique index")
+	require.ErrorContains(t, err, incumbent.ID().String())
+}