Skip to content

Add ErrorType method to MessageTooLargeError #1311

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

Merged

Conversation

AndrewShearBayer
Copy link
Contributor

@AndrewShearBayer AndrewShearBayer commented Jul 16, 2024

Description

When someone is trying to check an errors.Is comparison of the struct MessageTooLargeError there is no good way to check that the underlying error MessageSizeTooLarge. By exposing the underlying error with this new method it allows end users to be able to make a comparison using errors.Is().

Add ErrorType method to MessageTooLargeError struct to support functionality with the errors.Is function.

closes #1293

@jkoelker
Copy link

Instead of adding an ErrorType() method, I think implementing the Unwrap interface to return MessageSizeTooLarge would be better, then you can just call errors.Is with the wrapped error:

diff --git a/error.go b/error.go
index 7fbf3a732215..7db007a2ab50 100644
--- a/error.go
+++ b/error.go
@@ -656,16 +656,7 @@ func (e MessageTooLargeError) Error() string {
        return MessageSizeTooLarge.Error()
 }

-// ErrorType returns the specific error type associated with the MessageTooLargeError.
-// This function returns the predefined Error constant MessageSizeTooLarge, indicating
-// that the error occurred due to a message being too large to fit within the allowed size.
-//
-// Example:
-//
-//     err := MessageSizeTooLarge
-//     msgTooLarge := []Message{...}
-//     errors.Is(err, msgTooLarge.ErrorType())
-func (e MessageTooLargeError) ErrorType() Error {
+func (e MessageTooLargeError) Unwrap() error {
        return MessageSizeTooLarge
 }

diff --git a/error_test.go b/error_test.go
index cea810b15739..811243fe6d93 100644
--- a/error_test.go
+++ b/error_test.go
@@ -1,7 +1,6 @@
 package kafka

 import (
-       "errors"
        "fmt"
        "testing"

@@ -121,8 +120,8 @@ func TestError(t *testing.T) {
                        {Key: []byte("key"), Value: make([]byte, 1024*1024)},
                }
                msgTooLarge := messageTooLarge(msg, 1)
-               assert.False(t, errors.Is(err, msgTooLarge))
-               assert.Equal(t, err.Error(), msgTooLarge.Error())
-               assert.True(t, errors.Is(err, msgTooLarge.ErrorType()))
+               assert.NotErrorIs(t, err, msgTooLarge)
+               assert.Contains(t, msgTooLarge.Error(), MessageSizeTooLarge.Error())
+               assert.ErrorIs(t, msgTooLarge, MessageSizeTooLarge)
        })
 }

@AndrewShearBayer
Copy link
Contributor Author

Instead of adding an ErrorType() method, I think implementing the Unwrap interface to return MessageSizeTooLarge would be better, then you can just call errors.Is with the wrapped error:

diff --git a/error.go b/error.go
index 7fbf3a732215..7db007a2ab50 100644
--- a/error.go
+++ b/error.go
@@ -656,16 +656,7 @@ func (e MessageTooLargeError) Error() string {
        return MessageSizeTooLarge.Error()
 }

-// ErrorType returns the specific error type associated with the MessageTooLargeError.
-// This function returns the predefined Error constant MessageSizeTooLarge, indicating
-// that the error occurred due to a message being too large to fit within the allowed size.
-//
-// Example:
-//
-//     err := MessageSizeTooLarge
-//     msgTooLarge := []Message{...}
-//     errors.Is(err, msgTooLarge.ErrorType())
-func (e MessageTooLargeError) ErrorType() Error {
+func (e MessageTooLargeError) Unwrap() error {
        return MessageSizeTooLarge
 }

diff --git a/error_test.go b/error_test.go
index cea810b15739..811243fe6d93 100644
--- a/error_test.go
+++ b/error_test.go
@@ -1,7 +1,6 @@
 package kafka

 import (
-       "errors"
        "fmt"
        "testing"

@@ -121,8 +120,8 @@ func TestError(t *testing.T) {
                        {Key: []byte("key"), Value: make([]byte, 1024*1024)},
                }
                msgTooLarge := messageTooLarge(msg, 1)
-               assert.False(t, errors.Is(err, msgTooLarge))
-               assert.Equal(t, err.Error(), msgTooLarge.Error())
-               assert.True(t, errors.Is(err, msgTooLarge.ErrorType()))
+               assert.NotErrorIs(t, err, msgTooLarge)
+               assert.Contains(t, msgTooLarge.Error(), MessageSizeTooLarge.Error())
+               assert.ErrorIs(t, msgTooLarge, MessageSizeTooLarge)
        })
 }

Great recommendation! Implementation method has been updated.

@erikdw
Copy link
Contributor

erikdw commented Aug 22, 2024

@AndrewShearBayer & @jkoelker : any concern with me merging this?

@jkoelker
Copy link

No concerns here!

@erikdw erikdw merged commit a8e5eab into segmentio:main Aug 22, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MessageTooLargeError is not conforming to errors.Is
3 participants