Problems with PA-12308

dont-trust-a-context.md

Hey @here

So I've taken on board some useful guidance by @rory and @eric concerning the "right" way to generate a context.Context instance for use by Mothership guts.

I'm given to believe that:

• if we want to trace a DB query, it needs a Context
• if we want the DB query to succeed, that Context has to have been generated in the right way:

handler.Transact(func(ctx context.Context) {...})

instead of

handler.Ctx

But I'm having a real tough time getting this to happen in practice.

For one thing, it appears as if most of our Context instances are generated the "unsafe" way:

$ pwd 
/Users/shealen/go/src/github.com/parsable/mothership/controllers

$ grep '[h|H]andler.Ctx' ./*.go | wc -l
     236

$ grep '[h|H]andler.Transact' ./*.go | wc -l
     162

$ echo "or, ignoring test files and base.go"
or, ignoring test files and base.go

$ find -f *.go | grep -v _test.go | grep -v base.go | xargs -I{} grep '[h|H]andler.Ctx' {} | wc -l
     210

$ find -f *.go | grep -v _test.go | grep -v base.go | xargs -I{} grep '[H|h]andler.Transact' {} | wc -l
     161

This means I have a really hard time checking that my DB query is going to succeed. Given the call tree:

 ...  ... ...     ...  ... ...         ... ...  ...
  |    |   |       |    |   |           |   |    |
   _________        _________            ________
       |                |                   |
    CallerOne(ctx) CallerTwo(ctc) CallerThree(ctx)
            |           |           |
            _________________________
                        |
                MyDbMethod(ctx) {...}

There's an exponential number of places I have to check in order to ensure that each ctx was generated in a way that will let the DB query succeed. Of course, the call tree is rarely like that. But when you're trying to do a "small" fix, it sure feels exponentially complex.

It also means we have existing instances that are easy to find where an unsafe ctx was used to make a traced DB query.

For example, on my first try tracing an unsafe Context generation, I followed it down to an improper usage

controllers.JobController.IndexAllForTeam()
    --> models.LoadJobStepGroup()
        --> models.GetStepGroupsByIds()
            --> on line models/step_groups.go:849 we call Query() on a *datastore.TracedStmt:
                getStepGroupsByIdsStmt.Query(ctx, psql.CreatePSQLArrayFromMap(groupIds))

I think we have a problem here: We can't trust our context.Contexts to work for tracing DB queries.

In my mind, this needs to be dealt with before we can do work towards PA-12308. Since that is our highest priority tech-debt task, that makes fixing this our new king of the hill.

I'll humbly submit one solution: Create a new type. It's a Context that has a TxWrapper as a value -- guaranteed. Only instances of this type will be allowed to be utilized by methods that make DB calls on datastore.TracedStmt instances.

What's the benefit of creating a new context type (and the new functions that come with it) vs. just:

1. Convert as many improper usages as you can find
2. Make sure existing tests pass
3. Log any improper usages in the custom Query function
4. Push to staging for testing and catching any additional bad contexts

I believe the new Context type will involve a similarly lengthy conversion process. Unless we are just talking about syntactical advantages of calling the Query methods directly on the new context object? I am asking because Context is meant to do exactly that - wrapping contextual objects such as TxWrapper within these nested calls.