Skip to content

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

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

Merged
merged 4 commits into from
Apr 7, 2025
Merged

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

Show file tree
Hide file tree
Changes from 3 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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package integration

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

Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion mongo/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions mongo/client_bulk_write.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
4 changes: 2 additions & 2 deletions mongo/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 6 additions & 6 deletions mongo/collection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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...)
}
Expand Down
48 changes: 24 additions & 24 deletions mongo/collection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion mongo/cursor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions mongo/database_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend not exporting Wrapped to allow us to change how it's implemented in the future without breaking changes.

}

// 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 {
Expand Down
5 changes: 2 additions & 3 deletions mongo/index_view.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading