Migrations failure when using `unsafeExecuteSql` with terminating expression with semicolon

[nhanders]

We were experiencing a crash on app startup on a new app release which we hadn't picked up in testing. Upon inspecting the logs, the issue was found to be a syntax error in the generated migrations SQL.

What had caused this SQL error was not terminating the SQL provided to the unsafeExecuteSql with a semicolon. This meant that, when the migration was generated, the SQL statement had the unsafeExecuteSql migration step not terminated with a semicolon, causing a syntax error.

Here's an example:

const migrations = schemaMigrations({
  migrations: [
    {
      toVersion: 1,
      steps: [
        createTable({
          name: 'table_a',
          columns: [
            {
              name: 'column_a',
              type: 'string',
            },
          ],
        }),
      ],
    },
    {
      toVersion: 2,
      steps: [
        unsafeExecuteSql('DELETE FROM table_a'),
      ],
    },
    {
      toVersion: 3,
      steps: [
        createTable({
          name: 'table_b',
          columns: [
            {
              name: 'column_b',
              type: 'string',
            },
          ],
        }),
      ],
    },
  ],
});

When running migrations from v1 -> v2 or v2-v3, there are no issues since the generate migration SQL statement are just:

DELETE FROM table_a

CREATE TABLE table_b (
    column_b TEXT,
);

This is why we never picked this up in our testing.

However, when running migrations from v1 -> v3, a syntax error is thrown since the generated SQL is missing the termination semicolon in the first statement:

DELETE FROM ai_sizes_weekly_reports -- <-- missing semicolon
CREATE TABLE table_b (
    column_b TEXT,
); -- <-- end of statement

(not exactly what is generated, but it's the gist)

I'm creating this issue for two reasons:

1. to warn fellow application programmers using this library about this behaviour of the API
2. to propose a solution

I know the method used is rightfully named unsafeExecuteSql, but I still think programmers could be protected from this issue. Two simple ideas come to mind:

1. Add validation to the sql passed into unsafeExecuteSql to ensure it ends in a semicolon. Nice and simple.
2. Add validation to the migrations steps to ensure each step ends in a semicolon. Also should be nice and simple.

I'm happy to implement them, just looking if anyone out there has any feedback or better solutions.

[radex]

Good catch -- I think validation makes sense, especially that running only this migration will seem like it works, and it only breaks when combined with other statements. I'll look into it.

[radex]

@nhanders Fixed in https://github.com/Nozbe/WatermelonDB/pull/1812