Skip to content

GODRIVER-2775 Refactor replaceErrors to always wrap errors instead of replacing them. #2113

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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

matthewdale
Copy link
Collaborator

@matthewdale matthewdale commented Jun 23, 2025

GODRIVER-2775

Summary

  • Rename replaceErrors to wrapErrors.
  • Update the logic in wrapErrors to wrap specific error values instead of replacing the top-level error.
    • Because the error assertions all use errors.Is/errors.As instead of type assertions, it may match more errors now. The possible effect is that the error may be wrapped in an additional top-level error. Error values are no longer discarded, so there should be no risk that a user's errors.Is/errors.As assertions will starting to fail.
  • Add Unwrap methods to mongo.MarshalError and mongo.MongocryptError so that the wrapped errors can be inspected using errors.Is/errors.As.
  • Remove duplicated error message from mongo.MarshalError and mongo.MongocryptError and always returne the error string from the wrapped error.
  • Append the error string from the Wrapped error to the error string returned by mongo.CommandError.

Background & Motivation

The approach in this PR reduces the likelihood of causing bugs by wrapping error values, which is one of the main goals. However, it doesn't try to answer some of the questions in GODRIVER-2775 or try to establish any new usability patterns for handling errors returned by the Go Driver APIs. We should defer those questions to GODRIVER-3602.

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Jun 23, 2025
Copy link
Contributor

mongodb-drivers-pr-bot bot commented Jun 23, 2025

API Change Report

./v2/mongo

compatible changes

MarshalError.Unwrap: added
MongocryptError.Unwrap: added

@matthewdale matthewdale force-pushed the godriver2775-replaceerrors branch 3 times, most recently from bf769c0 to 374edb5 Compare June 26, 2025 02:18
@matthewdale matthewdale changed the title GODRIVER-2774 Refactor replaceErrors to use errors.Is/errors.As GODRIVER-2774 Refactor replaceErrors to always wrap errors instead of replacing them. Jun 26, 2025
@matthewdale matthewdale force-pushed the godriver2775-replaceerrors branch from 374edb5 to f3e687c Compare June 26, 2025 06:37
@matthewdale matthewdale marked this pull request as ready for review June 27, 2025 00:17
@matthewdale matthewdale requested a review from a team as a code owner June 27, 2025 00:17
@matthewdale matthewdale added priority-2-medium Medium Priority PR for Review and removed priority-3-low Low Priority PR for Review labels Jul 8, 2025
zhouselena
zhouselena previously approved these changes Jul 8, 2025
Copy link
Contributor

@zhouselena zhouselena left a comment

Choose a reason for hiding this comment

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

lgtm! just a note that the PR title is GODRIVER-2774, I believe it should be GODRIVER-2775

question to make sure I understood properly, replaceErrors is to convert errors defined in x package to the same errors defined in mongo package? also general curiosity why were these errors duplicated/defined in both packages initially?

}

func (me MarshalError) Unwrap() error { return me.Err }

// MongocryptError represents an libmongocrypt error during in-use encryption.
type MongocryptError struct {
Code int32
Message string
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "Message" field doesn't seem to be used anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, it's not used anymore. Message and Code are exported and in a stable package, so we can't remove them. I'll see if I can modify the Error() logic in a way that makes sense for both if MongocryptError is used as a wrapper and if it's used as a standalone error in some use case in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to work as both a standalone error and as a wrapper.

@matthewdale matthewdale changed the title GODRIVER-2774 Refactor replaceErrors to always wrap errors instead of replacing them. GODRIVER-2775 Refactor replaceErrors to always wrap errors instead of replacing them. Jul 10, 2025
@matthewdale
Copy link
Collaborator Author

@zhouselena The duplication is caused by needing to return errors with additional information (e.g. error code, server message, error labels, etc) but where the error originates from an x/ package API. We want users to be able to ignore the existence of the x/ packages, so they shouldn't have to use error types from that package to get access to that extra info.

For example, we want this:

package main

import (
    "fmt"

    "go.mongodb.org/mongo-driver/v2/mongo"
)

func main() {
    // ...
    _, err := coll.InsertOne(...)
    var cerr mongo.CommandError
    if errors.As(err, &cerr) {
        fmt.Println(cerr.Labels)
    }
    // ...

instead of this:

package main

import (
    "fmt"

    "go.mongodb.org/mongo-driver/v2/x/mongo/driver"
)

func main() {
    // ...
    _, err := coll.InsertOne(...)
    var derr driver.Error
    if errors.As(err, &derr) {
        fmt.Println(derr.Labels)
    }
    // ...

However, referencing the mongo package from any x/ package would create an import cycle (because mongo imports almost every x/ package), so we can't define the errors in the mongo package. We need to find an idiomatic way to use the errors already defined in the mongo package even though the errors originate from an x/ package API.

We might be able to move those error value definitions to an internal package that both mongo and x/ share, but that is probably a breaking change, so will have to wait for a future major version.

Copy link
Contributor

@zhouselena zhouselena left a comment

Choose a reason for hiding this comment

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

ty for the explanation!! lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-2-medium Medium Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants