Question: plans for context.Context support?

[ansel1]

Specifically, support passing contexts through to the new go1.8 sql.DB methods that take them. Something like ExecutorContext (and TransactorContext, etc), and I guess variations of the finalizer methods which take them.

[aarondl]

There's no roadmap item or anything for this. I was sort of waiting for the demand of this feature to be high enough to do it. Because there needs to be support in the drivers (lib/pq, go-sql-driver/mysql, etc.) to support this before it's of any use in SQLBoiler I thought we had a bit more time before there'd be a good reason to implement it.

It also uglies up our everything quite badly. Which is a squishy human reason for not wanting to do it. I've thought of three interfaces, none of which I really like (please help with suggestions if you have some):

1. Everywhere there's a db parameter, also pass in a context. This has the downsides of being a breaking change for everyone, being ugly, and requiring non-context users (probably most) to supply context.TODO everywhere.

   p, err := models.Pilots(db, ctx).All()

2. Have C variants to expose the same behavior as above so they become optional. We already have many method variants, this'd just be another one. It's better than the first since it's a non-breaking change.

   p, err := models.PilotsC(db, ctx).All()

3. Have a wrapper object whose bound to a context for the context calls and if the context is present in the wrapper attempt to upgrade the Executor to a ContextExecutor. This is probably the cleanest, and the upshot is there's no breaking change, no extra variant. The downside is that it's a bit hard to see if there's a context being passed in.

   
   type ContextDB struct {
   Executor
   ctx      context.Context
   }

db = boil.WithContext(db, c) p, err := models.Pilots(db).All()

[ansel1]

An issue with all these approaches is they don't follow the suggested practice of not storing contexts as attributes. I think the reason for that is to avoid accidentally hanging on to a context for longer than a single transaction/operation/thread of execution. The recommendation is to only pass contexts as arguments.

What if the terminal functions (All(), Bind(), Exec() etc) had variants:

p, err := models.Pilots(db).AllContext(ctx)

These variants would try and upgrade the Executor to ExecutorContext if possible.

[aarondl]

So we talked about this (sorry for the slow reply). Thanks for pointing out the best practices surrounding Context use. Despite hovering around a lot of Go stuff, I wasn't actually aware of this particular nit when using it.

One of the issues with your suggestion however is it does ugly up the API a certain amount in that the db is put far away from the context when in reality, they go together and are "one" thing (can't make the call without both). An example where this becomes kind of awkward is where you create a query and then execute it later inside a different function. The context ends up being "used" in a different function, rather than in the same one with the database handle. Also with the suggestion you've made we increase the amount of variants of the finishers (and update/find/upsert) a fair amount, and though in my previous post I thought that was okay, when you look at the number of methods it would spawn it seems less okay.

.All()
.AllP()
.AllContext()
.AllContextP()
Find()
FindG()
FindGP()
FindGContext()
FindGPContext()

The only way around the separation mentioned above that seems clean is to move them to one or the other side. Though this is a breaking change in both cases.

p, err := models.PilotsC(db, ctx).All()
p, err := models.Pilots().AllContext(db, ctx)

The only way around creating variants is one of two methods:

1. Wrap the db like I showed earlier, which goes against the very sane advice of only passing contexts as parameters.

   db = boil.WithContext(db, c)
   p, err := models.Pilots(db).All()

2. Force a context, and have people put todo everywhere. Also a big breaking change. Also goes against all the advice given above.

   p, err := models.Pilots(db, context.TODO()).All()

So as I've typed this all out, I realize that I've essentially said the same thing with a slight variation where we could break the API in order to move the arguments around so it's more consistent. This seems like it's the only middle ground between all the approaches. Since any that I've come up with hold the context (which you've rightly pointed out is bad) and any that don't break the API, or make it less pleasant.

It's a sort of impasse since the only reasonable change would be a breaking one. I wanted to ask what the urgency behind the request is, do you have a driver that supports contexts and you want to use SQLBoiler with it? Or is it that you want to pass your contexts through so that when the driver does finally support them that you'll be ready to go?

[ansel1]

I'm not currently using sqlboiler. We currently use gorm pretty extensively, but that also doesn't have context support, so I was looking around at alternatives.

Our desire for context support comes from the way our project is instrumented. The thing I liked about sqlboiler is that I don't need to pass a sql.DB to it. I can wrap the sql.DB in something that tracks metrics like the number of database calls, and time spent in database operations. And further, sqlboiler (or gorm) were context aware, that wrapper could use the context to track even more stuff, like total db time per request. We also use zipkin-style transaction tracing in our app, and we could add database operations to those traces with contexts.

The driver support for context deadlines is cool too, but that's not what we're itching for right now. It's really about tracing and metrics.

Personally, I still the think the right thing to do is variants of the finalizer methods. You mentioned above that it feels wrong to you that the context and the db instance could be so disassociated ("the db is put far away from the context"). But I think the that notion that they are coupled, are "one thing" that "go together" might not be correct. The context doesn't follow the db around. It follows around main thread of execution, the "hot path", where the interesting work is happening. In sqlboiler, I think that in the finalizers.

The stuff like models.Pilots(db), and the query modifiers, that's all basically constructors for re-usable queries. I could see projects setting up sets of common queries at initialization time and re-using them (a bit like "scopes" in rails). That means those queries might be long-lived things that span many requests or transactions. Which is why you wouldn't want to associated a context with them.

As far as API bloat...eh. I would just take the hit and add all the variants. It's not worth making a breaking change. It's what the go guys themselves did. I'd keep the the G variants as variants of the starters...I see it's pretty tough, because it's not only finishers that would need Context variants. You've got PilotsFind methods, the methods on the models (like Insert, Upsert, etc), Reload(), associations stuff, potentially supporting alternate hook interfaces which can accept them. It's a lot... Some of that could be simplified by attaching the context to the Query as an attribute. That's probably an acceptable compromise, since the Query already represents a single operation. Then maybe you could attach a context via a Query Modifier? And perhaps expand the hook support to give hooks access to the Query, and therefore the context. That might have other advantages as well, like allowing hooks to add additional modifiers to queries (a soft-delete feature might be implemented this way).

If there's anywhere I'd consider trimming, it would be the P variants. DB operations are intrinsically unpredictable (because they interact with an external system), so it seems appropriate to me to force the developer to handle the error. These operations will error sometimes, guaranteed. In go, I tend to reserve panics for situations like "this never should have happened, and if it did, all my assumptions are wrong, so OMG I'm outta here". I'd consider deprecating the P variants of the finishers

One other thing about context convention: for those functions/methods which accept a context, it should be the first argument, e.g. `models.Pilots(db).

[bjyoungblood]

I'm with @ansel1. The finalizer methods seem like the best place to add context.

To address API bloat, with if users could control generation of certain finalizer styles through config? I don't use any of the *P methods (in fact, I added a template override to my project so they aren't even generated).

Storing context in the query (using a query mod) seems like it would be fair as well (this would mirror how net/http handles context for outbound requests), but I think this makes it too easy to forget to pass context into a query.

[aarondl]

Sorry I've been on business and haven't been able to respond quickly.

I agree with the context being the first parameter, also following the guidelines and I'm happy to oblige.

I still dislike separating the db and the context - to me they're both "required to execute the query" and the only reason the db is where it is is because it made the API more consistent in that the first argument to the first function in a chain is always the db. Again I'd prefer to move the DB to the end if the context is going to be there too. Not sure we'll be able to reach agreement here. Keep in mind this means that we'd move the db, not the context. This actually could provide some more re-usable query fragments perhaps since the db is bound late. Proposal ends up being:

p, err := models.Pilots().All(ctx, db)

Also the query mod is no better than wrapping the DB in a contexted DB as it still stores the context which we're trying to avoid and is basically the same level of weirdness.

What I do really like is what bj says about controlling variant generation via config. Not only would this allow us to break the API for context (though depending on the extent the templates could get PRETTY messy) but I think it's also a fair point that P and G should be optional at generation time. If context was handled in this way we could also decide to not implement the variant, but simply put the context as required parameter everywhere. I think if you buy in to using context, you should be using it everywhere and so this is a good idea, please correct me if I'm wrong.

Join us on Slack if you'd like, hopefully we can get some more interactive discussions going on :) https://sqlboiler.from-the.cloud/

[ansel1]

Ok, I see...yep, I like it. Moving db param to the end of call chain, and using a generation flag to turn context variants on and off...that all feels pretty elegant.

[bjyoungblood]

I think I'd be in favor of moving DB to the end and keeping it with context. That would certainly make cloning queries simpler and safer.

[ansel1]

I guess hooks could be handled with the same kind of flag, generating context variants if requests, ie

// Define my hook function
func myHook(ctx context.Context, exec boil.Executor, p *Pilot) {
  // Do stuff
}

// Register my before insert hook for pilots
models.AddPilotHook(boil.BeforeInsertHook, myHook)

[aarondl]

Yep. I did have one more question before we can execute on this. Is it appropriate for a context to be used in multiple queries to the DB. For example an eager load could do N select queries and would be re-using the same context since at least with the current proposal would be no way to get a new one. Does this pose a problem?

[bjyoungblood]

Yep, that is definitely correct. Using the same context enables packages like github.com/ExpansiveWorlds/instrumentedsql to profile each of the queries in the operation separately, which would be super helpful for debugging slow eager loads (or slow queries in general).

[aarondl]

Implemented as discussed above. c78b67b

[aarondl]

Also worth noting that @bjyoungblood's previous suggestion to remove panic and global variants was taken seriously and they are now off by default and hidden by flags: 1121042