Skip to content
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

requestbody: Type-based error handling for MaxBytesError #6701

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

rishitashaw
Copy link
Contributor

Replaced the hardcoded error string comparison with a type assertion for http.MaxBytesError to make the code more robust and idiomatic.

Before:

if err != nil && err.Error() == "http: request body too large" {
    err = caddyhttp.Error(http.StatusRequestEntityTooLarge, err)
}

After:

if err != nil {
    if _, ok := err.(*http.MaxBytesError); ok {
        err = caddyhttp.Error(http.StatusRequestEntityTooLarge, err)
    }
}

Related to #6700

@CLAassistant
Copy link

CLAassistant commented Nov 22, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thank you! This is pretty good, but there's a lint failure. I have a suggestion that I think could work but I just did it here in the textbox, so if you want to verify locally that it compiles then I think we'll be all set. :)

Comment on lines 97 to 98
if err != nil {
if _, ok := err.(*http.MaxBytesError); ok {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
if _, ok := err.(*http.MaxBytesError); ok {
var mbe *http.MaxBytesError
if errors.As(err, &mbe) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry I didn’t see your comment earlier. I’ve made different changes. Specifically, I removed the redundant nil check and used the type assertion directly for *HTTP.MaxBytesError since the type assertion already guarantees that the error isn’t nil when ok is true.

That said, I really like this project and would love to be more involved. Would it be possible to connect on Slack? I’m eager to contribute more!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I still think errors.As() is recommended since the error value can be wrapped, thus failing a type assertion.

And yes! I'm running out the door but when I get back I'll send a Slack invite.

@francislavoie
Copy link
Member

francislavoie commented Nov 22, 2024

I was thinking we'd use https://pkg.go.dev/errors#example-Is errors.Is(err, http.MaxBytesError), then the if wouldn't need to be nested, it could just remain a one-liner condition.

@francislavoie francislavoie changed the title Switch to type-based error handling for "request body too large" requestbody: Type-based error handling for MaxBytesError Nov 22, 2024
@mholt
Copy link
Member

mholt commented Nov 22, 2024

@francislavoie It's a type, not a value.

@francislavoie
Copy link
Member

Oh right, they implemented MaxBytesError as a struct type, not as a var error. Gah.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! @rishitashaw What is a good email address for me to invite?

@mholt mholt enabled auto-merge (squash) November 22, 2024 19:36
@mholt mholt merged commit 8c3dd3d into caddyserver:master Nov 22, 2024
33 checks passed
@rishitashaw
Copy link
Contributor Author

Thanks @mholt
Please send the invite to [email protected]

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.

4 participants