Skip to content

Backend RFC: use generics to constrain DB store schemas

Warren Gifford requested to merge cc/rfc-generic-handles into main

Created by: camdencheek

This is an RFC implemented as a draft PR so I can demonstrate rather than speculate.

Proposal statement:

I propose we use generics type parameters to constrain the database migration state across the backend codebase.

What?

Basically, add a generic parameter T SchemaKind to each of our types that holds a database handle, and use that to enforce that the handles we are working with have the correct migrations applied. SchemaKind is one of Frontend, CodeIntel, CodeInsights, Any, or a type that combines some set of those such as Production, which is the union of Frontend and CodeIntel.

All stores must declare their required migration type, and it's impossible (or at least very difficult) to create a store from a handle that does not have those migrations applied.

Why?

We practically have five different possible database migration states across the backend:

  • Frontend
  • CodeIntel
  • CodeInsights
  • Production (Frontend + CodeIntel)
  • Any (doesn't care about the state of any migrations)

In the past, I've made efforts with types like database.DB to make it more clear what type of database is being used and when. However, existing types (database.DB and the new InsightsDB and CodeIntelDB) are only useful until they are used to create a store, (such as database.RepoStore), which embeds a basestore.Store that erases any type information we have about the handle's schema.

This is annoying when trying to figure out what schema a store operates on. You have to go back to the where the store was created and follow the call chain back until you get to one of the typed interfaces or you get to the the place where the actual migrations were run.

Additionally, there is currently nothing stopping you from creating a store using a handle from the incorrect schema. For example, NewInsightsStoreWith() will happily accept database.RepoStore as an implementer of ShareableHandle, but this would be incorrect and fail dramatically. This is not a huge problem now because tests will catch this, but we also have a large number of types in the backend just named Store, and I think we'd also get some readability benefit from this change.

On top of all this, I think this will make it easier to separate database packages into more granular packages because we won't have as much need for grand, unified interfaces like database.DB to communicate that "this is a handle to the frontend database."

How?

See inline comments for some key examples.

Goals

  • Determine whether people think we actually need this
  • Determine whether people actually want this
  • Solicit concerns about implementation

Non-goals

  • Merge this PR. It doesn't build anyways.
  • Implement this RFC just because generics are cool

Review instructions

  • If you approve, react with a thumbs up
  • If you would prefer this wasn't implemented, react with a thumbs down
  • If you have comments/questions/concerns, leave a comment
  • Note that this is completely untested and doesn't even build, so don't bother yourself with correctness reviews. Just looking for feedback on "do we want something like this" and "is this the pattern we want"

Merge request reports