Skip to content

Conversation

@jsha
Copy link
Contributor

@jsha jsha commented Jul 7, 2023

This change replaces gorp with borp.

The commits are split out meaningfully. The first commit adopts a version of borp shortly after we forked it, where the only meaningful changes are import paths. It updates our vendored copy, excludes mysql 1.7.1 in go.mod (so we don't accidentally upgrade and cause a performance regression), and fixes up a bunch of naming and comments.

The second commit updates the current version of borp, which has contexts for all relevant functions. It's a very large diff that consists mainly of adding context arguments and plumbing them through.

The last commit was a refactoring I realized during this process was needed: our own db.WrappedMap embeds a *borp.DbMap, which means methods it does not explicitly wrap get silently passed through. I made that a regular field, not an embed, and unexported it. That revealed a few additional methods we needed to implement. Also I had to add some accessors for the underlying *gorp.DbMap and *sql.Db in a couple of places that needed those.

Message for merge:

This change replaces gorp with borp.

The changes consist of a mass renaming of the import and comments / doc fixups, plus modifications of many call sites to provide a context.Context everywhere, since gorp newly requires this (this was one of the motivating factors for the borp fork).

This also refactors github.com/letsencrypt/boulder/db.WrappedMap and github.com/letsencrypt/boulder/db.Transaction to not embed their underlying gorp/borp objects, but to have them as plain fields. This ensures that we can only call methods on them that are specifically implemented in github.com/letsencrypt/boulder/db, so we don't miss wrapping any. This required introducing a NewWrappedMap method along with accessors SQLDb() and BorpDB() to get at the internal fields during metrics and logging setup.

Fixes #6944

@jsha jsha requested a review from a team as a code owner July 7, 2023 00:57
@jsha jsha requested a review from pgporada July 7, 2023 00:57
jsha added 3 commits July 6, 2023 18:00
This switches to an early version of borp, before the PR that adds
contexts to all of borp's relevant functions. It involves mainly the
dependency change, plus a bunch of renamings.
This updates to the version of borp that has contexts in all relevant
functions, and make corresponding Boulder changes so everything
compiles.
This was allowing some methods to pass through to the underlying
borp.DbMap accidentally, without getting wrapped.

Introduce accessors to get the underlying *borp.DbMap or *sql.DB when
needed.
@aarongable
Copy link
Contributor

(We'll probably want to rewrite the commit description before merging, since the references to individual commits won't be relevant once landed. Also it looks like the last paragraph got cut off?)

@jsha
Copy link
Contributor Author

jsha commented Jul 10, 2023

Fixed, thanks! And added a description suitable for merging.

test/db.go Outdated
Comment on lines 65 to 66
func deleteEverythingInAllTables(db CleanUpDB) error {
ts, err := allTableNamesInDB(db)
ctx := context.Background()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a) why does this not take a context, but allTableNamesInDB does?

then, probably non-actionable in this PR:
b) why does neither of these take a t and mark itself as t.Helper()?
c) gosh I wish that t came with a t.Context() that would give you a handy dandy context with the test's metadata and timeout (if any) already populated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wound up plumbing up the context as far as the two exported functions, ResetBoulderTestDatabase and ResetIncidentsTestDatabase. The former is called a lot of places. I don't really know of a principled approach to when we should pass through a context in test code or not. In general we're not actually applying timeouts or doing traces. That would argue for never passing a context and always using context.Background() all the way down at the leaf functions. But that feels... bad.

@jsha
Copy link
Contributor Author

jsha commented Jul 11, 2023

Pushed an update with most of the review feedback. Still working through changing Background to TODO in the various mains.

@jsha
Copy link
Contributor Author

jsha commented Jul 11, 2023

Ok, finished the changes to use TODO.

@jsha jsha requested a review from a team July 11, 2023 23:04
aarongable
aarongable previously approved these changes Jul 11, 2023
@pgporada
Copy link
Member

Running Lints
cmd/notify-mailer/main.go:1: : # github.com/letsencryptcmd/notify-mailer [github.com/letsencryptcmd/notify-mailer.test]
cmd/notify-mailer/main_test.go:540:9: not enough arguments in call to m.run
	have ()
	want ("context".Context)
cmd/notify-mailer/main_test.go:598:9: not enough arguments in call to m.run
	have ()
	want ("context".Context) (typecheck)

@jsha jsha merged commit 7d66d67 into main Jul 17, 2023
@jsha jsha deleted the borp branch July 17, 2023 21:38
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.

database: refactor WrappedMap to always take context.Context

4 participants