GODRIVER-2698 Make Comment fields any-type on all options structs and…

… setters (#1454)

internal/integration/unified/client_operation_execution.go

@@ -55,11 +55,7 @@ func executeCreateChangeStream(ctx context.Context, operation *operation) (*oper
 			}
 			opts.SetCollation(*collation)
 		case "comment":
-			commentString, err := createCommentString(val)
-			if err != nil {
-				return nil, fmt.Errorf("error creating comment: %v", err)
-			}
-			opts.SetComment(commentString)
+			opts.SetComment(val)
 		case "fullDocument":
 			switch fd := val.StringValue(); fd {
 			case "default":

internal/integration/unified/collection_operation_execution.go

@@ -60,13 +60,7 @@ func executeAggregate(ctx context.Context, operation *operation) (*operationResu
 			}
 			opts.SetCollation(collation)
 		case "comment":
-			// TODO(GODRIVER-2386): when document support for comments is added, we can replace this switch condition
-			// TODO with `opts.SetComment(val)`
-			commentString, err := createCommentString(val)
-			if err != nil {
-				return nil, fmt.Errorf("error creating comment: %v", err)
-			}
-			opts.SetComment(commentString)
+			opts.SetComment(val)
 		case "hint":
 			hint, err := createHint(val)
 			if err != nil {
@@ -188,13 +182,7 @@ func executeCountDocuments(ctx context.Context, operation *operation) (*operatio
 			}
 			opts.SetCollation(collation)
 		case "comment":
-			// TODO(GODRIVER-2386): when document support for comments is added, we can replace this switch condition
-			// TODO with `opts.SetComment(val)`
-			commentString, err := createCommentString(val)
-			if err != nil {
-				return nil, fmt.Errorf("error creating comment: %v", err)
-			}
-			opts.SetComment(commentString)
+			opts.SetComment(val)
 		case "filter":
 			filter = val.Document()
 		case "hint":
@@ -1393,13 +1381,7 @@ func createFindCursor(ctx context.Context, operation *operation) (*cursorResult,
 			}
 			opts.SetCollation(collation)
 		case "comment":
-			// TODO(GODRIVER-2386): when document support for comments is added, we can replace this switch condition
-			// TODO with `opts.SetComment(val)`
-			commentString, err := createCommentString(val)
-			if err != nil {
-				return nil, fmt.Errorf("error creating comment: %v", err)
-			}
-			opts.SetComment(commentString)
+			opts.SetComment(val)
 		case "filter":
 			filter = val.Document()
 		case "hint":

internal/integration/unified/crud_helpers.go

@@ -159,14 +159,3 @@ func createHint(val bson.RawValue) (interface{}, error) {
 	}
 	return hint, nil
 }
-
-func createCommentString(val bson.RawValue) (string, error) {
-	switch val.Type {
-	case bsontype.String:
-		return val.StringValue(), nil
-	case bsontype.EmbeddedDocument:
-		return val.String(), nil
-	default:
-		return "", fmt.Errorf("unrecognized 'comment' value type: %T", val)
-	}
-}

internal/integration/unified/matches.go

@@ -97,18 +97,6 @@ func verifyValuesMatchInner(ctx context.Context, expected, actual bson.RawValue)
 				return newMatchingError(fullKeyPath, "key not found in actual document")
 			}
 
-			// Check to see if the keypath requires us to convert actual/expected to make a true comparison.  If the
-			// comparison is not supported for the keypath, continue with the recursive strategy.
-			//
-			// TODO(GODRIVER-2386): this branch of logic will be removed once we add document support for comments
-			mixedTypeEvaluated, err := evaluateMixedTypeComparison(expectedKey, expectedValue, actualValue)
-			if err != nil {
-				return newMatchingError(fullKeyPath, "error doing mixed-type matching assertion: %v", err)
-			}
-			if mixedTypeEvaluated {
-				continue
-			}
-
 			// Nested documents cannot have extra keys, so we unconditionally pass false for extraKeysAllowed.
 			comparisonCtx := makeMatchContext(ctx, fullKeyPath, false)
 			if err := verifyValuesMatchInner(comparisonCtx, expectedValue, actualValue); err != nil {
@@ -185,37 +173,6 @@ func verifyValuesMatchInner(ctx context.Context, expected, actual bson.RawValue)
 	return nil
 }
 
-// compareDocumentToString will compare an expected document to an actual string by converting the document into a
-// string.
-func compareDocumentToString(expected, actual bson.RawValue) error {
-	expectedDocument, ok := expected.DocumentOK()
-	if !ok {
-		return fmt.Errorf("expected value to be a document but got a %s", expected.Type)
-	}
-
-	actualString, ok := actual.StringValueOK()
-	if !ok {
-		return fmt.Errorf("expected value to be a string but got a %s", actual.Type)
-	}
-
-	if actualString != expectedDocument.String() {
-		return fmt.Errorf("expected value %s, got %s", expectedDocument.String(), actualString)
-	}
-	return nil
-}
-
-// evaluateMixedTypeComparison compares an expected document with an actual string.  If this comparison occurs, then
-// the function will return `true` along with any resulting error.
-func evaluateMixedTypeComparison(expectedKey string, expected, actual bson.RawValue) (bool, error) {
-	switch expectedKey {
-	case "comment":
-		if expected.Type == bsontype.EmbeddedDocument && actual.Type == bsontype.String {
-			return true, compareDocumentToString(expected, actual)
-		}
-	}
-	return false, nil
-}
-
 func evaluateSpecialComparison(ctx context.Context, assertionDoc bson.Raw, actual bson.RawValue) error {
 	assertionElem := assertionDoc.Index(0)
 	assertion := assertionElem.Key()

mongo/change_stream.go

@@ -191,14 +191,14 @@ func newChangeStream(ctx context.Context, config changeStreamConfig, pipeline in
 	if cs.options.Collation != nil {
 		cs.aggregate.Collation(bsoncore.Document(cs.options.Collation.ToDocument()))
 	}
-	if comment := cs.options.Comment; comment != nil {
-		cs.aggregate.Comment(*comment)
-
-		commentVal, err := marshalValue(comment, cs.bsonOpts, cs.registry)
+	if cs.options.Comment != nil {
+		comment, err := marshalValue(cs.options.Comment, cs.bsonOpts, cs.registry)
 		if err != nil {
 			return nil, err
 		}
-		cs.cursorOptions.Comment = commentVal
+
+		cs.aggregate.Comment(comment)
+		cs.cursorOptions.Comment = comment
 	}
 	if cs.options.BatchSize != nil {
 		cs.aggregate.BatchSize(*cs.options.BatchSize)

mongo/collection.go

@@ -1046,13 +1046,13 @@ func aggregate(a aggregateParams) (cur *Cursor, err error) {
 		cursorOpts.MaxTimeMS = int64(*ao.MaxAwaitTime / time.Millisecond)
 	}
 	if ao.Comment != nil {
-		op.Comment(*ao.Comment)
-
-		commentVal, err := marshalValue(ao.Comment, a.bsonOpts, a.registry)
+		comment, err := marshalValue(ao.Comment, a.bsonOpts, a.registry)
 		if err != nil {
 			return nil, err
 		}
-		cursorOpts.Comment = commentVal
+
+		op.Comment(comment)
+		cursorOpts.Comment = comment
 	}
 	if ao.Hint != nil {
 		if isUnorderedMap(ao.Hint) {
@@ -1176,7 +1176,12 @@ func (coll *Collection) CountDocuments(ctx context.Context, filter interface{},
 		op.Collation(bsoncore.Document(countOpts.Collation.ToDocument()))
 	}
 	if countOpts.Comment != nil {
-		op.Comment(*countOpts.Comment)
+		comment, err := marshalValue(countOpts.Comment, coll.bsonOpts, coll.registry)
+		if err != nil {
+			return 0, err
+		}
+
+		op.Comment(comment)
 	}
 	if countOpts.Hint != nil {
 		if isUnorderedMap(countOpts.Hint) {
@@ -1531,13 +1536,13 @@ func (coll *Collection) Find(ctx context.Context, filter interface{},
 		op.Collation(bsoncore.Document(fo.Collation.ToDocument()))
 	}
 	if fo.Comment != nil {
-		op.Comment(*fo.Comment)
-
-		commentVal, err := marshalValue(fo.Comment, coll.bsonOpts, coll.registry)
+		comment, err := marshalValue(fo.Comment, coll.bsonOpts, coll.registry)
 		if err != nil {
 			return nil, err
 		}
-		cursorOpts.Comment = commentVal
+
+		op.Comment(comment)
+		cursorOpts.Comment = comment
 	}
 	if fo.CursorType != nil {
 		switch *fo.CursorType {
@@ -1637,27 +1642,13 @@ func (coll *Collection) Find(ctx context.Context, filter interface{},
 	return newCursorWithSession(bc, coll.bsonOpts, coll.registry, sess)
 }
 
-// FindOne executes a find command and returns a SingleResult for one document in the collection.
-//
-// The filter parameter must be a document containing query operators and can be used to select the document to be
-// returned. It cannot be nil. If the filter does not match any documents, a SingleResult with an error set to
-// ErrNoDocuments will be returned. If the filter matches multiple documents, one will be selected from the matched set.
-//
-// The opts parameter can be used to specify options for this operation (see the options.FindOneOptions documentation).
-//
-// For more information about the command, see https://www.mongodb.com/docs/manual/reference/command/find/.
-func (coll *Collection) FindOne(ctx context.Context, filter interface{},
-	opts ...*options.FindOneOptions) *SingleResult {
-
-	if ctx == nil {
-		ctx = context.Background()
-	}
-
+func newFindOptionsFromFindOneOptions(opts ...*options.FindOneOptions) []*options.FindOptions {
 	findOpts := make([]*options.FindOptions, 0, len(opts))
 	for _, opt := range opts {
 		if opt == nil {
 			continue
 		}
+
 		findOpts = append(findOpts, &options.FindOptions{
 			AllowPartialResults: opt.AllowPartialResults,
 			Collation:           opt.Collation,
@@ -1673,11 +1664,31 @@ func (coll *Collection) FindOne(ctx context.Context, filter interface{},
 			Sort:                opt.Sort,
 		})
 	}
-	// Unconditionally send a limit to make sure only one document is returned and the cursor is not kept open
-	// by the server.
+
+	// Unconditionally send a limit to make sure only one document is returned and
+	// the cursor is not kept open by the server.
 	findOpts = append(findOpts, options.Find().SetLimit(-1))
 
-	cursor, err := coll.Find(ctx, filter, findOpts...)
+	return findOpts
+}
+
+// FindOne executes a find command and returns a SingleResult for one document in the collection.
+//
+// The filter parameter must be a document containing query operators and can be used to select the document to be
+// returned. It cannot be nil. If the filter does not match any documents, a SingleResult with an error set to
+// ErrNoDocuments will be returned. If the filter matches multiple documents, one will be selected from the matched set.
+//
+// The opts parameter can be used to specify options for this operation (see the options.FindOneOptions documentation).
+//
+// For more information about the command, see https://www.mongodb.com/docs/manual/reference/command/find/.
+func (coll *Collection) FindOne(ctx context.Context, filter interface{},
+	opts ...*options.FindOneOptions) *SingleResult {
+
+	if ctx == nil {
+		ctx = context.Background()
+	}
+
+	cursor, err := coll.Find(ctx, filter, newFindOptionsFromFindOneOptions(opts...)...)
 	return &SingleResult{
 		ctx:      ctx,
 		cur:      cursor,

mongo/collection_test.go

@@ -217,3 +217,100 @@ func TestCollection(t *testing.T) {
 		assert.Equal(t, aggErr, err, "expected error %v, got %v", aggErr, err)
 	})
 }
+
+func TestNewFindOptionsFromFindOneOptions(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		opts []*options.FindOneOptions
+		want []*options.FindOptions
+	}{
+		{
+			name: "nil",
+			opts: nil,
+			want: []*options.FindOptions{
+				options.Find().SetLimit(-1),
+			},
+		},
+		{
+			name: "empty",
+			opts: []*options.FindOneOptions{},
+			want: []*options.FindOptions{
+				options.Find().SetLimit(-1),
+			},
+		},
+		{
+			name: "singleton",
+			opts: []*options.FindOneOptions{
+				options.FindOne().SetSkip(1),
+			},
+			want: []*options.FindOptions{
+				options.Find().SetSkip(1),
+				options.Find().SetLimit(-1),
+			},
+		},
+		{
+			name: "multiplicity",
+			opts: []*options.FindOneOptions{
+				options.FindOne().SetSkip(1),
+				options.FindOne().SetSkip(2),
+			},
+			want: []*options.FindOptions{
+				options.Find().SetSkip(1),
+				options.Find().SetSkip(2),
+				options.Find().SetLimit(-1),
+			},
+		},
+		{
+			name: "interior null",
+			opts: []*options.FindOneOptions{
+				options.FindOne().SetSkip(1),
+				nil,
+				options.FindOne().SetSkip(2),
+			},
+			want: []*options.FindOptions{
+				options.Find().SetSkip(1),
+				options.Find().SetSkip(2),
+				options.Find().SetLimit(-1),
+			},
+		},
+		{
+			name: "start null",
+			opts: []*options.FindOneOptions{
+				nil,
+				options.FindOne().SetSkip(1),
+				options.FindOne().SetSkip(2),
+			},
+			want: []*options.FindOptions{
+				options.Find().SetSkip(1),
+				options.Find().SetSkip(2),
+				options.Find().SetLimit(-1),
+			},
+		},
+		{
+			name: "end null",
+			opts: []*options.FindOneOptions{
+				options.FindOne().SetSkip(1),
+				options.FindOne().SetSkip(2),
+				nil,
+			},
+			want: []*options.FindOptions{
+				options.Find().SetSkip(1),
+				options.Find().SetSkip(2),
+				options.Find().SetLimit(-1),
+			},
+		},
+	}
+
+	for _, test := range tests {
+		test := test // Capture the range variable
+
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			got := newFindOptionsFromFindOneOptions(test.opts...)
+			assert.Equal(t, test.want, got)
+		})
+	}
+}

mongo/options/aggregateoptions.go

@@ -44,9 +44,10 @@ type AggregateOptions struct {
 	// This option is only valid for MongoDB versions >= 3.2 and is ignored for previous server versions.
 	MaxAwaitTime *time.Duration
 
-	// A string that will be included in server logs, profiling logs, and currentOp queries to help trace the operation.
-	// The default is nil, which means that no comment will be included in the logs.
-	Comment *string
+	// A string or document that will be included in server logs, profiling logs,
+	// and currentOp queries to help trace the operation. The default is nil,
+	// which means that no comment will be included in the logs.
+	Comment interface{}
 
 	// The index to use for the aggregation. This should either be the index name as a string or the index specification
 	// as a document. The hint does not apply to $lookup and $graphLookup aggregation stages. The driver will return an
@@ -111,8 +112,8 @@ func (ao *AggregateOptions) SetMaxAwaitTime(d time.Duration) *AggregateOptions {
 }
 
 // SetComment sets the value for the Comment field.
-func (ao *AggregateOptions) SetComment(s string) *AggregateOptions {
-	ao.Comment = &s
+func (ao *AggregateOptions) SetComment(comment interface{}) *AggregateOptions {
+	ao.Comment = comment
 	return ao
 }