Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept context args to db functions which spawn scopes #1231

Open
ansel1 opened this issue Oct 19, 2016 · 28 comments

Comments

Projects
None yet
@ansel1
Copy link
Contributor

commented Oct 19, 2016

The idea would be to have alternate versions of the db functions which ultimate create the scopes and execute the sql. These versions would accept a context arg first, following google's conventions for using golang contexts. For example, FirstWithCtx(), DeleteWithCtx() etc. Perhaps there is a cleaner way to do that, like a db.WithContext() function.

The purpose of carrying the context around would be to have it available in callback functions, where it could be used to enhance logging, trace execution times (with opentracing/zipkin/monkit etc), or perhaps carry some authentication context or user id to the callbacks.

@captncraig

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2017

This would be a great addition. Really any way to create a scope that carries arbitrary data for callbacks. If I could just do db.NewScope(model).SetData(ctx) or whatever, I could pull that data out in my callbacks. I also want this to do profiling on begin/end query. The individual profiler is linked to each http request context.

@ansel1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2017

looks like go1.8 has some context support in the database package as well. Haven't looked at it in detail yet, but adds more weight to the idea.

@captncraig

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2017

Could this be done now with callbacks and db.Get/Set?

db.Set("ctx",myCtx).Find(&obj)

and in callback:

ctx, ok := scope.Get("ctx")

Seems to work for me. I set up two callbacks for before and after query:

gorm.DefaultCallback.Query().Before("gorm:query").Register("myPreQuery", myPre)
gorm.DefaultCallback.Query().After("gorm:query").Register("myPostQuery", myPost)

and I appear to be able to pull the context in both. Is this abuse of the values map, or is this what it is intended for?

@jinzhu

This comment has been minimized.

Copy link
Owner

commented Feb 22, 2017

I am thinking about this, possible to add an API like this.

db.Context(ctx).Find(&obj)

also in callbacks, you could access the context with another API or Field

@ansel1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2017

That's probably the least disruptive way to add the support. I guess it fits with gorm's general design. Two other perspectives:

  1. go guys prefer context be passed as function args, not as struct attributes. Probably the "purest" approach would be adding variants of all the functions which execute queries:

     db.FindWithCtx(ctx, &obj)
    
  2. when go guys retrofitted contexts into http, they did something like you suggested, storing the context in an immutable private attribute of the request struct, but the naming convention was different:

     req = req.WithContext(ctx)
    
@smacker

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

I would love to have WithContext method. It will allow to get rid of wrapper that takes values from context and puts it in gorm using db.Set.

@ansel1

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2017

I'm going to hack at this a bit. I'm leaning toward adding variants of the terminal functions which take a context, i.e. FirstWithCtx(...). I know it's a lot more API, but it's more in the preferred style of the standard lib. I think it will also make it easier to migrate a codebase to using the newer, context variants, since you can do a simple search&replace.

@remohammadi

This comment has been minimized.

Copy link

commented Apr 4, 2017

Is @captncraig 's workaround usable to call 1.8 with-context variants of execute/... of the db.DB() (*sql.DB)? Reading the source code of gorm, I didn't found a way to do that. As I've understood, the actual query is called on a sqlCommon instance, which does not allow context to be passed.

Do you accept pull request on this issue? Is there an agreement on the approach? FirstWithCtx is more golang-ish, but on the other hand WithContext is more gorm-ish! Personally, I prefer the WithContext approach (a little more than the other).

(My use case: I have a function with multiple queries which a context with deadline is passed to, and I want to apply the remaining time as timeout on these queries. Using the 1.8 with-context variants make the code more readable, so much more readable!)

remohammadi added a commit to remohammadi/gorm that referenced this issue Apr 8, 2017

@remohammadi

This comment has been minimized.

Copy link

commented Apr 8, 2017

[WIP] Utilize go1.8 context support in database/sql

I've tried my best to keep compatibility with go 1.7 and below. I would greatly appreciate it if you kindly give me some feedback on my approach.

@theduke

This comment has been minimized.

Copy link

commented Sep 13, 2017

@jinzhu any movement on this?

1 similar comment
@pjebs

This comment has been minimized.

Copy link

commented Oct 2, 2017

@jinzhu any movement on this?

@remohammadi

This comment has been minimized.

Copy link

commented Oct 2, 2017

I can fix the conflict and update to work with 1.9, if you're ok with the approach of the changeset.
@jinzhu

remohammadi added a commit to remohammadi/gorm that referenced this issue Nov 18, 2017

remohammadi added a commit to remohammadi/gorm that referenced this issue Nov 18, 2017

glb added a commit to glb/gorm that referenced this issue Jan 22, 2018

@swojtasiak

This comment has been minimized.

Copy link

commented Feb 12, 2018

Really important feature. It's a blocker for us at the moment. Nice to know that it will be supported in near future.

@jinzhu jinzhu added this to the v2.0 milestone Feb 13, 2018

@xmirya

This comment has been minimized.

Copy link

commented Mar 18, 2018

To whoever interested: https://github.com/xmirya/gorm "master" branch adds (*gorm.DB).WithContext(context.Context) to the API that does the thing. The "rename" branch of the same repository renames the internal imports, so it can be used as-is with some vendor package management tool like glide (by setting version: to that branch head commit hash).

This can't be considered a non-API-breaking change as the public SQLCommon interface was updated/switched to contextified versions of database/sql APIs (so all 3rd-party implementations of SQLCommon are not guaranteed to work - though i believe whoever directly passes SQLCommon to GORM actually passes database/sql.DB where those methods are implemented). For that reason i'm not submitting a PR, though the code can naturally be pulled/merged for v2.0 if suitable

remohammadi added a commit to remohammadi/gorm that referenced this issue Mar 29, 2018

@posener posener referenced this issue Sep 22, 2018

Closed

add Context #1807

remohammadi added a commit to remohammadi/gorm that referenced this issue Oct 20, 2018

@emirb

This comment has been minimized.

Copy link
Collaborator

commented Jan 15, 2019

Can we get xmirya@38f5f04 merged to upstream?

@jirfag

This comment has been minimized.

Copy link

commented Jan 16, 2019

+1 to issue. As a workaround you can use trick we use: wrap sql.DB with context.Context and make gorm.DB from the wrapped one. Our code is here

@zhaogaolong

This comment has been minimized.

Copy link

commented Feb 20, 2019

I have a method change logger in http service.

var Store *gorm.DB

func init(){
	db, _:= gorm.Open("mysql", connParams)
	Store = d
}

func GetDB() *gorm.DB {
	db := Store.New()
	db.SetLogger(HaveContextFieldLogger)
	return db
}

This solution solves the problem of log tracking for HTTP requests。

@tylerb

This comment has been minimized.

Copy link

commented Mar 19, 2019

I would really like to see xmirya@38f5f04 merged upstream as well.

@THaGKI9

This comment has been minimized.

Copy link

commented Apr 3, 2019

any update on this topic?

@emirb

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2019

@jinzhu It has been almost 2 and a half years since this issue has been opened and a fix is rather easy. Any ETA?

@tzvatot

This comment has been minimized.

Copy link

commented Apr 21, 2019

+1

6 similar comments
@jeffizhungry

This comment has been minimized.

Copy link

commented May 1, 2019

+1

@doucheng

This comment has been minimized.

Copy link

commented May 2, 2019

+1

@gpopovic

This comment has been minimized.

Copy link

commented May 6, 2019

+1

@oourfali

This comment has been minimized.

Copy link

commented May 6, 2019

+1

@nimrodshn

This comment has been minimized.

Copy link

commented May 6, 2019

+1

@rawsyntax

This comment has been minimized.

Copy link

commented May 6, 2019

+1

remohammadi added a commit to remohammadi/gorm that referenced this issue May 8, 2019

remohammadi added a commit to remohammadi/gorm that referenced this issue May 8, 2019

remohammadi added a commit to remohammadi/gorm that referenced this issue May 8, 2019

@tkvw

This comment has been minimized.

Copy link

commented May 20, 2019

Any update on this issue? It's closed and should be fixed in the 2.0 milestone, but I don't see any progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.