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

feat: inject DB verb resources #2985

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

worstell
Copy link
Contributor

@worstell worstell commented Oct 3, 2024

you can declare a DB by defining a type that implements PostgresDatabaseConfig (the only supported interface implementing DatabaseConfig, at present):

type MyConfig struct {}

// name provides the DB name
func (MyConfig) Name() string { return "testdb" }

// tags the type as a DB decl
func (MyConfig) db()

// tags the DB type as `postgres`
func (MyConfig  pgdb()

to avoid the verbosity of implementing db() and pgdb(), you can instead embed ftl.DefaultPostgresDatabaseConfig.
this will implement these "tag" methods but not any required methods, e.g. Name() which must be explicitly provided in the user code.

type MyConfig struct {
   ftl.DefaultPostgresDatabaseConfig
}

// name provides the DB name
func (MyConfig) Name() string { return "testdb" }

you can then use the DB you've configured by injecting a handle around its config into a verb signature:

//ftl:verb
func Query(ctx context.Context, in Request, db ftl.DatabaseHandle[MyConfig]) (Response, error) {
  db.Get(ctx).Exec(...)
  // ...
}

eventually we will extend DatabaseConfig with additional methods, e.g. RAM, disk, max connections, etc.

@worstell worstell requested review from a team and alecthomas as code owners October 3, 2024 21:41
@worstell worstell requested review from matt2e and removed request for a team October 3, 2024 21:41
This was referenced Oct 3, 2024
@alecthomas alecthomas requested review from a team as code owners October 3, 2024 21:42
@alecthomas alecthomas requested review from stuartwdouglas and deniseli and removed request for a team October 3, 2024 21:42
@worstell worstell force-pushed the worstell/20240930-database-resources branch 2 times, most recently from b246f09 to 9fb57a9 Compare October 3, 2024 22:13
@alecthomas
Copy link
Collaborator

I can't review this fully at the moment, but is it possible for us to eliminate the string names now we're code-genning? They were only present because we needed them at runtime, but that is no longer the case and they are a source of errors.

@alecthomas
Copy link
Collaborator

ie. we use the variable name as the name, similar to what we do with eg. verb function names.

@worstell
Copy link
Contributor Author

worstell commented Oct 7, 2024

ie. we use the variable name as the name, similar to what we do with eg. verb function names.

the pattern for these are type alias declarations, e.g. type TestDb = ftl.PostgresDatabaseHandle

the limitation was that, afaik, using reflection at runtime we can't derive the alias name because unlike the pattern type TestDb ftl.PostgresDatabaseHandle, no new named type is declared, and the reflected type is always just ftl.PostgresDatabaseHandle. we can still get the alias name during static analysis, but just not at runtime (unless i'm mistaken).

type TestDb ftl.PostgresDatabaseHandle doesn't work because we need to supply member functions, e.g. .Get(ctx). i considered embedding but that just felt so much less ergonomic/intuitive and the string handling is mostly isolated to code-genned files, except unfortunately in ftltest.

@alecthomas
Copy link
Collaborator

we can still get the alias name during static analysis, but just not at runtime (unless i'm mistaken).

I was more suggesting changing to this:

var myDb = ftl.PostgresDatabase()

We'd register the variable with the statically extracted name "myDb" removing the redundancy/confusion of having to do:

var myDb = ftl.PostgresDatabase("myDb")

@worstell
Copy link
Contributor Author

worstell commented Oct 7, 2024

we can still get the alias name during static analysis, but just not at runtime (unless i'm mistaken).

I was more suggesting changing to this:

var myDb = ftl.PostgresDatabase()

We'd register the variable with the statically extracted name "myDb" removing the redundancy/confusion of having to do:

var myDb = ftl.PostgresDatabase("myDb")

using a type rather than a variable lets us pass the db decl into the verb signature like we're doing for the verb clients, e.g.

type TestDb = ftl.PostgresDatabaseHandle

//ftl:verb export
func Echo(ctx context.Context, req Request, db TestDb) (Response, error) {
	db.Get(ctx).Exec(...)
}

@worstell worstell force-pushed the worstell/20240930-database-resources branch 3 times, most recently from 9810956 to 106bea5 Compare October 7, 2024 21:18
@alecthomas
Copy link
Collaborator

alecthomas commented Oct 7, 2024

We're going to need a solution to this sooner or later. At some point the databases are going to have configuration options, eg. to set the instance size. If we can't uniquely identify a database handle we won't be able to identify the configuration for that handle.

ie. each database will be configurable with the RAM/CPU size

@worstell worstell force-pushed the worstell/20240930-database-resources branch 4 times, most recently from 67f0347 to 3959073 Compare October 9, 2024 21:43
@worstell worstell force-pushed the worstell/20240930-database-resources branch 7 times, most recently from 894663d to 766ad72 Compare October 10, 2024 19:09
@worstell worstell force-pushed the worstell/20240930-database-resources branch from 3d0f32e to ff67e61 Compare October 10, 2024 19:20
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.

3 participants