Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

split-sql-query breaks a trigger into multiple queries #298

Open
mcollina opened this issue Jun 29, 2023 · 4 comments
Open

split-sql-query breaks a trigger into multiple queries #298

mcollina opened this issue Jun 29, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@mcollina
Copy link
Contributor

Consider the following code:

const split = require('@databases/split-sql-query');
const sql = require('@databases/sql');
const assert = require('assert');

const sqlString = `-- Add SQL in this file to create the database tables for your API
CREATE TABLE IF NOT EXISTS movies (
  \`id\` INTEGER PRIMARY KEY,
  \`key\` TEXT NOT NULL,
  \`title\` TEXT NOT NULL
);

CREATE TRIGGER \`insert_movie\`
BEFORE INSERT ON \`movies\`
FOR EACH ROW
BEGIN
  IF NOT NEW.\`key\` REGEXP '^[A-Za-z0-9\\\\-_]+$' THEN
    SIGNAL SQLSTATE '45000'
    SET MESSAGE_TEXT = 'Only ASCII characters, dashes, underscores and numbers are allowed as movie key.';
  END IF;
END;
`

const query = sql`${sql.__dangerous__rawValue(sqlString)}`;

const queries = split(query);

for (const query of queries) {
  console.log(query)
}

assert.equal(queries.length, 2);

Currently split-sql-query splits this into 4 queries:

SQLQuery {
  _cache: Map(0) {},
  _items: [
    {
      type: 0,
      text: '\n' +
        'CREATE TABLE IF NOT EXISTS movies (\n' +
        '  `id` INTEGER PRIMARY KEY,\n' +
        '  `key` TEXT NOT NULL,\n' +
        '  `title` TEXT NOT NULL\n' +
        ')'
    }
  ]
}
SQLQuery {
  _cache: Map(0) {},
  _items: [
    {
      type: 0,
      text: '\n' +
        '\n' +
        'CREATE TRIGGER `insert_movie`\n' +
        'BEFORE INSERT ON `movies`\n' +
        'FOR EACH ROW\n' +
        'BEGIN\n' +
        "  IF NOT NEW.`key` REGEXP '^[A-Za-z0-9\\\\-_]+$' THEN\n" +
        "    SIGNAL SQLSTATE '45000'\n" +
        "    SET MESSAGE_TEXT = 'Only ASCII characters, dashes, underscores and numbers are allowed as movie key.'"
    }
  ]
}
SQLQuery {
  _cache: Map(0) {},
  _items: [ { type: 0, text: '\n  END IF' } ]
}
SQLQuery { _cache: Map(0) {}, _items: [ { type: 0, text: '\nEND' } ] }

While it should only be two.

This was originally reported at platformatic/platformatic#932
@mcollina mcollina mentioned this issue Jun 29, 2023
Open
@mcollina mcollina changed the title split-sql-query breaks a trigger split-sql-query breaks a trigger into multiple queries Jun 29, 2023
@ForbesLindesay
Copy link
Owner

Which db engine would you be using this with?

@ForbesLindesay
Copy link
Owner

So it looks like this is a possible issue or MySQL and SQLite.

With MySQL they suggest adding an extra delimiter keyword: https://dev.mysql.com/doc/refman/8.0/en/trigger-syntax.html

With SQLite I can't see any mention of the potential problem here: https://www.sqlite.org/lang_createtrigger.html

For Postgres, there's not really an issue sine you only have one statement and so there are no extra ;s.

I think there are 3 options here:

  1. Add a delimiter keyword that would be processed and removed by the @databases/split-sql-query package. This requires extra work from anyone using the library, which isn't ideal.
  2. Attempt to automatically detect the start & end of the CREATE TRIGGER statement. This would be very case-by-case if there are other statements like this - e.g. creating procedures or functions.
  3. Add a new sql.statement or something that lets you mark some SQL as being all one statement that should not be split by @databases/split-sql-query. This would not work with the sql.file utility, so that's not a good enough solution on its own.

@mcollina
Copy link
Contributor Author

mcollina commented Jul 5, 2023

I think delimiter is the best way to proceed. It isn't ideal but it provides an escape hatch for people. Adding a full parser to detect the CREATE TRIGGER statement would be best but I can see the implementation would be very tricky considering how the module is written.

@ForbesLindesay ForbesLindesay added the bug Something isn't working label Jul 5, 2023
@EnriqueJMP-holcim
Copy link

Do you have any updates on this issue? I'm encountering errors when executing queries that involve "complex" create triggers, specifically those that include IF... ENDIF or DELIMITER statements. I'm using MySQL. Do you have any workaround? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mcollina @ForbesLindesay @EnriqueJMP-holcim