GODRIVER-3086 Add ErrorCodes to ServerError API #1894

prestonvasquez · 2024-11-15T22:28:56Z

Summary

Extend the ServerError interface to include ErrorCodes which will return a list of deduplicated error codes returned by the server during the lifetime of operation execution.

Background & Motivation

The Go Driver provides a way to check if a ServerError has a specific error code, but no way to determine all error codes that occurred during the operation.

mongodb-drivers-pr-bot · 2024-11-15T22:52:09Z

API Change Report

./v2/mongo

compatible changes

BulkWriteException.ErrorCodes: added
CommandError.ErrorCodes: added
ServerError.ErrorCodes: added
WriteError.ErrorCodes: added
WriteException.ErrorCodes: added

FGasper

Thank you for this! :)

FGasper · 2024-11-18T15:49:22Z

mongo/errors.go

@@ -252,9 +252,23 @@ type ServerError interface {
 	// HasErrorCodeWithMessage returns true if any of the contained errors have the specified code and message.
 	HasErrorCodeWithMessage(int, string) bool

+	// ErrorCodes returns a deduplicated list of error codes returned by the
+	// server.


It’d be good to mention if there’s a defined sort order.

@prestonvasquez Thoughts?

Also, I suggest:

ErrorCodes returns all error codes (unsorted) in the server’s response. This includes nested errors (e.g., write concern errors) as well as the top-level error code.

FGasper · 2024-11-18T15:50:14Z

mongo/errors.go

+	return hasErrorCode(e, code)
+}
+
+// ErrorCodes returns a list of error codes returned by the server.


It’d be good to mention if there’s a defined sort order.

ErrorCodes returns all error codes (unsorted) in the server’s response. This includes nested errors (e.g., write concern errors) as well as the top-level error code.

FGasper · 2024-11-18T15:52:01Z

mongo/errors.go

 	}
-	return false
+
+	// Deduplicate error codes.


Not that it affects us :), but: I see the same logic here and in WriteException’s method. This seems like the sort of thing you’d deduplicate.

mongo/errors.go

…iver into GODRIVER-3086

matthewdale

Looks good! 👍

matthewdale · 2025-01-22T01:40:56Z

mongo/errors.go

@@ -252,9 +252,23 @@ type ServerError interface {
 	// HasErrorCodeWithMessage returns true if any of the contained errors have the specified code and message.
 	HasErrorCodeWithMessage(int, string) bool

+	// ErrorCodes returns a deduplicated list of error codes returned by the


The latest revision does not deduplicate error codes.

Suggested change

// ErrorCodes returns a deduplicated list of error codes returned by the

// ErrorCodes returns a list of error codes returned by the

FGasper

I’ve made a few suggestions on verbiage; see what you think.

Also: are there any cases where it is, in fact, correct just to check HasErrorCode() without also checking for other, unexpected error codes? If not, should HasErrorCode()’s documentation also be updated to discourage its use?

Basically ISTM that anyplace an application calls HasErrorCode(123) should instead be a check of whether ErrorCodes()’s return contains solely 123. Is that reasonable?

FGasper · 2025-01-29T19:14:48Z

mongo/errors.go

@@ -252,9 +252,23 @@ type ServerError interface {
 	// HasErrorCodeWithMessage returns true if any of the contained errors have the specified code and message.
 	HasErrorCodeWithMessage(int, string) bool

+	// ErrorCodes returns a deduplicated list of error codes returned by the
+	// server.


@prestonvasquez Thoughts?

FGasper · 2025-01-29T19:17:49Z

mongo/errors.go

@@ -252,9 +252,23 @@ type ServerError interface {
 	// HasErrorCodeWithMessage returns true if any of the contained errors have the specified code and message.
 	HasErrorCodeWithMessage(int, string) bool

+	// ErrorCodes returns a deduplicated list of error codes returned by the
+	// server.


Also, I suggest:

ErrorCodes returns all error codes (unsorted) in the server’s response. This includes nested errors (e.g., write concern errors) as well as the top-level error code.

FGasper · 2025-01-29T19:18:28Z

mongo/errors.go

+	return hasErrorCode(e, code)
+}
+
+// ErrorCodes returns a list of error codes returned by the server.


ErrorCodes returns all error codes (unsorted) in the server’s response. This includes nested errors (e.g., write concern errors) as well as the top-level error code.

prestonvasquez · 2025-01-29T20:09:02Z

@FGasper

I’ve made a few suggestions on verbiage; see what you think.

I agree with the changes to the interface documentation but don't think we need to be more specific on the methods.

anyplace an application calls HasErrorCode(123) should instead be a check of whether ErrorCodes()’s return contains solely 123. Is that reasonable?

HasErrorCode's logic has not changed and so we should maintain the current intentions and usage guidelines for our users. The current documentation correctly expresses the purpose of the method for each error type.

FGasper

LGTM. Thanks!

matthewdale · 2025-01-30T04:38:47Z

@FGasper I believe there are plenty of cases where HasErrorCode is sufficient, especially for single-read or single-write operations (i.e. non-bulk operations). I think it would be confusing to discourage its use.

matthewdale

Looks good! 👍

FGasper · 2025-01-30T13:59:33Z

I’ve moved the HasErrorCode discussion to Slack.

GODRIVER-3086 Add ErrorCodes to ServerError API

9e699c9

prestonvasquez marked this pull request as draft November 15, 2024 22:29

GODRIVER-3086 Remove empty lines and debug stmt

382b416

prestonvasquez marked this pull request as ready for review November 15, 2024 22:39

Merge branch 'master' into GODRIVER-3086

c4d1a72

prestonvasquez requested a review from matthewdale November 15, 2024 22:46

mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Nov 15, 2024

FGasper reviewed Nov 18, 2024

View reviewed changes

matthewdale reviewed Jan 17, 2025

View reviewed changes

mongo/errors.go Outdated Show resolved Hide resolved

mongo/errors.go Outdated Show resolved Hide resolved

mongo/errors.go Outdated Show resolved Hide resolved

prestonvasquez added 2 commits January 16, 2025 22:47

GODRIVER-3086 Revert single code checks and no deduplication

36f60b7

Merge branch 'GODRIVER-3086' of github.com:prestonvasquez/mongo-go-dr…

807a66c

…iver into GODRIVER-3086

prestonvasquez requested a review from matthewdale January 17, 2025 05:48

matthewdale previously approved these changes Jan 22, 2025

View reviewed changes

prestonvasquez requested a review from FGasper January 22, 2025 15:49

FGasper suggested changes Jan 29, 2025

View reviewed changes

GODRIVER-3086 Update ErrorCodes documentation

1abe21b

prestonvasquez dismissed matthewdale’s stale review via 1abe21b January 29, 2025 20:10

prestonvasquez requested review from matthewdale and FGasper January 29, 2025 20:10

FGasper approved these changes Jan 29, 2025

View reviewed changes

Merge branch 'master' into GODRIVER-3086

c0763b3

matthewdale approved these changes Jan 30, 2025

View reviewed changes

prestonvasquez merged commit 4e0ab11 into mongodb:master Jan 30, 2025
29 of 35 checks passed

prestonvasquez deleted the GODRIVER-3086 branch January 30, 2025 16:14

	// ErrorCodes returns a deduplicated list of error codes returned by the
	// ErrorCodes returns a list of error codes returned by the

GODRIVER-3086 Add ErrorCodes to ServerError API #1894

GODRIVER-3086 Add ErrorCodes to ServerError API #1894

Uh oh!

Conversation

prestonvasquez commented Nov 15, 2024

Summary

Background & Motivation

Uh oh!

mongodb-drivers-pr-bot bot commented Nov 15, 2024

API Change Report

./v2/mongo

compatible changes

Uh oh!

FGasper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matthewdale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FGasper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prestonvasquez commented Jan 29, 2025

Uh oh!

FGasper left a comment

Choose a reason for hiding this comment

Uh oh!

matthewdale commented Jan 30, 2025

Uh oh!

matthewdale left a comment

Choose a reason for hiding this comment

Uh oh!

FGasper commented Jan 30, 2025

Uh oh!

Uh oh!

Uh oh!