BCDA-9314: refactor API db connection globals#1171
Conversation
bcda/bcdacli/cli.go
Outdated
| app.Before = func(c *cli.Context) error { | ||
| db = database.Connection | ||
| r = postgres.NewRepository(db) | ||
| connection = database.GetConnection() |
There was a problem hiding this comment.
Here is an entrypoint at which we can create the database connection and pass it as a dependency to the routers and services that need it.
bcda/api/v1/api.go
Outdated
| } | ||
|
|
||
| func init() { | ||
| func NewApiV1(connection *sql.DB, pool *pgxv5Pool.Pool) *ApiV1 { |
There was a problem hiding this comment.
Following the example of the tech spec, we create a new struct for the API that clearly defines its dependencies (including the database connections!) with a constructor.
| app.Version = constants.Version | ||
| app.Before = func(c *cli.Context) error { | ||
| db = database.Connection | ||
| db = database.Connect() |
There was a problem hiding this comment.
Construct connections and provider at code entrypoint to be passed down to APIs, routers, etc.
There was a problem hiding this comment.
because we are no longer using the *cli.App for command line usage, it would be nice to eventually move away from this since this is where top level DI would occur. just a thought - way too big of a lift for this PR.
oversimplification, but something like:
type App {
api http.server
auth http.server
data http.server
pool sql.db/pgx
c config
s service
}
There was a problem hiding this comment.
I think this is a great idea, and I would vote to do this in a separate tech debt ticket that would remove the cli, if that works for you.
| river.WorkerDefaults[worker_types.CleanupJobArgs] | ||
| cleanupJob func(time.Time, models.JobStatus, models.JobStatus, ...string) error | ||
| archiveExpiring func(time.Time) error | ||
| cleanupJob func(*sql.DB, time.Time, models.JobStatus, models.JobStatus, ...string) error |
There was a problem hiding this comment.
probably not suited for changes in this PR, but if we are updating the DB depenedencies, would cleanupJob and archiveExpiring be better suited as methods with CleanupJobWorker being the receiver? they would have access to db via CleanupJobWorker.db instead of having to pass it in. Thoughts on maybe leaving a comment on here?
There was a problem hiding this comment.
Absolutely, I actually thought the same thing and wondered why they were in separate files.
I left a comment for now but I'm open to changing it if folks feel strongly about it.
There was a problem hiding this comment.
Maybe we can leave a TODO + a ticket for this one for future work to minimize risk?
|
|
||
| func NewAuthRouter(middlewares ...func(http.Handler) http.Handler) http.Handler { | ||
| func NewAuthRouter(provider Provider, middlewares ...func(http.Handler) http.Handler) http.Handler { | ||
| baseApi := NewBaseApi(provider) |
There was a problem hiding this comment.
I am certain this is user error, but I don't see the definition for this anywhere with cmd+f - does this func definition exist?
There was a problem hiding this comment.
Sorry, which function are we talking about? If it's NewBaseApi, I created that in bcda/auth/api.go
|
The two failing CI checks likely just need to update to latest changes on main. |
these failing on the aurora name? |
laurenkrugen-navapbc
left a comment
laurenkrugen-navapbc
left a comment
There was a problem hiding this comment.
Super happy to remove these inits from the codebase - thanks so much for knocking this out! 🎉
3 participants
🎫 Ticket
https://jira.cms.gov/browse/BCDA-9314
https://jira.cms.gov/browse/BCDA-9287
🛠 Changes
connectfunctions, which establish new connections to the database and replace init and global connection variablesconnectfunctions with separate health checksauth/Provider.goto not be a global, either, and instead be configured at setup and passed down like the database connectionsTo-Do:
ℹ️ Context
This PR demonstrates a strategy to refactor the database Connection -- specifically, to remove the global Connection variables defined in /database/connection.go which are created in an init() function, and instead do the following:
As a result, many other files needed to be changed to store the db connection within its state, including the API files and middleware. Notably, the Provider, which also depends on the db connection, was also being used as a global across many files, so it too had to be updated to a stateful type which is constructed at entrypoints.
Finally, many tests were updated to account for these changes. The majority of the test updates were completely innocuous -- replacing references to a global connection with references to a constructed connection -- but others involved untangling hidden dependencies on the database, provider, etc. that became very obvious after they were refactored.
🧪 Validation