Remove dbconn.Global
#28251
Merged
Remove dbconn.Global
#28251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
camdencheek
force-pushed
the
cc/remove-dbconn-global
branch
2 times, most recently
from
94c1589
to
775828e
Compare
This removes dbconn.Global references from the codeintel GraphQL resolvers by passing down a database handle through parent resolvers.
This inlines checkCurrentUserIsSiteAdmin because its only purpose was to call backend.CheckCurrentUserIsSiteAdmin with the global db connection. Instead, this inlines the function and passes in a db handle directly.
This replaces references to dbconn.Global in GetOrAssignUserCustomerID with a passed-in database handle. It also modifies it to rely on `tx.Done` for its error handling.
Now that we are passing a database.DB into GetOrAssignUserCustomerID, we can stop wrapping a `dbutil.DB` with `database.NewDB`.
This makes InitDB not set `dbconn.Global`, instead keeping the connection in a local variable and returning it.
Now that we don't reference dbconn.Global anywhere in the resolvers, we can get rid of this safety check.
This stops the worker db init function from setting dbconn.Global, instead creating and returning the connection the caller.
This stops the precise-code-intel-worker from initializing dbconn.Global. It creates the connection and returns it instead.
There is no need to set dbconn.Global anymore since it's not referenced.
The dbtesting package previously would set `dbconn.Global`, then return it from `GetDB`. Instead, this creates a global variable just for the `dbtesting` package so it will continue to reuse the same connection across all calls to `GetDB` (no change in behavior), except it will no longer set `dbconn.Global`. This package is deprecated, and will soon be removed, so I didn't feel the need to refactor this further.
The time has come.
camdencheek
force-pushed
the
cc/remove-dbconn-global
branch
from
775828e
to
ed9c82f
Compare
|
Notifying subscribers in CODENOTIFY files for diff 2ada491...ed9c82f.
|
eseliger
approved these changes
Nov 28, 2021
| connectOnce sync.Once | ||
| connectErr error | ||
| connectOnce sync.Once | ||
| globalTestDB *sql.DB |
There was a problem hiding this comment.
Any reason why this is still global?
There was a problem hiding this comment.
To preserve the behavior of GetDB. All calls to GetDB for a process will return the same handle right now, and not doing that might break some tests. I'm currently working on removing the dbtesting package entirely, so I didn't bother trying to make that behavior better
There was a problem hiding this comment.
The important part here is that globalTestDB is only accessible through GetDB, so it can't be referenced directly by other packages. So, when we replace GetDB, we can get rid of this.
There was a problem hiding this comment.
Makes sense! Thanks for the explanation 👍
unknwon
approved these changes
Nov 29, 2021
Comment on lines
-64
to
-65
| // TODO(jchen): This is an unfortunate compromise to not rewrite ossDB.ExternalServices for now. | ||
| dbconn.Global = db |
mrnugget
approved these changes
Nov 29, 2021
|
Nice!! Thanks you so much for driving this to completion! |
|
Thanks! I can't believe we finally did it! |
10 tasks
bobheadxi
added
dx
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The time has come.
Each commit has a message describing what it is doing, but the gist of this PR is:
dbconn.Globalis currently being useddbconn.Globalduring initializationdbconn.Global