Decide on Horizon's actions package

[bartekn]

@ire-and-curses brought up a good point that we should discuss the way forward for the actions/controllers in Horizon. We currently use three different solutions:

• Old actions structs (actions_*.go files).
• actions package introduced in #894.
• New approach taken by @tamirms and @abuiles in #1597.

I think the main idea behind this issue is to discuss pros and cons of different approaches and decide on a solution that works best for us and is then implemented consistently across codebase.

In #894 I outlined a new solution that would provide a separation of concerns:

Let's start with the requirements for the new design:

• Separation of concerns: App should initilize other components and make them work together, it shouldn't know how http server works, how actions work etc.
• Move actions to a separate package.
• Easier to test. If we want to test action, we shouldn't have to create App instance, only mock object used directly by actions (ex. db session objects).
• All methods return errors, no internal Err fields.
• And of course: no App in context :).

Here's a rough idea what we can do:

• http package where http server is created, router is initialized, middlewares added etc. Exposes a public function used to add routes -> handlers mapping (routes and handlers will be provided by App). We move code responsible for action rendering (basically actions.Base.Execute) there too, including interfaces. (Can be probably merged with existing httpx package.)
• actions package with actions code. I think that in the first iteration we should probably just remove:
  • Err: all internal methods return errors,
  • appCtx: http package should use Prepare function on actions to provide all necessary objects, like DB handler etc.
• App is used to get actions and pass them to http (or maybe, since actions shouldn't be needed by App anywhere, http should just import actions package itself?).

With something like this:

• We make App smaller - all actions and web related things are moved to corresponding packages.
• When http provides all necessary objects to actions there's no need to keep App in the request context.
• We can unit test actions by providing it with mock objects (ex. mock DB object that returns some test rows).

Please check #894 for my comments expressing concerns connected to the new actions package if you're interested (https://github.com/stellar/go/pull/894#discussion_r267111991 https://github.com/stellar/go/pull/894#discussion_r260852279 https://github.com/stellar/go/pull/894#discussion_r260901508 https://github.com/stellar/go/pull/894#discussion_r267119807).

Would be great to hear other ideas or discuss my solution above (especially it's cons).

TODO

• [ ] https://github.com/stellar/go/issues/1748

[tamirms]

The are two problems with the old actions structs

(1) it requires generated code (2) the handlers are stateful and are written in an unidiomatic way. Specifically, I'm referring to how errors are stored in action.Err

The problems with the approach introduced in https://github.com/stellar/go/pull/894 are: (1) there is frequent use of the interface {} type https://github.com/stellar/go/blob/e7f8ae273d436f7dee1c233ab48b2fe5d10a1f93/services/horizon/internal/handler.go#L201

https://github.com/stellar/go/blob/81d40f04ebe8fc8cea2de4f32aff7ef9ed1eb1bb/services/horizon/internal/action.go#L202

https://github.com/stellar/go/blob/e7f8ae273d436f7dee1c233ab48b2fe5d10a1f93/support/render/hal/handler.go#L10 (2) there is unnecessary use of reflection which makes the code difficult to follow https://github.com/stellar/go/blob/e7f8ae273d436f7dee1c233ab48b2fe5d10a1f93/support/render/httpjson/handler.go#L51-L116 (3) it is difficult to digest the http handlers because you need to look at several files ( services/horizon/internal/handler.go , services/horizon/internal/action.go , services/horizon/internal/actions/transaction.go to get the full picture of the execution flow from the moment a request is received to when a response is generated.

When we implemented the get offers by id endpoint we tried to take a simple approach:

• Implement a http.HandlerFunc or http.Handler which handles requests for the given endpoint.
• try to keep the handler implementation within a single file so that it's easy to follow the control flow logic
• create a middleware if there is control flow logic which could be shared by many different handlers. but make sure the middleware is well understood. If creating a middleware makes the code less understood then perhaps it's better to not use it

the end result looks like:

https://github.com/stellar/go/blob/ae5be009ef16c4d72872462163a36fba503774c9/services/horizon/internal/action.go#L226-L256

If we want to avoid relying on the App context, I'm totally fine with that. We can easily rewrite

func getOfferResource(w http.ResponseWriter, r *http.Request) {
  ...
}

as the following:

type GetOfferHandler struct {
   historyQ *history.Q
}
func (h GetOfferHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
...
}

Or, we could go further and use an interface to represent querying offers and ledgers:

type GetOfferHandler struct {
   offerStore   OfferStore
   ledgerStore  LedgerStore
}
func (h GetOfferHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
...
}

[abuiles]

Overall I like where approach 2 and 3 are going, specially since it makes things more explicit and easier to follow.

When I started looking at Horizon, it was very hard to put the pieces together and on top of that there was the extra overhead the generated code.

As @bartek mentioned in https://github.com/stellar/go/pull/894#discussion_r260852279 -- this abstraction adds more complexity than necessary without giving us much - we tried to take a different path by moving the query params parsing to the action.

https://github.com/stellar/go/blob/3b1bae68fa1ee67d604f182dabac6c8bae59f211/services/horizon/internal/action.go#L228-L232

However, this works fine for this scenario, but for more complex end-points we'll end up with a bunch of boilerplate code. I like the abstraction we use to have in approach 1, where we could define the shape of the query params through a struct and then have a function that could take care of extracting that out from the request. We can try to find something that simplifies this process without adding too much magic.

https://github.com/stellar/go/blob/3b1bae68fa1ee67d604f182dabac6c8bae59f211/services/horizon/internal/actions_ledger.go#L28

Finally, I like that in our final approach, it's easy to tell what's happening through the lifecycle of a given request. By looking at the following code, I can easily tell what's going to happen vs the old way, where we had a Struct.Handle which would do all kind of crazy magic. I'm not very familiar with other go frameworks, but it would be a good homework to see how they do query params and adapt what we can.

https://github.com/stellar/go/blob/3b1bae68fa1ee67d604f182dabac6c8bae59f211/services/horizon/internal/web.go#L182-L184

This might eventually be moved out to the top-level as most of our functions might have similar requirements (be HAL/JSON/Streamable), etc.

[bartekn]

Sorry for a huge delay commenting on this issue.

I like your proposals, when it comes function vs struct, so basically this:

func getOfferResource(w http.ResponseWriter, r *http.Request) {
  ...
}

vs

type GetOfferHandler struct {
   offerStore   OfferStore
   ledgerStore  LedgerStore
}
func (h GetOfferHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
...
}

I think the later is better because I imagine there will be many helper function connected to a given handler and we will need to make helper function names complicated (ex. GetOfferHandler.getParams vs getOfferResourceGetParams). This also makes it easier to test and init as you can use something like facebook/inject to inject services (or mocks).

One thing I'm thinking about is whether handlers should be stateful? There's one use case where this is helpful: streaming. The current code relies on stream.SentCount() vs keeping the last object paging token:

func (action *OperationIndexAction) SSE(stream *sse.Stream) error {
    action.Setup(
        action.EnsureHistoryFreshness,
        action.loadParams,
        action.ValidateCursorWithinHistory,
    )
    action.Do(
        action.loadRecords,
        action.loadLedgers,
        func() {
            stream.SetLimit(int(action.PagingParams.Limit))
            operationRecords := action.OperationRecords[stream.SentCount():]
//                                                                  ^

I think we should also automate as much as we can to remove boilerplate code. I want to expand @abuiles params suggestion. Each action now has loadParams method that can be automated using query structs. Example:

  type OperationPageRequest struct {
    Cursor int64 `name:"cursor", validate:"cursor"`
    Limit int `name:"limit", validate:"limit"`
    Order Order `name:"order", validate:"order"`
    IncludeFailed bool `name:"include_failed"`
  }

This could basically remove a need for loadParams methods and will provide params, validation and error rendering for free. There's already a package like this here: https://godoc.org/github.com/google/go-querystring/query but maybe we want to expand it with automatic validation etc.

Another automation/code simplicity idea is to move route and HTTP method to actual action. With this change we have all data related to the action in a single place. This has several advantages: possibility of router init automation, all properties of an action in a single place - adding new action requires creating a single file only (with no changes elsewhere, except creating an object in init stage). Unfortunately Golang does not support annotations that could work great here. Instead we could create an interface that must be implemented by the action:

type Action interface {
    Route() string // ex. `/operation/{id}`
    Method() string // GET, POST, ...
    RequiresNewIngestion() bool // if true add a middleware
    ServeHTTP(w http.ResponseWriter, r *http.Request)
    Stream() // TBD
}

This would allow use to remove mustInstallActions function completely. I imagine that both solutions (query structs and automate router init) would fit well in a new package (maybe even in /support). Then App struct would have no idea how HTTP server work, it would just start it.

Let me propose the starting plan to discuss:

• [x] Prerequisite: discuss if streaming would be OK in the new design.
• [ ] All new endpoints are made using the new method and added to actions package.
• [ ] Do some experiments connected to automation (query structs, automate router init).
• [ ] Migrate all actions to the new method.
• [ ] Extra point: what do you think about using dependency injection like facebook/inject? It doesn't sound actions related but it is, it would make building action structs (but also init all Horizon services) much easier.

[abuiles]

@tomquisel we are at the point now were we want to experiment with this on streaming, specifically by moving the end-point /accounts/{account_id}/offers to use ingestion and this new action pattern.

@bartekn suggested we discuss with you if/how do you use offers for account streaming in stellarx.

From Bartek:

The current streaming implementation doesn't make too much sense to me. it gives you all the new offers but doesn't give you updates to existing offers. I'm wondering if there's any use case for the current for the current implementation.

[tomquisel]

Hey @abuiles! StellarX streams effects and trades for a few different reasons, but it handles open offers for an account simply by polling. I think we realized that the existing streaming functionality didn't do the trick for us.

Starting to stream existing offers that change seems like a great idea, although I'd worry about backwards compatibility with whoever is using that endpoint. Maybe you could look at Kibana for the public Horizon to see how much it's used?

[bartekn]

Created a sample file with explanations here: https://github.com/stellar/go/pull/1699#discussion_r322770861

[abuiles]

Based on internal discussion, we'll move forward with the following approach taking some ideas from what @bartekn suggested here and here:

• [x] 1. Create a pageAction interface close to web.go (not in actions).

  type pageAction interface {
  GetObject(r *http.Request) (hal.Pageable, error)
  }

• [x] 2. Refactor actions/offers to implement the pageAction interface.
• [x] 3. Move https://github.com/stellar/go/blob/e6d7ddf198e3cde52085c9ca2892b6b5eb58ac24/services/horizon/internal/actions/offers.go#L136-L139 closer to web and use under the hood the response from the interface defined in 1.
• [ ] 4. ~We'll keep outside of the scope of this PR the methods Method(), Route(), RequiresNewIngestion() - we can experiment with this and root autogeneration in a different PR.~
• [ ] 5. ~We can add objectAction in a different PR since for this PR we won't use it, however, we can use it for /offers/{id} which already uses most of the patterns we have been experimenting with.~

cc @ire-and-curses

[bartekn]

I wanted to share an example where decoupling http connected code from App could really help in tests. Copying my first message in this issue:

• http package where http server is created, router is initialized, middlewares added etc. Exposes a public function used to add routes -> handlers mapping (routes and handlers will be provided by App). We move code responsible for action rendering (basically actions.Base.Execute) there too, including interfaces. (Can be probably merged with existing httpx package.)

In #1691 I wanted to create a full http request in tests to test one of the middlewares. Instead of simply creating a fully configured router and check the config field needed in tests I had to do something insanely unreadable:

    rh := StartHTTPTest(t, "paths")
    defer rh.Finish()

    rh.App.config.EnableExperimentalIngestion = true
    rh.App.web.router = chi.NewRouter()
    orderBookGraph := orderbook.NewOrderBookGraph()
    rh.App.web.mustInstallMiddlewares(rh.App, time.Minute)
    rh.App.web.mustInstallActions(
        rh.App.config,
        simplepath.NewInMemoryFinder(orderBookGraph),
    )
    rh.RH = test.NewRequestHelper(rh.App.web.router)

    err := rh.App.historyQ.UpdateExpStateInvalid(true)
    rh.Assert.NoError(err)

This piece of code basically recreates router and adds it to the app.

[abuiles]

Ideas 1 to 3 from https://github.com/stellar/go/issues/1606#issuecomment-530073484 are implemented in PR #1699

We defined the actionPage in handlers:

https://github.com/stellar/go/blob/d30b542f485f6d4352469e07aec2dbd81669329a/services/horizon/internal/handler.go#L406-L419

And examples of actions implementing the interface are

https://github.com/stellar/go/blob/d30b542f485f6d4352469e07aec2dbd81669329a/services/horizon/internal/actions/offer.go#L15-L21

And

https://github.com/stellar/go/blob/d30b542f485f6d4352469e07aec2dbd81669329a/services/horizon/internal/actions/offer.go#L58-L62

Adding a new route using this new method looks like this https://github.com/stellar/go/blob/d30b542f485f6d4352469e07aec2dbd81669329a/services/horizon/internal/web.go#L186-L203

The route linked above supports streaming, a route without streaming will use the restPageHandler

https://github.com/stellar/go/blob/d30b542f485f6d4352469e07aec2dbd81669329a/services/horizon/internal/web.go#L233-L239

Once we land #1699, we can work on query structs and handle scenarios like https://github.com/stellar/go/pull/1699/files#r323460947

We also need to fix SSE pagination when using cursor=now. It doesn't work at the moment, both with and without ingestion.

TODO

• [ ] Use query structs
• [ ] Fix SSE pagination for cursor=now

[bartekn]

To add to @abuiles TODO list, here are other issues to discuss connected to actions:

• Create a horizon/web or horizon/http package that will contain all router and rendering related stuff. This will help with testing (you can test full HTTP request without having to init full App as it works right now), remove a lot of code duplication and will be more clear.
• Move LedgerSource added in #1699 to ledger package.
• Should action unit tests use a DB or Q mock? Pros and cons.

[abuiles]

Added https://github.com/stellar/go/issues/1748 to keep track of the query params work