GODRIVER-1955 create labeledError interface #651
Conversation
|
We were originally returning a topology.ConnectionError, which wrapped a few layers of auth.Error, which wrapped a driver.Error, none of which are ServerErrors, so HasErrorLabel wasn't getting called on them. However, the driver.Error did have a HasErrorLabel function and a NetworkError label, and all those in between errors do have Unwrap(). IsTimeout was working because it does eventually unwrap to a context.DeadlineExceeded error. |
|
@iwysiu Makes sense. Would it maybe be more robust to keep checking for |
|
@divjotarora I'm ok with that change, but I'm not sure why its more robust? Are we worried about non driver.Error errors incorrectly having a "NetworkError" label? I'd like to keep labeledError around for the internal errorHasLabel helper either way, because I could see someone using it in the future and not realizing that they still have to worry about driver.Errors. |
|
@iwysiu I originally read the code incorrectly. I think the |
There was a problem hiding this comment.
The fix as-is LGTM! I have a question about a possible alternative. If that is worthwhile we can do that as follow-up.
I was somewhat surprised to find out we're returning topology errors to users, because those are from x, but that would be a much bigger change.
If we should not be returning any errors from x, can you file a ticket to track that?
| error | ||
| // HasErrorLabel returns true if the error contains the specified label. | ||
| HasErrorLabel(string) bool | ||
| } |
There was a problem hiding this comment.
This seems like a reasonable change to fix this bug.
To check my understanding:
driver.Errorrepresents a server error. It is usually hidden from the caller. We do not want to exposextypes directly.errors.replaceErrorswill replace the top-leveldriver.Errorwith aCommandError, which implementsServerError.- In this case, since the
driver.Erroris wrapped in atopology.ConnectionErrorand other auth errors, it won't be replaced.
Would another alternative be to have driver.Error implement the full ServerError interface? I am thinking it may let a caller use errors.As on a handshake error and call other ServerError methods.
|
@kevinAlbs I made GODRIVER-1991 to track transforming all the x errors and GODRIVER-1992 to implement ServerError for driver errors. I suspect that only one of those two things has to happen, but we can discuss what we want to do in triage. |
* 'master' of https://github.com/mongodb/mongo-go-driver: (39 commits) GODRIVER-2004 Add Versioned API connection examples for Docs (mongodb#665) GODRIVER-1961 Run OCSP tests against RHEL 7.0 (mongodb#664) GODRIVER-1844 finer precision for getSecondsSinceEpoch (mongodb#666) GODRIVER-1973 create internal copy of aws v4 signing code (mongodb#657) GODRIVER-1951 Update the Go version for Evergreen builds to 1.16 (mongodb#663) GODRIVER-1949 add more ignored killAllSessions errors for unified tes… (mongodb#658) GODRIVER-1963 remove dropDatabase result (mongodb#660) GODRIVER-1180 Remove legacy transform functions from mongo (mongodb#583) GODRIVER-1937 Update legacy ListCollections to support the BatchSize option for server version 2.6 (mongodb#656) GODRIVER-1933 remove xtrace from shell scripts (mongodb#661) fix README error handling of FindOne (mongodb#636) GODRIVER-1938 update mongocryptd serverSelectionTimeout to 10 seconds (mongodb#659) GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer (mongodb#653) GODRIVER-1955 create labeledError interface (mongodb#651) GODRIVER-1947 Unmarshal unicode surrogate pairs correctly in UnmarshalExtJSON. (mongodb#649) Changed order of actions in ObjectIDFromHex func (mongodb#637) GODRIVER-1750 Ensure contexts are always cancelled during server monitoring (mongodb#654) GODRIVER-1931 Sync new cursors and SDAM LB tests (mongodb#655) GODRIVER-1981 Sync new transactions tests (mongodb#652) GODRIVER-1931 Run tests against LBs in Evergreen (mongodb#648) ...
I could technically make labeledError a public interface, but I don't think users get much benefit from it.