From 206abe8edb5366bd2dcaa554589320ab2a6ee38c Mon Sep 17 00:00:00 2001 From: Dmitry Date: Fri, 30 Jun 2023 17:13:23 +0100 Subject: [PATCH] Add `insert` documents type validation (#2946) --- integration/insert_command_test.go | 30 ++++++++++++++++++++++++++++++ internal/handlers/common/insert.go | 19 +++++++++++++++++++ internal/handlers/pg/msg_insert.go | 25 +++++++------------------ 3 files changed, 56 insertions(+), 18 deletions(-) diff --git a/integration/insert_command_test.go b/integration/insert_command_test.go index 07a173b5cef..9e87a9bb599 100644 --- a/integration/insert_command_test.go +++ b/integration/insert_command_test.go @@ -62,6 +62,32 @@ func TestInsertCommandErrors(t *testing.T) { }, altMessage: "E11000 duplicate key error collection: TestInsertCommandErrors-InsertDuplicateKey.TestInsertCommandErrors-InsertDuplicateKey", }, + "InsertDuplicateKeyOrdered": { + toInsert: []any{ + bson.D{{"foo", "bar"}}, + bson.D{{"_id", "double"}}, + }, + ordered: true, + werr: &mongo.WriteError{ + Code: 11000, + Index: 1, + Message: `E11000 duplicate key error collection: ` + + `TestInsertCommandErrors-InsertDuplicateKeyOrdered.TestInsertCommandErrors-InsertDuplicateKeyOrdered index: _id_ dup key: { _id: "double" }`, + }, + altMessage: `E11000 duplicate key error collection: TestInsertCommandErrors-InsertDuplicateKeyOrdered.TestInsertCommandErrors-InsertDuplicateKeyOrdered`, + }, + "InsertArray": { + toInsert: []any{ + bson.D{{"a", int32(1)}}, + bson.A{}, + }, + ordered: true, + cerr: &mongo.CommandError{ + Code: 14, + Name: "TypeMismatch", + Message: "BSON field 'insert.documents.1' is the wrong type 'array', expected type 'object'", + }, + }, } { name, tc := name, tc t.Run(name, func(t *testing.T) { @@ -87,11 +113,15 @@ func TestInsertCommandErrors(t *testing.T) { if tc.cerr != nil { AssertEqualAltCommandError(t, *tc.cerr, tc.altMessage, err) + return } if tc.werr != nil { AssertEqualAltWriteError(t, *tc.werr, tc.altMessage, err) + return } + + require.NoError(t, err) }) } } diff --git a/internal/handlers/common/insert.go b/internal/handlers/common/insert.go index 7109d77fd1c..cbc30cb2e25 100644 --- a/internal/handlers/common/insert.go +++ b/internal/handlers/common/insert.go @@ -15,10 +15,14 @@ package common import ( + "fmt" + "go.uber.org/zap" + "github.com/FerretDB/FerretDB/internal/handlers/commonerrors" "github.com/FerretDB/FerretDB/internal/handlers/commonparams" "github.com/FerretDB/FerretDB/internal/types" + "github.com/FerretDB/FerretDB/internal/util/must" ) // InsertParams represents the parameters for an insert command. @@ -45,5 +49,20 @@ func GetInsertParams(document *types.Document, l *zap.Logger) (*InsertParams, er return nil, err } + for i := 0; i < params.Docs.Len(); i++ { + doc := must.NotFail(params.Docs.Get(i)) + + if _, ok := doc.(*types.Document); !ok { + return nil, commonerrors.NewCommandErrorMsg( + commonerrors.ErrTypeMismatch, + fmt.Sprintf( + "BSON field 'insert.documents.%d' is the wrong type '%s', expected type 'object'", + i, + commonparams.AliasFromType(doc), + ), + ) + } + } + return ¶ms, nil } diff --git a/internal/handlers/pg/msg_insert.go b/internal/handlers/pg/msg_insert.go index 9808c9cb210..bbb4ca26c07 100644 --- a/internal/handlers/pg/msg_insert.go +++ b/internal/handlers/pg/msg_insert.go @@ -23,7 +23,6 @@ import ( "github.com/FerretDB/FerretDB/internal/handlers/common" "github.com/FerretDB/FerretDB/internal/handlers/commonerrors" - "github.com/FerretDB/FerretDB/internal/handlers/commonparams" "github.com/FerretDB/FerretDB/internal/handlers/pg/pgdb" "github.com/FerretDB/FerretDB/internal/types" "github.com/FerretDB/FerretDB/internal/util/lazyerrors" @@ -87,7 +86,7 @@ func insertMany(ctx context.Context, dbPool *pgdb.Pool, qp *pgdb.QueryParams, do for i := 0; i < docs.Len(); i++ { doc := must.NotFail(docs.Get(i)) - err := insertDocument(ctx, tx, qp, doc) + err := insertDocument(ctx, tx, qp, doc.(*types.Document)) if err != nil { return err } @@ -98,7 +97,7 @@ func insertMany(ctx context.Context, dbPool *pgdb.Pool, qp *pgdb.QueryParams, do // try inserting one document at a time if err != nil { for i := 0; i < docs.Len(); i++ { - doc := must.NotFail(docs.Get(i)) + doc := must.NotFail(docs.Get(i)).(*types.Document) err := insertDocumentSeparately(ctx, dbPool, qp, doc) @@ -126,22 +125,12 @@ func insertMany(ctx context.Context, dbPool *pgdb.Pool, qp *pgdb.QueryParams, do } // insertDocument prepares and executes actual INSERT request to Postgres in provided transaction. -func insertDocument(ctx context.Context, tx pgx.Tx, qp *pgdb.QueryParams, doc any) error { - d, ok := doc.(*types.Document) - if !ok { - return commonerrors.NewCommandErrorMsg( - commonerrors.ErrBadValue, - fmt.Sprintf("document has invalid type %s", commonparams.AliasFromType(doc)), - ) - } - - toInsert := d - - if !toInsert.Has("_id") { - toInsert.Set("_id", types.NewObjectID()) +func insertDocument(ctx context.Context, tx pgx.Tx, qp *pgdb.QueryParams, doc *types.Document) error { + if !doc.Has("_id") { + doc.Set("_id", types.NewObjectID()) } - err := pgdb.InsertDocument(ctx, tx, qp.DB, qp.Collection, toInsert) + err := pgdb.InsertDocument(ctx, tx, qp.DB, qp.Collection, doc) switch { case err == nil: @@ -168,7 +157,7 @@ func insertDocument(ctx context.Context, tx pgx.Tx, qp *pgdb.QueryParams, doc an // insertDocumentSeparately prepares and executes actual INSERT request to Postgres in separate transaction. // // It should be used in places where we don't want to rollback previous inserted documents on error. -func insertDocumentSeparately(ctx context.Context, dbPool *pgdb.Pool, qp *pgdb.QueryParams, doc any) error { +func insertDocumentSeparately(ctx context.Context, dbPool *pgdb.Pool, qp *pgdb.QueryParams, doc *types.Document) error { return dbPool.InTransactionRetry(ctx, func(tx pgx.Tx) error { return insertDocument(ctx, tx, qp, doc) })