Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-10296

incorrect cross join in SQL Server

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.0.9
    • Fix Version/s: 3.0.10-RC1
    • Labels:
      None
    • Environment:
      PHP 5.1.2, SQL Server 2008, Any Browser

      Description

      Cross Joins are done using an approach like this:
      table_a, table_b
      but this generates an errore when there is also an inner join. This query, for example, is generated when trying to send a Mass Email to a group:

      SELECT u.user_email, u.username, u.username_clean, u.user_lang, u.user_jabber, u.user_notify_type FROM phpbb_users u, phpbb_user_group ug LEFT JOIN phpbb_banlist b ON (u.user_id = b.ban_userid) WHERE ug.group_id = 8 AND ug.user_pending = 0 AND u.user_id = ug.user_id AND u.user_allow_massemail = 1 AND u.user_type IN (0, 3) AND (b.ban_id IS NULL OR b.ban_exclude = 1) ORDER BY u.user_lang, u.user_notify_type

      and produces the following error:

      SQLSTATE: 42000 code: 4104 message: [Microsoft][SQL Server Native Client 10.0][SQL Server]The multi-part identifier "u.user_id" could not be bound. [4104]

      The problem can be solved using the "CROSS JOIN" keyword, instead of the comma, at row 612 of dbal.php file:

      $sql .= $this->_sql_custom_build('FROM', implode(' CROSS JOIN ', $table_array));

      After this change the query will be the following

      SELECT u.user_email, u.username, u.username_clean, u.user_lang, u.user_jabber, u.user_notify_type FROM phpbb_users u CROSS JOIN phpbb_user_group ug LEFT JOIN phpbb_banlist b ON (u.user_id = b.ban_userid) WHERE ug.group_id = 8 AND ug.user_pending = 0 AND u.user_id = ug.user_id AND u.user_allow_massemail = 1 AND u.user_type IN (0, 3) AND (b.ban_id IS NULL OR b.ban_exclude = 1) ORDER BY u.user_lang, u.user_notify_type

      and it will be correctly executed.

        Activity

        Hide
        Oleg Oleg [X] (Inactive) added a comment -

        Shouldn't this also affect mysql? Mysql recommends parenthesizing joined tables as follows: select * from (a, b) inner join c. Does this work for mssql?

        "In MySQL, CROSS JOIN is a syntactic equivalent to INNER JOIN (they can replace each other). In standard SQL, they are not equivalent. INNER JOIN is used with an ON clause, CROSS JOIN is used otherwise. "

        http://dev.mysql.com/doc/refman/5.0/en/join.html

        Show
        Oleg Oleg [X] (Inactive) added a comment - Shouldn't this also affect mysql? Mysql recommends parenthesizing joined tables as follows: select * from (a, b) inner join c. Does this work for mssql? "In MySQL, CROSS JOIN is a syntactic equivalent to INNER JOIN (they can replace each other). In standard SQL, they are not equivalent. INNER JOIN is used with an ON clause, CROSS JOIN is used otherwise. " http://dev.mysql.com/doc/refman/5.0/en/join.html
        Hide
        Noxwizard Patrick Webster added a comment -

        MySQL isn't affected because its _sql_custom_build() function parenthesizes the list. This only seems to affect MSSQL, PostgreSQL, and Oracle. A simple fix would be to swap around lines 88 and 89 of includes/acp/acp_email.php (https://github.com/phpbb/phpbb3/blob/develop-olympus/phpBB/includes/acp/acp_email.php#L88)

        Show
        Noxwizard Patrick Webster added a comment - MySQL isn't affected because its _sql_custom_build() function parenthesizes the list. This only seems to affect MSSQL, PostgreSQL, and Oracle. A simple fix would be to swap around lines 88 and 89 of includes/acp/acp_email.php ( https://github.com/phpbb/phpbb3/blob/develop-olympus/phpBB/includes/acp/acp_email.php#L88 )
        Hide
        Oleg Oleg [X] (Inactive) added a comment -

        Changing client code to work around a problem in sql generation by dbal would classify as an ugly hack rather than a fix.

        Looking at _sql_custom_build for mysql, writing a version that uses cross join instead of commas would at a minimum be ugly (as it would essentially have to do search and replace in sql) and potentially fragile/impossible (if from expressions can have entire subqueries). If we can simply parenthesize the entire expression as mysql does this would seem to me like the least invasive and a very reliable approach.

        Show
        Oleg Oleg [X] (Inactive) added a comment - Changing client code to work around a problem in sql generation by dbal would classify as an ugly hack rather than a fix. Looking at _sql_custom_build for mysql, writing a version that uses cross join instead of commas would at a minimum be ugly (as it would essentially have to do search and replace in sql) and potentially fragile/impossible (if from expressions can have entire subqueries). If we can simply parenthesize the entire expression as mysql does this would seem to me like the least invasive and a very reliable approach.
        Hide
        Oleg Oleg [X] (Inactive) added a comment -

        To the submitter, which page were you getting the error on?

        Show
        Oleg Oleg [X] (Inactive) added a comment - To the submitter, which page were you getting the error on?
        Hide
        dmauri dmauri added a comment -

        Hi Oleg. No, putting the tables that have to be cross-joined into parenthesis doesn't work. IMHO, since mysql doesn't follow the ANSI Standard, the query should be generated with the CROSS JOIN by default, switching to the comma-separated style only if the mysql driver has been choosen. This should be fairly easy to do, since the the only section that needs to be chaged is the _sql_custom_build function.

        Following this approach (but in the opposite way, just to make may installation works) I changed the code of the mssqslnative.php file in this way:

        function _sql_custom_build($stage, $data)
        {
        switch ($stage)

        { case 'FROM': $data = str_replace(",", " CROSS JOIN ", $data); break; }

        return $data;
        }

        and everything seems to work right now.

        > To the submitter, which page were you getting the error on?

        Mass Email

        Show
        dmauri dmauri added a comment - Hi Oleg. No, putting the tables that have to be cross-joined into parenthesis doesn't work. IMHO, since mysql doesn't follow the ANSI Standard, the query should be generated with the CROSS JOIN by default, switching to the comma-separated style only if the mysql driver has been choosen. This should be fairly easy to do, since the the only section that needs to be chaged is the _sql_custom_build function. Following this approach (but in the opposite way, just to make may installation works) I changed the code of the mssqslnative.php file in this way: function _sql_custom_build($stage, $data) { switch ($stage) { case 'FROM': $data = str_replace(",", " CROSS JOIN ", $data); break; } return $data; } and everything seems to work right now. > To the submitter, which page were you getting the error on? Mass Email
        Hide
        nickvergessen Joas Schilling added a comment -

        Please also add a test case to the unittests for this!

        Show
        nickvergessen Joas Schilling added a comment - Please also add a test case to the unittests for this!
        Hide
        nickvergessen Joas Schilling added a comment -

        Ticket author should be asked for his correct email before merging.

        Noxwizard tested Oracle and MSSQL (both fixed), I tested mysql and sqlite (both not affected) and postgres (fixed)

        Show
        nickvergessen Joas Schilling added a comment - Ticket author should be asked for his correct email before merging. Noxwizard tested Oracle and MSSQL (both fixed), I tested mysql and sqlite (both not affected) and postgres (fixed)

          People

          • Assignee:
            nickvergessen Joas Schilling
            Reporter:
            dmauri dmauri
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development