GODRIVER-1992 Implement ServerErrors interface for driver errors

[benjirewis]

GODRIVER-1992

Implements the ServerError interface for driver.WriteCommandError and driver.WriteConcernError, so users will have access to the error helpers HasErrorCode, HasErrorLabel, HasErrorMessage and HasErrorCodeWithMessage for those driver errors.

Adds tests for these new functions in errors_test.go.

[benjirewis]

I believe driver.WriteCommandError and driver.WriteConcernError are the only types that could implement ServerError in the driver package, but let me know if I'm missing some @iwysiu .

[benjirewis]

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 🌺

[iwysiu]

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.

[benjirewis]

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

[kevinAlbs]

@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.

[iwysiu]

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

[kevinAlbs]

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?

[matthewdale]

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.

[iwysiu]

driver.Error can also implement this interface

[iwysiu]

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.

[kevinAlbs]

My comment #651 (comment) spurred this ticket. But since this can have API impact, I have some high level questions about the motivation and alternatives.

I apologize for not asking about it sooner, I should have asked to clarify the motivation when triaging or the first round of review.

[kevinAlbs]

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?

[kevinAlbs]

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

IIUC, processWriteError rewrites them in terms of mongo errors

[kevinAlbs]

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.