Keystone: Keystonejs just dropped my production database

[Neo-Zhixing]

Bug report

Today I ran yarn upgrade-interactive and upgraded the keystone version to the newest. Then I deployed the image and saw this in the logs:

✔ Validated project entry file ./src/index.js
✔ Keystone server listening on port 3000
✔ Initialised Keystone instance
⠧ Connecting to database  knex:query select 1+1 as result undefined +0ms
  knex:bindings [] undefined +0ms
⠏ Connecting to database  knex:query select * from information_schema.tables where table_name = ? and table_schema = ? undefined +566ms
  knex:bindings [ 'User', 'public' ] undefined +565ms
⠹ Connecting to databaseKnex adapter: Dropping database
⠸ Connecting to database  knex:query DROP TABLE IF EXISTS "public"."users" CASCADE undefined +730ms

And it just dropped my entire users table, completely and silently. Thankfully I have database backups, but I still lost 18 hours of data.

Describe the bug

I'm using the tableName property to specify the table name in the database. However, Keystone decided to drop my table without any warnings. One would normally assume that running the program would be a safe operation, but keystone somehow decides to drop an important table during that process.

This is a major design flaw, and I couldn't get around to understand why would someone drop a database table during the startup sequence.

To Reproduce

Run the keystone instance

Expected behaviour

The database is safe

System information

• Database: Postgresql

Extra info

Please, with bugs like these hanging around do not call Keystone 5 "Production Ready..." It's not, and it's misleading to tell people otherwise.

1820

[Neo-Zhixing]

Obviously, the reason keystone dropped the database, is because in this piece of code,

    Object.values(this.listAdapters).forEach(listAdapter => {
      listAdapter._postConnect();
    });
    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 [];
    }

isSetup is evaluated to false.

The reason isSetup is false, is that the database doesn't contain the table "Object.keys(this.listAdapters)[0]"

Why it doesn't? Because in my case, the list name is "User" but the table name is "users". (See #1896 and #1903). So Keystone checked that the database doesn't contain "User" and decided that it's a good idea to drop "users".

A fix would be to use the table name here instead of the list name, but dropping a database table without any warnings is stupid af regardless.

[Neo-Zhixing]

OMG I just saw #2095 .......

[JedWatson]

Hey @Neo-Zhixing you're absolutely correct on this, we just found this a couple of days ago as you saw in #2095 and are working urgently on a fix. We're very sorry that you were also affected by it.

Aside from making the check more robust, we'll be putting this behaviour behind a much clearer opt-in flag so there's no chance of anybody getting tripped up by it again in the future.

[jesstelford]

This really really sucks! I am so sorry this happened @Neo-Zhixing, it is squarely a bug in Keystone.

Silently dropping a production database is not acceptable for any tool, stable or not.

Please accept our apologies and know we'll do better in the future!

Sending you lots of hugops for what I know is a horribly stressful situation ❤️

The issue @molomby reported (#2095) has triggered a deeper investigation into why this happens and we are actively working on a solution.

For an important issue like this we want to be sure we are keeping track of the details in one place, so we'll close this issue as a duplicate and focus on #2095.

[Neo-Zhixing]

@jesstelford @JedWatson Thank you for your support! I believe that with your dedication, Keystone v5 will eventually grow into a robust platform just like its predecessor!

[jesstelford]

Thank you for the support @Neo-Zhixing 😊

We'll keep you up to date with the fix as we progress 👍

[jesstelford]

@Neo-Zhixing 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.