Keystone: Knex adapter drops all tables if no table exists for the first list created

[molomby]

The Knex DB adapters postConnect() function seeds an initial DB for you, based on the lists defined, if it detects the DB hasn't been setup yet. However the check it uses is fairly naive -- it checks whether the first list created (ie. first call to keystone.createList()) has a table in the DB. If no table is found the dropDatabase() function is called which explicitly drops any other list tables before recreating them based on the list schemas.

adapter-knex/lib/adapter-knex.js#L71-L77

const isSetup = await this.schema().hasTable(Object.keys(this.listAdapters)[0]);
if (this.config.dropDatabase || !isSetup) {
  console.log('Knex adapter: Dropping database');
  await this.dropDatabase();
} else {
  return [];
}

This code was intended to be useful behaviour early in app development -- it allows iterating on list schemas without needing any schema migrations and seeding DB structure when first starting work on an app. The problem occurs if a _new_ list is added to the app (and happens to be the first call to keystone.createList()) without a migration that adds the table. Starting an app in this state will cause data loss.

Needless to say, the current check is insufficient for protecting production data.

An argument could be made this oversteps the mandate given to a DB adapter and should be removed entirely. If this or similar behaviour is still desired, it might be better structure in a separate package that could be added as a dev dependency.

[jesstelford]

We've released @keystonejs/[email protected] which includes a change to make the check safer.

We're still working on getting a better fix (there are issues with the first-start developer experience we want to make sure we address too), but at the least this will stop dropping the database except for explicitly setting dropDatabase on the Keystone config object.

[thekevinbrown]

@jesstelford could you let us know what the plan is for the better fix?

Obviously it could have some pretty far reaching implications to server clustering and other things, so if possible I'd like to be able to start planning for the eventual implementation on my side.

[MadeByMike]

I think the long term plan is to move all these initialise Db steps out of postConnect so that the user can explicitly decide when and where they are run.

We'd then call something like an initialiseDB method in an NPM script or similar. The added bonus to this approach being this can differ per environment.

@molomby or @timleslie might have more detailed plans but I believe that was the general direction from initial discussions.

[molomby]

@thekevinbrown -- Yeah as @MadeByMike says, DB init/maintenance should almost always be separate from the main app process. The cost of this is slightly more friction in dev and the initial "getting started" flows but I'm sure we can mitigate this with sensible error messages.

Ie. rather than Keystone detecting the DB hasn't been initialised and taking it upon itself to "restore" the schema, it should probably still do the detection but instruct the user how to resolve the problem themselves. I'm imagining something like..

DB schema missing -- The current database does not contain tables for lists defined in the Keystone schema. Check your DB connection details or, if this is a new project, you may need to run yarn db:init

[thekevinbrown]

@molomby / @MadeByMike Makes sense, thanks!