From f6e663963fd5c13e2b0a3ee4a6a215e2186ca81c Mon Sep 17 00:00:00 2001 From: Adolfo Builes Date: Fri, 22 Nov 2019 10:25:54 -0500 Subject: [PATCH 1/5] Add regression for #1965. --- .../horizon/internal/actions/helpers_test.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/services/horizon/internal/actions/helpers_test.go b/services/horizon/internal/actions/helpers_test.go index d9beffde95..e173b393c6 100644 --- a/services/horizon/internal/actions/helpers_test.go +++ b/services/horizon/internal/actions/helpers_test.go @@ -426,7 +426,7 @@ func TestPath(t *testing.T) { tt.Assert.Equal("/foo-bar/blah", action.Path()) } -func TestGetURLParam(t *testing.T) { +func TestBaseGetURLParam(t *testing.T) { tt := test.Start(t) defer tt.Finish() action := makeTestAction() @@ -446,6 +446,23 @@ func TestGetURLParam(t *testing.T) { tt.Assert.Equal(false, ok) } +func TestGetURLParam(t *testing.T) { + tt := test.Start(t) + defer tt.Finish() + r := makeAction("/accounts/{account_id}/operations?limit=100", nil).R + + // simulates a request where the named param is not passed. + // Regression for https://github.com/stellar/go/issues/1965 + rctx := chi.RouteContext(r.Context()) + rctx.URLParams.Keys = []string{ + "account_id", + } + + val, ok := GetURLParam(r, "account_id") + tt.Assert.Empty(val) + tt.Assert.False(ok) +} + func TestGetAssets(t *testing.T) { rctx := chi.NewRouteContext() From 6a3bf294d997b6fcd044f41abfe9017f3f6dbecd Mon Sep 17 00:00:00 2001 From: Adolfo Builes Date: Fri, 22 Nov 2019 10:30:46 -0500 Subject: [PATCH 2/5] Return early if there are no values for URLParams. --- services/horizon/internal/actions/helpers.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/services/horizon/internal/actions/helpers.go b/services/horizon/internal/actions/helpers.go index 295ec506ea..38e9267008 100644 --- a/services/horizon/internal/actions/helpers.go +++ b/services/horizon/internal/actions/helpers.go @@ -607,6 +607,13 @@ func (base *Base) GetTimeMillis(name string) (timeMillis time.Millis) { // Ref: https://github.com/go-chi/chi/blob/d132b31857e5922a2cc7963f4fcfd8f46b3f2e97/context.go#L69 func GetURLParam(r *http.Request, key string) (string, bool) { rctx := chi.RouteContext(r.Context()) + + // Return immediately if there are no values for the URLParams. + // This can happen when a named param is not specified. + if len(rctx.URLParams.Values) == 0 { + return "", false + } + for k := len(rctx.URLParams.Keys) - 1; k >= 0; k-- { if rctx.URLParams.Keys[k] == key { return rctx.URLParams.Values[k], true From 4f2fa644050b8b9c278558964e335e34e984c4cc Mon Sep 17 00:00:00 2001 From: Adolfo Builes Date: Fri, 22 Nov 2019 10:46:07 -0500 Subject: [PATCH 3/5] Add accountID validation on GetAccountOffersHandler --- services/horizon/internal/actions/offer.go | 4 ++-- services/horizon/internal/actions/offer_test.go | 11 +++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/services/horizon/internal/actions/offer.go b/services/horizon/internal/actions/offer.go index 7b30efce58..62b3d73092 100644 --- a/services/horizon/internal/actions/offer.go +++ b/services/horizon/internal/actions/offer.go @@ -121,14 +121,14 @@ func (handler GetAccountOffersHandler) parseOffersQuery(r *http.Request) (histor return history.OffersQuery{}, err } - seller, err := GetString(r, "account_id") + seller, err := GetAccountID(r, "account_id") if err != nil { return history.OffersQuery{}, err } query := history.OffersQuery{ PageQuery: pq, - SellerID: seller, + SellerID: seller.Address(), } return query, nil diff --git a/services/horizon/internal/actions/offer_test.go b/services/horizon/internal/actions/offer_test.go index 533a217ea9..dfbac000f6 100644 --- a/services/horizon/internal/actions/offer_test.go +++ b/services/horizon/internal/actions/offer_test.go @@ -393,6 +393,17 @@ func TestGetAccountOffersHandler(t *testing.T) { for _, offer := range offers { tt.Assert.Equal(issuer.Address(), offer.Seller) } + + records, err = handler.GetResourcePage( + httptest.NewRecorder(), + makeRequest( + t, + map[string]string{}, + map[string]string{}, + q.Session, + ), + ) + tt.Assert.Error(err) } func pageableToOffers(t *testing.T, page []hal.Pageable) []horizon.Offer { From 6de127c774ccef32e2290e260c09f66110ce59db Mon Sep 17 00:00:00 2001 From: Adolfo Builes Date: Fri, 22 Nov 2019 11:18:05 -0500 Subject: [PATCH 4/5] Use query structs for GetAccountOffersHandler. --- services/horizon/internal/actions/offer.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/services/horizon/internal/actions/offer.go b/services/horizon/internal/actions/offer.go index 62b3d73092..748209cc7a 100644 --- a/services/horizon/internal/actions/offer.go +++ b/services/horizon/internal/actions/offer.go @@ -110,6 +110,11 @@ func (handler GetOffersHandler) GetResourcePage( return offers, nil } +// AccountOffersQuery query struct for offers end-point +type AccountOffersQuery struct { + AccountID string `schema:"account_id" valid:"accountID,required"` +} + // GetAccountOffersHandler is the action handler for the // `/accounts/{account_id}/offers` endpoint when using experimental ingestion. type GetAccountOffersHandler struct { @@ -121,14 +126,15 @@ func (handler GetAccountOffersHandler) parseOffersQuery(r *http.Request) (histor return history.OffersQuery{}, err } - seller, err := GetAccountID(r, "account_id") + qp := AccountOffersQuery{} + err = GetParams(&qp, r) if err != nil { return history.OffersQuery{}, err } query := history.OffersQuery{ PageQuery: pq, - SellerID: seller.Address(), + SellerID: qp.AccountID, } return query, nil From 5e6119b26f4345e19a234dd197f1a8f8d89cb533 Mon Sep 17 00:00:00 2001 From: Adolfo Builes Date: Fri, 22 Nov 2019 11:41:14 -0500 Subject: [PATCH 5/5] Use GetURLParam in GetParams. --- services/horizon/internal/actions/helpers.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/services/horizon/internal/actions/helpers.go b/services/horizon/internal/actions/helpers.go index 38e9267008..c708b6ff35 100644 --- a/services/horizon/internal/actions/helpers.go +++ b/services/horizon/internal/actions/helpers.go @@ -608,9 +608,10 @@ func (base *Base) GetTimeMillis(name string) (timeMillis time.Millis) { func GetURLParam(r *http.Request, key string) (string, bool) { rctx := chi.RouteContext(r.Context()) - // Return immediately if there are no values for the URLParams. + // Return immediately if keys does not match Values // This can happen when a named param is not specified. - if len(rctx.URLParams.Values) == 0 { + // This is a bug in chi: https://github.com/go-chi/chi/issues/426 + if len(rctx.URLParams.Keys) != len(rctx.URLParams.Values) { return "", false } @@ -697,7 +698,8 @@ func GetParams(dst interface{}, r *http.Request) error { ) } - query.Set(key, rctx.URLParam(key)) + param, _ := GetURLParam(r, key) + query.Set(key, param) } }