Skip to content

Add custom err handler #207

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.

Sign up for GitHub

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
fabiante wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add custom err handler #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d78018c
Add custom err handler
fabiante Oct 20, 2024
c1463cb
Use custom err handler
fabiante Oct 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions api/err.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,16 @@ func respondWithError(ctx *gin.Context, status int, err error) {

ctx.AbortWithStatusJSON(status, response)
}

type ErrHandler func(c *gin.Context) error

func AsErrHandler(h ErrHandler) gin.HandlerFunc {
return func(ctx *gin.Context) {
err := h(ctx)

if err != nil {
_ = ctx.AbortWithError(500, err)
return
}
}
}
8 changes: 4 additions & 4 deletions api/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,19 @@ func NewServer(resolver app.ResolveServiceInterface, admin app.AdminServiceInter
return &Server{resolver: resolver, admin: admin, user: user}
}

func (s *Server) Resolve(ctx *gin.Context) {
func (s *Server) Resolve(ctx *gin.Context) error {
domain := ctx.Param("domain")
name := ctx.Param("name")

target, err := s.resolver.Resolve(domain, name)
switch true {
case err == nil:
ctx.Redirect(http.StatusFound, target)
return
return nil
case errors.Is(err, app.ErrNotFound):
respondWithError(ctx, http.StatusNotFound, err)
return
return nil
default:
respondWithError(ctx, http.StatusInternalServerError, err)
return err
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider the implications of removing respondWithError

By directly returning the error instead of calling respondWithError, we're no longer immediately responding to the client with an error status. This might lead to inconsistent error responses if not handled properly by the caller.

Consider one of the following options:

  1. Keep the respondWithError call before returning the error.
  2. Ensure that the caller (likely the AsErrHandler wrapper) properly handles all error cases and responds to the client appropriately.

Copy link
Copy Markdown
Owner Author

@fabiante fabiante Oct 20, 2024

Choose a reason for hiding this comment

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

Propose an alteration to AsErrHandler which makes use of a customer error type containing the error message and status to respond with.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Oct 20, 2024

Choose a reason for hiding this comment

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

@fabiante

Certainly! Here's an updated version of AsErrHandler that uses a custom error type containing the error message and status code:

// Define a custom error type
type HTTPError struct {
    StatusCode int
    Err        error
}

func (e *HTTPError) Error() string {
    return e.Err.Error()
}

// Update AsErrHandler to handle the custom error type
func AsErrHandler(h ErrHandler) gin.HandlerFunc {
    return func(ctx *gin.Context) {
        if err := h(ctx); err != nil {
            // Check if the error is of type *HTTPError
            if httpErr, ok := err.(*HTTPError); ok {
                ctx.AbortWithError(httpErr.StatusCode, httpErr.Err)
            } else {
                ctx.AbortWithError(http.StatusInternalServerError, err)
            }
        }
    }
}

Then, in your handler functions like Resolve, you can return an *HTTPError with the desired status code and error:

func (s *Server) Resolve(ctx *gin.Context) error {
    domain := ctx.Param("domain")
    name := ctx.Param("name")

    target, err := s.resolver.Resolve(domain, name)
    switch {
    case err == nil:
        ctx.Redirect(http.StatusFound, target)
        return nil
    case errors.Is(err, app.ErrNotFound):
        return &HTTPError{
            StatusCode: http.StatusNotFound,
            Err:        err,
        }
    default:
        return &HTTPError{
            StatusCode: http.StatusInternalServerError,
            Err:        err,
        }
    }
}

This approach allows AsErrHandler to respond with the appropriate HTTP status codes based on the custom error returned by your handlers, ensuring consistent and flexible error handling.

✏️ Learnings added 
Learnt from: fabiante
PR: fabiante/persurl#207
File: api/server.go:34-34
Timestamp: 2024-10-20T10:36:41.825Z
Learning: In the `api/err.go` file, when using `AsErrHandler` in the Gin framework (Go), prefer using a custom error type (`HTTPError`) that contains the status code and error message to respond with, allowing for consistent and flexible error handling.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}
}
2 changes: 1 addition & 1 deletion api/server_routes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func SetupRouting(r gin.IRouter, s *Server) {

resolve.Use(validDomain, validName)

resolve.GET("/:domain/:name", s.Resolve)
resolve.GET("/:domain/:name", AsErrHandler(s.Resolve))
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Oct 20, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Approve and Recommend Expanding Error Handling Consistency

The modification to wrap s.Resolve with AsErrHandler effectively standardizes error handling for this endpoint, enhancing consistency in error responses.

Recommendations:

  1. Extend AsErrHandler Usage: Apply this error handling pattern to other route handlers (e.g., in admin and system groups) to ensure uniform error management across the API.
  2. Update Documentation: Revise the API documentation to outline the standardized error responses provided by AsErrHandler, aiding developers in understanding and utilizing the error handling mechanism effectively.
🔗 Analysis chain

Approve change and suggest consistent application

The modification to wrap s.Resolve with AsErrHandler is a good improvement for standardized error handling. This change likely enhances error reporting and handling consistency for the resolve endpoint.

Consider the following recommendations:

  1. Apply this pattern to other routes (e.g., in the admin and system groups) for consistency across the API.
  2. Update the API documentation to reflect any changes in error responses that may result from using AsErrHandler.

To ensure the change doesn't introduce unexpected behavior, please verify the following:

  1. The implementation of AsErrHandler in api/err.go.
  2. The impact on API responses, especially for error cases.

You can use the following script to check the AsErrHandler implementation and its usage:

This will help ensure that the error handling is implemented correctly and used consistently.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check AsErrHandler implementation and usage

# Check AsErrHandler implementation
echo "AsErrHandler implementation:"
rg -A 10 "func AsErrHandler" api/err.go

# Check AsErrHandler usage across the codebase
echo "\nAsErrHandler usage:"
rg "AsErrHandler\(" api/

Length of output: 500

}

// Admin endpoints
Expand Down