Invalid queries with modr8 module from hook_db_rewrite_sql

This looks like an error in the use of hook_rewrite_sql or db_rewrite_sql - so either mine or Views.

Did the API changed for db_rewrite_sql()?

try this patch, but I think Views might be violating the normal usage...

ok - though this patch might still be a good idea.

Hi,

I am getting a similar error with Views 2.2 and modr8 1.0:

user warning: Column 'nid' in field list is ambiguous query: SELECT COUNT(*) FROM (SELECT nid FROM node node LEFT JOIN node n ON node.nid = n.nid WHERE ((n.moderate != 1 OR n.uid = 5)) AND ( (node.status <> 0 OR node.uid = 5 or 0 = 1) AND (node.type in ('video')) )) count_alias in /var/www/drupal-6.9/sites/all/modules/views/includes/view.inc on line 699.

user warning: Column 'nid' in field list is ambiguous query: SELECT nid, node.title AS node_title FROM node node LEFT JOIN node n ON node.nid = n.nid WHERE ((n.moderate != 1 OR n.uid = 5)) AND ( (node.status <> 0 OR node.uid = 5 or 0 = 1) AND (node.type in ('video')) )ORDER BY node_title ASC LIMIT 0, 10 in /var/www/drupal-6.9/sites/all/modules/views/includes/view.inc on line 725.

This is due to the SQL code that modr8 adds in modr8_db_rewrite_sql(). It introduces a second nid field and Views only SELECTs "nid" and not "node.nid".

Should this be reported to the Views module team?

Looks again like this is a views problem.

LEFT JOIN node n ON node.nid = n.nid

That JOIN is both not right and not Views.

I do note that I remember something about some field added by some module that added a 'nid' without a table, but Views itself doesn't do this, that I know of. If you've got an unadulterated query where Views does this I'll happily fix it, but I'd need to see a view.

@merlinofchaos - the error is being cause by this part of the query:

SELECT nid FROM {node} node which it seems should be SELECT node.nid FROM {node} node

but perhaps the bigger problem is that views is using 'node' as the alias for {node} when db_rewrite_sql() expects 'n' as the alias afaik.

I can confirm this is still happening and I don't think it's Views, it modr8 doing something in hook_db_rewrite_sql()

For me, the broken query is:

Message You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'GROUP BY n.nid HAVING count(n.nid)>='1') ORDER BY n.title' at line 1 query: SELECT DISTINCT n.title, n.nid FROM drupal_node n WHERE ((n.moderate != 1)) AND ( n.type='book' AND n.status = 1 AND n.nid != '228' AND n.nid IN (select n.nid FROM drupal_node n INNER JOIN drupal_term_node tn ON n.vid = tn.vid WHERE tn.tid IN (8) )GROUP BY n.nid HAVING count(n.nid)>='1') ORDER BY n.title in /etc/drupal/all/themes/pixture_reloaded/node-book.tpl.php on line 220. 

The ((n.moderate != 1)) part is being injected.

Please review, it's clobbering my logs with errors so I need to disable the module. Raising to critical.

Cheers,

Miles

Renaming title.

I've posted a similar issue for a couple of other modules, as well as for the i18n module (another module which implements hook_db_rewrite_sql()) but so far haven't seen a response from any of the module owners. As I'm currently working on a site for a well known international organization which requires multi-language support this is a fairly critical issue for me, so I've looked into it fairly deeply.

First, db_rewrite_sql() does not "expect" n as an alias. It will, however, default to 'n' for the primary table name (and 'nid' for the primary field) if those arguments are not provided in the function call, and that's part of what causes the confusion.

When db_rewrite_sql() runs, the first thing it does is get a list of modules which implement the hook (using module_implements('db_rewrite_sql')) then does a module_invoke for each of those modules. That, in of itself, is error prone because a sort parameter is not provided and even if it was, the order in which each module rewrites the query can't be determined, and unless a module which calls module_implements() directly, it won't know what other modules which may be enabled on the site are going to rewrite the query. When db_rewrite_sql() executes the module invoke for each module it's just going to pass the arguments to the hook implementation, and what makes it confusing is that it sets $primary_table to 'n', but 'n' is not a table name. It's an alias for a table name. In the case, of the i18n module it will only rewrite queries if the $primary_table variable is 'node' or 'n', which means if you don't provide that argument when you make the db_rewrite_sql() function call, it's still going to rewrite the query with the assumption that the primary table name was 'n'. It the case of the i18n module, it adds a WHERE clause to the query (i.e WHERE n.language='en'....). If the original query did not specify n as an alias, the query is going to fail. If if it does specify n as an alias, if the table it's referencing does not contain a language column, the query is going to fail. I had thought that the i18n module could just check to see if the language column existed before trying to rewrite the query but that's not going to work either. The database.inc provides a db_column_exists($table_name, $column) function to test for the existence of a column name within a table. However, it will produce an error if the table name does not exist (which, it would almost always do, because there wouldn't be a table called 'n').

The db_rewrite_sql() function also sets the $primary_field variable to 'nid'. In the "node_db_rewrite_sql()" hook implementation it tests if $primary_field == 'nid' (which would be true if you let db_rewrite_sql() set it to the default value) and that implementation would rewrite the query. If your table doesn't contain a nid field, the query will fail.

Since the modr8_db_rewrite_sql() function tests if the $primary_field == 'nid' (so that it only deals with node objects) it may still try to rewrite a query on a table which is *not* a node object, merely because the db_rewrite_sql() function sets that variable to 'nid' as a default.

To me, the problem lies with the db_rewrite_sql() API documentation for not clearly defining what the requirements are when calling the function. There is also a chicken-egg issue in that a module developer can't possibly know what other modules will be enabled on a site which implements the db_rewrite_sql hook, and a hook implementation can only know so much about query that's asked to rewrite. When the db_rewrite_sql() function is setting some default values that might not make sense for the original query, you just asking for trouble.

Similar error here with modr8 and aggregator; here the problem is a missing join rather than a superfluous one:

Unknown column 'n.moderate' in 'where clause' query:
SELECT aggregator_item.iid AS iid, aggregator_item.link AS aggregator_item_link, aggregator_item.title AS aggregator_item_title, aggregator_item.timestamp AS aggregator_item_timestamp
  FROM aggregator_item aggregator_item
  LEFT JOIN aggregator_feed aggregator_feed ON aggregator_item.fid = aggregator_feed.fid
  WHERE ((n.moderate != 1)) AND ( aggregator_feed.fid = 2 )
  ORDER BY aggregator_item_timestamp DESC
in /snip/drupal-6.19/includes/common.inc(1695) : eval()'d code on line 9.

(Line breaks added for readability.)

drupal 6.19
modr8-6.x-1.3

Looking at function modr8_db_rewrite_sql, it seems that for this query $primary_table is 'n', hence "LEFT JOIN {node} modr_n..." is not added and WHERE clause "(n.moderate != 1)" is. Whether the bug here lies with modr8 or aggregator I don't know.

Dave

With version 1.3 of this module simply using "if ($primary_table != 'n' && $primary_table != 'node') {" on line 220 of modr8.module prevents an extra join to the node table if the $primary_table variable does not equal node. This issue occurs because views does not pass in $primary_table as the alias "n" but instead passes in the $primary_table variable as "node".

Sorry, this is the correct patch from my message above.