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.

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

R4R: generalize query response with height #4573

Merged
merged 19 commits into from
Jul 1, 2019
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
1 change: 1 addition & 0 deletions .pending/improvements/sdk/4573-adds-height-in-
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#4573 adds height in response for validator endpoints
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions client/rpc/block.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func BlockRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}
rest.PostProcessResponse(w, cliCtx, output)
rest.PostProcessResponse(w, cliCtx, output, 0)
}
}

Expand All @@ -144,6 +144,6 @@ func LatestBlockRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
return
}

rest.PostProcessResponse(w, cliCtx, output)
rest.PostProcessResponse(w, cliCtx, output, 0)
}
}
2 changes: 1 addition & 1 deletion client/rpc/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func NodeInfoRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
}

nodeInfo := status.NodeInfo
rest.PostProcessResponse(w, cliCtx, nodeInfo)
rest.PostProcessResponse(w, cliCtx, nodeInfo, 0)
}
}

Expand Down
4 changes: 2 additions & 2 deletions client/rpc/validators.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ func ValidatorSetRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}
rest.PostProcessResponse(w, cliCtx, output)
rest.PostProcessResponse(w, cliCtx, output, 0)
}
}

Expand All @@ -189,6 +189,6 @@ func LatestValidatorSetRequestHandlerFn(cliCtx context.CLIContext) http.HandlerF
return
}

rest.PostProcessResponse(w, cliCtx, output)
rest.PostProcessResponse(w, cliCtx, output, 0)
}
}
37 changes: 36 additions & 1 deletion types/rest/rest.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package rest

import (
"encoding/json"
"errors"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -222,9 +223,17 @@ func ParseQueryHeightOrReturnBadRequest(w http.ResponseWriter, cliCtx context.CL
}

// PostProcessResponse performs post processing for a REST response.
func PostProcessResponse(w http.ResponseWriter, cliCtx context.CLIContext, response interface{}) {
// If the height is greater than zero it will be injected into the body of
// the response.An internal server error is written to the response if the
// height is negative or an encoding/decoding error occurs.
func PostProcessResponse(w http.ResponseWriter, cliCtx context.CLIContext, response interface{}, height int64) {
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
var output []byte

if height < 0 {
WriteErrorResponse(w, http.StatusInternalServerError, fmt.Errorf("negative height").Error())
return
}

switch response.(type) {
case []byte:
output = response.([]byte)
Expand All @@ -236,6 +245,32 @@ func PostProcessResponse(w http.ResponseWriter, cliCtx context.CLIContext, respo
} else {
output, err = cliCtx.Codec.MarshalJSON(response)
}

if err != nil {
WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}
}

// inject the height into the response by:
// - decoding into a map
// - adding the height to the map
// - encoding using standard JSON library
if height > 0 {
m := make(map[string]interface{})
err := json.Unmarshal(output, &m)
if err != nil {
WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

m["height"] = height

if cliCtx.Indent {
output, err = json.MarshalIndent(m, "", " ")
} else {
output, err = json.Marshal(m)
}
if err != nil {
WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
Expand Down
96 changes: 95 additions & 1 deletion types/rest/rest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,22 @@
package rest

import (
"encoding/json"
"fmt"
"io"
"io/ioutil"
"net/http"
"net/http/httptest"
"strconv"
"testing"

"github.com/cosmos/cosmos-sdk/client/context"
"github.com/stretchr/testify/require"
"github.com/tendermint/tendermint/crypto/secp256k1"

"github.com/cosmos/cosmos-sdk/client/context"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
Copy link
Contributor

Choose a reason for hiding this comment

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

Top level packages must not depend on x sub packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Offf, good catch -- I missed this!

Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably remove this test all together @colin-axner and move it to the LCD tests in Gaia.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don’t need LCD tests for queries as those are covered by the contract tests

Copy link
Contributor Author

@colin-axner colin-axner Jul 1, 2019

Choose a reason for hiding this comment

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

Makes sense, should I remove and move the tests elsewhere or should I refactor the test to use a mock version of account?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the latter

)

type mockResponseWriter struct{}
Expand Down Expand Up @@ -138,6 +145,93 @@ func TestParseQueryHeight(t *testing.T) {
}
}

func TestProcessPostResponse(t *testing.T) {
// setup
ctx := context.NewCLIContext()
height := int64(194423)

privKey := secp256k1.GenPrivKey()
pubKey := privKey.PubKey()
addr := types.AccAddress(pubKey.Address())
coins := types.NewCoins(types.NewCoin("atom", types.NewInt(100)), types.NewCoin("tree", types.NewInt(125)))
accNumber := uint64(104)
sequence := uint64(32)

acc := authtypes.NewBaseAccount(addr, coins, pubKey, accNumber, sequence)
cdc := codec.New()
authtypes.RegisterBaseAccount(cdc)
ctx = ctx.WithCodec(cdc)

// setup expected json responses with zero height
jsonNoHeight, err := cdc.MarshalJSON(acc)
require.Nil(t, err)
require.NotNil(t, jsonNoHeight)
jsonIndentNoHeight, err := cdc.MarshalJSONIndent(acc, "", " ")
require.Nil(t, err)
require.NotNil(t, jsonIndentNoHeight)

// decode into map to order alphabetically
m := make(map[string]interface{})
err = json.Unmarshal(jsonNoHeight, &m)
require.Nil(t, err)
jsonMap, err := json.Marshal(m)
require.Nil(t, err)
jsonWithHeight := append(append([]byte(`{"height":`), []byte(strconv.Itoa(int(height))+",")...), jsonMap[1:]...)
jsonIndentMap, err := json.MarshalIndent(m, "", " ")
jsonIndentWithHeight := append(append([]byte(`{`+"\n "+` "height": `), []byte(strconv.Itoa(int(height))+",")...), jsonIndentMap[1:]...)

// check that negative height writes an error
w := httptest.NewRecorder()
PostProcessResponse(w, ctx, acc, -1)
require.Equal(t, http.StatusInternalServerError, w.Code)

// check that zero height returns expected response
runPostProcessResponse(t, ctx, acc, jsonNoHeight, 0, false)
// chcek zero height with indent
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
runPostProcessResponse(t, ctx, acc, jsonIndentNoHeight, 0, true)
// check that height returns expected response
runPostProcessResponse(t, ctx, acc, jsonWithHeight, height, false)
// check height with indent
runPostProcessResponse(t, ctx, acc, jsonIndentWithHeight, height, true)
}

// asserts that ResponseRecorder returns the expected code and body
// runs PostProcessResponse on the objects regular interface and on
// the marshalled struct.
func runPostProcessResponse(t *testing.T, ctx context.CLIContext, obj interface{},
expectedBody []byte, height int64, indent bool,
) {
if indent {
ctx.Indent = indent
}

// test using regular struct
w := httptest.NewRecorder()
PostProcessResponse(w, ctx, obj, height)
require.Equal(t, http.StatusOK, w.Code, w.Body)
resp := w.Result()
body, err := ioutil.ReadAll(resp.Body)
require.Nil(t, err)
require.Equal(t, expectedBody, body)

var marshalled []byte
if indent {
marshalled, err = ctx.Codec.MarshalJSONIndent(obj, "", " ")
} else {
marshalled, err = ctx.Codec.MarshalJSON(obj)
}
require.Nil(t, err)

// test using marshalled struct
w = httptest.NewRecorder()
PostProcessResponse(w, ctx, marshalled, height)
require.Equal(t, http.StatusOK, w.Code, w.Body)
resp = w.Result()
body, err = ioutil.ReadAll(resp.Body)
require.Nil(t, err)
require.Equal(t, expectedBody, body)
}

func mustNewRequest(t *testing.T, method, url string, body io.Reader) *http.Request {
req, err := http.NewRequest(method, url, body)
require.NoError(t, err)
Expand Down
2 changes: 1 addition & 1 deletion x/auth/client/rest/broadcast.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,6 @@ func BroadcastTxRequest(cliCtx context.CLIContext) http.HandlerFunc {
return
}

rest.PostProcessResponse(w, cliCtx, res)
rest.PostProcessResponse(w, cliCtx, res, 0)
}
}
2 changes: 1 addition & 1 deletion x/auth/client/rest/encode.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,6 @@ func EncodeTxRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
txBytesBase64 := base64.StdEncoding.EncodeToString(txBytes)

response := EncodeResp{Tx: txBytesBase64}
rest.PostProcessResponse(w, cliCtx, response)
rest.PostProcessResponse(w, cliCtx, response, 0)
}
}
17 changes: 5 additions & 12 deletions x/auth/client/rest/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,6 @@ import (
"github.com/cosmos/cosmos-sdk/x/auth/types"
)

// AccountWithHeight wraps the embedded Account with the height it was queried
// at.
type AccountWithHeight struct {
types.Account `json:"account"`
Height int64 `json:"height"`
}

// query accountREST Handler
func QueryAccountRequestHandlerFn(storeName string, cliCtx context.CLIContext) http.HandlerFunc {

Expand All @@ -42,7 +35,7 @@ func QueryAccountRequestHandlerFn(storeName string, cliCtx context.CLIContext) h
accGetter := types.NewAccountRetriever(cliCtx)
// the query will return empty account if there is no data
if err := accGetter.EnsureExists(addr); err != nil {
rest.PostProcessResponse(w, cliCtx, types.BaseAccount{})
rest.PostProcessResponse(w, cliCtx, types.BaseAccount{}, cliCtx.Height)
return
}

Expand All @@ -52,7 +45,7 @@ func QueryAccountRequestHandlerFn(storeName string, cliCtx context.CLIContext) h
return
}

rest.PostProcessResponse(w, cliCtx, AccountWithHeight{account, cliCtx.Height})
rest.PostProcessResponse(w, cliCtx, account, cliCtx.Height)
}
}

Expand All @@ -79,7 +72,7 @@ func QueryTxsByTagsRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc
}

if len(r.Form) == 0 {
rest.PostProcessResponse(w, cliCtx, txs)
rest.PostProcessResponse(w, cliCtx, txs, cliCtx.Height)
return
}

Expand All @@ -95,7 +88,7 @@ func QueryTxsByTagsRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc
return
}

rest.PostProcessResponse(w, cliCtx, searchResult)
rest.PostProcessResponse(w, cliCtx, searchResult, cliCtx.Height)
}
}

Expand Down Expand Up @@ -125,6 +118,6 @@ func QueryTxRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
rest.WriteErrorResponse(w, http.StatusNotFound, fmt.Sprintf("no transaction found with hash %s", hashHexStr))
}

rest.PostProcessResponse(w, cliCtx, output)
rest.PostProcessResponse(w, cliCtx, output, cliCtx.Height)
}
}
6 changes: 3 additions & 3 deletions x/bank/client/rest/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,18 @@ func QueryBalancesRequestHandlerFn(cliCtx context.CLIContext) http.HandlerFunc {
return
}

res, _, err := cliCtx.QueryWithData("custom/bank/balances", bz)
res, height, err := cliCtx.QueryWithData("custom/bank/balances", bz)
if err != nil {
rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
return
}

// the query will return empty if there is no data for this account
if len(res) == 0 {
rest.PostProcessResponse(w, cliCtx, sdk.Coins{})
rest.PostProcessResponse(w, cliCtx, sdk.Coins{}, height)
return
}

rest.PostProcessResponse(w, cliCtx, res)
rest.PostProcessResponse(w, cliCtx, res, height)
}
}
Loading