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

Add context to all database calls #302

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add context to all database calls #302

wants to merge 1 commit into from

Conversation

SuperSandro2000
Copy link
Member

Also fix all unused parameters

I wanted to achieve a maintainable solution that also works for future addition. For that reason I choose to mirror the interface in internal/keppel/database.go and mark all the functions deprecated which then get marked ci golangci-lint.

Also fix all unused parameters
@majewsky
Copy link
Contributor

I saw several places where WithContext is not used, especially around transactions. The problem is that db.WithContext(ctx) does not have a Begin method. Apparently Gorp expects one to do Begin without context and then use tx.WithContext(ctx) after the transaction has already started.

Since there are some places where WithContext is not used, I tried to have an interface that more strongly discourages misuse. For example, in internal/api/keppel/api.go,

type API struct {
  ...
  db *keppel.DB
  ...
}

could be changed to

type API struct {
  ...
  dbWithoutContext *keppel.DB // do not use directly
  ...
}

func (a*API) db(ctx context.Context) gorp.SqlExecutor {
  return a.db.WithContext(ctx)
}

to make it clear that the DB handle should only be accessed through the helper method that needs a context. But then we have a gorp.SqlExecutor which is missing some vital methods, specifically Begin and Prepare (and therefore also cannot be cast into sqlext.Executor which several library functions take). This looks like a significant issue for context coverage. Opinions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants