-
Notifications
You must be signed in to change notification settings - Fork 21
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
Implement Dummy API for Project Understanding #445
Changes from 3 commits
171aca9
5fdfdbc
b1bcdc7
3f38f34
2587139
106acf7
91bb9b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package models | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
) | ||
|
||
func RequiredFieldError(field string) error { | ||
return errors.New(field + " is required") | ||
} | ||
|
||
func MinLengthError(field string, min int) error { | ||
return errors.New(fmt.Sprintf("%s must be at least %d characters", field, min)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package models | ||
|
||
type HelloRequest struct { | ||
|
||
// New nickname to say hello | ||
Nickname string `json:"nickname"` | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package models | ||
|
||
type HelloResponse struct { | ||
|
||
// Welcome message | ||
Message string `json:"message"` | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package models | ||
|
||
func (r *HelloRequest) Validate() error { | ||
if r.Nickname == "" { | ||
return RequiredFieldError("nickname") | ||
} | ||
|
||
if len(r.Nickname) < 2 { | ||
return MinLengthError("nickname", 2) | ||
} | ||
|
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
package hello | ||
|
||
import ( | ||
"github.com/labstack/echo/v4" | ||
"github.com/yorkie-team/codepair/backend/api/codepair/v1/models" | ||
"github.com/yorkie-team/codepair/backend/internal/transport/rest" | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Type Mismatch Issue in newHttpExceptionResponse
🔗 Analysis chainVerify rest package compatibility. The static analysis indicates a type mismatch in the rest package where I'll gather more details about the definition of 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check the rest package implementation for type compatibility issues
# Check the error_responses.go file implementation
ast-grep --pattern 'type statusCode int32'
# Check for any type conversion between int32 and int in error handling
rg -A 5 'statusCode.*int'
Length of output: 583 Script: #!/bin/bash
# Check the models package for the HttpExceptionResponse type definition and its fields.
rg -C 5 "type HttpExceptionResponse" --type go
Length of output: 724 🧰 Tools🪛 golangci-lint (1.62.2)6-6: could not import github.com/yorkie-team/codepair/backend/internal/transport/rest (-: # github.com/yorkie-team/codepair/backend/internal/transport/rest (typecheck) |
||
|
||
type Handler struct { | ||
helloService Service | ||
kokodak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Service is implemented as a concrete type rather than an interface.
🔗 Analysis chainVerify Service interface definition. The Handler struct depends on a Service type that needs to be properly defined with the required methods. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check the Service interface definition
# Look for Service interface definition
ast-grep --pattern 'type Service interface {
$$$
}'
# Look for Service implementation
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) HelloCodePair($_, $_) ($_, $_) {
$$$
}'
Length of output: 158 Script: #!/bin/bash
# Check for the Service interface definition using ripgrep
echo "Searching for 'type Service interface' definitions:"
rg -n "type Service interface" .
# Additionally, search for any mention of HelloCodePair
echo "Searching for 'HelloCodePair' method definitions:"
rg -n "func .*HelloCodePair" .
Length of output: 548 |
||
|
||
// NewHandler creates a new handler for hello. | ||
func NewHandler(service Service) Handler { | ||
return Handler{ | ||
helloService: service, | ||
} | ||
} | ||
|
||
// HelloCodePair returns a hello message for a given CodePairVisitor. | ||
func (h Handler) HelloCodePair(e echo.Context) error { | ||
data := new(models.HelloRequest) | ||
|
||
if err := rest.BindAndValidateRequest(e, data); err != nil { | ||
return err | ||
} | ||
|
||
helloMessage, err := h.helloService.HelloCodePair(e, CodePairVisitor{ | ||
Nickname: data.Nickname, | ||
}) | ||
if err != nil { | ||
return rest.NewErrorResponse(e, err) | ||
} | ||
|
||
return rest.NewOkResponse(e, models.HelloResponse{ | ||
Message: helloMessage, | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package hello | ||
|
||
// CodePairVisitor is a visitor for CodePair | ||
type CodePairVisitor struct { | ||
// Nickname is the nickname of the visitor | ||
Nickname string | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
package hello | ||
|
||
type Repository interface { | ||
// ReadHelloMessageFor reads a hello message for a given CodePairVisitor | ||
ReadHelloMessageFor(codePairVisitor CodePairVisitor) (string, error) | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,28 @@ | ||||||||||||||||||||||||||||||||
package hello | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||
"github.com/labstack/echo/v4" | ||||||||||||||||||||||||||||||||
"github.com/yorkie-team/codepair/backend/internal/transport/rest" | ||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
type Service struct { | ||||||||||||||||||||||||||||||||
helloRepository Repository | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
// NewService creates a new service for hello. | ||||||||||||||||||||||||||||||||
func NewService(repository Repository) Service { | ||||||||||||||||||||||||||||||||
return Service{ | ||||||||||||||||||||||||||||||||
helloRepository: repository, | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
// HelloCodePair returns a hello message for a given CodePairVisitor | ||||||||||||||||||||||||||||||||
func (s Service) HelloCodePair(e echo.Context, codePairVisitor CodePairVisitor) (string, error) { | ||||||||||||||||||||||||||||||||
helloMessage, err := s.helloRepository.ReadHelloMessageFor(codePairVisitor) | ||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||
e.Logger().Fatal(err) | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace Fatal logging with Error level logging. Using Apply this diff: - e.Logger().Fatal(err)
+ e.Logger().Error(err) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
return "", rest.InternalServerError | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Wrap the error to preserve context. The original error context is lost when returning a generic internal server error. Wrap the error to maintain the error chain for better debugging. Apply this diff: - return "", rest.InternalServerError
+ return "", fmt.Errorf("failed to read hello message: %w", err) Don't forget to add the import: +import "fmt" 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
return helloMessage, nil | ||||||||||||||||||||||||||||||||
devleejb marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,14 @@ | ||||||||||||||||||||
package mongo | ||||||||||||||||||||
|
||||||||||||||||||||
import "github.com/yorkie-team/codepair/backend/internal/core/hello" | ||||||||||||||||||||
|
||||||||||||||||||||
type HelloRepository struct{} | ||||||||||||||||||||
|
||||||||||||||||||||
// NewHelloRepository creates a new HelloRepository. | ||||||||||||||||||||
func NewHelloRepository() HelloRepository { | ||||||||||||||||||||
return HelloRepository{} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
func (h HelloRepository) ReadHelloMessageFor(codePairVisitor hello.CodePairVisitor) (string, error) { | ||||||||||||||||||||
return "Hello, " + codePairVisitor.Nickname + "!", nil | ||||||||||||||||||||
} | ||||||||||||||||||||
Comment on lines
+12
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add input validation and error handling. The current implementation could be improved with:
Consider this safer implementation: func (h HelloRepository) ReadHelloMessageFor(codePairVisitor hello.CodePairVisitor) (string, error) {
+ if codePairVisitor.Nickname == "" {
+ return "", errors.New("invalid visitor: nickname is required")
+ }
return "Hello, " + codePairVisitor.Nickname + "!", nil
} 📝 Committable suggestion
Suggested change
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,20 @@ | ||
package server | ||
|
||
import ( | ||
"fmt" | ||
"net/http" | ||
|
||
"github.com/labstack/echo/v4" | ||
"github.com/yorkie-team/codepair/backend/internal/core/hello" | ||
"github.com/yorkie-team/codepair/backend/internal/infra/database/mongo" | ||
) | ||
|
||
func RegisterRoutes(e *echo.Echo) { | ||
e.GET("/", func(c echo.Context) error { | ||
err := c.String(http.StatusOK, "Hello, World!") | ||
return fmt.Errorf("error: %w", err) | ||
}) | ||
// Repositories | ||
helloRepository := mongo.NewHelloRepository() | ||
kokodak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Services | ||
helloService := hello.NewService(helloRepository) | ||
|
||
// Handlers | ||
helloHandler := hello.NewHandler(helloService) | ||
|
||
e.POST("/hello", helloHandler.HelloCodePair) | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,16 @@ | ||||||||||||||||||||||||||||||||||
package rest | ||||||||||||||||||||||||||||||||||
kokodak marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||
"errors" | ||||||||||||||||||||||||||||||||||
"github.com/yorkie-team/codepair/backend/api/codepair/v1/models" | ||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
// ConvertErrorToResponse converts an error to a response. | ||||||||||||||||||||||||||||||||||
func ConvertErrorToResponse(err error) models.HttpExceptionResponse { | ||||||||||||||||||||||||||||||||||
switch { | ||||||||||||||||||||||||||||||||||
case errors.Is(err, InternalServerError): | ||||||||||||||||||||||||||||||||||
return NewInternalServerErrorResponse() | ||||||||||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||||||||||
return NewInvalidJsonErrorResponse() | ||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve error handling in the default case. The default case returns an invalid JSON error response, which might be misleading for other types of errors. Consider adding more error cases and using a generic error response for unhandled errors. func ConvertErrorToResponse(err error) models.HttpExceptionResponse {
switch {
case errors.Is(err, InternalServerError):
return NewInternalServerErrorResponse()
+ case errors.Is(err, &models.ValidationError{}):
+ return NewValidationErrorResponse(err.Error())
default:
- return NewInvalidJsonErrorResponse()
+ return newHttpExceptionResponse(http.StatusInternalServerError, err.Error())
}
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package rest | ||
|
||
import ( | ||
"github.com/yorkie-team/codepair/backend/api/codepair/v1/models" | ||
"net/http" | ||
) | ||
|
||
func newHttpExceptionResponse(statusCode int32, message string) models.HttpExceptionResponse { | ||
return models.HttpExceptionResponse{ | ||
StatusCode: statusCode, | ||
Message: message, | ||
} | ||
} | ||
|
||
// NewInvalidJsonErrorResponse creates a HttpExceptionResponse that represents an invalid JSON error. | ||
func NewInvalidJsonErrorResponse() models.HttpExceptionResponse { | ||
return newHttpExceptionResponse(http.StatusBadRequest, "Invalid JSON") | ||
} | ||
|
||
// NewValidationErrorResponse creates a HttpExceptionResponse that represents a validation error. | ||
func NewValidationErrorResponse(reason string) models.HttpExceptionResponse { | ||
return newHttpExceptionResponse(http.StatusBadRequest, reason) | ||
} | ||
|
||
// NewInternalServerErrorResponse creates a HttpExceptionResponse that represents a internal server error. | ||
func NewInternalServerErrorResponse() models.HttpExceptionResponse { | ||
return newHttpExceptionResponse(http.StatusInternalServerError, "Internal Server Error") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package rest | ||
|
||
import "errors" | ||
|
||
var ( | ||
// InternalServerError is an internal server error | ||
InternalServerError = errors.New("internal server error") | ||
) |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,35 @@ | ||||||||||
package rest | ||||||||||
|
||||||||||
import ( | ||||||||||
"github.com/labstack/echo/v4" | ||||||||||
"net/http" | ||||||||||
) | ||||||||||
|
||||||||||
type request interface { | ||||||||||
Validate() error | ||||||||||
} | ||||||||||
|
||||||||||
// BindAndValidateRequest binds and validates the request. | ||||||||||
// If the request is invalid, it returns an error response. | ||||||||||
func BindAndValidateRequest(e echo.Context, req request) error { | ||||||||||
if err := e.Bind(req); err != nil { | ||||||||||
return e.JSON(http.StatusBadRequest, NewInvalidJsonErrorResponse()) | ||||||||||
} | ||||||||||
if err := req.Validate(); err != nil { | ||||||||||
return e.JSON(http.StatusBadRequest, NewValidationErrorResponse(err.Error())) | ||||||||||
} | ||||||||||
return nil | ||||||||||
} | ||||||||||
|
||||||||||
// NewErrorResponse handles the creation and response of an error. | ||||||||||
// It converts the provided error into a response structure and sends it as a JSON response. | ||||||||||
// The response status code is determined by the error's associated status. | ||||||||||
func NewErrorResponse(e echo.Context, err error) error { | ||||||||||
resp := ConvertErrorToResponse(err) | ||||||||||
return e.JSON(int(resp.StatusCode), ConvertErrorToResponse(err)) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Remove duplicate error conversion.
Apply this diff: - resp := ConvertErrorToResponse(err)
- return e.JSON(int(resp.StatusCode), ConvertErrorToResponse(err))
+ resp := ConvertErrorToResponse(err)
+ return e.JSON(int(resp.StatusCode), resp) 📝 Committable suggestion
Suggested change
|
||||||||||
} | ||||||||||
|
||||||||||
// NewOkResponse sends a JSON response with a status code of 200. | ||||||||||
func NewOkResponse(e echo.Context, resp interface{}) error { | ||||||||||
return e.JSON(http.StatusOK, resp) | ||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about writing a Request, Response, and Validator of the same model (
Hello
) in the same file? It seems unnecessary to write the Request and Response in different files.Maybe it would be better to write them together in the router file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally agree with @blurfx's suggestion, as this style of micro-separating so-called DTOs is not a common approach in Golang and may lead to an unnecessary proliferation of files for DTO management.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One important thing to keep in mind is that all request and response types are auto-generated from the OpenAPI specification.
Validator
Since these files are auto-generated, declaring validation functions within the same file is risky, as they might be overwritten during regeneration.
Type Placement
Moving these types to their corresponding packages is quite challenging, as it is auto-generated. Additionally, the directory structure is similar to yorkie. I believe these types correspond to files like
admin.pb.go
andyorkie.pb.go
.DTO
I’m unsure how we should manage request and response types for REST APIs in Go. Could you provide an example or some guidance on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn’t realize these files were auto-generated by OpenAPI generation. If that’s the case, I think it’s great!