Take `App` struct out from actions and refactor handlers

[howardtw]

We don't have to embed App struct in actions. Instead, we can make App a pointer receiver of the actual function. App actually shouldn't be a part of an Action.

For instance:

func (a *App) getOperationsForAccount(...) (AccountOperation, error) {
  logic for getting operations goes here
}

Then we can do something like this:

m.Handle("/accounts/{account_id}/operations", jsonHandler(a.getOperationsForAccount))

This issue is relatively a big one, and I'm thinking about how to break this down.

[bartekn]

Can you elaborate on the contents of getOperationsForAccount and similar functions? Are you going to move actions code there?

The main thing that needs to be solved to close #869 and #868 is fixing circular dependency. If we move actions to a separate package we'll have:

1. App in main package requiring actions package to assign handlers to the web router.
2. Actions in actions package that require App from time to time, ex. to get config, db handle etc.

Now, the easiest way to solve this is to create an interface that defines getters for everything that a single action may need and make sure that App implements it. However, we also have actions' tests that depend on App and I'm not sure how much time it would take to rewrite them (and if simply introducing an interface would be enough).

I agree that design around actions is not perfect but I don't think it slows down a development process. So maybe it's not something we should care about much right now? I can vote for proceeding on this issue but a) I'd like to know upfront how packages structure will look like and how the pieces will work together and b) we are sure there are no other issues that are more urgent right now.

Thoughts? CC: @tomquisel @ire-and-curses ?

[howardtw]

Can you elaborate on the contents of getOperationsForAccount and similar functions? Are you going to move actions code there?

getOperationsForAccount itself will not be in the actions package. It will remain in the horizon package, which is in horizon/internal. ( I also find it a little bit odd that the package name is not the same as the directory name. I'll probably address it altogether. It usually only happens to cmd directory where we use the main package to spin up the application process.)

The structure I'm thinking will be something like: In internal/actions.go

func (a *APP) getOperationsForAccount(ctx context.Context, q pageQuery) (interface{}, string, error) {
	ops, cursor, err := a.actionIndexer.Operations(ctx, q.Filter, q.FilterParams, q.Cursor, limit)
	return ops, cursor, err
}

That actionIndexer will be defined in the actions package (alternatively we can called it query package to be specific), somewhere like internal/actions/index.go, and we can put the main logic for getting operations in internal/actions/operations.go

App in main package requiring actions package to assign handlers to the web router.

That's exactly why we don't want App to be a part of the actions(IMO). App is like an api instance, and requests can query actions through our api. Embedding App in all the actions doesn't seem right to me.

Actions in actions package that require App from time to time, ex. to get config, db handle etc.

We can put config and db handle in the actionIndexer I mentioned above.

I agree that design around actions is not perfect but I don't think it slows down a development process.

I'm not sure if I agree. Not having a cleaner code structure introduces a lot of unnecessary/duplicate code, which makes the code base hard to maintain.

[tomquisel]

It sounds like a great call to remove App from the actions. Ideally they'd be decoupled so you could use the actions in a different setting that has a different App-like implementation. That said, it's probably not the most confusing or error-prone corner of the code base, and it seems like it'd take a year or more for the time it takes to do the refactor to be repaid by time savings from the cleaner code.

Let's do background work when we have time to come up with a restructuring plan that we're all excited about, and we can implement it when the time seems right. What do you think? Am I underestimating the cost of the code being in its current form or overestimating the time it'll take to restructure?

[howardtw]

@tomquisel My gut feeling is that it won't take that long (definitely not a year). I plan on having an experimental branch working on a single endpoint to start small, and I'll see what it takes to get there. If everyone likes the change, it can be easily expand to other endpoints. I really don't feel good about the current structure (making an action a pointer receiver of a http handler like func (action SomeAction) Handle(w http.ResponseWriter, r *http.Request)).

Of course, it won't be the only thing I focus on.

[tomquisel]

@howardtw cool, I like the experiment + incremental idea. I'm excited to see what you come up with. And I didn't mean that it would take a year to implement the refactor 😄 . I meant it would probably take a year for the refactor to pay off, that is, for the time saved by it to be equal to the time we spent on it.

[bartekn]

I'd like to see an experiment too. I'm not sure why we need actionIndexer (if dependency on App is removed from actions) so would be great to see a sample code.

Other than that I agree with @tomquisel's cost vs potential gains analysis.

[howardtw]

https://github.com/stellar/go/pull/894 ^ I just hit a milestone here @tomquisel @bartekn cc @tomerweller