Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement Dummy API for Project Understanding #445
Changes from 2 commits
171aca9
5fdfdbc
b1bcdc7
3f38f34
2587139
106acf7
91bb9b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Fix string conversion in error message.
The current implementation uses
string(min)
which converts the integer to a single rune character instead of a string representation of the number.Use
strconv.Itoa
to properly convert the integer to a string:📝 Committable suggestion
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 9-9: redefines-builtin-id: redefinition of the built-in function min
(revive)
10-10: stringintconv: conversion from int to string yields a string of one rune, not a string of digits
(govet)
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!
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.
💡 Codebase verification
Type Mismatch Issue in newHttpExceptionResponse
The function in
backend-go/internal/transport/rest/error_responses.go
is declared withstatusCode
as anint32
parameter, but it directly assigns this value toStatusCode
in amodels.HttpExceptionResponse
, which is defined as anint
. This mismatch requires either an explicit conversion (e.g.,StatusCode: int(statusCode)
) or a change of the parameter’s type toint
to ensure type compatibility.backend-go/internal/transport/rest/error_responses.go: newHttpExceptionResponse
🔗 Analysis chain
Verify rest package compatibility.
The static analysis indicates a type mismatch in the rest package where
statusCode
(int32) is being used where an int is expected. This could affect error handling in this handler.I'll gather more details about the definition of
HttpExceptionResponse
in the models package to verify if there is any type mismatch with thestatusCode
parameter in the rest package.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 583
Script:
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
internal/transport/rest/error_responses.go:10:15: cannot use statusCode (variable of type int32) as int value in struct literal)
(typecheck)
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.
💡 Codebase verification
Service is implemented as a concrete type rather than an interface.
type Service interface
definition returned no results.HelloCodePair
method is defined on a concreteService
type inbackend-go/internal/core/hello/service.go
.🔗 Analysis chain
Verify Service interface definition.
The Handler struct depends on a Service type that needs to be properly defined with the required methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 158
Script:
Length of output: 548
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.
Replace Fatal logging with Error level logging.
Using
Fatal
logging here is problematic as it terminates the program. For recoverable errors, useError
level logging instead to maintain service availability.Apply this diff:
📝 Committable suggestion
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.
🛠️ 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:
Don't forget to add the import:
+import "fmt"
📝 Committable suggestion
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.
🛠️ Refactor suggestion
Add input validation and error handling.
The current implementation could be improved with:
Consider this safer implementation:
📝 Committable suggestion
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.
🛠️ 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.
📝 Committable suggestion
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.
🛠️ Refactor suggestion
Remove duplicate error conversion.
ConvertErrorToResponse
is called twice unnecessarily. Store the result in a variable to avoid the duplicate conversion.Apply this diff:
📝 Committable suggestion