QUAL: Add sqlfluff (SQL code and style check)

[mdeweerd]

QUAL: Add sqlfluff (SQL code and style check)

This adds a validity and style check on the .sql files. The same tool can be used to fix style (which can be set up as a pre-commit hook).

Curerntly there is an issue in sqlfluff as it does not accept empty "--" comments.

It also notifies that 'rowid' is a keyword. It may be a biter overzealous there. The message can be avoided by quoting the rowid field in backticks ("rowid").

[mdeweerd]

@eldy

adding primary key on multiple columns:

sqlfluff does not accept (parse) a line like this:

ALTER TABLE example ADD PRIMARY KEY pk_example (fk_table1, fk_table2);

but it accepts

ALTER TABLE table0 ADD CONSTRAINT pk_example PRIMARY KEY (fk_table1, fk_table2);

Which matches an example in:
https://www.w3schools.com/sql/sql_primarykey.ASP#midcontentadcontainer

Such a case can be found in htdocs/install/mysql/migration/3.1.0-3.2.0.sql on line 160:

ALTER TABLE llx_categorie_fournisseur ADD PRIMARY KEY pk_categorie_fournisseur (fk_categorie, fk_societe);

I did not examine mysql's documentation on this.

Something that I should try to get parsed by sqlfluff or something that should be updated in dolibarr ?

Use of rowid

rowid is considered a keyword and it is also colored in my editor:

A solution is to quote the field `rowid`

Ignore the sqlfluf notice or update the sql files to quote the rowid field name.

EDIT:
According to the BNF on this page: https://mariadb.com/kb/en/alter-table/ , PRIMARY KEY can be followed by the index type, not an index name. And index_type is USING {BTREE | HASH | RTREE} so the sql statement does not match the BNF definition (which means that sqlfluff rightfully fails to parse the statement).

[eldy]

May be we can exclude files into dev/initdemo/ to avoid errors
"Length of file 'dev/initdemo/mysqldump_dolibarr_19.0.0.sql' is 5198834 bytes which is over the limit...

We don't mind scanning such files mysqldump as they are no code written by user but generated by mysql dump.

[eldy]

According to the BNF on this page: https://mariadb.com/kb/en/alter-table/ , PRIMARY KEY can be followed by the index type, not an index name. And index_type is USING {BTREE | HASH | RTREE} so the sql statement does not match the BNF definition (which means that sqlfluff rightfully fails to parse the statement).

Yes, the SQL has a wrong syntax. Howerver, it works correctly. As such sql files were provided on old version (and were working correctly), it is not a good idea to modify files tagged on old past version.
Can we exclude files into /install/mysql/migration ?

[mdeweerd]

May be we can exclude files into dev/initdemo/ to avoid errors
These are warnings, but I can add them to the exclude list to avoid the notifications.

Yes, the SQL has a wrong syntax. Howerver, it works correctly. As such sql files were provided on old version (and were working correctly), it is not a good to modify files tagged on old past version.

They are still executed on more recent versions of mysql in theory they should be updated - I think that the syntax is also wrong for the old sql versions.

However to avoid the work of modifying them, they can be excluded.

[mdeweerd]

@eldy
I ignored the sql files that were either too bit or resulted in notifications.

I propose to:

• Run this only on changed SQL files in PR's;
• Run it on all SQL files in develop and mainstream versions.

[mdeweerd]

The CI will run sqlfluff on all enabled files in the main branches, only on changed files in pull requets.