encore icon indicating copy to clipboard operation
encore copied to clipboard

Add option for avoiding using `Named` for getting the driver for the current system's DB

Open bih opened this issue 3 years ago • 7 comments
trafficstars

See Slack thread: https://encoredev.slack.com/archives/CQFNUESN9/p1664736264406129

bih avatar Oct 03 '22 09:10 bih

I'll start on that one for Hacktoberfest

Minivera avatar Oct 04 '22 13:10 Minivera

I'm not convinced this is a good idea. I'm worried it increases the risk of deleting or modifying data you didn't intend to, because a code snippet was moved from one service to another.

What's the pain point you were feeling that motivated your wanting this, @Minivera?

eandre avatar Oct 04 '22 14:10 eandre

The pain point comes from using a third-party ORM or another tool for connecting to a database within a service, for example in this guide: https://encore.dev/docs/how-to/entgo-orm. I've had to refactor some code between services on some instances and I've had multiple errors because I forgot to rename the Named parameter, which can be tough to debug when you don't expect it. Named also seems to not be checking for the database's existence (? not sure), so errors are tough to catch. For me, the worst ones are when I rename a system's folder (Example, I want to rename usr to user, I have to find the Named calls and make sure to also rename them, otherwise it fails silently. Always connecting to the current db would solve that.)

From my limited perspective, this risk seems to be the same for both methods? (Or even higher using Named)

  • If using Named followed by Stdlib, you'll connect to that database. If a file is moved from one system to another, that connection still happens even if you moved the migrations around. It would seem like that is still at a risk of deleting data or running into errors if you changed the database you connected to, but forgot to change the name in Named. Let's say for example I moved a file from the user to the product system, but forgot to rename the Named and left some user code in there, the product system might modify the user db without me realizing it?
  • If always connecting to the current service's DB. If you move a file to another system, that connection will also change which db it connects to. If you also moved the migration files, then most of the "new" tables should be empty? If not, the likelihood of a table being exactly the same is fairly low, so the risk is more about running into SQL errors telling you that the SQL queries you're trying to run are not valid, and less about you running into the risk of deleting stuff.

I'm still confusing myself with the system v.s. service terminology, so hopefully this helps.

Minivera avatar Oct 04 '22 14:10 Minivera

In my opinion, controlling and creating databases through code would be better, but this small change could also help a lot in the meantime.

Minivera avatar Oct 04 '22 14:10 Minivera

I think introducing sqldb.NewDatabase("name") and have that return a *sqldb.DB would solve a lot of these issues, and give you more control over database creation and decouple the name of the database from the name of the service the code lives in. And if the compiler doesn't catch the issue of sqldb.Named("blah") referencing a database that doesn't exist, we should fix that. Would those two changes combine address the issues you're describing?

eandre avatar Oct 05 '22 08:10 eandre

That would yes! Do you think the first one (sqldb.NewDatabase("name")) would be something worth investigating during hacktoberfest? I can work on the second one no problem, but the first one might be more complex (Happy to start with a proposal there)

Minivera avatar Oct 05 '22 14:10 Minivera

I created the issue for the sqldb.NewDatabase("name") proposal, feel free to edit it or close it if that's not what you have in mind. I had some thoughts and I like to throw things down in written form before I forget.

Minivera avatar Oct 07 '22 21:10 Minivera