From 080a08770dff9c1401511a98721c64709994e9bc Mon Sep 17 00:00:00 2001 From: Felipe Gasper Date: Fri, 22 Nov 2024 13:28:32 -0500 Subject: [PATCH 1/8] first stab --- internal/util/collections.go | 79 +++++ internal/verifier/migration_verifier.go | 265 ++++++++++---- internal/verifier/migration_verifier_test.go | 2 + option/bson.go | 49 +++ option/json.go | 37 ++ option/option.go | 153 ++++++++ option/unit_test.go | 351 +++++++++++++++++++ option/validate.go | 28 ++ 8 files changed, 900 insertions(+), 64 deletions(-) create mode 100644 internal/util/collections.go create mode 100644 option/bson.go create mode 100644 option/json.go create mode 100644 option/option.go create mode 100644 option/unit_test.go create mode 100644 option/validate.go diff --git a/internal/util/collections.go b/internal/util/collections.go new file mode 100644 index 00000000..10e5e3ef --- /dev/null +++ b/internal/util/collections.go @@ -0,0 +1,79 @@ +package util + +import ( + "context" + + "github.com/10gen/migration-verifier/option" + "github.com/pkg/errors" + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/primitive" + "go.mongodb.org/mongo-driver/mongo" +) + +// CollectionSpec is like mongo.CollectionSpecification, +type CollectionSpec struct { + Name string + Type string + Options bson.Raw + Info struct { + ReadOnly bool `bson:"readOnly"` + UUID primitive.Binary + + Extra map[string]any + } + IDIndex bson.Raw `bson:"idIndex"` + + Extra map[string]any +} + +// Returns full name of collection including database name +func FullName(collection *mongo.Collection) string { + return collection.Database().Name() + "." + collection.Name() +} + +func GetCollectionSpec( + ctx context.Context, + coll *mongo.Collection, +) (option.Option[CollectionSpec], error) { + cursor, err := coll.Database().ListCollections(ctx, bson.M{"name": coll.Name()}) + if err != nil { + return option.None[CollectionSpec](), errors.Wrapf( + err, + "failed to fetch %#q's specification", + FullName(coll), + ) + } + + var specs []CollectionSpec + err = cursor.All(ctx, &specs) + if err != nil { + return option.None[CollectionSpec](), errors.Wrapf( + err, + "failed to parse %#q's specification", + FullName(coll), + ) + } + + switch len(specs) { + case 0: + return option.None[CollectionSpec](), nil + case 1: + if len(specs[0].Extra) > 0 || len(specs[0].Info.Extra) > 0 { + return option.None[CollectionSpec](), errors.Wrapf( + err, + "%#q's specification (%v) contains unrecognized fields", + FullName(coll), + specs[0], + ) + } + + return option.Some(specs[0]), nil + } + + return option.None[CollectionSpec](), errors.Wrapf( + err, + "received multiple results (%v) when fetching %#q's specification", + specs, + FullName(coll), + ) +} diff --git a/internal/verifier/migration_verifier.go b/internal/verifier/migration_verifier.go index c37eea7d..6438036a 100644 --- a/internal/verifier/migration_verifier.go +++ b/internal/verifier/migration_verifier.go @@ -8,7 +8,6 @@ import ( "math/rand" _ "net/http/pprof" "os" - "reflect" "sort" "strconv" "strings" @@ -21,7 +20,10 @@ import ( "github.com/10gen/migration-verifier/internal/reportutils" "github.com/10gen/migration-verifier/internal/retry" "github.com/10gen/migration-verifier/internal/types" + "github.com/10gen/migration-verifier/internal/util" "github.com/10gen/migration-verifier/internal/uuidutil" + "github.com/10gen/migration-verifier/mbson" + "github.com/10gen/migration-verifier/option" "github.com/olekukonko/tablewriter" "github.com/pkg/errors" "go.mongodb.org/mongo-driver/bson" @@ -796,34 +798,22 @@ func (verifier *Verifier) partitionAndInspectNamespace(ctx context.Context, name return partitionList, shardKeys, srcDocs, srcBytes, nil } -func (verifier *Verifier) getCollectionSpecification(ctx context.Context, collection *mongo.Collection) (*mongo.CollectionSpecification, error) { - filter := bson.D{{"name", collection.Name()}} - specifications, err := collection.Database().ListCollectionSpecifications(ctx, filter) - if err != nil { - return nil, err - } - if len(specifications) > 1 { - return nil, errors.Errorf("Too many collections named %s %+v", FullName(collection), specifications) - } - if len(specifications) == 1 { - verifier.logger.Debug().Msgf("Collection specification: %+v", specifications[0]) - return specifications[0], nil - } - - // Collection not found. - return nil, nil -} - // Returns a slice of VerificationResults with the differences, and a boolean indicating whether or // not the collection data can be safely verified. -func (verifier *Verifier) compareCollectionSpecifications(srcNs string, dstNs string, srcSpec *mongo.CollectionSpecification, dstSpec *mongo.CollectionSpecification) ([]VerificationResult, bool) { - if srcSpec == nil { +func (verifier *Verifier) compareCollectionSpecifications( + srcNs, dstNs string, + srcSpecOpt, dstSpecOpt option.Option[util.CollectionSpec], +) ([]VerificationResult, bool) { + srcSpec, hasSrcSpec := srcSpecOpt.Get() + dstSpec, hasDstSpec := dstSpecOpt.Get() + + if !hasSrcSpec { return []VerificationResult{{ NameSpace: srcNs, Cluster: ClusterSource, Details: Missing}}, false } - if dstSpec == nil { + if !hasDstSpec { return []VerificationResult{{ NameSpace: dstNs, Cluster: ClusterTarget, @@ -838,12 +828,12 @@ func (verifier *Verifier) compareCollectionSpecifications(srcNs string, dstNs st // If the types differ, the rest is not important. } var results []VerificationResult - if srcSpec.ReadOnly != dstSpec.ReadOnly { + if srcSpec.Info.ReadOnly != dstSpec.Info.ReadOnly { results = append(results, VerificationResult{ NameSpace: dstNs, Cluster: ClusterTarget, Field: "ReadOnly", - Details: Mismatch + fmt.Sprintf(" : src: %v, dst: %v", srcSpec.ReadOnly, dstSpec.ReadOnly)}) + Details: Mismatch + fmt.Sprintf(" : src: %v, dst: %v", srcSpec.Info.ReadOnly, dstSpec.Info.ReadOnly)}) } if !bytes.Equal(srcSpec.Options, dstSpec.Options) { mismatchDetails, err := BsonUnorderedCompareRawDocumentWithDetails(srcSpec.Options, dstSpec.Options) @@ -875,8 +865,59 @@ func (verifier *Verifier) compareCollectionSpecifications(srcNs string, dstNs st return results, canCompareData } -func compareIndexSpecifications(srcSpec *mongo.IndexSpecification, dstSpec *mongo.IndexSpecification) []VerificationResult { - var results []VerificationResult +func (verifier *Verifier) doIndexSpecsMatch(ctx context.Context, srcSpec bson.Raw, dstSpec bson.Raw) (bool, error) { + // If the byte buffers match, then we’re done. + if bytes.Equal(srcSpec, dstSpec) { + return true, nil + } + + // Next check to see if the only differences are type differences. + // If we didn’t support pre-v5 servers we could use a $documents aggregation, + // but we can’t do that, so we write to a temporary collection. + coll := verifier.metaClient.Database(verifier.metaDBName).Collection("indexCompare") + insert, err := coll.InsertOne( + ctx, + bson.M{"spec": srcSpec}, + ) + if err != nil { + return false, errors.Wrap(err, "failed to persist index specification to metadata") + } + + defer func() { + coll.DeleteOne(ctx, bson.M{"_id": insert.InsertedID}) + }() + + cursor, err := coll.Aggregate( + ctx, + mongo.Pipeline{ + { + {"$match", bson.D{ + {"_id", insert.InsertedID}, + {"$expr", bson.D{ + {"$eq", bson.A{ + "$spec", + dstSpec, + }}, + }}, + }}, + {"$project", bson.D{{"_id", 1}}}, + }, + }, + ) + if err != nil { + return false, errors.Wrap(err, "failed to check index specification match in metadata") + } + var docs []bson.Raw + err = cursor.All(ctx, &docs) + if err != nil { + return false, errors.Wrap(err, "failed to parse index specification match’s result") + } + + return len(docs) == 1, nil +} + +/* + // Order is always significant in the keys document. if !bytes.Equal(srcSpec.KeysDocument, dstSpec.KeysDocument) { results = append(results, VerificationResult{ @@ -926,6 +967,7 @@ func compareIndexSpecifications(srcSpec *mongo.IndexSpecification, dstSpec *mong } return results } +*/ func nilableToString[T any](ptr *T) string { if ptr == nil { @@ -953,50 +995,131 @@ func (verifier *Verifier) markCollectionFailed(workerNum int, task *Verification Details: Failed + fmt.Sprintf(" %v", err)}) } -func verifyIndexes(ctx context.Context, _ int, _ *VerificationTask, srcColl, dstColl *mongo.Collection, - srcIdIndexSpec, dstIdIndexSpec *mongo.IndexSpecification) ([]VerificationResult, error) { - srcSpecs, err := srcColl.Indexes().ListSpecifications(ctx) +func getIndexesMap( + ctx context.Context, + coll *mongo.Collection, +) (map[string]bson.Raw, error) { + + var specs []bson.Raw + specsMap := map[string]bson.Raw{} + + cursor, err := coll.Indexes().List(ctx) if err != nil { - return nil, err + return nil, errors.Wrapf( + err, + "failed to read %#q’s indexes", + FullName(coll), + ) + } + err = cursor.All(ctx, &specs) + if err != nil { + return nil, errors.Wrapf( + err, + "failed to parse %#q’s indexes", + FullName(coll), + ) } + + for _, spec := range specs { + var name string + has, err := mbson.RawLookup(spec, &name, "name") + + if !has { + return nil, errors.Errorf( + "%#q has an unnamed index (%+v)", + FullName(coll), + spec, + ) + } + + if err != nil { + return nil, errors.Wrapf( + err, + "failed to extract %#q from %#q's index specification (%+v)", + "name", + FullName(coll), + spec, + ) + } + + specsMap[name] = spec + } + + return specsMap, nil +} + +func (verifier *Verifier) verifyIndexes( + ctx context.Context, + srcColl, dstColl *mongo.Collection, + srcIdIndexSpec, dstIdIndexSpec bson.Raw, +) ([]VerificationResult, error) { + + srcMap, err := getIndexesMap(ctx, srcColl) + if err != nil { + return nil, errors.Wrapf( + err, + "failed to fetch %#q's indexes on source", + FullName(srcColl), + ) + } + if srcIdIndexSpec != nil { - srcSpecs = append(srcSpecs, srcIdIndexSpec) + srcMap["_id"] = srcIdIndexSpec } - dstSpecs, err := dstColl.Indexes().ListSpecifications(ctx) + + dstMap, err := getIndexesMap(ctx, dstColl) if err != nil { - return nil, err + return nil, errors.Wrapf( + err, + "failed to fetch %#q's indexes on destination", + FullName(dstColl), + ) } + if dstIdIndexSpec != nil { - dstSpecs = append(dstSpecs, dstIdIndexSpec) + dstMap["_id"] = dstIdIndexSpec } + var results []VerificationResult - srcMap := map[string](*mongo.IndexSpecification){} srcMapUsed := map[string]bool{} - for _, srcSpec := range srcSpecs { - srcMap[srcSpec.Name] = srcSpec - } - for _, dstSpec := range dstSpecs { - srcSpec := srcMap[dstSpec.Name] - if srcSpec == nil { + + for indexName, dstSpec := range dstMap { + srcSpec, exists := srcMap[indexName] + if exists { + srcMapUsed[indexName] = true + theyMatch, err := verifier.doIndexSpecsMatch(ctx, srcSpec, dstSpec) + if err != nil { + return nil, errors.Wrapf( + err, + "failed to check whether %#q's source & desstination %#q indexes match", + FullName(srcColl), + indexName, + ) + } + + if !theyMatch { + results = append(results, VerificationResult{ + NameSpace: FullName(dstColl), + Cluster: ClusterTarget, + ID: indexName, + Details: Mismatch + fmt.Sprintf(": src: %v, dst: %v", srcSpec, dstSpec), + }) + } + } else { results = append(results, VerificationResult{ - ID: dstSpec.Name, + ID: indexName, Details: Missing, Cluster: ClusterSource, - NameSpace: FullName(srcColl)}) - } else { - srcMapUsed[srcSpec.Name] = true - compareSpecResults := compareIndexSpecifications(srcSpec, dstSpec) - if compareSpecResults != nil { - results = append(results, compareSpecResults...) - } + NameSpace: FullName(srcColl), + }) } } // Find any index specs which existed in the source cluster but not the target cluster. - for _, srcSpec := range srcSpecs { - if !srcMapUsed[srcSpec.Name] { + for indexName := range srcMap { + if !srcMapUsed[indexName] { results = append(results, VerificationResult{ - ID: srcSpec.Name, + ID: indexName, Details: Missing, Cluster: ClusterTarget, NameSpace: FullName(dstColl)}) @@ -1011,12 +1134,12 @@ func (verifier *Verifier) verifyMetadataAndPartitionCollection(ctx context.Conte srcNs := FullName(srcColl) dstNs := FullName(dstColl) - srcSpec, srcErr := verifier.getCollectionSpecification(ctx, srcColl) + srcSpecOpt, srcErr := util.GetCollectionSpec(ctx, srcColl) if srcErr != nil { verifier.markCollectionFailed(workerNum, task, ClusterSource, srcNs, srcErr) } - dstSpec, dstErr := verifier.getCollectionSpecification(ctx, dstColl) + dstSpecOpt, dstErr := util.GetCollectionSpec(ctx, dstColl) if dstErr != nil { verifier.markCollectionFailed(workerNum, task, ClusterTarget, dstNs, dstErr) } @@ -1028,17 +1151,26 @@ func (verifier *Verifier) verifyMetadataAndPartitionCollection(ctx context.Conte insertFailedCollection := func() { _, err := verifier.InsertFailedCollectionVerificationTask(srcNs) if err != nil { - verifier. - logger. - Fatal(). + verifier.logger.Fatal(). + Int("workerNum", workerNum). + Str("srcNamespace", srcNs). + Str("dstNamespace", dstNs). Err(err). - Msg("Unrecoverable error in inserting failed collection verification task") + Msg("Failed to persist collection verification task.") } } - if dstSpec == nil { - if srcSpec == nil { - verifier.logger.Info().Msgf("[Worker %d] Collection not present on either cluster: %s -> %s", workerNum, srcNs, dstNs) + srcSpec, hasSrcSpec := srcSpecOpt.Get() + dstSpec, hasDstSpec := dstSpecOpt.Get() + + if !hasDstSpec { + if !hasSrcSpec { + verifier.logger.Info(). + Int("workerNum", workerNum). + Str("srcNamespace", srcNs). + Str("dstNamespace", dstNs). + Msg("Collection not present on either cluster.") + // This counts as success. task.Status = verificationTaskCompleted return @@ -1047,7 +1179,7 @@ func (verifier *Verifier) verifyMetadataAndPartitionCollection(ctx context.Conte // Fall through here; comparing the collection specifications will produce the correct // failure output. } - specificationProblems, verifyData := verifier.compareCollectionSpecifications(srcNs, dstNs, srcSpec, dstSpec) + specificationProblems, verifyData := verifier.compareCollectionSpecifications(srcNs, dstNs, srcSpecOpt, dstSpecOpt) if specificationProblems != nil { insertFailedCollection() task.FailedDocs = specificationProblems @@ -1067,10 +1199,15 @@ func (verifier *Verifier) verifyMetadataAndPartitionCollection(ctx context.Conte return } - indexProblems, err := verifyIndexes(ctx, workerNum, task, srcColl, dstColl, srcSpec.IDIndex, dstSpec.IDIndex) + indexProblems, err := verifier.verifyIndexes(ctx, srcColl, dstColl, srcSpec.IDIndex, dstSpec.IDIndex) if err != nil { task.Status = verificationTaskFailed - verifier.logger.Error().Msgf("[Worker %d] Error getting indexes for collection: %+v", workerNum, err) + verifier.logger.Error(). + Int("workerNum", workerNum). + Str("namespace", srcNs). + Err(err). + Msgf("Failed to compare collection indexes.") + return } if indexProblems != nil { diff --git a/internal/verifier/migration_verifier_test.go b/internal/verifier/migration_verifier_test.go index d6e9d8e5..63a25d05 100644 --- a/internal/verifier/migration_verifier_test.go +++ b/internal/verifier/migration_verifier_test.go @@ -996,6 +996,7 @@ func (suite *IntegrationTestSuite) TestVerifierCompareIndexes() { } } +/* func TestVerifierCompareIndexSpecs(t *testing.T) { // Index specification keysDoc1 := bson.D{{"a", 1}, {"b", -1}} @@ -1149,6 +1150,7 @@ func TestVerifierCompareIndexSpecs(t *testing.T) { assert.NotRegexp(t, regexp.MustCompile("0x"), result.Details) } } +*/ func (suite *IntegrationTestSuite) TestVerifierNamespaceList() { verifier := suite.BuildVerifier() diff --git a/option/bson.go b/option/bson.go new file mode 100644 index 00000000..3394185c --- /dev/null +++ b/option/bson.go @@ -0,0 +1,49 @@ +package option + +import ( + "github.com/pkg/errors" + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/bsontype" + "go.mongodb.org/mongo-driver/bson/primitive" +) + +// MarshalBSONValue implements bson.ValueMarshaler. +func (o Option[T]) MarshalBSONValue() (bsontype.Type, []byte, error) { + val, exists := o.Get() + if !exists { + return bson.MarshalValue(primitive.Null{}) + } + + return bson.MarshalValue(val) +} + +// UnmarshalBSONValue implements bson.ValueUnmarshaler. +func (o *Option[T]) UnmarshalBSONValue(bType bsontype.Type, raw []byte) error { + + switch bType { + case bson.TypeNull: + o.val = nil + + default: + valPtr := new(T) + + err := bson.UnmarshalValue(bType, raw, &valPtr) + if err != nil { + return errors.Wrapf(err, "failed to unmarshal %T", *o) + } + + // This may not even be possible, but we should still check. + if isNil(*valPtr) { + return errors.Wrapf(err, "refuse to unmarshal nil %T value", *o) + } + + o.val = valPtr + } + + return nil +} + +// IsZero implements bsoncodec.Zeroer. +func (o Option[T]) IsZero() bool { + return o.IsNone() +} diff --git a/option/json.go b/option/json.go new file mode 100644 index 00000000..b5290bbf --- /dev/null +++ b/option/json.go @@ -0,0 +1,37 @@ +package option + +import ( + "bytes" + "encoding/json" +) + +var _ json.Marshaler = &Option[int]{} +var _ json.Unmarshaler = &Option[int]{} + +// MarshalJSON encodes Option into json. +func (o Option[T]) MarshalJSON() ([]byte, error) { + val, exists := o.Get() + if exists { + return json.Marshal(val) + } + + return json.Marshal(nil) +} + +// UnmarshalJSON decodes Option from json. +func (o *Option[T]) UnmarshalJSON(b []byte) error { + if bytes.Equal(b, []byte("null")) { + o.val = nil + } else { + val := *new(T) + + err := json.Unmarshal(b, &val) + if err != nil { + return err + } + + o.val = &val + } + + return nil +} diff --git a/option/option.go b/option/option.go new file mode 100644 index 00000000..03706a9d --- /dev/null +++ b/option/option.go @@ -0,0 +1,153 @@ +// Package option implements [option types] in Go. +// It takes inspiration from [samber/mo] but also works with BSON and exposes +// a (hopefully) more refined interface. +// +// Option types facilitate avoidance of nil-dereference bugs, at the cost of a +// bit more overhead. +// +// A couple special notes: +// - nil values inside the Option, like `Some([]int(nil))`, are forbidden. +// - Option’s BSON marshaling/unmarshaling interoperates with the [bson] +// package’s handling of nilable pointers. So any code that uses nilable +// pointers to represent optional values can switch to Option and +// should continue working with existing persisted data. +// - Because encoding/json provides no equivalent to bsoncodec.Zeroer, +// Option always marshals to JSON null if empty. +// +// Prefer Option to nilable pointers in all new code, and consider +// changing existing code to use it. +// +// [option types]: https://en.wikipedia.org/wiki/Option_type +package option + +import ( + "fmt" + "reflect" + + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/bsoncodec" +) + +var _ bson.ValueMarshaler = &Option[int]{} +var _ bson.ValueUnmarshaler = &Option[int]{} +var _ bsoncodec.Zeroer = &Option[int]{} + +// Option represents a possibly-empty value. +// Its zero value is the empty case. +type Option[T any] struct { + val *T +} + +// Some creates an Option with a value. +func Some[T any](value T) Option[T] { + if isNil(value) { + panic(fmt.Sprintf("Option forbids nil value (%T).", value)) + } + + return Option[T]{&value} +} + +// None creates an Option with no value. +// +// Note that `None[T]()` is interchangeable with `Option[T]{}`. +func None[T any]() Option[T] { + return Option[T]{} +} + +// FromPointer will convert a nilable pointer into its +// equivalent Option. +func FromPointer[T any](valPtr *T) Option[T] { + if valPtr == nil { + return None[T]() + } + + if isNil(*valPtr) { + panic(fmt.Sprintf("Given pointer (%T) refers to nil, which is forbidden.", valPtr)) + } + + myCopy := *valPtr + + return Option[T]{&myCopy} +} + +// IfNotZero returns an Option that’s populated if & only if +// the given value is a non-zero value. (NB: The zero value +// for slices & maps is nil, not empty!) +// +// This is useful, e.g., to interface with code that uses +// nil to indicate a missing slice or map. +func IfNotZero[T any](value T) Option[T] { + + // copied from samber/mo.EmptyableToOption: + if reflect.ValueOf(&value).Elem().IsZero() { + return Option[T]{} + } + + return Option[T]{&value} +} + +// Get “unboxes” the Option’s internal value. +// The boolean indicates whether the value exists. +func (o Option[T]) Get() (T, bool) { + if o.val == nil { + return *new(T), false + } + + return *o.val, true +} + +// MustGet is like Get but panics if the Option is empty. +func (o Option[T]) MustGet() T { + val, exists := o.Get() + if !exists { + panic(fmt.Sprintf("MustGet() called on empty %T", o)) + } + + return val +} + +// OrZero returns either the Option’s internal value or +// the type’s zero value. +func (o Option[T]) OrZero() T { + val, exists := o.Get() + if exists { + return val + } + + return *new(T) +} + +// OrElse returns either the Option’s internal value or +// the given `fallback`. +func (o Option[T]) OrElse(fallback T) T { + val, exists := o.Get() + if exists { + return val + } + + return fallback +} + +// ToPointer converts the Option to a nilable pointer. +// The internal value (if it exists) is (shallow-)copied. +func (o Option[T]) ToPointer() *T { + val, exists := o.Get() + if exists { + theCopy := val + return &theCopy + } + + return nil +} + +// IsNone returns a boolean indicating whether or not the option is a None +// value. +func (o Option[T]) IsNone() bool { + return o.val == nil +} + +// IsSome returns a boolean indicating whether or not the option is a Some +// value. +func (o Option[T]) IsSome() bool { + return o.val != nil +} diff --git a/option/unit_test.go b/option/unit_test.go new file mode 100644 index 00000000..157bac89 --- /dev/null +++ b/option/unit_test.go @@ -0,0 +1,351 @@ +package option + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/suite" + "go.mongodb.org/mongo-driver/bson" +) + +type mySuite struct { + suite.Suite +} + +func TestUnitTestSuite(t *testing.T) { + suite.Run(t, &mySuite{}) +} + +func (s *mySuite) Test_Option_BSON() { + type MyType struct { + IsNone Option[int] + IsNoneOmitEmpty Option[int] `bson:",omitempty"` + IsSome Option[bool] + } + + type MyTypePtrs struct { + IsNone *int + IsNoneOmitEmpty *int `bson:",omitempty"` + IsSome *bool + } + + s.Run( + "marshal pointer, unmarshal Option", + func() { + + bytes, err := bson.Marshal(MyTypePtrs{ + IsNoneOmitEmpty: pointerTo(234), + IsSome: pointerTo(false), + }) + s.Require().NoError(err) + + rt := MyType{} + s.Require().NoError(bson.Unmarshal(bytes, &rt)) + + s.Assert().Equal( + MyType{ + IsNoneOmitEmpty: Some(234), + IsSome: Some(false), + }, + rt, + ) + }, + ) + + s.Run( + "marshal Option, unmarshal pointer", + func() { + + bytes, err := bson.Marshal(MyType{ + IsNoneOmitEmpty: Some(234), + IsSome: Some(false), + }) + s.Require().NoError(err) + + rt := MyTypePtrs{} + s.Require().NoError(bson.Unmarshal(bytes, &rt)) + + s.Assert().Equal( + MyTypePtrs{ + IsNoneOmitEmpty: pointerTo(234), + IsSome: pointerTo(false), + }, + rt, + ) + }, + ) + + s.Run( + "round-trip bson.D", + func() { + simpleDoc := bson.D{ + {"a", None[int]()}, + {"b", Some(123)}, + } + + bytes, err := bson.Marshal(simpleDoc) + s.Require().NoError(err) + + rt := bson.D{} + s.Require().NoError(bson.Unmarshal(bytes, &rt)) + + s.Assert().Equal( + bson.D{{"a", nil}, {"b", int32(123)}}, + rt, + ) + }, + ) + + s.Run( + "round-trip struct", + func() { + myThing := MyType{None[int](), None[int](), Some(true)} + + bytes, err := bson.Marshal(&myThing) + s.Require().NoError(err) + + // Unmarshal to a bson.D to test `omitempty`. + rtDoc := bson.D{} + s.Require().NoError(bson.Unmarshal(bytes, &rtDoc)) + + keys := make([]string, 0) + for _, el := range rtDoc { + keys = append(keys, el.Key) + } + + s.Assert().ElementsMatch( + []string{"isnone", "issome"}, + keys, + ) + + rtStruct := MyType{} + s.Require().NoError(bson.Unmarshal(bytes, &rtStruct)) + s.Assert().Equal( + myThing, + rtStruct, + ) + }, + ) +} + +func (s *mySuite) Test_Option_JSON() { + type MyType struct { + IsNone Option[int] + Omitted Option[int] + IsSome Option[bool] + } + + type MyTypePtrs struct { + IsNone *int + Omitted *int + IsSome *bool + } + + s.Run( + "marshal pointer, unmarshal Option", + func() { + + bytes, err := json.Marshal(MyTypePtrs{ + IsNone: pointerTo(234), + IsSome: pointerTo(false), + }) + s.Require().NoError(err) + + rt := MyType{} + s.Require().NoError(json.Unmarshal(bytes, &rt)) + + s.Assert().Equal( + MyType{ + IsNone: Some(234), + IsSome: Some(false), + }, + rt, + ) + }, + ) + + s.Run( + "marshal Option, unmarshal pointer", + func() { + + bytes, err := json.Marshal(MyType{ + IsNone: Some(234), + IsSome: Some(false), + }) + s.Require().NoError(err) + + rt := MyTypePtrs{} + s.Require().NoError(json.Unmarshal(bytes, &rt)) + + s.Assert().Equal( + MyTypePtrs{ + IsNone: pointerTo(234), + IsSome: pointerTo(false), + }, + rt, + ) + }, + ) + + s.Run( + "round-trip bson.D", + func() { + simpleDoc := bson.D{ + {"a", None[int]()}, + {"b", Some(123)}, + } + + bytes, err := json.Marshal(simpleDoc) + s.Require().NoError(err) + + rt := bson.D{} + s.Require().NoError(json.Unmarshal(bytes, &rt)) + + s.Assert().Equal( + bson.D{{"a", nil}, {"b", float64(123)}}, + rt, + ) + }, + ) + + s.Run( + "round-trip struct", + func() { + myThing := MyType{None[int](), None[int](), Some(true)} + + bytes, err := json.Marshal(&myThing) + s.Require().NoError(err) + + rtStruct := MyType{} + s.Require().NoError(json.Unmarshal(bytes, &rtStruct)) + s.Assert().Equal( + myThing, + rtStruct, + ) + }, + ) +} + +func (s *mySuite) Test_Option_NoNilSome() { + assertPanics(s, (chan int)(nil)) + assertPanics(s, (func())(nil)) + assertPanics(s, any(nil)) + assertPanics(s, map[int]any(nil)) + assertPanics(s, []any(nil)) + assertPanics(s, (*any)(nil)) +} + +func (s *mySuite) Test_Option_Pointer() { + opt := Some(123) + ptr := opt.ToPointer() + *ptr = 1234 + + s.Assert().Equal( + Some(123), + opt, + "ToPointer() sholuldn’t let caller alter Option value", + ) + + opt2 := FromPointer(ptr) + *ptr = 2345 + s.Assert().Equal( + Some(1234), + opt2, + "FromPointer() sholuldn’t let caller alter Option value", + ) +} + +func (s *mySuite) Test_Option() { + + //nolint:testifylint // None is, in fact, the expected value. + s.Assert().Equal( + None[int](), + Option[int]{}, + "zero value is None", + ) + + //nolint:testifylint + s.Assert().Equal(Some(1), Some(1), "same internal value") + s.Assert().NotEqual(Some(1), Some(2), "different internal value") + + foo := "foo" + fooPtr := Some(foo).ToPointer() + + s.Assert().Equal(&foo, fooPtr) + + s.Assert().Equal(Some(foo), FromPointer(fooPtr)) + + s.Assert().Equal( + foo, + Some(foo).OrZero(), + ) + + s.Assert().Equal( + "", + None[string]().OrZero(), + ) + + s.Assert().Equal( + "elf", + None[string]().OrElse("elf"), + ) + + val, has := Some(123).Get() + s.Assert().True(has) + s.Assert().Equal(123, val) + + val, has = None[int]().Get() + s.Assert().False(has) + s.Assert().Equal(0, val) + + some := Some(456) + s.Assert().True(some.IsSome()) + s.Assert().False(some.IsNone()) + + none := None[int]() + s.Assert().False(none.IsSome()) + s.Assert().True(none.IsNone()) +} + +func (s *mySuite) Test_Option_IfNonZero() { + assertIfNonZero(s, 0, 1) + assertIfNonZero(s, "", "a") + assertIfNonZero(s, []int(nil), []int{}) + assertIfNonZero(s, map[int]int(nil), map[int]int{}) + assertIfNonZero(s, any(nil), any(0)) + assertIfNonZero(s, bson.D(nil), bson.D{}) + + type myStruct struct { + name string + } + + assertIfNonZero(s, myStruct{}, myStruct{"foo"}) +} + +func assertIfNonZero[T any](s *mySuite, zeroVal, nonZeroVal T) { + noneOpt := IfNotZero(zeroVal) + someOpt := IfNotZero(nonZeroVal) + + s.Assert().Equal(None[T](), noneOpt) + s.Assert().Equal(Some(nonZeroVal), someOpt) +} + +func pointerTo[T any](val T) *T { + return &val +} + +func assertPanics[T any](s *mySuite, val T) { + s.T().Helper() + + s.Assert().Panics( + func() { Some(val) }, + "Some(%T)", + val, + ) + + s.Assert().Panics( + func() { FromPointer(&val) }, + "FromPointer(&%T)", + val, + ) +} diff --git a/option/validate.go b/option/validate.go new file mode 100644 index 00000000..698ae75b --- /dev/null +++ b/option/validate.go @@ -0,0 +1,28 @@ +package option + +import ( + "reflect" + + mapset "github.com/deckarep/golang-set/v2" +) + +var nilable = mapset.NewThreadUnsafeSet( + reflect.Chan, + reflect.Func, + reflect.Interface, + reflect.Map, + reflect.Pointer, + reflect.Slice, +) + +func isNil(val any) bool { + if val == nil { + return true + } + + if nilable.Contains(reflect.TypeOf(val).Kind()) { + return reflect.ValueOf(val).IsNil() + } + + return false +} From a48050a0b6d508952aca157f39d12bf50b83c40c Mon Sep 17 00:00:00 2001 From: Felipe Gasper Date: Fri, 22 Nov 2024 13:38:48 -0500 Subject: [PATCH 2/8] fix aggregation --- internal/verifier/migration_verifier.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/verifier/migration_verifier.go b/internal/verifier/migration_verifier.go index 6438036a..30b9d5d2 100644 --- a/internal/verifier/migration_verifier.go +++ b/internal/verifier/migration_verifier.go @@ -891,8 +891,8 @@ func (verifier *Verifier) doIndexSpecsMatch(ctx context.Context, srcSpec bson.Ra ctx, mongo.Pipeline{ { + {"$match", bson.D{{"_id", insert.InsertedID}}}, {"$match", bson.D{ - {"_id", insert.InsertedID}, {"$expr", bson.D{ {"$eq", bson.A{ "$spec", From 2e666dbe0db5b036717f608d53b3a1ca9d0048bf Mon Sep 17 00:00:00 2001 From: Felipe Gasper Date: Fri, 22 Nov 2024 14:00:57 -0500 Subject: [PATCH 3/8] update tests --- internal/verifier/migration_verifier.go | 33 +-- internal/verifier/migration_verifier_test.go | 213 ++++++------------- 2 files changed, 76 insertions(+), 170 deletions(-) diff --git a/internal/verifier/migration_verifier.go b/internal/verifier/migration_verifier.go index 30b9d5d2..d1261054 100644 --- a/internal/verifier/migration_verifier.go +++ b/internal/verifier/migration_verifier.go @@ -887,33 +887,22 @@ func (verifier *Verifier) doIndexSpecsMatch(ctx context.Context, srcSpec bson.Ra coll.DeleteOne(ctx, bson.M{"_id": insert.InsertedID}) }() - cursor, err := coll.Aggregate( + count, err := coll.CountDocuments( ctx, - mongo.Pipeline{ - { - {"$match", bson.D{{"_id", insert.InsertedID}}}, - {"$match", bson.D{ - {"$expr", bson.D{ - {"$eq", bson.A{ - "$spec", - dstSpec, - }}, - }}, - }}, - {"$project", bson.D{{"_id", 1}}}, - }, + bson.D{ + {"_id", insert.InsertedID}, + {"spec", dstSpec}, }, ) - if err != nil { - return false, errors.Wrap(err, "failed to check index specification match in metadata") - } - var docs []bson.Raw - err = cursor.All(ctx, &docs) - if err != nil { - return false, errors.Wrap(err, "failed to parse index specification match’s result") + + switch count { + case 0: + return false, nil + case 1: + return true, nil } - return len(docs) == 1, nil + return false, errors.Errorf("weirdly received %d matching index docs (should be 0 or 1)", count) } /* diff --git a/internal/verifier/migration_verifier_test.go b/internal/verifier/migration_verifier_test.go index 63a25d05..4b552778 100644 --- a/internal/verifier/migration_verifier_test.go +++ b/internal/verifier/migration_verifier_test.go @@ -996,161 +996,78 @@ func (suite *IntegrationTestSuite) TestVerifierCompareIndexes() { } } -/* -func TestVerifierCompareIndexSpecs(t *testing.T) { - // Index specification - keysDoc1 := bson.D{{"a", 1}, {"b", -1}} - // We marshal the key document twice so they are physically separate memory. - keysRaw1, err := bson.Marshal(keysDoc1) - require.NoError(t, err) - keysRaw2, err := bson.Marshal(keysDoc1) - require.NoError(t, err) - simpleIndexSpec1 := mongo.IndexSpecification{ - Name: "testIndex", - Namespace: "testDB.testIndex", - KeysDocument: keysRaw1, - Version: 1} - - simpleIndexSpec2 := mongo.IndexSpecification{ - Name: "testIndex", - Namespace: "testDB.testIndex", - KeysDocument: keysRaw2, - Version: 2} - - results := compareIndexSpecifications(&simpleIndexSpec1, &simpleIndexSpec2) - assert.Nil(t, results) - - // Changing version should not be an issue - simpleIndexSpec3 := simpleIndexSpec2 - simpleIndexSpec3.Version = 4 - results = compareIndexSpecifications(&simpleIndexSpec1, &simpleIndexSpec3) - assert.Nil(t, results) - - // Changing the key spec order matters - keysDoc3 := bson.D{{"b", -1}, {"a", 1}} - keysRaw3, err := bson.Marshal(keysDoc3) - require.NoError(t, err) - simpleIndexSpec3 = simpleIndexSpec2 - simpleIndexSpec3.KeysDocument = keysRaw3 - results = compareIndexSpecifications(&simpleIndexSpec1, &simpleIndexSpec3) - if assert.Equalf(t, 1, len(results), "Actual mismatches: %+v", results) { - result := results[0] - assert.Equal(t, "testIndex", result.ID) - assert.Equal(t, "testDB.testIndex", result.NameSpace) - assert.Equal(t, "KeysDocument", result.Field) - assert.Regexp(t, regexp.MustCompile("^"+Mismatch), result.Details) - assert.NotRegexp(t, regexp.MustCompile("0x"), result.Details) - } - - // Shortening the key mattes - keysDoc3 = bson.D{{"a", 1}} - keysRaw3, err = bson.Marshal(keysDoc3) - require.NoError(t, err) - simpleIndexSpec3 = simpleIndexSpec2 - simpleIndexSpec3.KeysDocument = keysRaw3 - results = compareIndexSpecifications(&simpleIndexSpec1, &simpleIndexSpec3) - if assert.Equalf(t, 1, len(results), "Actual mismatches: %+v", results) { - result := results[0] - assert.Equal(t, "testIndex", result.ID) - assert.Equal(t, "testDB.testIndex", result.NameSpace) - assert.Equal(t, "KeysDocument", result.Field) - assert.Regexp(t, regexp.MustCompile("^"+Mismatch), result.Details) - assert.NotRegexp(t, regexp.MustCompile("0x"), result.Details) - } +func (suite *IntegrationTestSuite) TestVerifierCompareIndexSpecs() { + ctx := suite.Context() + verifier := suite.BuildVerifier() - var expireAfterSeconds30, expireAfterSeconds0_1, expireAfterSeconds0_2 int32 - expireAfterSeconds30 = 30 - expireAfterSeconds0_1, expireAfterSeconds0_2 = 0, 0 - sparseTrue := true - sparseFalse_1, sparseFalse_2 := false, false - uniqueTrue := true - uniqueFalse_1, uniqueFalse_2 := false, false - clusteredTrue := true - clusteredFalse_1, clusteredFalse_2 := false, false - fullIndexSpec1 := mongo.IndexSpecification{ - Name: "testIndex", - Namespace: "testDB.testIndex", - KeysDocument: keysRaw1, - Version: 1, - ExpireAfterSeconds: &expireAfterSeconds0_1, - Sparse: &sparseFalse_1, - Unique: &uniqueFalse_1, - Clustered: &clusteredFalse_1} - - fullIndexSpec2 := mongo.IndexSpecification{ - Name: "testIndex", - Namespace: "testDB.testIndex", - KeysDocument: keysRaw2, - Version: 2, - ExpireAfterSeconds: &expireAfterSeconds0_2, - Sparse: &sparseFalse_2, - Unique: &uniqueFalse_2, - Clustered: &clusteredFalse_2} - - results = compareIndexSpecifications(&fullIndexSpec1, &fullIndexSpec2) - assert.Nil(t, results) - - // The full index spec should not equal the equivalent simple index spec. - results = compareIndexSpecifications(&fullIndexSpec1, &simpleIndexSpec2) - var diffFields []interface{} - for _, result := range results { - assert.Equal(t, "testIndex", result.ID) - assert.Equal(t, "testDB.testIndex", result.NameSpace) - assert.Regexp(t, regexp.MustCompile("^"+Mismatch), result.Details) - assert.NotRegexp(t, regexp.MustCompile("0x"), result.Details) - diffFields = append(diffFields, result.Field) - } - assert.ElementsMatch(t, []string{"Sparse", "Unique", "ExpireAfterSeconds", "Clustered"}, diffFields) - - fullIndexSpec3 := fullIndexSpec2 - fullIndexSpec3.ExpireAfterSeconds = &expireAfterSeconds30 - results = compareIndexSpecifications(&fullIndexSpec1, &fullIndexSpec3) - if assert.Equalf(t, 1, len(results), "Actual mismatches: %+v", results) { - result := results[0] - assert.Equal(t, "testIndex", result.ID) - assert.Equal(t, "testDB.testIndex", result.NameSpace) - assert.Equal(t, "ExpireAfterSeconds", result.Field) - assert.Regexp(t, regexp.MustCompile("^"+Mismatch), result.Details) - assert.NotRegexp(t, regexp.MustCompile("0x"), result.Details) - } + cases := []struct { + label string + src bson.D + dst bson.D + shouldMatch bool + }{ + { + label: "simple", + src: bson.D{ + {"name", "testIndex"}, + {"key", bson.M{"foo": 123}}, + }, + dst: bson.D{ + {"name", "testIndex"}, + {"key", bson.M{"foo": 123}}, + }, + shouldMatch: true, + }, + { + label: "ignore number types", + src: bson.D{ + {"name", "testIndex"}, + {"key", bson.M{"foo": 123}}, + }, + dst: bson.D{ + {"name", "testIndex"}, + {"key", bson.M{"foo": float64(123)}}, + }, + shouldMatch: true, + }, - fullIndexSpec3 = fullIndexSpec2 - fullIndexSpec3.Sparse = &sparseTrue - results = compareIndexSpecifications(&fullIndexSpec1, &fullIndexSpec3) - if assert.Equalf(t, 1, len(results), "Actual mismatches: %+v", results) { - result := results[0] - assert.Equal(t, "testIndex", result.ID) - assert.Equal(t, "testDB.testIndex", result.NameSpace) - assert.Equal(t, "Sparse", result.Field) - assert.Regexp(t, regexp.MustCompile("^"+Mismatch), result.Details) - assert.NotRegexp(t, regexp.MustCompile("0x"), result.Details) - } + { + label: "find number differences", + src: bson.D{ + {"name", "testIndex"}, + {"key", bson.M{"foo": 1}}, + }, + dst: bson.D{ + {"name", "testIndex"}, + {"key", bson.M{"foo": -1}}, + }, + shouldMatch: false, + }, - fullIndexSpec3 = fullIndexSpec2 - fullIndexSpec3.Unique = &uniqueTrue - results = compareIndexSpecifications(&fullIndexSpec1, &fullIndexSpec3) - if assert.Equalf(t, 1, len(results), "Actual mismatches: %+v", results) { - result := results[0] - assert.Equal(t, "testIndex", result.ID) - assert.Equal(t, "testDB.testIndex", result.NameSpace) - assert.Equal(t, "Unique", result.Field) - assert.Regexp(t, regexp.MustCompile("^"+Mismatch), result.Details) - assert.NotRegexp(t, regexp.MustCompile("0x"), result.Details) + { + label: "key order differences", + src: bson.D{ + {"name", "testIndex"}, + {"key", bson.D{{"foo", 1}, {"bar", 1}}}, + }, + dst: bson.D{ + {"name", "testIndex"}, + {"key", bson.D{{"bar", 1}, {"foo", 1}}}, + }, + shouldMatch: false, + }, } - fullIndexSpec3 = fullIndexSpec2 - fullIndexSpec3.Clustered = &clusteredTrue - results = compareIndexSpecifications(&fullIndexSpec1, &fullIndexSpec3) - if assert.Equalf(t, 1, len(results), "Actual mismatches: %+v", results) { - result := results[0] - assert.Equal(t, "testIndex", result.ID) - assert.Equal(t, "testDB.testIndex", result.NameSpace) - assert.Equal(t, "Clustered", result.Field) - assert.Regexp(t, regexp.MustCompile("^"+Mismatch), result.Details) - assert.NotRegexp(t, regexp.MustCompile("0x"), result.Details) + for _, curCase := range cases { + matchYN, err := verifier.doIndexSpecsMatch( + ctx, + testutil.MustMarshal(curCase.src), + testutil.MustMarshal(curCase.dst), + ) + suite.Require().NoError(err) + suite.Assert().Equal(curCase.shouldMatch, matchYN, curCase.label) } } -*/ func (suite *IntegrationTestSuite) TestVerifierNamespaceList() { verifier := suite.BuildVerifier() From 1a99b12043872bd9bce65862a479fd34f451cb2e Mon Sep 17 00:00:00 2001 From: Felipe Gasper Date: Fri, 22 Nov 2024 14:09:24 -0500 Subject: [PATCH 4/8] docs --- internal/util/collections.go | 11 ++++++++--- internal/verifier/migration_verifier.go | 4 ++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/util/collections.go b/internal/util/collections.go index 10e5e3ef..00d2d819 100644 --- a/internal/util/collections.go +++ b/internal/util/collections.go @@ -10,7 +10,9 @@ import ( "go.mongodb.org/mongo-driver/mongo" ) -// CollectionSpec is like mongo.CollectionSpecification, +// CollectionSpec is like mongo.CollectionSpecification except: +// - IDIndex is a bson.Raw rather than mongo.IndexSpecification. +// - It can detect unexpected fields. type CollectionSpec struct { Name string Type string @@ -26,12 +28,15 @@ type CollectionSpec struct { Extra map[string]any } -// Returns full name of collection including database name +// FullName returns the collection's full namespace. func FullName(collection *mongo.Collection) string { return collection.Database().Name() + "." + collection.Name() } -func GetCollectionSpec( +// GetCollectionSpecIfExists returns the given collection’s specification, +// or empty if the collection doesn’t exist. If any unexpected properties +// exist in the collection specification then an error is returned. +func GetCollectionSpecIfExists( ctx context.Context, coll *mongo.Collection, ) (option.Option[CollectionSpec], error) { diff --git a/internal/verifier/migration_verifier.go b/internal/verifier/migration_verifier.go index d1261054..fbcd54b2 100644 --- a/internal/verifier/migration_verifier.go +++ b/internal/verifier/migration_verifier.go @@ -1123,12 +1123,12 @@ func (verifier *Verifier) verifyMetadataAndPartitionCollection(ctx context.Conte srcNs := FullName(srcColl) dstNs := FullName(dstColl) - srcSpecOpt, srcErr := util.GetCollectionSpec(ctx, srcColl) + srcSpecOpt, srcErr := util.GetCollectionSpecIfExists(ctx, srcColl) if srcErr != nil { verifier.markCollectionFailed(workerNum, task, ClusterSource, srcNs, srcErr) } - dstSpecOpt, dstErr := util.GetCollectionSpec(ctx, dstColl) + dstSpecOpt, dstErr := util.GetCollectionSpecIfExists(ctx, dstColl) if dstErr != nil { verifier.markCollectionFailed(workerNum, task, ClusterTarget, dstNs, dstErr) } From 9bd5c078b4045c5966c65abe9f6777f2565557b9 Mon Sep 17 00:00:00 2001 From: Felipe Gasper Date: Fri, 22 Nov 2024 14:22:34 -0500 Subject: [PATCH 5/8] =?UTF-8?q?accommodate=204.2=20indexes=20that=20have?= =?UTF-8?q?=20=E2=80=9Cns=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/verifier/migration_verifier.go | 40 +++++++++++++++++--- internal/verifier/migration_verifier_test.go | 15 ++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/internal/verifier/migration_verifier.go b/internal/verifier/migration_verifier.go index aaa0746a..3cf8bfd6 100644 --- a/internal/verifier/migration_verifier.go +++ b/internal/verifier/migration_verifier.go @@ -23,6 +23,7 @@ import ( "github.com/10gen/migration-verifier/internal/util" "github.com/10gen/migration-verifier/internal/uuidutil" "github.com/10gen/migration-verifier/mbson" + "github.com/10gen/migration-verifier/mslices" "github.com/10gen/migration-verifier/option" "github.com/olekukonko/tablewriter" "github.com/pkg/errors" @@ -887,14 +888,44 @@ func (verifier *Verifier) doIndexSpecsMatch(ctx context.Context, srcSpec bson.Ra coll.DeleteOne(ctx, bson.M{"_id": insert.InsertedID}) }() - count, err := coll.CountDocuments( + cursor, err := coll.Aggregate( ctx, - bson.D{ - {"_id", insert.InsertedID}, - {"spec", dstSpec}, + mongo.Pipeline{ + // Select our source spec. + {{"$match", bson.D{{"_id", insert.InsertedID}}}}, + + // Add the destination spec. + {{"$addFields", bson.D{ + {"dstSpec", dstSpec}, + }}}, + + // Remove the “ns” field from both. (NB: 4.4+ don’t create these, + // though in-place upgrades may cause them still to exist.) + {{"$addFields", bson.D{ + {"spec.ns", "$$REMOVE"}, + {"dstSpec.ns", "$$REMOVE"}, + }}}, + + // Now check to be sure that those specs match. + {{"$match", bson.D{ + {"$expr", bson.D{ + {"$eq", mslices.Of("$spec", "$dstSpec")}, + }}, + }}}, }, ) + if err != nil { + return false, errors.Wrap(err, "failed to check index specification match in metadata") + } + var docs []bson.Raw + err = cursor.All(ctx, &docs) + if err != nil { + return false, errors.Wrap(err, "failed to parse index specification match’s result") + } + + count := len(docs) + switch count { case 0: return false, nil @@ -1212,7 +1243,6 @@ func (verifier *Verifier) verifyMetadataAndPartitionCollection(ctx context.Conte partitions, shardKeys, docsCount, bytesCount, err := verifier.partitionAndInspectNamespace(ctx, srcNs) if err != nil { task.Status = verificationTaskFailed - verifier.logger.Error().Msgf("[Worker %d] Error partitioning collection: %+v", workerNum, err) return } diff --git a/internal/verifier/migration_verifier_test.go b/internal/verifier/migration_verifier_test.go index b16da4ba..10317f76 100644 --- a/internal/verifier/migration_verifier_test.go +++ b/internal/verifier/migration_verifier_test.go @@ -1019,6 +1019,21 @@ func (suite *IntegrationTestSuite) TestVerifierCompareIndexSpecs() { }, shouldMatch: true, }, + + { + label: "ignore `ns` field", + src: bson.D{ + {"name", "testIndex"}, + {"key", bson.M{"foo": 123}}, + {"ns", "foo.bar"}, + }, + dst: bson.D{ + {"name", "testIndex"}, + {"key", bson.M{"foo": 123}}, + }, + shouldMatch: true, + }, + { label: "ignore number types", src: bson.D{ From 24a404a4ea4c92de0352c3153f6df4fd646d4ebd Mon Sep 17 00:00:00 2001 From: Felipe Gasper Date: Fri, 22 Nov 2024 14:29:00 -0500 Subject: [PATCH 6/8] lint --- internal/verifier/migration_verifier.go | 63 +------------------------ 1 file changed, 1 insertion(+), 62 deletions(-) diff --git a/internal/verifier/migration_verifier.go b/internal/verifier/migration_verifier.go index 3cf8bfd6..77651665 100644 --- a/internal/verifier/migration_verifier.go +++ b/internal/verifier/migration_verifier.go @@ -885,7 +885,7 @@ func (verifier *Verifier) doIndexSpecsMatch(ctx context.Context, srcSpec bson.Ra } defer func() { - coll.DeleteOne(ctx, bson.M{"_id": insert.InsertedID}) + _, _ = coll.DeleteOne(ctx, bson.M{"_id": insert.InsertedID}) }() cursor, err := coll.Aggregate( @@ -936,67 +936,6 @@ func (verifier *Verifier) doIndexSpecsMatch(ctx context.Context, srcSpec bson.Ra return false, errors.Errorf("weirdly received %d matching index docs (should be 0 or 1)", count) } -/* - - // Order is always significant in the keys document. - if !bytes.Equal(srcSpec.KeysDocument, dstSpec.KeysDocument) { - results = append(results, VerificationResult{ - NameSpace: dstSpec.Namespace, - Cluster: ClusterTarget, - ID: dstSpec.Name, - Field: "KeysDocument", - Details: Mismatch + fmt.Sprintf(" : src: %v, dst: %v", srcSpec.KeysDocument, dstSpec.KeysDocument)}) - } - - // We don't check version because it may change when migrating between server versions. - - if !reflect.DeepEqual(srcSpec.ExpireAfterSeconds, dstSpec.ExpireAfterSeconds) { - results = append(results, VerificationResult{ - NameSpace: dstSpec.Namespace, - Cluster: ClusterTarget, - ID: dstSpec.Name, - Field: "ExpireAfterSeconds", - Details: Mismatch + fmt.Sprintf(" : src: %s, dst: %s", nilableToString(srcSpec.ExpireAfterSeconds), nilableToString(dstSpec.ExpireAfterSeconds))}) - } - - if !reflect.DeepEqual(srcSpec.Sparse, dstSpec.Sparse) { - results = append(results, VerificationResult{ - NameSpace: dstSpec.Namespace, - Cluster: ClusterTarget, - ID: dstSpec.Name, - Field: "Sparse", - Details: Mismatch + fmt.Sprintf(" : src: %s, dst: %s", nilableToString(srcSpec.Sparse), nilableToString(dstSpec.Sparse))}) - } - - if !reflect.DeepEqual(srcSpec.Unique, dstSpec.Unique) { - results = append(results, VerificationResult{ - NameSpace: dstSpec.Namespace, - Cluster: ClusterTarget, - ID: dstSpec.Name, - Field: "Unique", - Details: Mismatch + fmt.Sprintf(" : src: %s, dst: %s", nilableToString(srcSpec.Unique), nilableToString(dstSpec.Unique))}) - } - - if !reflect.DeepEqual(srcSpec.Clustered, dstSpec.Clustered) { - results = append(results, VerificationResult{ - NameSpace: dstSpec.Namespace, - Cluster: ClusterTarget, - ID: dstSpec.Name, - Field: "Clustered", - Details: Mismatch + fmt.Sprintf(" : src: %s, dst: %s", nilableToString(srcSpec.Clustered), nilableToString(dstSpec.Clustered))}) - } - return results -} -*/ - -func nilableToString[T any](ptr *T) string { - if ptr == nil { - return "(unset)" - } - - return fmt.Sprintf("%v", *ptr) -} - func (verifier *Verifier) ProcessCollectionVerificationTask(ctx context.Context, workerNum int, task *VerificationTask) { verifier.logger.Debug().Msgf("[Worker %d] Processing collection", workerNum) verifier.verifyMetadataAndPartitionCollection(ctx, workerNum, task) From c71de2ec844491995ad7aacd34b4dfade8c35964 Mon Sep 17 00:00:00 2001 From: Felipe Gasper Date: Mon, 25 Nov 2024 08:51:59 -0500 Subject: [PATCH 7/8] Update option/bson.go Co-authored-by: Jian Guan <61915096+tdq45gj@users.noreply.github.com> --- option/bson.go | 1 - 1 file changed, 1 deletion(-) diff --git a/option/bson.go b/option/bson.go index 3394185c..956fa5d6 100644 --- a/option/bson.go +++ b/option/bson.go @@ -19,7 +19,6 @@ func (o Option[T]) MarshalBSONValue() (bsontype.Type, []byte, error) { // UnmarshalBSONValue implements bson.ValueUnmarshaler. func (o *Option[T]) UnmarshalBSONValue(bType bsontype.Type, raw []byte) error { - switch bType { case bson.TypeNull: o.val = nil From eafda64988f1b570872c22a1d34f39430cbf05b4 Mon Sep 17 00:00:00 2001 From: Felipe Gasper Date: Mon, 25 Nov 2024 09:32:26 -0500 Subject: [PATCH 8/8] Switch to $documents for index comparison. --- internal/verifier/migration_verifier.go | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/internal/verifier/migration_verifier.go b/internal/verifier/migration_verifier.go index 77651665..52525a89 100644 --- a/internal/verifier/migration_verifier.go +++ b/internal/verifier/migration_verifier.go @@ -873,26 +873,15 @@ func (verifier *Verifier) doIndexSpecsMatch(ctx context.Context, srcSpec bson.Ra } // Next check to see if the only differences are type differences. - // If we didn’t support pre-v5 servers we could use a $documents aggregation, - // but we can’t do that, so we write to a temporary collection. - coll := verifier.metaClient.Database(verifier.metaDBName).Collection("indexCompare") - insert, err := coll.InsertOne( - ctx, - bson.M{"spec": srcSpec}, - ) - if err != nil { - return false, errors.Wrap(err, "failed to persist index specification to metadata") - } - - defer func() { - _, _ = coll.DeleteOne(ctx, bson.M{"_id": insert.InsertedID}) - }() - - cursor, err := coll.Aggregate( + // (We can safely use $documents here since this is against the metadata + // cluster, which we can require to be v5+.) + db := verifier.metaClient.Database(verifier.metaDBName) + cursor, err := db.Aggregate( ctx, mongo.Pipeline{ - // Select our source spec. - {{"$match", bson.D{{"_id", insert.InsertedID}}}}, + {{"$documents", []bson.D{ + {{"spec", srcSpec}}, + }}}, // Add the destination spec. {{"$addFields", bson.D{