Add basic test on POST, JSON, error checks:

It'd be nice if we had POST param unit tests but we don't :(

clients/stellarcore/client.go

@@ -1,11 +1,11 @@
 package stellarcore
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"net/http"
 	"net/url"
 	"path"
@@ -21,7 +21,7 @@ import (
 // Client represents a client that is capable of communicating with a
 // stellar-core server using HTTP
 type Client struct {
-	// HTTP is the client to use when communicating with stellar-core.  If nil,
+	// HTTP is the client to use when communicating with stellar-core. If nil,
 	// http.DefaultClient will be used.
 	HTTP HTTP
 
@@ -36,7 +36,7 @@ type Client struct {
 // in case an error was encountered during either the draining or closing of the
 // stream, that error would be returned.
 func drainReponse(hresp *http.Response, close bool, err *error) (outerror error) {
-	_, err2 := io.Copy(ioutil.Discard, hresp.Body)
+	_, err2 := io.Copy(io.Discard, hresp.Body)
 	if err2 != nil {
 		if err != nil && *err == nil {
 			*err = errors.Wrap(err2, "unable to read excess data from response")
@@ -75,9 +75,8 @@ func (c *Client) Upgrade(ctx context.Context, version int) (err error) {
 	}
 	defer drainReponse(hresp, true, &err) //nolint:errcheck
 
-	if !(hresp.StatusCode >= 200 && hresp.StatusCode < 300) {
-		err = errors.New("http request failed with non-200 status code")
-		return
+	if hresp.StatusCode < 200 || hresp.StatusCode >= 300 {
+		return errors.New("http request failed with non-200 status code")
 	}
 	return nil
 }
@@ -152,9 +151,9 @@ func (c *Client) SetCursor(ctx context.Context, id string, cursor int32) (err er
 	return nil
 }
 
-func (c *Client) GetLedgerEntries(ctx context.Context, ledgerSeq uint32, keys ...xdr.LedgerKey) (*proto.GetLedgerEntriesResponse, error) {
-	var resp *proto.GetLedgerEntriesResponse
-	return resp, c.makeLedgerKeyRequest(ctx, resp, "getledgerentries", ledgerSeq, keys...)
+func (c *Client) GetLedgerEntries(ctx context.Context, ledgerSeq uint32, keys ...xdr.LedgerKey) (proto.GetLedgerEntriesResponse, error) {
+	var resp proto.GetLedgerEntriesResponse
+	return resp, c.makeLedgerKeyRequest(ctx, &resp, "getledgerentry", ledgerSeq, keys...)
 }
 
 //lint:ignore U1000 Ignore unused function until it's supported in Core
@@ -182,8 +181,15 @@ func (c *Client) SubmitTransaction(ctx context.Context, envelope string) (resp *
 	}
 
 	var hresp *http.Response
-	hresp, err = c.getResponse(req)
+	hresp, err = c.http().Do(req)
 	if err != nil {
+		err = errors.Wrap(err, "http request errored")
+		return
+	}
+	defer drainReponse(hresp, true, &err) //nolint:errcheck
+
+	if hresp.StatusCode < 200 || hresp.StatusCode >= 300 {
+		err = errors.New("http request failed with non-200 status code")
 		return
 	}
 
@@ -222,7 +228,6 @@ func (c *Client) WaitForNetworkSync(ctx context.Context) error {
 
 // ManualClose closes a ledger when Core is running in `MANUAL_CLOSE` mode
 func (c *Client) ManualClose(ctx context.Context) (err error) {
-
 	q := url.Values{}
 
 	var req *http.Request
@@ -232,9 +237,17 @@ func (c *Client) ManualClose(ctx context.Context) (err error) {
 		return
 	}
 
-	hresp, err := c.getResponse(req)
+	var hresp *http.Response
+	hresp, err = c.http().Do(req)
 	if err != nil {
-		return errors.Wrap(err, "http request errored")
+		err = errors.Wrap(err, "http request errored")
+		return
+	}
+	defer drainReponse(hresp, true, &err) //nolint:errcheck
+
+	if hresp.StatusCode < 200 || hresp.StatusCode >= 300 {
+		err = errors.New("http request failed with non-200 status code")
+		return
 	}
 
 	// verify there wasn't an exception
@@ -270,31 +283,48 @@ func (c *Client) simpleGet(
 	newPath string,
 	query url.Values,
 ) (*http.Request, error) {
-	q := ""
+	u, err := url.Parse(c.URL)
+	if err != nil {
+		return nil, errors.Wrap(err, "unparseable url")
+	}
+
+	u.Path = path.Join(u.Path, newPath)
 	if query != nil {
-		q = query.Encode()
+		u.RawQuery = query.Encode()
 	}
-	return c.rawGet(ctx, newPath, q)
+	newURL := u.String()
+
+	var req *http.Request
+	req, err = http.NewRequestWithContext(ctx, http.MethodGet, newURL, nil)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to create request")
+	}
+
+	return req, nil
 }
 
-// rawGet returns a new GET request to the connected stellar-core using the
-// provided path and a raw query string to construct the result.
-func (c *Client) rawGet(
+// rawPost returns a new POST request to the connected stellar-core using the
+// provided path and the params values encoded as the request body to construct
+// the result.
+func (c *Client) rawPost(
 	ctx context.Context,
 	newPath string,
-	query string,
+	params string,
 ) (*http.Request, error) {
 	u, err := url.Parse(c.URL)
 	if err != nil {
 		return nil, errors.Wrap(err, "unparseable url")
 	}
 
 	u.Path = path.Join(u.Path, newPath)
-	u.RawQuery = query
 	newURL := u.String()
 
 	var req *http.Request
-	req, err = http.NewRequestWithContext(ctx, http.MethodGet, newURL, nil)
+	req, err = http.NewRequestWithContext(
+		ctx,
+		http.MethodPost,
+		newURL,
+		bytes.NewBuffer([]byte(params)))
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to create request")
 	}
@@ -304,13 +334,14 @@ func (c *Client) rawGet(
 
 // makeLedgerKeyRequest is a generic method to perform a request in the form
 // `key=...&key=...&ledgerSeq=...` which is useful because three Stellar Core
-// endpoints all use this request format.
+// endpoints all use this request format. Be sure to pass `target` by reference.
 func (c *Client) makeLedgerKeyRequest(
 	ctx context.Context,
 	target interface{},
 	endpoint string,
 	ledgerSeq uint32,
-	keys ...xdr.LedgerKey) error {
+	keys ...xdr.LedgerKey,
+) error {
 	q, err := buildMultiKeyRequest(keys...)
 	if err != nil {
 		return err
@@ -319,35 +350,24 @@ func (c *Client) makeLedgerKeyRequest(
 	}
 
 	var req *http.Request
-	req, err = c.rawGet(ctx, endpoint, q)
-	if err != nil {
-		return err
-	}
-
-	hresp, err := c.getResponse(req)
+	req, err = c.rawPost(ctx, endpoint, q)
 	if err != nil {
 		return err
 	}
 
-	// returns nil if the error is nil
-	return errors.Wrap(json.NewDecoder(hresp.Body).Decode(&target), "json decode failed")
-}
-
-// getResponse is an abstraction method to perform a request and check for a
-// non-2xx status code, returning the whole response
-func (c *Client) getResponse(req *http.Request) (*http.Response, error) {
 	var hresp *http.Response
-	hresp, err := c.http().Do(req)
+	hresp, err = c.http().Do(req)
 	if err != nil {
-		return hresp, errors.Wrap(err, "http request errored")
+		return errors.Wrap(err, "http request errored")
 	}
 	defer drainReponse(hresp, true, &err) //nolint:errcheck
 
 	if hresp.StatusCode < 200 || hresp.StatusCode >= 300 {
-		return hresp, fmt.Errorf("http request failed with non-200 status code (%d)", hresp.StatusCode)
+		return fmt.Errorf("http request failed with non-200 status code (%d)", hresp.StatusCode)
 	}
 
-	return hresp, nil
+	// wrap returns nil if the inner error is nil
+	return errors.Wrap(json.NewDecoder(hresp.Body).Decode(&target), "json decode failed")
 }
 
 // buildMultiKeyRequest is a workaround helper because, unfortunately,
@@ -356,17 +376,22 @@ func (c *Client) getResponse(req *http.Request) (*http.Response, error) {
 func buildMultiKeyRequest(keys ...xdr.LedgerKey) (string, error) {
 	// The average ledger key length, according to a simple
 	//
-	// SELECT AVG(LENGTH(HEX(key))) / 2 FROM ledger_entries;
+	//     SELECT AVG(LENGTH(HEX(key))) / 2 FROM ledger_entries;
 	//
 	// on a pubnet RPC instance is ~57.6. We can use this to preallocate a
 	// string buffer for performance.
+	//
+	// We know that these endpoints will almost exclusively be used for
+	// ContractData and the like, so we could optimize the buffer further for
+	// that, but that data is harder to query since it'd involve parsing the XDR
+	// from the DB to check the key type.
 	q := strings.Builder{}
 	q.Grow(50 * len(keys))
 
 	for _, key := range keys {
 		keyB64, err := key.MarshalBinaryBase64()
 		if err != nil {
-			return q.String(), errors.Wrapf(err, "failed to encode LedgerKey")
+			return q.String(), errors.Wrap(err, "failed to encode LedgerKey")
 		}
 		q.WriteString("key=" + url.QueryEscape(keyB64) + "&")
 	}

clients/stellarcore/client_test.go

@@ -6,9 +6,12 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
+	"github.com/stellar/go/keypair"
 	proto "github.com/stellar/go/protocols/stellarcore"
 	"github.com/stellar/go/support/http/httptest"
+	"github.com/stellar/go/xdr"
 )
 
 func TestSubmitTransaction(t *testing.T) {
@@ -75,3 +78,41 @@ func TestManualClose_NotAvailable(t *testing.T) {
 
 	assert.EqualError(t, err, "exception in response: Set MANUAL_CLOSE=true")
 }
+
+func TestGetLedgerEntries(t *testing.T) {
+	hmock := httptest.NewClient()
+	c := &Client{HTTP: hmock, URL: "http://localhost:11626"}
+
+	// build a fake response body
+	mockResp := proto.GetLedgerEntriesResponse{
+		Ledger: 1215, // checkpoint align on expected request
+		Entries: []proto.LedgerEntryResponse{
+			{
+				Entry: "pretend this is XDR lol",
+				State: proto.DeadState,
+			},
+			{
+				Entry: "pretend this is another XDR lol",
+				State: proto.ArchivedStateNoProof,
+			},
+		},
+	}
+
+	// happy path - fetch an entry
+	hmock.On("POST", "http://localhost:11626/getledgerentry").
+		ReturnJSON(http.StatusOK, &mockResp)
+
+	var key xdr.LedgerKey
+	acc, err := xdr.AddressToAccountId(keypair.MustRandom().Address())
+	require.NoError(t, err)
+	key.SetAccount(acc)
+
+	resp, err := c.GetLedgerEntries(context.Background(), 1234, key)
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+
+	require.EqualValues(t, 1215, resp.Ledger)
+	require.Len(t, resp.Entries, 2)
+	require.Equal(t, resp.Entries[0].State, proto.DeadState)
+	require.Equal(t, resp.Entries[1].State, proto.ArchivedStateNoProof)
+}

services/horizon/internal/ingest/fsm.go

@@ -765,11 +765,6 @@ func (v verifyRangeState) run(s *system) (transition, error) {
 			"ledger":   true,
 			"commit":   true,
 		}).Info("Processing ledger")
-
-		if sequence == 52885867 || sequence == 52885868 {
-			log.Error("We are in the problem zone.")
-		}
-
 		startTime := time.Now()
 
 		if err = s.historyQ.Begin(s.ctx); err != nil {