Merge branch 'main' into validate-db-2749

FerretDB · Jun 30, 2023 · 409396c · 409396c
2 parents 37757ed + e78942e
commit 409396c
Show file tree

Hide file tree

Showing 8 changed files with 326 additions and 19 deletions.
diff --git a/integration/delete_command_compat_test.go b/integration/delete_command_compat_test.go
@@ -0,0 +1,155 @@
+// Copyright 2021 FerretDB Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package integration
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.mongodb.org/mongo-driver/bson"
+
+	"github.com/FerretDB/FerretDB/integration/setup"
+	"github.com/FerretDB/FerretDB/integration/shareddata"
+)
+
+// deleteCommandCompatTestCase describes delete compatibility test case.
+type deleteCommandCompatTestCase struct {
+	skip    string // optional, skip test with a specified reason
+	deletes bson.A // required
+}
+
+func testDeleteCommandCompat(t *testing.T, testCases map[string]deleteCommandCompatTestCase) {
+	t.Helper()
+
+	for name, tc := range testCases {
+		name, tc := name, tc
+		t.Run(name, func(t *testing.T) {
+			t.Helper()
+
+			if tc.skip != "" {
+				t.Skip(tc.skip)
+			}
+
+			t.Parallel()
+
+			// Use per-test setup because deletes modify data set.
+			ctx, targetCollections, compatCollections := setup.SetupCompat(t)
+
+			for i := range targetCollections {
+				targetCollection := targetCollections[i]
+				compatCollection := compatCollections[i]
+				t.Run(targetCollection.Name(), func(t *testing.T) {
+					t.Helper()
+
+					var targetRes, compatRes bson.D
+					targetErr := targetCollection.Database().RunCommand(ctx,
+						bson.D{
+							{"delete", targetCollection.Name()},
+							{"deletes", tc.deletes},
+						}).Decode(&targetRes)
+					compatErr := compatCollection.Database().RunCommand(ctx,
+						bson.D{
+							{"delete", compatCollection.Name()},
+							{"deletes", tc.deletes},
+						}).Decode(&compatRes)
+
+					if targetErr != nil {
+						t.Logf("Target error: %v", targetErr)
+						t.Logf("Compat error: %v", compatErr)
+
+						// error messages are intentionally not compared
+						AssertMatchesBulkException(t, compatErr, targetErr)
+
+						return
+					}
+					require.NoError(t, compatErr, "compat error; target returned no error")
+
+					t.Logf("Compat (expected) result: %v", compatRes)
+					t.Logf("Target (actual)   result: %v", targetRes)
+					assert.Equal(t, compatRes, targetRes)
+
+					targetDocs := FindAll(t, ctx, targetCollection)
+					compatDocs := FindAll(t, ctx, compatCollection)
+
+					t.Logf("Compat (expected) IDs: %v", CollectIDs(t, compatDocs))
+					t.Logf("Target (actual)   IDs: %v", CollectIDs(t, targetDocs))
+					AssertEqualDocumentsSlice(t, compatDocs, targetDocs)
+				})
+			}
+		})
+	}
+}
+
+func TestDeleteCommandCompat(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]deleteCommandCompatTestCase{
+		"OneLimited": {
+			deletes: bson.A{
+				bson.D{{"q", bson.D{{"v", int32(0)}}}, {"limit", 1}},
+			},
+			skip: "https://github.com/FerretDB/FerretDB/issues/2935",
+		},
+		"TwoLimited": {
+			deletes: bson.A{
+				bson.D{{"q", bson.D{{"v", int32(42)}}}, {"limit", 1}},
+				bson.D{{"q", bson.D{{"v", "foo"}}}, {"limit", 1}},
+			},
+			skip: "https://github.com/FerretDB/FerretDB/issues/2935",
+		},
+		"DuplicateFilter": {
+			deletes: bson.A{
+				bson.D{{"q", bson.D{{"v", "foo"}}}, {"limit", 1}},
+				bson.D{{"q", bson.D{{"v", "foo"}}}, {"limit", 1}},
+			},
+			skip: "https://github.com/FerretDB/FerretDB/issues/2935",
+		},
+	}
+
+	testDeleteCommandCompat(t, testCases)
+}
+
+func TestDeleteCommandCompatNotExistingDatabase(t *testing.T) {
+	t.Parallel()
+
+	res := setup.SetupCompatWithOpts(t, &setup.SetupCompatOpts{
+		Providers: []shareddata.Provider{shareddata.Int32s},
+	})
+	ctx, targetColl, compatColl := res.Ctx, res.TargetCollections[0], res.CompatCollections[0]
+
+	targetDB := targetColl.Database().Client().Database("doesnotexist")
+	compatDB := compatColl.Database().Client().Database("doesnotexist")
+
+	var targetRes, compatRes bson.D
+
+	targetErr := targetDB.RunCommand(ctx, bson.D{
+		{"delete", "test"},
+		{"deletes", bson.A{
+			bson.D{{"q", bson.D{{"v", "foo"}}}, {"limit", 1}},
+		}},
+	}).Decode(&targetRes)
+	compatErr := compatDB.RunCommand(ctx, bson.D{
+		{"delete", "test"},
+		{"deletes", bson.A{
+			bson.D{{"q", bson.D{{"v", "foo"}}}, {"limit", 1}},
+		}},
+	}).Decode(&compatRes)
+
+	assert.NoError(t, targetErr)
+	assert.NoError(t, compatErr)
+
+	assert.Equal(t, compatRes, targetRes)
+}
diff --git a/integration/delete_compat_test.go b/integration/delete_compat_test.go
@@ -38,11 +38,6 @@ func TestDeleteCompat(t *testing.T) {
 	t.Parallel()
 
 	testCases := map[string]deleteCompatTestCase{
-		"Empty": {
-			filters:    []bson.D{},
-			resultType: emptyResult,
-		},
-
 		"One": {
 			filters: []bson.D{
 				{{"v", int32(42)}},
@@ -166,17 +161,20 @@ func testDeleteCompat(t *testing.T, testCases map[string]deleteCompatTestCase) {
 
 					if targetErr != nil {
 						t.Logf("Target error: %v", targetErr)
-						targetErr = UnsetRaw(t, targetErr)
-						compatErr = UnsetRaw(t, compatErr)
-						assert.Equal(t, compatErr, targetErr)
-					} else {
+						t.Logf("Compat error: %v", compatErr)
+
+						// error messages are intentionally not compared
+						AssertMatchesBulkException(t, compatErr, targetErr)
+					} else { // we have to check the results in case of error because some documents may be deleted
 						require.NoError(t, compatErr, "compat error; target returned no error")
 					}
 
 					if pointer.Get(targetRes).DeletedCount > 0 || pointer.Get(compatRes).DeletedCount > 0 {
 						nonEmptyResults = true
 					}
 
+					t.Logf("Compat (expected) result: %v", compatRes)
+					t.Logf("Target (actual)   result: %v", targetRes)
 					assert.Equal(t, compatRes, targetRes)
 
 					targetDocs := FindAll(t, ctx, targetCollection)

diff --git a/internal/backends/collection.go b/internal/backends/collection.go
@@ -68,6 +68,8 @@ type QueryResult struct {
 
 // Query executes a query against the collection.
 //
+// If the collection does not exist it returns empty iterator.
+//
 // The passed context should be used for canceling the initial query.
 // It also can be used to close the returned iterator and free underlying resources,
 // but doing so is not necessary - the handler will do that anyway.
@@ -132,6 +134,8 @@ type DeleteResult struct {
 }
 
 // Delete deletes documents in collection.
+//
+// If requested database or collection does not exist it returns 0 deleted documents.
 func (cc *collectionContract) Delete(ctx context.Context, params *DeleteParams) (res *DeleteResult, err error) {
 	defer observability.FuncCall(ctx)()
 	defer checkError(err)

diff --git a/internal/backends/sqlite/collection.go b/internal/backends/sqlite/collection.go
@@ -19,6 +19,9 @@ import (
 	"errors"
 	"fmt"
 
+	"modernc.org/sqlite"
+	sqlitelib "modernc.org/sqlite/lib"
+
 	"github.com/FerretDB/FerretDB/internal/backends"
 	"github.com/FerretDB/FerretDB/internal/backends/sqlite/metadata"
 	"github.com/FerretDB/FerretDB/internal/handlers/sjson"
@@ -59,6 +62,12 @@ func (c *collection) Query(ctx context.Context, params *backends.QueryParams) (*
 
 	rows, err := db.QueryContext(ctx, query)
 	if err != nil {
+		// No such table, return empty result.
+		var e *sqlite.Error
+		if errors.As(err, &e) && e.Code() == sqlitelib.SQLITE_ERROR {
+			return &backends.QueryResult{Iter: newQueryIterator(ctx, nil)}, nil
+		}
+
 		return nil, lazyerrors.Error(err)
 	}
 
@@ -119,7 +128,7 @@ func (c *collection) Update(ctx context.Context, params *backends.UpdateParams)
 
 	tableName := c.r.CollectionToTable(c.name)
 
-	query := fmt.Sprintf(`UPDATE %q SET _ferretdb_sjson = ? WHERE json_extract(_ferretdb_sjson, '$._id') = ?`, tableName)
+	query := fmt.Sprintf(`UPDATE %q SET _ferretdb_sjson = ? WHERE _ferretdb_sjson -> '$._id' = ?`, tableName)
 
 	var res backends.UpdateResult
 
@@ -164,9 +173,35 @@ func (c *collection) Update(ctx context.Context, params *backends.UpdateParams)
 
 // Delete implements backends.Collection interface.
 func (c *collection) Delete(ctx context.Context, params *backends.DeleteParams) (*backends.DeleteResult, error) {
-	// TODO https://github.com/FerretDB/FerretDB/issues/2751
+	db := c.r.DatabaseGetExisting(ctx, c.dbName)
+	if db == nil {
+		return &backends.DeleteResult{Deleted: 0}, nil
+	}
+
+	tableName := c.r.CollectionToTable(c.name)
+
+	query := fmt.Sprintf(`DELETE FROM %q WHERE _ferretdb_sjson -> '$._id' = ?`, tableName)
+
+	var deleted int64
+
+	for _, id := range params.IDs {
+		idArg := string(must.NotFail(sjson.MarshalSingleValue(id)))
+
+		res, err := db.ExecContext(ctx, query, idArg)
+		if err != nil {
+			return nil, lazyerrors.Error(err)
+		}
+
+		rowsAffected, err := res.RowsAffected()
+		if err != nil {
+			return nil, lazyerrors.Error(err)
+		}
+
+		deleted += rowsAffected
+	}
+
 	return &backends.DeleteResult{
-		Deleted: 0,
+		Deleted: deleted,
 	}, nil
 }
 

diff --git a/internal/handlers/common/delete.go b/internal/handlers/common/delete.go
@@ -33,6 +33,7 @@ type DeleteParams struct {
 	Let *types.Document `ferretdb:"let,unimplemented"`
 
 	WriteConcern *types.Document `ferretdb:"writeConcern,ignored"`
+	LSID         any             `ferretdb:"lsid,ignored"`
 }
 
 // Delete represents single delete operation parameters.

diff --git a/internal/handlers/pg/msg_delete.go b/internal/handlers/pg/msg_delete.go
@@ -81,14 +81,15 @@ func (h *Handler) MsgDelete(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg,
 	}
 
 	replyDoc := must.NotFail(types.NewDocument(
-		"ok", float64(1),
+		"n", deleted,
 	))
 
 	if delErrors.Len() > 0 {
-		replyDoc = delErrors.Document()
+		// "writeErrors" should be after "n" field
+		replyDoc.Set("writeErrors", must.NotFail(delErrors.Document().Get("writeErrors")))
 	}
 
-	replyDoc.Set("n", deleted)
+	replyDoc.Set("ok", float64(1))
 
 	var reply wire.OpMsg
 	must.NoError(reply.SetSections(wire.OpMsgSection{