GNOME Bugzilla – Bug 785983
gom_command_builder_build_insert does not wrap table name in single-quotes
Last modified: 2017-08-09 11:21:14 UTC
Created attachment 357170 [details] [review] quote table name during insert STEPS TO REPRODUCE 1. Create a GomResource subclass 2. In the class_init, call gom_resource_class_set_table, and set the table name to a SQLite keyword (such as insert, set, update) EXPECTED RESULTS The SQL INSERT command to succeed. ACTUAL RESULTS The SQL INSERT fails with sqlite3_prepare_v2 failing. NOTES This can be reproduced by using the test-gom-insert.c test, changing the table name of ItemResource to 'insert', and running test-gom-insert. This is fixed by the attached patch.
Review of attachment 357170 [details] [review]: A separate patch to show the bug (the same one as in the description) would also be appreciated. ::: gom/gom-command-builder.c @@ +736,2 @@ str = g_string_new("INSERT INTO "); + g_string_append_printf(str, "'%s' (", klass->table); The table name should have already been transformed into a unique not-clashing name at this point. I'd make gom_resource_class_set_table() fail if the name is a reserved one.
Created attachment 357225 [details] [review] patch to modify test-gom-insert to use a failing table name Quick patch to cause the test-gom-insert test to fail with a reserved word table name ("insert").
(In reply to Bastien Nocera from comment #1) > Review of attachment 357170 [details] [review] [review]: > > A separate patch to show the bug (the same one as in the description) would > also be appreciated. Attached in comment #2. > ::: gom/gom-command-builder.c > @@ +736,2 @@ > str = g_string_new("INSERT INTO "); > + g_string_append_printf(str, "'%s' (", klass->table); > > The table name should have already been transformed into a unique > not-clashing name at this point. Unless I missed it, I briefly traced through gom_resource_class_set_table_name and gom_command_builder_build_insert. I didn't see anywhere that the table class variable was modified. It's run through snprintf() in _set_table_name, but it's not modified in any way. g_snprintf(resource_class->table, sizeof(resource_class->table), "%s", table); > I'd make gom_resource_class_set_table() fail if the name is a reserved one. I thought about this too. SQLite declares a list of reserved keywords[1]. They still recommend quoting English words even when unnecessary. I think perhaps this patch, plus an error check earlier in _set_table_name would be the best approach. > SQLite adds new keywords from time to time when it takes on new features. So to prevent your code from being broken by future enhancements, you should normally quote any identifier that is an English language word, even if you do not have to. 1: http://sqlite.org/lang_keywords.html
Created attachment 357252 [details] [review] tests: Add test for invalid table names
Created attachment 357253 [details] [review] gom: Add a private header to list reserved keywords
Created attachment 357254 [details] [review] gom: Verify whether a table name is valid before setting it
Created attachment 357255 [details] [review] tests: And check for assertion due to invalid table name The old test would simply crash when attempting to use a resource with the invalid table name for the first time. With the validity checks, we now assert much earlier. Adapt the test to succeed if the check fails as expected.
The table name must now not be a reserved keyword, or the program will assert() (as it's a programmer error). Thanks for the report! Attachment 357252 [details] pushed as 4ce0cb6 - tests: Add test for invalid table names Attachment 357253 [details] pushed as 1df4aa6 - gom: Add a private header to list reserved keywords Attachment 357254 [details] pushed as 8e17777 - gom: Verify whether a table name is valid before setting it Attachment 357255 [details] pushed as 7cb346a - tests: And check for assertion due to invalid table name