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.

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

Jump to bottom

Replace all REST query empty result HTTP Status codes to 200 #4065

Merged
merged 6 commits into from
Apr 10, 2019
Merged

Replace all REST query empty result HTTP Status codes to 200 #4065

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
549a1d5
Replace http status code with http.StatusOK for empty query results
MarinX Apr 6, 2019
b501a2c
Fix test case to require http.StatusOK instead of http.StatusNoContent
MarinX Apr 6, 2019
5f32f04
Revert back status codes
MarinX Apr 8, 2019
18a65fc
Adds test for empty results
MarinX Apr 8, 2019
c23b54a
Update changelog entry
MarinX Apr 9, 2019
32df57f
Update changelog entry
MarinX Apr 10, 2019
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
2 changes: 1 addition & 1 deletion client/lcd/lcd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func TestCoinSend(t *testing.T) {

// query empty
res, body := Request(t, port, "GET", fmt.Sprintf("/auth/accounts/%s", someFakeAddr), nil)
require.Equal(t, http.StatusNoContent, res.StatusCode, body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should add check for empty account (eg. Account{})

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 added a test TestAccountBalanceQuery which covers checking if we are returning empty Account{}

require.Equal(t, http.StatusOK, res.StatusCode, body)

acc := getAccount(t, port, addr)
initialBalance := acc.GetCoins()
Expand Down
4 changes: 2 additions & 2 deletions x/auth/client/rest/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func QueryAccountRequestHandlerFn(

// the query will return empty if there is no data for this account
if len(res) == 0 {
w.WriteHeader(http.StatusNoContent)
w.WriteHeader(http.StatusOK)
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
return
}

Expand Down Expand Up @@ -88,7 +88,7 @@ func QueryBalancesRequestHandlerFn(

// the query will return empty if there is no data for this account
if len(res) == 0 {
w.WriteHeader(http.StatusNoContent)
w.WriteHeader(http.StatusOK)
Copy link
Collaborator

Choose a reason for hiding this comment

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

curious if this function is a duplicate from the one above ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of it is duplicated, only change is response. Eg. for /auth/accounts/{address} we are returning Account{} which in case /bank/balances/{address} we are returning sdk.Coins

The balance endpoint does not make sense for me, it should be /auth/accounts/{address}/balances and the same function should check the endpoint and return corresponding model. But this is a API breaking change so maybe it should be a separated issue for code cleanup :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree it doesn’t make much sense. Let’s open an issue for it

return
}

Expand Down
12 changes: 6 additions & 6 deletions x/gov/client/rest/rest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ func queryDepositHandlerFn(cdc *codec.Codec, cliCtx context.CLIContext) http.Han

if len(strProposalID) == 0 {
err := errors.New("proposalId required but not specified")
rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
rest.WriteErrorResponse(w, http.StatusOK, err.Error())
return
}

Expand All @@ -321,7 +321,7 @@ func queryDepositHandlerFn(cdc *codec.Codec, cliCtx context.CLIContext) http.Han

if len(bechDepositorAddr) == 0 {
err := errors.New("depositor address required but not specified")
rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
rest.WriteErrorResponse(w, http.StatusOK, err.Error())
return
}

Expand Down Expand Up @@ -387,7 +387,7 @@ func queryVoteHandlerFn(cdc *codec.Codec, cliCtx context.CLIContext) http.Handle

if len(strProposalID) == 0 {
err := errors.New("proposalId required but not specified")
rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
rest.WriteErrorResponse(w, http.StatusOK, err.Error())
return
}

Expand All @@ -398,7 +398,7 @@ func queryVoteHandlerFn(cdc *codec.Codec, cliCtx context.CLIContext) http.Handle

if len(bechVoterAddr) == 0 {
err := errors.New("voter address required but not specified")
rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
rest.WriteErrorResponse(w, http.StatusOK, err.Error())
return
}

Expand Down Expand Up @@ -464,7 +464,7 @@ func queryVotesOnProposalHandlerFn(cdc *codec.Codec, cliCtx context.CLIContext)

if len(strProposalID) == 0 {
err := errors.New("proposalId required but not specified")
rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
rest.WriteErrorResponse(w, http.StatusOK, err.Error())
return
}

Expand Down Expand Up @@ -579,7 +579,7 @@ func queryTallyOnProposalHandlerFn(cdc *codec.Codec, cliCtx context.CLIContext)

if len(strProposalID) == 0 {
err := errors.New("proposalId required but not specified")
rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noted. Reverted.

rest.WriteErrorResponse(w, http.StatusOK, err.Error())
return
}

Expand Down
13 changes: 4 additions & 9 deletions x/slashing/client/rest/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,6 @@ func signingInfoHandlerFn(cliCtx context.CLIContext, storeName string, cdc *code
return
}

if code == http.StatusNoContent {
w.WriteHeader(http.StatusNoContent)
return
}

rest.PostProcessResponse(w, cdc, signingInfo, cliCtx.Indent)
}
}
Expand All @@ -64,7 +59,7 @@ func signingInfoHandlerListFn(cliCtx context.CLIContext, storeName string, cdc *

_, page, limit, err := rest.ParseHTTPArgs(r)
if err != nil {
rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is also correct and shouldn't be removed, since this checks for badly parsed query parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Reverted

rest.WriteErrorResponse(w, http.StatusOK, err.Error())
return
}

Expand All @@ -81,7 +76,7 @@ func signingInfoHandlerListFn(cliCtx context.CLIContext, storeName string, cdc *
}

if len(validators.Validators) == 0 {
w.WriteHeader(http.StatusNoContent)
w.WriteHeader(http.StatusOK)
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
return
}

Expand All @@ -100,7 +95,7 @@ func signingInfoHandlerListFn(cliCtx context.CLIContext, storeName string, cdc *
}

if len(signingInfoList) == 0 {
w.WriteHeader(http.StatusNoContent)
w.WriteHeader(http.StatusOK)
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
return
}

Expand Down Expand Up @@ -132,7 +127,7 @@ func getSigningInfo(cliCtx context.CLIContext, storeName string, cdc *codec.Code
}

if len(res) == 0 {
code = http.StatusNoContent
code = http.StatusOK
return
}

Expand Down
2 changes: 1 addition & 1 deletion x/staking/client/rest/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ func delegatorTxsHandlerFn(cliCtx context.CLIContext, cdc *codec.Codec) http.Han
actions = append(actions, staking.MsgBeginRedelegate{}.Type())
actions = append(actions, tags.ActionCompleteRedelegation)
default:
w.WriteHeader(http.StatusNoContent)
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
w.WriteHeader(http.StatusOK)
return
}

Expand Down