services/horizon/internal/actions: Remove inappropriate use of context in ingestion endpoints

[tamirms]

PR Checklist

PR Structure

• This PR has reasonably narrow scope (if not, break it down into smaller PRs).
• This PR avoids mixing refactoring changes with feature changes (split into two PRs
  otherwise).
• This PR's title starts with name of package that is most changed in the PR, ex.
  services/friendbot, or all or doc if the changes are broad or impact many
  packages.

Thoroughness

• This PR adds tests for the most critical parts of the new functionality or fixes.
• I've updated any docs (developer docs, .md
  files, etc... affected by this change). Take a look in the docs folder for a given service,
  like this one.

Release planning

• I've updated the relevant CHANGELOG (here for Horizon) if
  needed with deprecations, added features, breaking changes, and DB schema changes.
• I've decided if this PR requires a new major/minor version according to
  semver, or if it's mainly a patch change. The PR is targeted at the next
  release branch if it's not a patch change.

Summary

Commit 9161997 used a request context to ensure
that all queries to the horizon database from ingestion endpoints were wrapped
in a repeatable read transaction.

This commit refactors the ingestion endpoints to acheive the same effect without
using a context to store a request scoped database session.

See #1847 (comment) for more background on the motivation for this change.

Known limitations & issues

There is a some extra boilerplate compared to passing the db session via the request context

[bartekn]

I quickly checked the PR (will do a full review later if @tamirms and @leighmcculloch feel strongly about it) and I think the use of context in 9161997 is actually correct. The DB connection/transaction is request scoped after 9161997 and I think this is exactly the use case of context. If we ever need to make an extra query in another middleware in the future we'll have to do refactoring again. And as you noticed it requires some extra factories to build actions and we wanted to avoid boilerplate code in the new design.

It would be a different story if we were passing a connection pool that's shared by all the requests (like we did before 9161997).

[tamirms]

@bartekn I understand your point of view. I'm not certain that this PR is significantly better than the old code given the added boilerplate.

Another option would be to call historyQ, err := repeatableReadSession() and defer historyQ.Rollback() in each of the handlers. This solution still has some boilerplate.

I can't think of a way to completely eliminate the boilerplate code without relying on context values

[leighmcculloch]

There is a some extra boilerplate

@tamirms On first glance for someone who hasn't read this code before the changes definitely appeared quite complex. But after looking through the change this is really just moving existing logic around. I don't see a lot of new logic being added, what's the boilerplate you're referring to?

I think this does a good job of making it clear that the handler is dependent on the history.Q since it is now included in the handler's struct and its constructor. I have one question inline, but it's more thinking out loud and asking a question than suggesting a change.

---

The DB connection/transaction is request scoped after 9161997 and I think this is exactly the use case of context

@bartekn The dependency (DB connection) is limited to this request, but not everything limited to this request belongs in the context. It's still a required dependency being injected. The code will break without it and we should make it explicit that it is needed either by including it in a function signature, type constructor, or as a field on the handler's type. Putting it in the context obscures it from view and it's a lot harder to keep track of what the dependencies of our code are if they aren't explicit.

There are a few good blog posts on this, my favourite probably being this one:
https://peter.bourgon.org/blog/2016/07/11/context.html

But also, this isn't a hard Go rule we're breaking. We can use context in any way we want to, it's a tradeoff. Is the convenience of passing the dependency around using context worth the tradeoff of reducing the clarity of the dependencies in our code? I don't think so, but you might disagree.

---

I think this looks good, but I don't feel qualified to ✅this PR alone. There's a lot going on here that I don't have much context. If the PR is to be merged @bartekn should 👀too.

[bartekn]

@tamirms On first glance for someone who hasn't read this code before the changes definitely appeared quite complex. But after looking through the change this is really just moving existing logic around. I don't see a lot of new logic being added, what's the boilerplate you're referring to?

My understanding is that for each action we need an extra function that creates a transaction. The reason we decided to rewrite actions is to remove a need for duplicate code so we try to avoid it now if possible.

@bartekn The dependency (DB connection) is limited to this request, but not everything limited to this request belongs in the context. It's still a required dependency being injected. The code will break without it and we should make it explicit that it is needed either by including it in a function signature, type constructor, or as a field on the handler's type. Putting it in the context obscures it from view and it's a lot harder to keep track of what the dependencies of our code are if they aren't explicit.

There are a few good blog posts on this, my favourite probably being this one:
https://peter.bourgon.org/blog/2016/07/11/context.html

But also, this isn't a hard Go rule we're breaking. We can use context in any way we want to, it's a tradeoff. Is the convenience of passing the dependency around using context worth the tradeoff of reducing the clarity of the dependencies in our code? I don't think so, but you might disagree.

I understand the reasoning here and I'm for dependency injection, I advocated several times to use automatic dependency injection (something like facebookgo/inject) for all services in Horizon. However, I can see several problems with using injection here:

1. Ideally, DB transaction should span across middlewares and the handler so that all DB queries that happen along the way are using the same DB snapshot. It works here but if we move middleware that starts a transaction to the parent package and use the standard middleware API (http.Handler -> http.Handler), the only way to pass a transaction to the next middleware is by using context. In the article shared, they are passing the shared DB connection, not a per request transaction.
2. If we're injecting history.Q we should also inject logger but it requires some larger refactoring. We create logger using support/log.Ctx method in many places, if we remove this we'll lose information about requests where logger is not passed directly.
3. Again, boilerplate code. Having to remember to add some extra code per action is prone to errors. We could use go generate but it's also one thing we wanted to avoid in the new design (see internal/main_generated.go file).

Is it fine for you if we think about it more and decide what to do? I think we should really try to avoid another rewrite of actions package in the near future.

[abuiles]

@tamirms shall we use the same convention as we were using before so it's clear that we are creating a handler? so instead of NewAccounts we use NewAccountsHandler -- it's more explicit about what we are getting.

[leighmcculloch]

I advocated several times to use automatic dependency injection (something like facebookgo/inject)

If we are considering automatic dependency injection, we should evaluate https://github.com/google/wire which does compile time dependency injection instead of injection using reflection.

If we're injecting history.Q we should also inject logger

I don't think this is a fact. log.Ctx() is resilient to context not containing the logger. If something is not a required dependency, and not core to what the code is doing, then I think we can put it in the context if it relates specifically to that request. The code obscurity arises when our required dependencies are hidden and implicit, rather than explicit and obvious.