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

exp/lighthorizon/actions: use standard Problem model on API error responses #4542

Merged
merged 4 commits into from
Aug 17, 2022
Merged

exp/lighthorizon/actions: use standard Problem model on API error responses #4542

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
128b699
#4472: applied the standard Problem model for error responses
sreuland Aug 13, 2022
2f356d8
Merge remote-tracking branch 'upstream/lighthorizon' into 4472_error_…
sreuland Aug 15, 2022
8c9f811
#4472: aligned detail and invalid_field error attributes, pr feedback
sreuland Aug 16, 2022
77bdead
#4472: use require in test error checking, pr feedback
sreuland Aug 16, 2022
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
34 changes: 20 additions & 14 deletions exp/lighthorizon/actions/accounts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package actions

import (
"errors"
"net/http"
"strconv"

Expand All @@ -11,26 +12,25 @@ import (
hProtocol "github.com/stellar/go/protocols/horizon"
"github.com/stellar/go/protocols/horizon/operations"
"github.com/stellar/go/support/render/hal"
supportProblem "github.com/stellar/go/support/render/problem"
"github.com/stellar/go/toid"
)

const (
urlAccountId = "account_id"
)

func accountRequestParams(w http.ResponseWriter, r *http.Request) (string, pagination) {
func accountRequestParams(w http.ResponseWriter, r *http.Request) (string, pagination, error) {
var accountId string
var accountErr bool

if accountId, accountErr = getURLParam(r, urlAccountId); !accountErr {
sendErrorResponse(w, http.StatusBadRequest, "")
return "", pagination{}
return "", pagination{}, errors.New("unable to find account_id in url path")
Shaptic marked this conversation as resolved.
Show resolved Hide resolved
}

paginate, err := paging(r)
if err != nil {
sendErrorResponse(w, http.StatusBadRequest, string(invalidPagingParameters))
return "", pagination{}
return "", pagination{}, err
}

if paginate.Cursor < 1 {
Expand All @@ -41,16 +41,19 @@ func accountRequestParams(w http.ResponseWriter, r *http.Request) (string, pagin
paginate.Limit = 10
}

return accountId, paginate
return accountId, paginate, nil
}

func NewTXByAccountHandler(lightHorizon services.LightHorizon) func(http.ResponseWriter, *http.Request) {
return func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
var accountId string
var paginate pagination
var err error

if accountId, paginate = accountRequestParams(w, r); accountId == "" {
if accountId, paginate, err = accountRequestParams(w, r); err != nil {
errorMsg := supportProblem.MakeInvalidFieldProblem("invalid request parameters", err)
sendErrorResponse(r.Context(), w, *errorMsg)
return
}

Expand All @@ -65,7 +68,7 @@ func NewTXByAccountHandler(lightHorizon services.LightHorizon) func(http.Respons
txns, err := lightHorizon.Transactions.GetTransactionsByAccount(ctx, paginate.Cursor, paginate.Limit, accountId)
if err != nil {
log.Error(err)
sendErrorResponse(w, http.StatusInternalServerError, err.Error())
sendErrorResponse(r.Context(), w, supportProblem.ServerError)
return
}

Expand All @@ -74,15 +77,15 @@ func NewTXByAccountHandler(lightHorizon services.LightHorizon) func(http.Respons
response, err = adapters.PopulateTransaction(r, &txn)
if err != nil {
log.Error(err)
sendErrorResponse(w, http.StatusInternalServerError, err.Error())
sendErrorResponse(r.Context(), w, supportProblem.ServerError)
return
}

page.Add(response)
}

page.PopulateLinks()
sendPageResponse(w, page)
sendPageResponse(r.Context(), w, page)
}
}

Expand All @@ -91,8 +94,11 @@ func NewOpsByAccountHandler(lightHorizon services.LightHorizon) func(http.Respon
ctx := r.Context()
var accountId string
var paginate pagination
var err error

if accountId, paginate = accountRequestParams(w, r); accountId == "" {
if accountId, paginate, err = accountRequestParams(w, r); err != nil {
errorMsg := supportProblem.MakeInvalidFieldProblem("invalid request parameters", err)
sendErrorResponse(r.Context(), w, *errorMsg)
return
}

Expand All @@ -107,7 +113,7 @@ func NewOpsByAccountHandler(lightHorizon services.LightHorizon) func(http.Respon
ops, err := lightHorizon.Operations.GetOperationsByAccount(ctx, paginate.Cursor, paginate.Limit, accountId)
if err != nil {
log.Error(err)
sendErrorResponse(w, http.StatusInternalServerError, err.Error())
sendErrorResponse(r.Context(), w, supportProblem.ServerError)
return
}

Expand All @@ -116,14 +122,14 @@ func NewOpsByAccountHandler(lightHorizon services.LightHorizon) func(http.Respon
response, err = adapters.PopulateOperation(r, &op)
if err != nil {
log.Error(err)
sendErrorResponse(w, http.StatusInternalServerError, err.Error())
sendErrorResponse(r.Context(), w, supportProblem.ServerError)
return
}

page.Add(response)
}

page.PopulateLinks()
sendPageResponse(w, page)
sendPageResponse(r.Context(), w, page)
}
}
151 changes: 151 additions & 0 deletions exp/lighthorizon/actions/accounts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
package actions

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

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"

"github.com/stellar/go/exp/lighthorizon/common"
"github.com/stellar/go/exp/lighthorizon/services"
"github.com/stellar/go/support/render/problem"
)

func TestTxByAccountMissingParamError(t *testing.T) {
recorder := httptest.NewRecorder()
request := makeRequest(
t,
map[string]string{},
map[string]string{},
)

mockOperationService := &services.MockOperationService{}
mockTransactionService := &services.MockTransactionService{}

lh := services.LightHorizon{
Operations: mockOperationService,
Transactions: mockTransactionService,
}

handler := NewTXByAccountHandler(lh)
handler(recorder, request)

resp := recorder.Result()
assert.Equal(t, http.StatusBadRequest, resp.StatusCode)

raw, err := ioutil.ReadAll(resp.Body)
assert.NoError(t, err)

var problem problem.P
err = json.Unmarshal(raw, &problem)
assert.NoError(t, err)
assert.Equal(t, "Bad Request", problem.Title)
assert.Equal(t, "invalid request parameters", problem.Extras["invalid_field"])
Shaptic marked this conversation as resolved.
Show resolved Hide resolved
assert.Equal(t, "unable to find account_id in url path", problem.Extras["reason"])
}

func TestTxByAccountServerError(t *testing.T) {
recorder := httptest.NewRecorder()
pathParams := make(map[string]string)
pathParams["account_id"] = "G1234"
request := makeRequest(
t,
map[string]string{},
pathParams,
)

mockOperationService := &services.MockOperationService{}
mockTransactionService := &services.MockTransactionService{}
Copy link
Contributor

Choose a reason for hiding this comment

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

By using mocking here, are we omitting testing of the actual behavior in relation to transaction querying? e.g. I don't see tests for 404s on a missing account. And a lot of these errors actually happen before TransactionService.GetTransactionsByAccount() gets invoked, right? Parameter validation, etc. happens beforehand. I think we'd get better coverage if we mock exactly what needs to be mocked (e.g. 500s).

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 think that is what is mocked here, have told the data access function to throw an error not good which represents unrecoverable/unexpected err from code, equates to server-side 500 response.

Haven't added tests on 404's because the app layer hasn't determined that yet, it's returning empty list responses when the index reports no NextActive(), maybe because absence of the account is not as deterministic here since we don't have current state, just what exists after some range is indexed? maybe we just need to be consistent and choose whether to return 404's or empty list, I think with a stateless cursor API, the empty list response may feel more natural. since client would get data for each cursor fetch iteration, until the last cursor fetch would give empty list instead of 404.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I put this comment in the wrong place. I meant for it to apply to the test in TestTxByAccountMissingParamError, but I don't think it applies there anymore, either, thinking about it more. Since the mocked code (TransactionService) never gets reached before hitting the 400 error like I said, it doesn't matter if it's mocked out or not.

Re: 404s, we can still distinguish between account_id not being in the index (which would be a 404) vs. NextActive() returning EOF (which would be an empty tx list). I see what you mean about a partial view of the world, but it still counts as a 404 imo: if the ledger range you indexed doesn't have any activity for an account, a 404 is a valid description of the account.

mockTransactionService.On("GetTransactionsByAccount", mock.Anything, mock.Anything, mock.Anything, "G1234").Return([]common.Transaction{}, errors.New("not good"))

lh := services.LightHorizon{
Operations: mockOperationService,
Transactions: mockTransactionService,
}

handler := NewTXByAccountHandler(lh)
handler(recorder, request)

resp := recorder.Result()
assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)

raw, err := ioutil.ReadAll(resp.Body)
assert.NoError(t, err)

var problem problem.P
err = json.Unmarshal(raw, &problem)
assert.NoError(t, err)
assert.Equal(t, "Internal Server Error", problem.Title)
}

func TestOpsByAccountMissingParamError(t *testing.T) {
recorder := httptest.NewRecorder()
request := makeRequest(
t,
map[string]string{},
map[string]string{},
)

mockOperationService := &services.MockOperationService{}
mockTransactionService := &services.MockTransactionService{}

lh := services.LightHorizon{
Operations: mockOperationService,
Transactions: mockTransactionService,
}

handler := NewOpsByAccountHandler(lh)
handler(recorder, request)

resp := recorder.Result()
assert.Equal(t, http.StatusBadRequest, resp.StatusCode)

raw, err := ioutil.ReadAll(resp.Body)
assert.NoError(t, err)

var problem problem.P
err = json.Unmarshal(raw, &problem)
assert.NoError(t, err)
assert.Equal(t, "Bad Request", problem.Title)
assert.Equal(t, "invalid request parameters", problem.Extras["invalid_field"])
assert.Equal(t, "unable to find account_id in url path", problem.Extras["reason"])
}

func TestOpsByAccountServerError(t *testing.T) {
recorder := httptest.NewRecorder()
pathParams := make(map[string]string)
pathParams["account_id"] = "G1234"
request := makeRequest(
t,
map[string]string{},
pathParams,
)

mockOperationService := &services.MockOperationService{}
mockTransactionService := &services.MockTransactionService{}
mockOperationService.On("GetOperationsByAccount", mock.Anything, mock.Anything, mock.Anything, "G1234").Return([]common.Operation{}, errors.New("not good"))

lh := services.LightHorizon{
Operations: mockOperationService,
Transactions: mockTransactionService,
}

handler := NewOpsByAccountHandler(lh)
handler(recorder, request)

resp := recorder.Result()
assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)

raw, err := ioutil.ReadAll(resp.Body)
assert.NoError(t, err)

var problem problem.P
err = json.Unmarshal(raw, &problem)
assert.NoError(t, err)
assert.Equal(t, "Internal Server Error", problem.Title)
}
3 changes: 2 additions & 1 deletion exp/lighthorizon/actions/apidocs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package actions

import (
supportProblem "github.com/stellar/go/support/render/problem"
"net/http"
)

Expand All @@ -10,7 +11,7 @@ func ApiDocs() func(http.ResponseWriter, *http.Request) {
r.URL.Host = "localhost:8080"

if r.Method != "GET" {
sendErrorResponse(w, http.StatusMethodNotAllowed, "")
sendErrorResponse(r.Context(), w, supportProblem.BadRequest)
return
}

Expand Down
33 changes: 33 additions & 0 deletions exp/lighthorizon/actions/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package actions
Copy link
Contributor

Choose a reason for hiding this comment

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

file naming: maybe test_helpers.go? since there are no tests here. I expected to see tests for helpers heh. tiny nitpick though so feel free to ignore.


import (
"context"
"net/http"
"net/url"
"testing"

"github.com/go-chi/chi"
)

func makeRequest(
t *testing.T,
queryParams map[string]string,
routeParams map[string]string,
) *http.Request {
request, err := http.NewRequest("GET", "/", nil)
if err != nil {
t.Fatalf("unexpected error %v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

might as well use require here for brevity

Suggested change
request, err := http.NewRequest("GET", "/", nil)
if err != nil {
t.Fatalf("unexpected error %v", err)
}
request, err := http.NewRequest("GET", "/", nil)
require.NoError(t, err)

query := url.Values{}
for key, value := range queryParams {
query.Set(key, value)
}
request.URL.RawQuery = query.Encode()

chiRouteContext := chi.NewRouteContext()
for key, value := range routeParams {
chiRouteContext.URLParams.Add(key, value)
}
ctx := context.WithValue(context.Background(), chi.RouteCtxKey, chiRouteContext)
return request.WithContext(ctx)
}
Loading