From a2d3e43ff487d40d5b5c1f70515bc964c9ec6214 Mon Sep 17 00:00:00 2001 From: Elena Grahovac Date: Fri, 30 Jun 2023 16:32:09 +0200 Subject: [PATCH] Implement proper response for `createIndexes` (#2936) Closes #2845. --- integration/indexes_command_compat_test.go | 495 +++++++++++++++++++++ integration/indexes_compat_test.go | 378 +--------------- internal/handlers/pg/msg_createindexes.go | 23 +- internal/handlers/pg/msg_listindexes.go | 3 +- internal/handlers/pg/msg_update.go | 3 +- internal/handlers/pg/pgdb/collections.go | 14 +- internal/handlers/pg/pgdb/indexes.go | 18 +- internal/handlers/pg/pgdb/indexes_test.go | 17 +- internal/handlers/pg/pgdb/insert.go | 3 +- internal/handlers/pg/pgdb/pgdb_test.go | 15 +- 10 files changed, 567 insertions(+), 402 deletions(-) create mode 100644 integration/indexes_command_compat_test.go diff --git a/integration/indexes_command_compat_test.go b/integration/indexes_command_compat_test.go new file mode 100644 index 000000000000..9b9c946ed163 --- /dev/null +++ b/integration/indexes_command_compat_test.go @@ -0,0 +1,495 @@ +// 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" + "go.mongodb.org/mongo-driver/mongo" + + "github.com/FerretDB/FerretDB/integration/setup" + "github.com/FerretDB/FerretDB/integration/shareddata" +) + +// TestCreateIndexesCommandCompat tests specific behavior for index creation that can be only provided through RunCommand. +func TestCreateIndexesCommandCompat(t *testing.T) { + setup.SkipForTigrisWithReason(t, "Indexes creation is not supported for Tigris") + + t.Parallel() + + ctx, targetCollections, compatCollections := setup.SetupCompat(t) + targetCollection := targetCollections[0] + compatCollection := compatCollections[0] + + for name, tc := range map[string]struct { //nolint:vet // for readability + collectionName any + indexName any + key any + unique any + resultType compatTestCaseResultType // defaults to nonEmptyResult + skip string // optional, skip test with a specified reason + }{ + "InvalidCollectionName": { + collectionName: 42, + key: bson.D{{"v", -1}}, + indexName: "custom-name", + resultType: emptyResult, + }, + "NilCollectionName": { + collectionName: nil, + key: bson.D{{"v", -1}}, + indexName: "custom-name", + resultType: emptyResult, + }, + "EmptyCollectionName": { + collectionName: "", + key: bson.D{{"v", -1}}, + indexName: "custom-name", + resultType: emptyResult, + }, + "IndexNameNotSet": { + collectionName: "test", + key: bson.D{{"v", -1}}, + indexName: nil, + resultType: emptyResult, + }, + "EmptyIndexName": { + collectionName: "test", + key: bson.D{{"v", -1}}, + indexName: "", + resultType: emptyResult, + }, + "NonStringIndexName": { + collectionName: "test", + key: bson.D{{"v", -1}}, + indexName: 42, + resultType: emptyResult, + }, + "ExistingNameDifferentKeyLength": { + collectionName: "test", + key: bson.D{{"_id", 1}, {"v", 1}}, + indexName: "_id_", // the same name as the default index + }, + "InvalidKey": { + collectionName: "test", + key: 42, + resultType: emptyResult, + }, + "EmptyKey": { + collectionName: "test", + key: bson.D{}, + resultType: emptyResult, + }, + "KeyNotSet": { + collectionName: "test", + resultType: emptyResult, + }, + "UniqueFalse": { + collectionName: "unique_false", + key: bson.D{{"v", 1}}, + indexName: "unique_false", + unique: false, + }, + "UniqueTypeDocument": { + collectionName: "test", + key: bson.D{{"v", 1}}, + indexName: "test", + unique: bson.D{}, + resultType: emptyResult, + }, + } { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + if tc.skip != "" { + t.Skip(tc.skip) + } + + t.Helper() + t.Parallel() + + indexesDoc := bson.D{} + + if tc.key != nil { + indexesDoc = append(indexesDoc, bson.E{Key: "key", Value: tc.key}) + } + + if tc.indexName != nil { + indexesDoc = append(indexesDoc, bson.E{"name", tc.indexName}) + } + + if tc.unique != nil { + indexesDoc = append(indexesDoc, bson.E{Key: "unique", Value: tc.unique}) + } + + var targetRes bson.D + targetErr := targetCollection.Database().RunCommand( + ctx, bson.D{ + {"createIndexes", tc.collectionName}, + {"indexes", bson.A{indexesDoc}}, + }, + ).Decode(&targetRes) + + var compatRes bson.D + compatErr := compatCollection.Database().RunCommand( + ctx, bson.D{ + {"createIndexes", tc.collectionName}, + {"indexes", bson.A{indexesDoc}}, + }, + ).Decode(&compatRes) + + if targetErr != nil { + t.Logf("Target error: %v", targetErr) + t.Logf("Compat error: %v", compatErr) + + // error messages are intentionally not compared + AssertMatchesCommandError(t, compatErr, targetErr) + + return + } + require.NoError(t, compatErr, "compat error; target returned no error") + + if tc.resultType == emptyResult { + require.Nil(t, targetRes) + require.Nil(t, compatRes) + } + + assert.Equal(t, compatRes, targetRes) + + targetCursor, targetErr := targetCollection.Indexes().List(ctx) + compatCursor, compatErr := compatCollection.Indexes().List(ctx) + + if targetCursor != nil { + defer targetCursor.Close(ctx) + } + if compatCursor != nil { + defer compatCursor.Close(ctx) + } + + require.NoError(t, targetErr) + require.NoError(t, compatErr) + + targetListRes := FetchAll(t, ctx, targetCursor) + compatListRes := FetchAll(t, ctx, compatCursor) + + assert.Equal(t, compatListRes, targetListRes) + + targetSpec, targetErr := targetCollection.Indexes().ListSpecifications(ctx) + compatSpec, compatErr := compatCollection.Indexes().ListSpecifications(ctx) + + require.NoError(t, compatErr) + require.NoError(t, targetErr) + + assert.Equal(t, compatSpec, targetSpec) + }) + } +} + +// TestCreateIndexesCommandCompatCheckFields check that the response contains response's fields +// such as numIndexBefore, numIndexAfter, createdCollectionAutomatically +// contain the correct values. +func TestCreateIndexesCommandCompatCheckFields(t *testing.T) { + setup.SkipForTigrisWithReason(t, "Indexes creation is not supported for Tigris") + + t.Parallel() + + ctx, targetCollections, compatCollections := setup.SetupCompat(t) + targetCollection := targetCollections[0] + compatCollection := compatCollections[0] + + // Create an index for a non-existent collection, expect createdCollectionAutomatically to be true. + collectionName := "newCollection" + indexesDoc := bson.D{{"key", bson.D{{"v", 1}}}, {"name", "v_1"}} + + var targetRes bson.D + targetErr := targetCollection.Database().RunCommand( + ctx, bson.D{ + {"createIndexes", collectionName}, + {"indexes", bson.A{indexesDoc}}, + }, + ).Decode(&targetRes) + + var compatRes bson.D + compatErr := compatCollection.Database().RunCommand( + ctx, bson.D{ + {"createIndexes", collectionName}, + {"indexes", bson.A{indexesDoc}}, + }, + ).Decode(&compatRes) + + require.NoError(t, compatErr) + require.NoError(t, targetErr, "target error; compat returned no error") + + compatDoc := ConvertDocument(t, compatRes) + createdCollectionAutomatically, err := compatDoc.Get("createdCollectionAutomatically") + require.NoError(t, err) + require.True(t, createdCollectionAutomatically.(bool)) // must be true because a new collection was created + + AssertEqualDocuments(t, compatRes, targetRes) + + // Now this collection exists, so we create another index and expect createdCollectionAutomatically to be false. + indexesDoc = bson.D{{"key", bson.D{{"foo", 1}}}, {"name", "foo_1"}} + + targetErr = targetCollection.Database().RunCommand( + ctx, bson.D{ + {"createIndexes", collectionName}, + {"indexes", bson.A{indexesDoc}}, + }, + ).Decode(&targetRes) + + compatErr = compatCollection.Database().RunCommand( + ctx, bson.D{ + {"createIndexes", collectionName}, + {"indexes", bson.A{indexesDoc}}, + }, + ).Decode(&compatRes) + + require.NoError(t, compatErr) + require.NoError(t, targetErr, "target error; compat returned no error") + + compatDoc = ConvertDocument(t, compatRes) + createdCollectionAutomatically, err = compatDoc.Get("createdCollectionAutomatically") + require.NoError(t, err) + require.False(t, createdCollectionAutomatically.(bool)) // must be false because the collection already exists + + AssertEqualDocuments(t, compatRes, targetRes) + + // Call index creation for the index that already exists, expect note to be set. + indexesDoc = bson.D{{"key", bson.D{{"foo", 1}}}, {"name", "foo_1"}} + + targetErr = targetCollection.Database().RunCommand( + ctx, bson.D{ + {"createIndexes", collectionName}, + {"indexes", bson.A{indexesDoc}}, + }, + ).Decode(&targetRes) + + compatErr = compatCollection.Database().RunCommand( + ctx, bson.D{ + {"createIndexes", collectionName}, + {"indexes", bson.A{indexesDoc}}, + }, + ).Decode(&compatRes) + + require.NoError(t, compatErr) + require.NoError(t, targetErr, "target error; compat returned no error") + + compatDoc = ConvertDocument(t, compatRes) + createdCollectionAutomatically, err = compatDoc.Get("note") + require.NoError(t, err) + + // note must be set because no new indexes were created: + require.Equal(t, "all indexes already exist", createdCollectionAutomatically.(string)) + + AssertEqualDocuments(t, compatRes, targetRes) +} + +func TestDropIndexesCommandCompat(t *testing.T) { + setup.SkipForTigrisWithReason(t, "Indexes are not supported for Tigris") + + t.Parallel() + + for name, tc := range map[string]struct { //nolint:vet // for readability + toCreate []mongo.IndexModel // optional, if set, create the given indexes before drop is called + toDrop any // required, index to drop + + resultType compatTestCaseResultType // optional, defaults to nonEmptyResult + skip string // optional, skip test with a specified reason + }{ + "MultipleIndexesByName": { + toCreate: []mongo.IndexModel{ + {Keys: bson.D{{"v", -1}}}, + {Keys: bson.D{{"v", 1}, {"foo", 1}}}, + {Keys: bson.D{{"v.foo", -1}}}, + }, + toDrop: bson.A{"v_-1", "v_1_foo_1"}, + }, + "MultipleIndexesByKey": { + toCreate: []mongo.IndexModel{ + {Keys: bson.D{{"v", -1}}}, + {Keys: bson.D{{"v.foo", -1}}}, + }, + toDrop: bson.A{bson.D{{"v", -1}}, bson.D{{"v.foo", -1}}}, + resultType: emptyResult, + }, + "NonExistentMultipleIndexes": { + toDrop: bson.A{"non-existent", "invalid"}, + resultType: emptyResult, + }, + "InvalidMultipleIndexType": { + toDrop: bson.A{1}, + resultType: emptyResult, + }, + "DocumentIndex": { + toCreate: []mongo.IndexModel{ + {Keys: bson.D{{"v", -1}}}, + }, + toDrop: bson.D{{"v", -1}}, + }, + "DropAllExpression": { + toCreate: []mongo.IndexModel{ + {Keys: bson.D{{"v", -1}}}, + {Keys: bson.D{{"foo.bar", 1}}}, + {Keys: bson.D{{"foo", 1}, {"bar", 1}}}, + }, + toDrop: "*", + }, + "WrongExpression": { + toCreate: []mongo.IndexModel{ + {Keys: bson.D{{"v", -1}}}, + {Keys: bson.D{{"foo.bar", 1}}}, + {Keys: bson.D{{"foo", 1}, {"bar", 1}}}, + }, + toDrop: "***", + resultType: emptyResult, + }, + "NonExistentDescendingID": { + toDrop: bson.D{{"_id", -1}}, + resultType: emptyResult, + }, + "MultipleKeyIndex": { + toCreate: []mongo.IndexModel{ + {Keys: bson.D{{"_id", -1}, {"v", 1}}}, + }, + toDrop: bson.D{ + {"_id", -1}, + {"v", 1}, + }, + }, + } { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + if tc.skip != "" { + t.Skip(tc.skip) + } + + t.Helper() + t.Parallel() + + require.NotNil(t, tc.toDrop, "toDrop must not be nil") + + // It's enough to use a single provider for drop indexes test as indexes work the same for different collections. + s := setup.SetupCompatWithOpts(t, &setup.SetupCompatOpts{ + Providers: []shareddata.Provider{shareddata.Composites}, + AddNonExistentCollection: true, + }) + ctx, targetCollections, compatCollections := s.Ctx, s.TargetCollections, s.CompatCollections + + var nonEmptyResults bool + for i := range targetCollections { + targetCollection := targetCollections[i] + compatCollection := compatCollections[i] + + t.Run(targetCollection.Name(), func(t *testing.T) { + t.Helper() + + if tc.toCreate != nil { + _, targetErr := targetCollection.Indexes().CreateMany(ctx, tc.toCreate) + _, compatErr := compatCollection.Indexes().CreateMany(ctx, tc.toCreate) + require.NoError(t, compatErr) + require.NoError(t, targetErr) + + // List indexes to see they are identical after creation. + targetCursor, targetListErr := targetCollection.Indexes().List(ctx) + compatCursor, compatListErr := compatCollection.Indexes().List(ctx) + + if targetCursor != nil { + defer targetCursor.Close(ctx) + } + if compatCursor != nil { + defer compatCursor.Close(ctx) + } + + require.NoError(t, targetListErr) + require.NoError(t, compatListErr) + + targetList := FetchAll(t, ctx, targetCursor) + compatList := FetchAll(t, ctx, compatCursor) + + require.Equal(t, compatList, targetList) + } + + targetCommand := bson.D{ + {"dropIndexes", targetCollection.Name()}, + {"index", tc.toDrop}, + } + + compatCommand := bson.D{ + {"dropIndexes", compatCollection.Name()}, + {"index", tc.toDrop}, + } + + var targetRes bson.D + targetErr := targetCollection.Database().RunCommand(ctx, targetCommand).Decode(&targetRes) + + var compatRes bson.D + compatErr := compatCollection.Database().RunCommand(ctx, compatCommand).Decode(&compatRes) + + if targetErr != nil { + t.Logf("Target error: %v", targetErr) + t.Logf("Compat error: %v", compatErr) + + // error messages are intentionally not compared + AssertMatchesCommandError(t, compatErr, targetErr) + + return + } + require.NoError(t, compatErr, "compat error; target returned no error") + + if tc.resultType == emptyResult { + require.Nil(t, targetRes) + require.Nil(t, compatRes) + } + + require.Equal(t, compatRes, targetRes) + + if compatErr == nil { + nonEmptyResults = true + } + + // List indexes to see they are identical after deletion. + targetCursor, targetListErr := targetCollection.Indexes().List(ctx) + compatCursor, compatListErr := compatCollection.Indexes().List(ctx) + + if targetCursor != nil { + defer targetCursor.Close(ctx) + } + if compatCursor != nil { + defer compatCursor.Close(ctx) + } + + require.NoError(t, targetListErr) + require.NoError(t, compatListErr) + + targetList := FetchAll(t, ctx, targetCursor) + compatList := FetchAll(t, ctx, compatCursor) + + assert.Equal(t, compatList, targetList) + }) + } + + switch tc.resultType { + case nonEmptyResult: + require.True(t, nonEmptyResults, "expected non-empty results (some indexes should be deleted)") + case emptyResult: + require.False(t, nonEmptyResults, "expected empty results (no indexes should be deleted)") + default: + t.Fatalf("unknown result type %v", tc.resultType) + } + }) + } +} diff --git a/integration/indexes_compat_test.go b/integration/indexes_compat_test.go index 928cdb525c62..e54149c406bd 100644 --- a/integration/indexes_compat_test.go +++ b/integration/indexes_compat_test.go @@ -311,179 +311,6 @@ func TestCreateIndexesCompat(t *testing.T) { } } -// TestCreateIndexesCommandCompat tests specific behavior for index creation that can be only provided through RunCommand. -func TestCreateIndexesCommandCompat(t *testing.T) { - setup.SkipForTigrisWithReason(t, "Indexes creation is not supported for Tigris") - - t.Parallel() - - ctx, targetCollections, compatCollections := setup.SetupCompat(t) - targetCollection := targetCollections[0] - compatCollection := compatCollections[0] - - for name, tc := range map[string]struct { //nolint:vet // for readability - collectionName any - indexName any - key any - unique any - resultType compatTestCaseResultType // defaults to nonEmptyResult - skip string // optional, skip test with a specified reason - }{ - "InvalidCollectionName": { - collectionName: 42, - key: bson.D{{"v", -1}}, - indexName: "custom-name", - resultType: emptyResult, - }, - "NilCollectionName": { - collectionName: nil, - key: bson.D{{"v", -1}}, - indexName: "custom-name", - resultType: emptyResult, - }, - "EmptyCollectionName": { - collectionName: "", - key: bson.D{{"v", -1}}, - indexName: "custom-name", - resultType: emptyResult, - }, - "IndexNameNotSet": { - collectionName: "test", - key: bson.D{{"v", -1}}, - indexName: nil, - resultType: emptyResult, - }, - "EmptyIndexName": { - collectionName: "test", - key: bson.D{{"v", -1}}, - indexName: "", - resultType: emptyResult, - }, - "NonStringIndexName": { - collectionName: "test", - key: bson.D{{"v", -1}}, - indexName: 42, - resultType: emptyResult, - }, - "ExistingNameDifferentKeyLength": { - collectionName: "test", - key: bson.D{{"_id", 1}, {"v", 1}}, - indexName: "_id_", // the same name as the default index - }, - "InvalidKey": { - collectionName: "test", - key: 42, - resultType: emptyResult, - }, - "EmptyKey": { - collectionName: "test", - key: bson.D{}, - resultType: emptyResult, - }, - "KeyNotSet": { - collectionName: "test", - resultType: emptyResult, - }, - "UniqueFalse": { - collectionName: "unique_false", - key: bson.D{{"v", 1}}, - indexName: "unique_false", - unique: false, - }, - "UniqueTypeDocument": { - collectionName: "test", - key: bson.D{{"v", 1}}, - indexName: "test", - unique: bson.D{}, - resultType: emptyResult, - }, - } { - name, tc := name, tc - t.Run(name, func(t *testing.T) { - if tc.skip != "" { - t.Skip(tc.skip) - } - - t.Helper() - t.Parallel() - - indexesDoc := bson.D{} - - if tc.key != nil { - indexesDoc = append(indexesDoc, bson.E{Key: "key", Value: tc.key}) - } - - if tc.indexName != nil { - indexesDoc = append(indexesDoc, bson.E{"name", tc.indexName}) - } - - if tc.unique != nil { - indexesDoc = append(indexesDoc, bson.E{Key: "unique", Value: tc.unique}) - } - - var targetRes bson.D - targetErr := targetCollection.Database().RunCommand( - ctx, bson.D{ - {"createIndexes", tc.collectionName}, - {"indexes", bson.A{indexesDoc}}, - }, - ).Decode(&targetRes) - - var compatRes bson.D - compatErr := compatCollection.Database().RunCommand( - ctx, bson.D{ - {"createIndexes", tc.collectionName}, - {"indexes", bson.A{indexesDoc}}, - }, - ).Decode(&compatRes) - - if targetErr != nil { - t.Logf("Target error: %v", targetErr) - t.Logf("Compat error: %v", compatErr) - - // error messages are intentionally not compared - AssertMatchesCommandError(t, compatErr, targetErr) - - return - } - require.NoError(t, compatErr, "compat error; target returned no error") - - if tc.resultType == emptyResult { - require.Nil(t, targetRes) - require.Nil(t, compatRes) - } - - assert.Equal(t, compatRes, targetRes) - - targetCursor, targetErr := targetCollection.Indexes().List(ctx) - compatCursor, compatErr := compatCollection.Indexes().List(ctx) - - if targetCursor != nil { - defer targetCursor.Close(ctx) - } - if compatCursor != nil { - defer compatCursor.Close(ctx) - } - - require.NoError(t, targetErr) - require.NoError(t, compatErr) - - targetListRes := FetchAll(t, ctx, targetCursor) - compatListRes := FetchAll(t, ctx, compatCursor) - - assert.Equal(t, compatListRes, targetListRes) - - targetSpec, targetErr := targetCollection.Indexes().ListSpecifications(ctx) - compatSpec, compatErr := compatCollection.Indexes().ListSpecifications(ctx) - - require.NoError(t, compatErr) - require.NoError(t, targetErr) - - assert.Equal(t, compatSpec, targetSpec) - }) - } -} - func TestDropIndexesCompat(t *testing.T) { setup.SkipForTigrisWithReason(t, "Indexes are not supported for Tigris") @@ -618,202 +445,6 @@ func TestDropIndexesCompat(t *testing.T) { } } -func TestDropIndexesCommandCompat(t *testing.T) { - setup.SkipForTigrisWithReason(t, "Indexes are not supported for Tigris") - - t.Parallel() - - for name, tc := range map[string]struct { //nolint:vet // for readability - toCreate []mongo.IndexModel // optional, if set, create the given indexes before drop is called - toDrop any // required, index to drop - - resultType compatTestCaseResultType // optional, defaults to nonEmptyResult - skip string // optional, skip test with a specified reason - }{ - "MultipleIndexesByName": { - toCreate: []mongo.IndexModel{ - {Keys: bson.D{{"v", -1}}}, - {Keys: bson.D{{"v", 1}, {"foo", 1}}}, - {Keys: bson.D{{"v.foo", -1}}}, - }, - toDrop: bson.A{"v_-1", "v_1_foo_1"}, - }, - "MultipleIndexesByKey": { - toCreate: []mongo.IndexModel{ - {Keys: bson.D{{"v", -1}}}, - {Keys: bson.D{{"v.foo", -1}}}, - }, - toDrop: bson.A{bson.D{{"v", -1}}, bson.D{{"v.foo", -1}}}, - resultType: emptyResult, - }, - "NonExistentMultipleIndexes": { - toDrop: bson.A{"non-existent", "invalid"}, - resultType: emptyResult, - }, - "InvalidMultipleIndexType": { - toDrop: bson.A{1}, - resultType: emptyResult, - }, - "DocumentIndex": { - toCreate: []mongo.IndexModel{ - {Keys: bson.D{{"v", -1}}}, - }, - toDrop: bson.D{{"v", -1}}, - }, - "DropAllExpression": { - toCreate: []mongo.IndexModel{ - {Keys: bson.D{{"v", -1}}}, - {Keys: bson.D{{"foo.bar", 1}}}, - {Keys: bson.D{{"foo", 1}, {"bar", 1}}}, - }, - toDrop: "*", - }, - "WrongExpression": { - toCreate: []mongo.IndexModel{ - {Keys: bson.D{{"v", -1}}}, - {Keys: bson.D{{"foo.bar", 1}}}, - {Keys: bson.D{{"foo", 1}, {"bar", 1}}}, - }, - toDrop: "***", - resultType: emptyResult, - }, - "NonExistentDescendingID": { - toDrop: bson.D{{"_id", -1}}, - resultType: emptyResult, - }, - "MultipleKeyIndex": { - toCreate: []mongo.IndexModel{ - {Keys: bson.D{{"_id", -1}, {"v", 1}}}, - }, - toDrop: bson.D{ - {"_id", -1}, - {"v", 1}, - }, - }, - } { - name, tc := name, tc - t.Run(name, func(t *testing.T) { - if tc.skip != "" { - t.Skip(tc.skip) - } - - t.Helper() - t.Parallel() - - require.NotNil(t, tc.toDrop, "toDrop must not be nil") - - // It's enough to use a single provider for drop indexes test as indexes work the same for different collections. - s := setup.SetupCompatWithOpts(t, &setup.SetupCompatOpts{ - Providers: []shareddata.Provider{shareddata.Composites}, - AddNonExistentCollection: true, - }) - ctx, targetCollections, compatCollections := s.Ctx, s.TargetCollections, s.CompatCollections - - var nonEmptyResults bool - for i := range targetCollections { - targetCollection := targetCollections[i] - compatCollection := compatCollections[i] - - t.Run(targetCollection.Name(), func(t *testing.T) { - t.Helper() - - if tc.toCreate != nil { - _, targetErr := targetCollection.Indexes().CreateMany(ctx, tc.toCreate) - _, compatErr := compatCollection.Indexes().CreateMany(ctx, tc.toCreate) - require.NoError(t, compatErr) - require.NoError(t, targetErr) - - // List indexes to see they are identical after creation. - targetCursor, targetListErr := targetCollection.Indexes().List(ctx) - compatCursor, compatListErr := compatCollection.Indexes().List(ctx) - - if targetCursor != nil { - defer targetCursor.Close(ctx) - } - if compatCursor != nil { - defer compatCursor.Close(ctx) - } - - require.NoError(t, targetListErr) - require.NoError(t, compatListErr) - - targetList := FetchAll(t, ctx, targetCursor) - compatList := FetchAll(t, ctx, compatCursor) - - require.Equal(t, compatList, targetList) - } - - targetCommand := bson.D{ - {"dropIndexes", targetCollection.Name()}, - {"index", tc.toDrop}, - } - - compatCommand := bson.D{ - {"dropIndexes", compatCollection.Name()}, - {"index", tc.toDrop}, - } - - var targetRes bson.D - targetErr := targetCollection.Database().RunCommand(ctx, targetCommand).Decode(&targetRes) - - var compatRes bson.D - compatErr := compatCollection.Database().RunCommand(ctx, compatCommand).Decode(&compatRes) - - if targetErr != nil { - t.Logf("Target error: %v", targetErr) - t.Logf("Compat error: %v", compatErr) - - // error messages are intentionally not compared - AssertMatchesCommandError(t, compatErr, targetErr) - - return - } - require.NoError(t, compatErr, "compat error; target returned no error") - - if tc.resultType == emptyResult { - require.Nil(t, targetRes) - require.Nil(t, compatRes) - } - - require.Equal(t, compatRes, targetRes) - - if compatErr == nil { - nonEmptyResults = true - } - - // List indexes to see they are identical after deletion. - targetCursor, targetListErr := targetCollection.Indexes().List(ctx) - compatCursor, compatListErr := compatCollection.Indexes().List(ctx) - - if targetCursor != nil { - defer targetCursor.Close(ctx) - } - if compatCursor != nil { - defer compatCursor.Close(ctx) - } - - require.NoError(t, targetListErr) - require.NoError(t, compatListErr) - - targetList := FetchAll(t, ctx, targetCursor) - compatList := FetchAll(t, ctx, compatCursor) - - assert.Equal(t, compatList, targetList) - }) - } - - switch tc.resultType { - case nonEmptyResult: - require.True(t, nonEmptyResults, "expected non-empty results (some indexes should be deleted)") - case emptyResult: - require.False(t, nonEmptyResults, "expected empty results (no indexes should be deleted)") - default: - t.Fatalf("unknown result type %v", tc.resultType) - } - }) - } -} - func TestCreateIndexesCompatUnique(t *testing.T) { setup.SkipForTigrisWithReason(t, "Indexes creation is not supported for Tigris") @@ -855,6 +486,15 @@ func TestCreateIndexesCompatUnique(t *testing.T) { insertDoc: bson.D{{"not-existing-field", "value"}}, skip: "https://github.com/FerretDB/FerretDB/issues/2830", }, + "NotUniqueIndex": { + models: []mongo.IndexModel{ + { + Keys: bson.D{{"v", 1}}, + Options: options.Index().SetUnique(false), + }, + }, + insertDoc: bson.D{{"v", "value"}}, + }, "CompoundIndex": { models: []mongo.IndexModel{ { diff --git a/internal/handlers/pg/msg_createindexes.go b/internal/handlers/pg/msg_createindexes.go index c45dc311a2a8..bd417f1e55cb 100644 --- a/internal/handlers/pg/msg_createindexes.go +++ b/internal/handlers/pg/msg_createindexes.go @@ -109,7 +109,7 @@ func (h *Handler) MsgCreateIndexes(ctx context.Context, msg *wire.OpMsg) (*wire. indexes := map[*types.Document]*pgdb.Index{} - var isUniqueSpecified bool + var collCreated bool var numIndexesBefore, numIndexesAfter int32 err = dbPool.InTransactionRetry(ctx, func(tx pgx.Tx) error { var indexesBefore []pgdb.Index @@ -212,7 +212,7 @@ func (h *Handler) MsgCreateIndexes(ctx context.Context, msg *wire.OpMsg) (*wire. indexes[indexDoc] = index - err = pgdb.CreateIndexIfNotExists(ctx, tx, db, collection, index) + collCreated, err = pgdb.CreateIndexIfNotExists(ctx, tx, db, collection, index) if errors.Is(err, pgdb.ErrIndexKeyAlreadyExist) && index.Name == "_id_1" { // ascending _id index is created by default return nil @@ -221,8 +221,6 @@ func (h *Handler) MsgCreateIndexes(ctx context.Context, msg *wire.OpMsg) (*wire. if err != nil { return err } - - isUniqueSpecified = index.Unique != nil } }) @@ -254,10 +252,13 @@ func (h *Handler) MsgCreateIndexes(ctx context.Context, msg *wire.OpMsg) (*wire. res := new(types.Document) - if isUniqueSpecified { - res.Set("numIndexesBefore", numIndexesBefore) - res.Set("numIndexesAfter", numIndexesAfter) - res.Set("createdCollectionAutomatically", true) + res.Set("numIndexesBefore", numIndexesBefore) + res.Set("numIndexesAfter", numIndexesAfter) + + if numIndexesBefore != numIndexesAfter { + res.Set("createdCollectionAutomatically", collCreated) + } else { + res.Set("note", "all indexes already exist") } res.Set("ok", float64(1)) @@ -375,7 +376,7 @@ func processIndexOptions(indexDoc *types.Document) (*pgdb.Index, error) { case "unique": v := must.NotFail(indexDoc.Get("unique")) - _, ok := v.(bool) + unique, ok := v.(bool) if !ok { return nil, commonerrors.NewCommandErrorMsgWithArgument( commonerrors.ErrTypeMismatch, @@ -401,7 +402,9 @@ func processIndexOptions(indexDoc *types.Document) (*pgdb.Index, error) { ) } - index.Unique = pointer.ToBool(true) + if unique { + index.Unique = pointer.ToBool(true) + } case "background": // ignore deprecated options diff --git a/internal/handlers/pg/msg_listindexes.go b/internal/handlers/pg/msg_listindexes.go index c146bd4a00d2..a86ce89b9a54 100644 --- a/internal/handlers/pg/msg_listindexes.go +++ b/internal/handlers/pg/msg_listindexes.go @@ -100,7 +100,8 @@ func (h *Handler) MsgListIndexes(ctx context.Context, msg *wire.OpMsg) (*wire.Op "name", index.Name, )) - if index.Unique != nil && index.Name != "_id_" { + // only non-default unique indexes should have unique field in the response + if index.Unique != nil && *index.Unique && index.Name != "_id_" { indexDoc.Set("unique", *index.Unique) } diff --git a/internal/handlers/pg/msg_update.go b/internal/handlers/pg/msg_update.go index 553e638535fa..3fcc3485b81e 100644 --- a/internal/handlers/pg/msg_update.go +++ b/internal/handlers/pg/msg_update.go @@ -48,7 +48,8 @@ func (h *Handler) MsgUpdate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, } err = dbPool.InTransactionRetry(ctx, func(tx pgx.Tx) error { - return pgdb.CreateCollectionIfNotExists(ctx, tx, params.DB, params.Collection) + _, err = pgdb.CreateCollectionIfNotExists(ctx, tx, params.DB, params.Collection) + return err }) switch { diff --git a/internal/handlers/pg/pgdb/collections.go b/internal/handlers/pg/pgdb/collections.go index dce2d369e6b3..4543907a40ed 100644 --- a/internal/handlers/pg/pgdb/collections.go +++ b/internal/handlers/pg/pgdb/collections.go @@ -143,7 +143,7 @@ func CreateCollection(ctx context.Context, tx pgx.Tx, db, collection string) err Unique: pointer.ToBool(true), } - if err := CreateIndexIfNotExists(ctx, tx, db, collection, indexParams); err != nil { + if _, err := CreateIndexIfNotExists(ctx, tx, db, collection, indexParams); err != nil { return lazyerrors.Error(err) } @@ -153,18 +153,22 @@ func CreateCollection(ctx context.Context, tx pgx.Tx, db, collection string) err // CreateCollectionIfNotExists ensures that given FerretDB database and collection exist. // If the database does not exist, it will be created. // +// It returns true if the collection was created, false if it already existed or an error occurred. +// // It returns possibly wrapped error: // - ErrInvalidDatabaseName - if the given database name doesn't conform to restrictions. // - ErrInvalidCollectionName - if the given collection name doesn't conform to restrictions. // - *transactionConflictError - if a PostgreSQL conflict occurs (the caller could retry the transaction). -func CreateCollectionIfNotExists(ctx context.Context, tx pgx.Tx, db, collection string) error { +func CreateCollectionIfNotExists(ctx context.Context, tx pgx.Tx, db, collection string) (bool, error) { err := CreateCollection(ctx, tx, db, collection) switch { - case err == nil, errors.Is(err, ErrAlreadyExist): - return nil + case err == nil: + return true, nil + case errors.Is(err, ErrAlreadyExist): + return false, nil default: - return lazyerrors.Error(err) + return false, lazyerrors.Error(err) } } diff --git a/internal/handlers/pg/pgdb/indexes.go b/internal/handlers/pg/pgdb/indexes.go index 1dc92d9fb324..e83b7790f635 100644 --- a/internal/handlers/pg/pgdb/indexes.go +++ b/internal/handlers/pg/pgdb/indexes.go @@ -64,16 +64,20 @@ func Indexes(ctx context.Context, tx pgx.Tx, db, collection string) ([]Index, er // CreateIndexIfNotExists creates a new index for the given params if such an index doesn't exist. // +// If index creation also caused the collection to be created, it returns true as the first return value. +// // If the index exists, it doesn't return an error. -// If the collection doesn't exist, it will be created and then the index will be created. -func CreateIndexIfNotExists(ctx context.Context, tx pgx.Tx, db, collection string, i *Index) error { - if err := CreateCollectionIfNotExists(ctx, tx, db, collection); err != nil { - return err +func CreateIndexIfNotExists(ctx context.Context, tx pgx.Tx, db, collection string, i *Index) (bool, error) { + var collCreated bool + var err error + + if collCreated, err = CreateCollectionIfNotExists(ctx, tx, db, collection); err != nil { + return false, err } pgTable, pgIndex, err := newMetadataStorage(tx, db, collection).setIndex(ctx, i.Name, i.Key, i.Unique) if err != nil { - return err + return false, err } var unique bool @@ -82,10 +86,10 @@ func CreateIndexIfNotExists(ctx context.Context, tx pgx.Tx, db, collection strin } if err := createPgIndexIfNotExists(ctx, tx, db, pgTable, pgIndex, i.Key, unique); err != nil { - return err + return false, err } - return nil + return collCreated, nil } // Equal returns true if the given index key is equal to the current one. diff --git a/internal/handlers/pg/pgdb/indexes_test.go b/internal/handlers/pg/pgdb/indexes_test.go index 21b7a86c8263..31b00e95e9d3 100644 --- a/internal/handlers/pg/pgdb/indexes_test.go +++ b/internal/handlers/pg/pgdb/indexes_test.go @@ -102,7 +102,7 @@ func TestCreateIndexIfNotExists(t *testing.T) { t.Run(name, func(t *testing.T) { t.Helper() err := pool.InTransaction(ctx, func(tx pgx.Tx) error { - if err := CreateIndexIfNotExists(ctx, tx, databaseName, collectionName, &tc.index); err != nil { + if _, err := CreateIndexIfNotExists(ctx, tx, databaseName, collectionName, &tc.index); err != nil { return err } return nil @@ -144,7 +144,8 @@ func TestDropIndexes(t *testing.T) { setupDatabase(ctx, t, pool, databaseName) err := pool.InTransaction(ctx, func(tx pgx.Tx) error { - return CreateCollectionIfNotExists(ctx, tx, databaseName, collectionName) + _, err := CreateCollectionIfNotExists(ctx, tx, databaseName, collectionName) + return err }) require.NoError(t, err) @@ -272,9 +273,13 @@ func TestDropIndexes(t *testing.T) { err := pool.InTransaction(ctx, func(tx pgx.Tx) error { for _, idx := range tc.toCreate { - if err := CreateIndexIfNotExists(ctx, tx, databaseName, collectionName, &idx); err != nil { + var collCreated bool + + if collCreated, err = CreateIndexIfNotExists(ctx, tx, databaseName, collectionName, &idx); err != nil { return err } + + assert.False(t, collCreated) } return nil @@ -371,7 +376,11 @@ func TestDropIndexesStress(t *testing.T) { Key: indexKeys, } - return CreateIndexIfNotExists(ctx, tx, databaseName, collectionName, &idx) + var collCreated bool + collCreated, err = CreateIndexIfNotExists(ctx, tx, databaseName, collectionName, &idx) + assert.False(t, collCreated) + + return err }) require.NoError(t, err) diff --git a/internal/handlers/pg/pgdb/insert.go b/internal/handlers/pg/pgdb/insert.go index 3d54c6f073b0..57175fd60430 100644 --- a/internal/handlers/pg/pgdb/insert.go +++ b/internal/handlers/pg/pgdb/insert.go @@ -44,8 +44,7 @@ func InsertDocument(ctx context.Context, tx pgx.Tx, db, collection string, doc * var err error - err = CreateCollectionIfNotExists(ctx, tx, db, collection) - if err != nil { + if _, err = CreateCollectionIfNotExists(ctx, tx, db, collection); err != nil { return lazyerrors.Error(err) } diff --git a/internal/handlers/pg/pgdb/pgdb_test.go b/internal/handlers/pg/pgdb/pgdb_test.go index a144e912ec26..d1e91fb7d347 100644 --- a/internal/handlers/pg/pgdb/pgdb_test.go +++ b/internal/handlers/pg/pgdb/pgdb_test.go @@ -260,7 +260,10 @@ func TestCreateCollectionIfNotExists(t *testing.T) { setupDatabase(ctx, t, pool, databaseName) err := pool.InTransaction(ctx, func(tx pgx.Tx) error { - return CreateCollectionIfNotExists(ctx, tx, databaseName, collectionName) + created, err := CreateCollectionIfNotExists(ctx, tx, databaseName, collectionName) + assert.True(t, created) + + return err }) require.NoError(t, err) }) @@ -277,7 +280,10 @@ func TestCreateCollectionIfNotExists(t *testing.T) { return err } - return CreateCollectionIfNotExists(ctx, tx, databaseName, collectionName) + created, err := CreateCollectionIfNotExists(ctx, tx, databaseName, collectionName) + assert.True(t, created) + + return err }) require.NoError(t, err) }) @@ -298,7 +304,10 @@ func TestCreateCollectionIfNotExists(t *testing.T) { return err } - return CreateCollectionIfNotExists(ctx, tx, databaseName, collectionName) + created, err := CreateCollectionIfNotExists(ctx, tx, databaseName, collectionName) + assert.False(t, created) + + return err }) require.NoError(t, err) })