kalkun-sms / Kalkun

Open Source Web based SMS Manager
https://kalkun.sourceforge.io/
GNU General Public License v2.0
210 stars 131 forks source link

Fixed error while preparing sqlite db #482

Closed heaveaxy closed 1 year ago

heaveaxy commented 1 year ago

Fixed error while preparing sqlite database with INSERT INTO user_settings statement: sqlite shows error about count of values - table contains 10 but 9 values passed.

tenzap commented 1 year ago

Thanks for the pull request.

Looks like there is a difference in the DB schema between mysql and the 2 others.

Could you investigate this?

Also, if you change the things for one DB please apply the same change to the others as stated on the wiki's developer section: https://github.com/kalkun-sms/Kalkun/wiki/Developing#pull-request-checklist

heaveaxy commented 1 year ago

I checked installation with mariadb 10.9.3, db schema updated successfully without any changes in SQL-files. Postgres I have no possibility to test, I can prepare some tests if needed.

Also checked db schemas, I didn't find any differences. I think this issue is not schema-related:

INSERT INTO "user_settings" VALUES (1, 'green', 'false;--\nPut your signature here', 'false', 20, 'true;background.jpg', 'default', 'english', 'asc');

Sqlite fails query although early table defined with default value for field 'country_code'. When I add value of 'country_code' into INSERT statement, it completes without error.

The bug is in INSERT statement. As I understand from docs (https://www.sqlite.org/lang_insert.html), we must define values of all fields when we not defining fields names:

If the column-name list after table-name is omitted then the number of values inserted into each row must be the same as the number of columns in the table

i.e.

INSERT INTO 'tablename' VALUES(...);

in this case in "VALUES" must be defined values of all fields.

otherwise, if we making query as something following:

INSERT INTO "user_settings" (id_user, theme, signature, permanent_delete, paging, bg_image, delivery_report, language, conversation_sort) VALUES (1, 'green', 'false;--\nPut your signature here', 'false', 20, 'true;background.jpg', 'default', 'english', 'asc');

note: in this example field 'country_code' not defined in fields list, therefore it fills with it's default value according to table schema.

in this case DEFAULT statement works correctly.

tenzap commented 1 year ago

Thanks for the investigation.

I think we should then put the missing 'US' values in the INSERT statement of each database. Maintenance will be easier.

BTW, I noticed that there is also an issue with the signature field. The "\n" is not replaced by a "new line" anymore which is now expected. Could you correct this for both sqlite & mysql so that there really is a new line inserted into the DB instead of a "\n"?

I personally use PostgreSQL and the statement (including your fix) should be. So please use it when updating the PR.

INSERT INTO "user_settings" VALUES (1, 'green', 'false;--
Put your signature here', 'false', 20, 'true;background.jpg', 'default', 'english', 'asc', 'US');
heaveaxy commented 1 year ago

I unified SQL statement in all 3 SQL-files, replacing \n to real newline. Tested on sqlite and mariadb - works properly. Postgres unable to test, I think (hope) you right about this :) Also I saw difference in INSERT INTO 'user_settings' ... statement for mysql - it uses fields enumeration. Ithink it's good to use same form in all SQL-files - without filed names enumeration. Less code it's easier to understand.

tenzap commented 1 year ago

I unified SQL statement in all 3 SQL-files, replacing \n to real newline. Tested on sqlite and mariadb - works properly. Postgres unable to test, I think (hope) you right about this :) Also I saw difference in INSERT INTO 'user_settings' ... statement for mysql - it uses fields enumeration. Ithink it's good to use same form in all SQL-files - without filed names enumeration. Less code it's easier to understand.

Thanks. Ok for mysql syntax simplification. For postgres, I tested the line I gave you, so it's ok. Thanks for confirming you tested on mariadb/mysql & sqlite. :)