Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GODRIVER-2704 Replace input validation error constants with "InvalidArgumentError". #1989

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+66 −48
Open

GODRIVER-2704 Replace input validation error constants with "InvalidArgumentError". #1989

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd32d09
Replace input validation error constants with "InvalidArgumentError".
qingyang-hu Mar 14, 2025
c0d274d
updates
qingyang-hu Mar 20, 2025
6525042
updates
qingyang-hu Mar 24, 2025
9ec0367
unexpose "wrapped" in `InvalidArgumentError`.
qingyang-hu Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion internal/integration/collection_test.go
View file
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@ package integration

import (
"context"
"errors"
"strings"
"testing"

@@ -522,7 +523,7 @@ func TestCollection(t *testing.T) {
})
mt.Run("nil id", func(mt *mtest.T) {
_, err := mt.Coll.UpdateByID(context.Background(), nil, bson.D{{"$inc", bson.D{{"x", 1}}}})
assert.Equal(mt, err, mongo.ErrNilValue, "expected %v, got %v", mongo.ErrNilValue, err)
assert.True(mt, errors.Is(err, mongo.ErrNilValue), "expected %v, got %v", mongo.ErrNilValue, err)
})
mt.RunOpts("found", noClientOpts, func(mt *mtest.T) {
testCases := []struct {
2 changes: 1 addition & 1 deletion mongo/client.go
View file
Original file line number Diff line number Diff line change
@@ -888,7 +888,7 @@ func (c *Client) BulkWrite(ctx context.Context, writes []ClientBulkWrite,
}

if len(writes) == 0 {
return nil, ErrEmptySlice
return nil, fmt.Errorf("invalid writes: %w", ErrEmptySlice)
}
bwo, err := mongoutil.NewOptions(opts...)
if err != nil {
6 changes: 3 additions & 3 deletions mongo/client_bulk_write.go
View file
Original file line number Diff line number Diff line change
@@ -50,11 +50,11 @@ type clientBulkWrite struct {

func (bw *clientBulkWrite) execute(ctx context.Context) error {
if len(bw.writePairs) == 0 {
return ErrEmptySlice
return fmt.Errorf("invalid writes: %w", ErrEmptySlice)
}
for _, m := range bw.writePairs {
for i, m := range bw.writePairs {
if m.model == nil {
return ErrNilDocument
return fmt.Errorf("error from model at index %d: %w", i, ErrNilDocument)
}
}
batches := &modelBatches{
4 changes: 2 additions & 2 deletions mongo/client_test.go
View file
Original file line number Diff line number Diff line change
@@ -81,10 +81,10 @@ func TestClient(t *testing.T) {
assert.Equal(t, watchErr, err, "expected error %v, got %v", watchErr, err)

_, err = client.ListDatabases(bgCtx, nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = client.ListDatabaseNames(bgCtx, nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)
})
t.Run("read preference", func(t *testing.T) {
t.Run("absent", func(t *testing.T) {
12 changes: 6 additions & 6 deletions mongo/collection.go
View file
Original file line number Diff line number Diff line change
@@ -193,7 +193,7 @@ func (coll *Collection) BulkWrite(ctx context.Context, models []WriteModel,
opts ...options.Lister[options.BulkWriteOptions]) (*BulkWriteResult, error) {

if len(models) == 0 {
return nil, ErrEmptySlice
return nil, fmt.Errorf("invalid models: %w", ErrEmptySlice)
}

if ctx == nil {
@@ -221,9 +221,9 @@ func (coll *Collection) BulkWrite(ctx context.Context, models []WriteModel,

selector := makePinnedSelector(sess, coll.writeSelector)

for _, model := range models {
for i, model := range models {
if model == nil {
return nil, ErrNilDocument
return nil, fmt.Errorf("invalid model at index %d: %w", i, ErrNilDocument)
}
}

@@ -407,10 +407,10 @@ func (coll *Collection) InsertMany(

dv := reflect.ValueOf(documents)
if dv.Kind() != reflect.Slice {
return nil, ErrNotSlice
return nil, fmt.Errorf("invalid documents: %w", ErrNotSlice)
}
if dv.Len() == 0 {
return nil, ErrEmptySlice
return nil, fmt.Errorf("invalid documents: %w", ErrEmptySlice)
}

docSlice := make([]interface{}, 0, dv.Len())
@@ -729,7 +729,7 @@ func (coll *Collection) UpdateByID(
opts ...options.Lister[options.UpdateOneOptions],
) (*UpdateResult, error) {
if id == nil {
return nil, ErrNilValue
return nil, fmt.Errorf("invalid id: %w", ErrNilValue)
}
return coll.UpdateOne(ctx, bson.D{{"_id", id}}, update, opts...)
}
48 changes: 24 additions & 24 deletions mongo/collection_test.go
View file
Original file line number Diff line number Diff line change
@@ -150,76 +150,76 @@ func TestCollection(t *testing.T) {
doc := bson.D{}

_, err := coll.InsertOne(bgCtx, nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.InsertMany(bgCtx, nil)
assert.Equal(t, ErrNotSlice, err, "expected error %v, got %v", ErrNotSlice, err)
assert.True(t, errors.Is(err, ErrNotSlice), "expected error %v, got %v", ErrNotSlice, err)

_, err = coll.InsertMany(bgCtx, []interface{}{})
assert.Equal(t, ErrEmptySlice, err, "expected error %v, got %v", ErrEmptySlice, err)
assert.True(t, errors.Is(err, ErrEmptySlice), "expected error %v, got %v", ErrEmptySlice, err)

_, err = coll.InsertMany(bgCtx, "x")
assert.Equal(t, ErrNotSlice, err, "expected error %v, got %v", ErrNotSlice, err)
assert.True(t, errors.Is(err, ErrNotSlice), "expected error %v, got %v", ErrNotSlice, err)

_, err = coll.DeleteOne(bgCtx, nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.DeleteMany(bgCtx, nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.UpdateOne(bgCtx, nil, doc)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.UpdateOne(bgCtx, doc, nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.UpdateMany(bgCtx, nil, doc)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.UpdateMany(bgCtx, doc, nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.ReplaceOne(bgCtx, nil, doc)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.ReplaceOne(bgCtx, doc, nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.CountDocuments(bgCtx, nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

err = coll.Distinct(bgCtx, "x", nil).Err()
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.Find(bgCtx, nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

err = coll.FindOne(bgCtx, nil).Err()
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

err = coll.FindOneAndDelete(bgCtx, nil).Err()
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

err = coll.FindOneAndReplace(bgCtx, nil, doc).Err()
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

err = coll.FindOneAndReplace(bgCtx, doc, nil).Err()
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

err = coll.FindOneAndUpdate(bgCtx, nil, doc).Err()
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

err = coll.FindOneAndUpdate(bgCtx, doc, nil).Err()
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = coll.BulkWrite(bgCtx, nil)
assert.Equal(t, ErrEmptySlice, err, "expected error %v, got %v", ErrEmptySlice, err)
assert.True(t, errors.Is(err, ErrEmptySlice), "expected error %v, got %v", ErrEmptySlice, err)

_, err = coll.BulkWrite(bgCtx, []WriteModel{})
assert.Equal(t, ErrEmptySlice, err, "expected error %v, got %v", ErrEmptySlice, err)
assert.True(t, errors.Is(err, ErrEmptySlice), "expected error %v, got %v", ErrEmptySlice, err)

_, err = coll.BulkWrite(bgCtx, []WriteModel{nil})
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

aggErr := errors.New("can only marshal slices and arrays into aggregation pipelines, but got invalid")
_, err = coll.Aggregate(bgCtx, nil)
2 changes: 1 addition & 1 deletion mongo/cursor.go
View file
Original file line number Diff line number Diff line change
@@ -97,7 +97,7 @@ func NewCursorFromDocuments(documents []interface{}, preloadedErr error, registr
for i, doc := range documents {
switch t := doc.(type) {
case nil:
return nil, ErrNilDocument
return nil, fmt.Errorf("invalid document at index %d: %w", i, ErrNilDocument)
case []byte:
// Slight optimization so we'll just use MarshalBSON and not go through the codec machinery.
doc = bson.Raw(t)
6 changes: 3 additions & 3 deletions mongo/database_test.go
View file
Original file line number Diff line number Diff line change
@@ -141,16 +141,16 @@ func TestDatabase(t *testing.T) {
db := setupDb("foo")

err := db.RunCommand(bgCtx, nil).Err()
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = db.Watch(context.Background(), nil)
watchErr := errors.New("can only marshal slices and arrays into aggregation pipelines, but got invalid")
assert.Equal(t, watchErr, err, "expected error %v, got %v", watchErr, err)

_, err = db.ListCollections(context.Background(), nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)

_, err = db.ListCollectionNames(context.Background(), nil)
assert.Equal(t, ErrNilDocument, err, "expected error %v, got %v", ErrNilDocument, err)
assert.True(t, errors.Is(err, ErrNilDocument), "expected error %v, got %v", ErrNilDocument, err)
})
}
26 changes: 22 additions & 4 deletions mongo/errors.go
View file
Original file line number Diff line number Diff line change
@@ -24,17 +24,35 @@ import (
// ErrClientDisconnected is returned when disconnected Client is used to run an operation.
var ErrClientDisconnected = errors.New("client is disconnected")

// InvalidArgumentError wraps an invalid argument error.
type InvalidArgumentError struct {
wrapped error
}

// Error implements the error interface.
func (e InvalidArgumentError) Error() string {
return e.wrapped.Error()
}

// Unwrap returns the underlying error.
func (e InvalidArgumentError) Unwrap() error {
return e.wrapped
}

// ErrMultipleIndexDrop is returned if multiple indexes would be dropped from a call to IndexView.DropOne.
var ErrMultipleIndexDrop error = InvalidArgumentError{errors.New("multiple indexes would be dropped")}

// ErrNilDocument is returned when a nil document is passed to a CRUD method.
var ErrNilDocument = errors.New("document is nil")
var ErrNilDocument error = InvalidArgumentError{errors.New("document is nil")}

// ErrNilValue is returned when a nil value is passed to a CRUD method.
var ErrNilValue = errors.New("value is nil")
var ErrNilValue error = InvalidArgumentError{errors.New("value is nil")}

// ErrEmptySlice is returned when an empty slice is passed to a CRUD method that requires a non-empty slice.
var ErrEmptySlice = errors.New("must provide at least one element in input slice")
var ErrEmptySlice error = InvalidArgumentError{errors.New("must provide at least one element in input slice")}

// ErrNotSlice is returned when a type other than slice is passed to InsertMany.
var ErrNotSlice = errors.New("must provide a non-empty slice")
var ErrNotSlice error = InvalidArgumentError{errors.New("must provide a non-empty slice")}

// ErrMapForOrderedArgument is returned when a map with multiple keys is passed to a CRUD method for an ordered parameter
type ErrMapForOrderedArgument struct {
5 changes: 2 additions & 3 deletions mongo/index_view.go
View file
Original file line number Diff line number Diff line change
@@ -29,11 +29,10 @@ import (
var ErrInvalidIndexValue = errors.New("invalid index value")

// ErrNonStringIndexName is returned if an index is created with a name that is not a string.
//
// Deprecated: it will be removed in the next major release
var ErrNonStringIndexName = errors.New("index name must be a string")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error value appears to be unused. Can we mark it as deprecated, or should it be returned in some case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems not being used in the current v1 code base either. Is removing it considered a breaking change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, removing it is technically breaking because someone's code might reference the symbol, which would cause a compilation error if we remove it. The best we can do is mark it deprecated and note that it's no longer used.


// ErrMultipleIndexDrop is returned if multiple indexes would be dropped from a call to IndexView.DropOne.
var ErrMultipleIndexDrop = errors.New("multiple indexes would be dropped")

// IndexView is a type that can be used to create, drop, and list indexes on a collection. An IndexView for a collection
// can be created by a call to Collection.Indexes().
type IndexView struct {