Bug: No DBPrefix or backticks inside SQL functions in QueryBuilder

[objecttothis]

PHP Version

8.2

CodeIgniter4 Version

4.5.1

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

apache

Database

MySQL 8.2.0

What happened?

Backticks and DBPrefix are missing on SQL generated by QueryBuilder function calls when SQL functions are in the parameters.

Steps to Reproduce

This code

$builder = $this->db->table('attribute_links');
$builder->join('attribute_values', 'attribute_values.attribute_id = attribute_links.attribute_id', 'inner');
$builder->set('attribute_links.attribute_id', "IF((attribute_values.attribute_value IN('false','0','') OR (attribute_values.attribute_value IS NULL)), $checkbox_attribute_values[0], $checkbox_attribute_values[1])", false);
$builder->where('attribute_links.definition_id', $definition_id);
log_message('error', $builder->getCompiledUpdate(false));

Yields

UPDATE `ospos_attribute_links` SET `ospos_attribute_links`.`attribute_id` = IF((attribute_values.attribute_value IN('false','0','') OR (attribute_values.attribute_value IS NULL)), 45873, 25595)
WHERE `ospos_attribute_links`.`definition_id` = 6

Note the missing backticks and dbprefix prepended to the attribute_values table and attribute value column inside the IF()

Expected Output

The missing join is a completely different issue and I've already submitted a feature request for that, but this affects ISNULL(), IF(), DATE() and CONCAT() that I've seen.

UPDATE `ospos_attribute_links`
JOIN `ospos_attribute_values` ON `ospos_attribute_values`.`attribute_id` = `ospos_attribute_links`.`attribute_id` 
SET `ospos_attribute_links`.`attribute_id` = IF((`ospos_attribute_values`.`attribute_value` IN('false','0','') OR (`ospos_attribute_values`.`attribute_value` IS NULL)), 45873, 25595) 
WHERE `ospos_attribute_links`.`definition_id` = 6

Anything else?

See https://forum.codeigniter.com/showthread.php?tid=90798 for more examples of what I mean.

[objecttothis]

In order to get the expected output (minus the missing JOIN) I have to change the code to this:

$db_prefix = $this->db->getPrefix();
$builder = $this->db->table('attribute_links');
$builder->join('attribute_values', 'attribute_values.attribute_id = attribute_links.attribute_id', 'inner');
$builder->set('attribute_links.attribute_id', "IF((`$db_prefix" . "attribute_values`.`attribute_value` IN('false','0','') OR (`". $db_prefix ."attribute_values`.`attribute_value` IS NULL)), $checkbox_attribute_values[0], $checkbox_attribute_values[1])", false);
$builder->where('attribute_links.definition_id', $definition_id);
log_message('error', $builder->getCompiledUpdate(false));

[kenjis]

I don't think Query Builder supports such complicated queries. https://codeigniter4.github.io/CodeIgniter4/database/query_builder.html#builder-set

[objecttothis]

I don't think Query Builder supports such complicated queries. https://codeigniter4.github.io/CodeIgniter4/database/query_builder.html#builder-set

Can you clarify what you mean by complicated in this query? I understand the JOIN is not supported in the UPDATE, but this issue occurs outside of things like that.

[kenjis]

I mean the second parameter is complicated.

$builder->set(
    'attribute_links.attribute_id', 
    "IF((attribute_values.attribute_value IN('false','0','') OR (attribute_values.attribute_value IS NULL)), $checkbox_attribute_values[0], $checkbox_attribute_values[1])", 
    false
);

[objecttothis]

OK, here is a simpler example.

This code:

$builder = $this->db->table('sales');
$builder->select('AVG(sales_items.discount)');
$selectQuery = $builder->getCompiledSelect();

Produces

SELECT AVG(sales_items.discount)
FROM `ospos_sales`

Surely this isn't too complicated and it points out that anything inside SQL functions (AVG in this case) is not getting db_prefix nor backticks.

[objecttothis]

The issue is not just SQL functions but parenthesis in general. If I add parenthesis to the where() call (such as might be done using the OR or AND operators to indicate order of operations) the same thing happens.

[neznaika0]

When we specify the exact argument for the function (for example,$qb->table($table)), it is very easy to understand that it is possible to add a prefix here.

Well, how do you see the solution to the problem? How does the script understand that a prefix is needed in custom queries?

Temporary fix: You can specify aliases (_attributevalues AS t1) for tables and use fewer $db_prefix glues.

[objecttothis]

My initial thought was to parse inside SQL functions. For example, since AVG() expects a column or table.column, to assume that when you come across that in the Select() function that you need to prepend the db_prefix, but this can get complicated since SQL allows subqueries.

I think one potential way to easily identify and deal with table.column references would be for CI4 to store the current schema in memory.

SELECT TABLE_NAME AS `Table`,
       COLUMN_NAME AS `Column`
FROM INFORMATION_SCHEMA.COLUMNS
WHERE TABLE_SCHEMA = 'foo'
ORDER BY TABLE_NAME, ORDINAL_POSITION;

The query takes nothing to run. Strip the db_prefix from the table names and you now have a list tables which need to have db_prefix prepended and tables and columns which need to have backticks added. Take that and any time you come across foo.bar format in calls to select(), where(), etc, you have a formula for what to escape.

Perhaps I'm missing something here, but maybe it gives someone an idea.

[objecttothis]

The sticky spot is going to be not just replacing everything that matches a table or column name since it's very likely for someone to have a table or column name that may match search criteria in a Where() call for example. I think this can be overcome by either only replacing and adding back ticks when table.column format is encountered or replacing them in context of what is being passed.

[neznaika0]

I would write such complex queries directly.

[objecttothis]

I would write such complex queries directly.

To clarify, you're saying this counts as complex?

$builder = $this->db->table('sales');
$builder->select('AVG(sales_items.discount)');
$selectQuery = $builder->getCompiledSelect();

[neznaika0]

Yes. Anything with inaccurate parameters can produce unpredictable results. For basic tasks, there are prepared methods $builder->selectAvg('age'); https://codeigniter4.github.io/userguide/database/query_builder.html#builder-selectavg

[kenjis]

The issue is not just SQL functions but parenthesis in general. If I add parenthesis to the where() call (such as might be done using the OR or AND operators to indicate order of operations) the same thing happens.

Yes. I don't know why, but that is the current specification. Simply put, Query Builder does not support SQL functions or ().

[kenjis]

I don't think this issue is easy to fix. If someone can fix the current behavior without breaking the existing apps and compromising performance, please send a PR to the 4.6 branch.

[michalsn]

I don't think it can be fixed. To handle complex SQL, we would have to add support for more methods to generate the appropriate SQL instead of trying to analyze the string with regex.

[objecttothis]

I don't think it can be fixed. To handle complex SQL, we would have to add support for more methods to generate the appropriate SQL instead of trying to analyze the string with regex.

Understood. Adding support for JOIN inside an UPDATE and a QueryBuilder function to handle IF() statements would go a long way in handling my initial code example. As @neznaika0 pointed out, there's already a function for AVG()