Skip to content

GODRIVER-1992 Implement ServerErrors interface for driver errors #667

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

Closed
wants to merge 3 commits into from
Closed

GODRIVER-1992 Implement ServerErrors interface for driver errors #667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc68f4d
Implement ServerError interface for driver errors
May 13, 2021
9e3d74c
Add tests for ServerError implementation
May 13, 2021
159fd65
Implement ServerError for driver Error
May 13, 2021
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
14 changes: 3 additions & 11 deletions mongo/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,13 +202,14 @@ type ServerError interface {
HasErrorMessage(string) bool
// HasErrorCodeWithMessage returns true if any of the contained errors have the specified code and message.
HasErrorCodeWithMessage(int, string) bool

serverError()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementing ServerError for types outside of the mongo package (in this case in driver) means that I must remove this guard. Without serverError(), users can now implement the ServerError interface for their own types. We do document that this is discouraged, but if there's some way I'm missing to keep this guard in, let me know 🌺

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't necessarily think its better, but we could alternatively have a drivers.ServerError interface that has the same functions and have the helpers that check for the ServerError interface also check for the drivers.ServerError interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't love the repetition, but it might be better than removing the guard altogether. Any thoughts @kevinAlbs?

Copy link
Contributor

Choose a reason for hiding this comment

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

@iwysiu does copying the interface take away from the usability?

I thought the motivation of the ServerError interface was to be a convenient interface to inspect all server errors.

_, err = client.Database("db").Collection("coll").InsertOne(ctx, bson.D{{"_id", "foo"}})
	if err != nil {
		fmt.Println("Insert failed", err)
		var serverError mongo.ServerError

		if errors.As(err, &serverError) {
			if serverError.HasErrorLabel("RetryableWrite") {
				fmt.Println("Going to retry...")
			}
		}
	}

A distinct driver.ServerError would be less accessible if it is in the x package, and it would require the user to check for both interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would make it more annoying for end users. If we're not worried about users implementing it, i think removing the guard is fine

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not too familiar with the nuances of whether this should be a sealed interface.

No functions take a ServerError as a direct argument. The helper IsDuplicateKeyError could accept an error that the user has defined which implements ServerError. But that seems harmless to me.

Are there other risks?

Copy link
Collaborator

@matthewdale matthewdale May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think there's much risk to "unsealing" the ServerError interface. As @kevinAlbs mentioned, nothing uses it as an argument directly, and even if someone implemented the interface, they wouldn't get any unexpected behavior, it just doesn't make much sense to do so.

Alternately, there's actually no reason that ServerError needs to be exported. We could leave ServerError as is, define a new unexported interface like serverError with the same function signatures, add a note to ServerError that it's unused and only there to maintain public API compatibility.

Edit:
My understanding is that a common use case is for users to type-assert an error is mongo.ServerError so they can access functions like HasErrorCode() and HasErrorLabel().

E.g.

err := mongo.Something()
if sErr, ok := err.(mongo.ServerError); ok {
    fmt.Println(sErr.HasErrorCode(21))
}

In that case, creating a new unexported serverError that is implemented by errors returned by packages in x would not satisfy an expected use case of ServerError.

}

var _ ServerError = CommandError{}
var _ ServerError = WriteException{}
var _ ServerError = BulkWriteException{}
var _ ServerError = driver.WriteCommandError{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would returning a driver.WriteCommandError or driver.WriteConcernError just be a bug?

IIUC, processWriteError rewrites them in terms of mongo errors

var _ ServerError = driver.WriteConcernError{}
var _ ServerError = driver.Error{}
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue that spurred this ticket was a deeply wrapped driver.Error that contained error labels: #651 (comment)

Would a preferable alternative be to have the error translation functions translate wrapped errors as well? E.g. have replaceErrors also inspect wrapped errors?

@iwysiu would the fix of GODRIVER-1991 solve this problem entirely? If no errors are returned in x, I imagine the fix would involve having to rewrite wrapped errors as well.


// CommandError represents a server error during execution of a command. This can be returned by any operation.
type CommandError struct {
Expand Down Expand Up @@ -264,9 +265,6 @@ func (e CommandError) IsMaxTimeMSExpiredError() bool {
return e.Code == 50 || e.Name == "MaxTimeMSExpired"
}

// serverError implements the ServerError interface.
func (e CommandError) serverError() {}

// WriteError is an error that occurred during execution of a write operation. This error type is only returned as part
// of a WriteException or BulkWriteException.
type WriteError struct {
Expand Down Expand Up @@ -395,9 +393,6 @@ func (mwe WriteException) HasErrorCodeWithMessage(code int, message string) bool
return false
}

// serverError implements the ServerError interface.
func (mwe WriteException) serverError() {}

func convertDriverWriteConcernError(wce *driver.WriteConcernError) *WriteConcernError {
if wce == nil {
return nil
Expand Down Expand Up @@ -498,9 +493,6 @@ func (bwe BulkWriteException) HasErrorCodeWithMessage(code int, message string)
return false
}

// serverError implements the ServerError interface.
func (bwe BulkWriteException) serverError() {}

// returnResult is used to determine if a function calling processWriteError should return
// the result or return nil. Since the processWriteError function is used by many different
// methods, both *One and *Many, we need a way to differentiate if the method should return
Expand Down
114 changes: 114 additions & 0 deletions mongo/integration/errors_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/integration/mtest"
"go.mongodb.org/mongo-driver/mongo/options"
"go.mongodb.org/mongo-driver/x/mongo/driver"
)

type netErr struct {
Expand Down Expand Up @@ -300,6 +301,119 @@ func TestErrors(t *testing.T) {
false,
false,
},
{
"driver WriteCommandError all in writeConcernErorr",
driver.WriteCommandError{
&driver.WriteConcernError{"name", matchCode, "foo", nil, []string{label}, nil},
nil,
[]string{"otherError"},
},
true,
true,
true,
true,
false,
},
{
"driver WriteCommandError all in writeError",
driver.WriteCommandError{
nil,
driver.WriteErrors{
driver.WriteError{0, matchCode, "foo"},
driver.WriteError{0, otherCode, "bar"},
},
[]string{"otherError"},
},
true,
false,
true,
true,
false,
},
{
"driver WriteCommandError all false",
driver.WriteCommandError{
&driver.WriteConcernError{"name", otherCode, "bar", nil, []string{"otherError"}, nil},
driver.WriteErrors{
driver.WriteError{0, otherCode, "baz"},
},
[]string{"otherError"},
},
false,
false,
false,
false,
false,
},
{
"driver WriteCommandError HasErrorCodeAndMessage false",
driver.WriteCommandError{
&driver.WriteConcernError{"name", matchCode, "bar", nil, []string{}, nil},
driver.WriteErrors{
driver.WriteError{0, otherCode, "foo"},
},
[]string{"otherError"},
},
true,
false,
true,
false,
false,
},
{
"driver WriteConcernError all true",
driver.WriteConcernError{"wce", matchCode, "foo", nil, []string{label}, nil},
true,
true,
true,
true,
false,
},
{
"driver WriteConcernError all false",
driver.WriteConcernError{"wce", otherCode, "bar", nil, []string{"otherError"}, nil},
false,
false,
false,
false,
false,
},
{
"driver WriteConcernError has code not message",
driver.WriteConcernError{"wce", matchCode, "bar", nil, []string{"otherError"}, nil},
true,
false,
false,
false,
false,
},
{
"driver Error all true",
driver.Error{matchCode, "foo", []string{label}, "e", matchWrapped, nil},
true,
true,
true,
true,
true,
},
{
"driver Error all false",
driver.Error{otherCode, "bar", []string{"otherError"}, "e", otherWrapped, nil},
false,
false,
false,
false,
false,
},
{
"driver Error has code not message",
driver.Error{matchCode, "bar", []string{"otherError"}, "e", nil, nil},
true,
false,
false,
false,
false,
},
}
for _, tc := range testCases {
mt.Run(tc.name, func(mt *mtest.T) {
Expand Down
96 changes: 96 additions & 0 deletions x/mongo/driver/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,60 @@ func (wce WriteCommandError) Retryable(wireVersion *description.VersionRange) bo
return (*wce.WriteConcernError).Retryable()
}

// HasErrorCode returns true if the error has the specified code.
func (wce WriteCommandError) HasErrorCode(code int) bool {
if wce.WriteConcernError != nil && wce.WriteConcernError.HasErrorCode(code) {
return true
}
for _, we := range wce.WriteErrors {
if int(we.Code) == code {
return true
}
}
return false
}

// HasErrorLabel returns true if the error contains the specified label.
func (wce WriteCommandError) HasErrorLabel(label string) bool {
if wce.WriteConcernError != nil && wce.WriteConcernError.HasErrorLabel(label) {
return true
}
if wce.Labels != nil {
for _, l := range wce.Labels {
if l == label {
return true
}
}
}
return false
}

// HasErrorMessage returns true if the error contains the specified message.
func (wce WriteCommandError) HasErrorMessage(message string) bool {
if wce.WriteConcernError != nil && wce.WriteConcernError.HasErrorMessage(message) {
return true
}
for _, we := range wce.WriteErrors {
if strings.Contains(we.Message, message) {
return true
}
}
return false
}

// HasErrorCodeWithMessage returns true if any of the contained errors have the specified code and message.
func (wce WriteCommandError) HasErrorCodeWithMessage(code int, message string) bool {
if wce.WriteConcernError != nil && wce.WriteConcernError.HasErrorCodeWithMessage(code, message) {
return true
}
for _, we := range wce.WriteErrors {
if int(we.Code) == code && strings.Contains(we.Message, message) {
return true
}
}
return false
}

// WriteConcernError is a write concern failure that occurred as a result of a
// write operation.
type WriteConcernError struct {
Expand Down Expand Up @@ -181,6 +235,33 @@ func (wce WriteConcernError) NotMaster() bool {
return hasNoCode && strings.Contains(wce.Message, "not master")
}

// HasErrorCode returns true if the error has the specified code.
func (wce WriteConcernError) HasErrorCode(code int) bool {
return int(wce.Code) == code
}

// HasErrorLabel returns true if the error contains the specified label.
func (wce WriteConcernError) HasErrorLabel(label string) bool {
if wce.Labels != nil {
for _, l := range wce.Labels {
if l == label {
return true
}
}
}
return false
}

// HasErrorMessage returns true if the error contains the specified message.
func (wce WriteConcernError) HasErrorMessage(message string) bool {
return strings.Contains(wce.Message, message)
}

// HasErrorCodeWithMessage returns true if the error has the specified code and Message contains the specified message.
func (wce WriteConcernError) HasErrorCodeWithMessage(code int, message string) bool {
return int(wce.Code) == code && strings.Contains(wce.Message, message)
}

// WriteError is a non-write concern failure that occurred as a result of a write
// operation.
type WriteError struct {
Expand Down Expand Up @@ -236,6 +317,11 @@ func (e Error) Unwrap() error {
return e.Wrapped
}

// HasErrorCode returns true if the error has the specified code.
func (e Error) HasErrorCode(code int) bool {
return int(e.Code) == code
}

// HasErrorLabel returns true if the error contains the specified label.
func (e Error) HasErrorLabel(label string) bool {
if e.Labels != nil {
Expand All @@ -248,6 +334,16 @@ func (e Error) HasErrorLabel(label string) bool {
return false
}

// HasErrorMessage returns true if the error contains the specified message.
func (e Error) HasErrorMessage(message string) bool {
return strings.Contains(e.Message, message)
}

// HasErrorCodeWithMessage returns true if the error has the specified code and Message contains the specified message.
func (e Error) HasErrorCodeWithMessage(code int, message string) bool {
return int(e.Code) == code && strings.Contains(e.Message, message)
}

// RetryableRead returns true if the error is retryable for a read operation
func (e Error) RetryableRead() bool {
for _, label := range e.Labels {
Expand Down