Create default roles on `reset` if they don't exist

psirenny commented 1 year ago

Feature description

It would be a great developer experience if the Owner and Authenticator roles could be created on reset if they don't already exist. The tool could create the roles automatically or allow developers to do so themselves in hooks.

If the tool handled it automatically, then it could take additional placeholder variables:

{
  "placeholders": {
    "DATABASE_AUTHENTICATOR": "!ENV",
    "DATABASE_AUTHENTICATOR_PASSWORD": "!ENV",
    "DATABASE_OWNER_PASSWORD": "!ENV"
    // …
  }
}

Motivating example

I'm initializing a database for the first time and trying to avoid writing a one-off setup_db.js script by hooking into the graphile-migrate event system.

I tried to create the default roles in an !afterReset.sql script like so:

BEGIN;

DO $$
BEGIN
  CREATE ROLE :DATABASE_OWNER WITH LOGIN PASSWORD :DATABASE_OWNER_PASSWORD SUPERUSER;
  EXCEPTION WHEN duplicate_object THEN RAISE NOTICE '%, skipping', SQLERRM USING ERRCODE = SQLSTATE;
END;
$$;

DO $$
BEGIN
  CREATE ROLE :DATABASE_AUTHENTICATOR WITH LOGIN PASSWORD :DATABASE_AUTHENTICATOR_PASSWORD NOINHERIT;
  EXCEPTION WHEN duplicate_object THEN RAISE NOTICE '%, skipping', SQLERRM USING ERRCODE = SQLSTATE;
END
$$;

…

However, this fails because graphile-migrate attempts to run the script as the Owner before the Owner is created:

graphile-migrate: dropped database 'my_database'
Error: Failed to create database 'my_database' with owner 'my_database_owner': role "my_database_owner" does not exist

I was surprised by the error because I expected graphile-migrate to connect as the root user.

Alternatively, I tried creating the default roles in a !beforeReset.sql script. However, this fails because the beforeReset hook expects the database to exist before it's been created:

error: database "my_database" does not exist

Creating these roles is something all graphile-migrate users have to do, so all of us could benefit from this feature 😄.

Breaking changes

I don't think so.

Supporting development

I [tick all that apply]:

[x] am interested in building this feature myself
[x] am interested in collaborating on building this feature
[x] am willing to help testing this feature before it's released
[x] am willing to write a test-driven test suite for this feature (before it exists)
[ ] am a Graphile sponsor ❤️
[ ] have an active support or consultancy contract with Graphile

benjie commented 1 year ago

One of our "opinions" is:

Roles should be managed outside of migrations (since they can be shared between databases)

Managing them in lifecycle scripts is not unreasonable. I personally handle role creation in a separate process, but I can understand the desire to centralize.

beforeReset was intended to be a chance to dump your old DB (or similar) if you wanted to, before it's deleted and a new one created.

I think maybe we want a new hook beforeDatabaseCreate that runs after beforeReset, after the database is dropped, but before the database is recreated and before afterReset.

This would be a special hook since we know that the database doesn't exist; so it wouldn't make sense to pass it the connection string to the database. Instead it should use the rootConnectionString. I'm thinking a !! prefix to indicate this, such as beforeDatabaseCreate:["!!createRoles.sql"].

Would you like to experiment with this approach?

aakside commented 1 year ago

@benjie what's your separate process to handle role creation? Is it manual?

benjie commented 1 year ago

E.g. https://github.com/graphile/starter/blob/main/scripts/setup_db.js

graphile / migrate