-
Notifications
You must be signed in to change notification settings - Fork 197
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
generic API responses #1823
generic API responses #1823
Conversation
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.
partial review
api/node/routes.go
Outdated
c.JSON( | ||
http.StatusOK, | ||
core.GenericAPIResponse{ | ||
Data: gin.H{"details": details}, |
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.
why wrapping it again?
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.
in order to group them under a functionality/component. I did the same everywhere. I might rename details
to metrics
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.
no, the ideea from the meeting was that, under the "data" field we should have the raw object. That is what I have understood. @ccorcoveanu ?
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.
A cleaner approach is to have this extra property wrap inside data
, this way the response is verbose enough so you know what you are getting back.
api/node/routes.go
Outdated
c.JSON( | ||
http.StatusOK, | ||
core.GenericAPIResponse{ | ||
Data: gin.H{"details": details}, |
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.
also here?
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.
in order to group them under a functionality/component. I did the same everywhere. I might rename details to metrics
api/address/routes.go
Outdated
shared.GenericAPIResponse{ | ||
Data: nil, | ||
Error: errors.ErrInvalidAppContext.Error(), | ||
Code: string(shared.ReturnCodeInternalError), |
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 think you can make Code
inside GenericAPIResponse
of ReturnCode
type so you would get rid of all this casts
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.
Also, return code suggests integer, so I would use integers for this. If we have the ReturnCode
type, it could implement Stringer
interface. We can have a method: FormatError(code ReturnCode, err error)
that returns code + ":" + err.Error()
, and this can be our Error for GenericAPIResponse
.
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.
If I would set it to ReturnCode
type, why would this help us? You can see the return code of each API call without the need to be included in the response payload. The idea of return code is that we can define custom return codes that can be customized in the future. For now, I only 'copied' the 200, 400 and 500 return codes.
@andreibancioiu can you please provide an input?
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 was saying to use the ReturnCode type directly in the structure so you don’t have to do string(ReturnCode)
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.
Done. also, changed return codes messages to snake case
api/node/routes.go
Outdated
c.JSON( | ||
http.StatusOK, | ||
core.GenericAPIResponse{ | ||
Data: gin.H{"details": details}, |
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.
A cleaner approach is to have this extra property wrap inside data
, this way the response is verbose enough so you know what you are getting back.
api/address/routes.go
Outdated
shared.GenericAPIResponse{ | ||
Data: nil, | ||
Error: errors.ErrInvalidAppContext.Error(), | ||
Code: string(shared.ReturnCodeInternalError), |
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.
Also, return code suggests integer, so I would use integers for this. If we have the ReturnCode
type, it could implement Stringer
interface. We can have a method: FormatError(code ReturnCode, err error)
that returns code + ":" + err.Error()
, and this can be our Error for GenericAPIResponse
.
# Conflicts: # api/address/routes_test.go # api/transaction/routes.go # api/transaction/routes_test.go
The base branch was changed.
The base branch was changed.
63a8dcd
# Conflicts: # api/transaction/routes.go # api/transaction/routes_test.go
@@ -279,10 +272,10 @@ func PeerInfo(c *gin.Context) { | |||
if !ok { | |||
c.JSON( | |||
http.StatusInternalServerError, |
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.
Maybe remove the struct from routes_test.go? The one with the TODO, also the TODO on L352 should be fixed. Thanks.
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.
done
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.
System tests passed.
Changed the API response structure for all existing routes to include 3 fields:
bad_request
)