Doctrine 1
  1. Doctrine 1
  2. DC-727

ReOpen DC-46 - Unexpected behavior with whereIn() and empty array

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.2.1, 1.2.2, 1.2.3
    • Fix Version/s: None
    • Component/s: Query
    • Labels:
      None
    • Environment:

      Description

      I reopen the DC-46 as it'seems not fix at all.
      When I do a whereIn with empty array, the condition is just drop and I get no Exception.

      Here is a simple test case:

      require_once('doctrine/lib/Doctrine.php');
      spl_autoload_register(array('Doctrine', 'autoload'));
      $manager = Doctrine_Manager::getInstance();
      $manager->setAttribute(Doctrine::ATTR_EXPORT, Doctrine::EXPORT_ALL);
      $conn = Doctrine_Manager::connection('mysql://root:root@localhost/test_doctrine');
      echo "Connection is set up\n";
      
      class Record extends Doctrine_Record {
          public function setTableDefinition(){
              $this->setTableName('record');
          }
      }
      
      // Create the db
      try {Doctrine::dropDatabases();}catch(Exception $e){} // Drop if exist :-)
      Doctrine::createDatabases();Doctrine::createTablesFromArray(array('Record'));
      
      // Test
      echo Doctrine::getTable('Record')->createQuery()->select('id')->whereIn('id', array())->getSqlQuery() , "\n";
      echo Doctrine::getTable('Record')->createQuery()->select('id')->whereIn('id', array())->fetchArray() , "\n";
      
      // Result is:
      // SELECT r.id AS r__id FROM record r
      // Array
      
      1. DC-727_refactored.patch
        2 kB
        Guilliam X
      2. DC-727_with_duplicates.patch
        2 kB
        Guilliam X
      3. DC727TestCase_more_specific.php
        1 kB
        Guilliam X
      4. DC727TestCase.php
        1 kB
        Guilliam X

        Activity

        David Jeanmonod created issue -
        David Jeanmonod made changes -
        Field Original Value New Value
        Affects Version/s 1.2.2 [ 10047 ]
        Environment PHP 5.3.1 (cli) (built: Feb 11 2010 02:32:22)
        mysql Ver 14.14 Distrib 5.1.41, for apple-darwin9.5.0 (i386) using readline 5.1
        Doctrine version 1.2.2 from SVN: http://doctrine.mirror.svn.symfony-project.com/tags/1.2.2/lib/Doctrine.php
        Issue Type Improvement [ 4 ] Bug [ 1 ]
        Description See trac #2064 ( http://trac.doctrine-project.org/ticket/2064 )

        Jwage wrote "This is expected behavior. We cannot and will not add a fake invalid condition to the where string. I think we need to make it throw an exception if you try and pass an empty array."

        I agree that an exception should be thrown.

        But AFAIK this hasn't been implemented yet.

        The issue is quite urgent in my opinion. Example:

        I had a query where I absolutely had to restrict the result set to allowed objects (IDs) only. I used a whereIn() condition for this restriction.

        Problem is: when the user wasn't assigned to any groups, the id-array was empty, and Doctrine removed the WHERE ... IN completely! That resulted in a result set containing all records!

        I would expect
        - either to return no records (correct behavior could be to add "0=1" as a replacement for "... IN ()" expression)
        - or let the database decide (= submitting the query with an empty list which would result - at least for MySQL - in a database exception)
        - or Doctrine could throw an exception

        Would IMO be way better (as its IMO a more expected behavior) than simply not adding the "... IN ()" to the query.

        I reopen the DC-46 as it'seems not fix at all.
        When I do a whereIn with empty array, the condition is just drop and I get no Exception.

        Here is a simple test case:

        {code}
        require_once('doctrine/lib/Doctrine.php');
        spl_autoload_register(array('Doctrine', 'autoload'));
        $manager = Doctrine_Manager::getInstance();
        $manager->setAttribute(Doctrine::ATTR_EXPORT, Doctrine::EXPORT_ALL);
        $conn = Doctrine_Manager::connection('mysql://root:root@localhost/test_doctrine');
        echo "Connection is set up\n";

        class Record extends Doctrine_Record {
            public function setTableDefinition(){
                $this->setTableName('record');
            }
        }

        // Create the db
        try {Doctrine::dropDatabases();}catch(Exception $e){} // Drop if exist :-)
        Doctrine::createDatabases();Doctrine::createTablesFromArray(array('Record'));

        // Test
        echo Doctrine::getTable('Record')->createQuery()->select('id')->whereIn('id', array())->getSqlQuery() , "\n";
        echo Doctrine::getTable('Record')->createQuery()->select('id')->whereIn('id', array())->fetchArray() , "\n";

        // Result is:
        // SELECT r.id AS r__id FROM record r
        // Array
        {code}
        Hide
        Guilliam X added a comment -

        The problem is that the change for "new" behavior (throw exception instead of return unchanged query) was only done in _processWhereIn() but not cascaded to andWhereIn() and orWhereIn() (another example we should avoid code duplication).
        The patch is simple, but it causes Doctrine_Ticket_1558_TestCase to fail. Indeed that (old) test expects the "old" behavior (return unchanged query, don't throw exception)... So the 2 fixes are incompatible, you'll have to choose :/

        I still attach a new test case and 2 versions of a patch (the first one just applies changes of _processWhereIn also to the 2 other functions but adds more duplicate code, the second is a little refactored and seems better to me).

        Regards

        Show
        Guilliam X added a comment - The problem is that the change for "new" behavior (throw exception instead of return unchanged query) was only done in _ processWhereIn() but not cascaded to andWhereIn() and orWhereIn() (another example we should avoid code duplication). The patch is simple, but it causes Doctrine_Ticket_1558_TestCase to fail. Indeed that (old) test expects the "old" behavior (return unchanged query, don't throw exception)... So the 2 fixes are incompatible, you'll have to choose :/ I still attach a new test case and 2 versions of a patch (the first one just applies changes of _processWhereIn also to the 2 other functions but adds more duplicate code, the second is a little refactored and seems better to me). Regards
        Guilliam X made changes -
        Attachment DC727TestCase.php [ 10657 ]
        Attachment DC-727_with_duplicates.patch [ 10658 ]
        Attachment DC-727_refactored.patch [ 10659 ]
        Hide
        Guilliam X added a comment -

        added a more specific test case (expects Doctrine_Query_Exception instead of simple Exception)

        Show
        Guilliam X added a comment - added a more specific test case (expects Doctrine_Query_Exception instead of simple Exception)
        Guilliam X made changes -
        Attachment DC727TestCase_more_specific.php [ 10660 ]
        Roman S. Borschel made changes -
        Assignee Roman S. Borschel [ romanb ] Jonathan H. Wage [ jwage ]
        Hide
        Jan Schütze added a comment -

        Hi,

        the issue is still present in 1.2.3. Are there any plans to apply it?

        Kind regards,

        Show
        Jan Schütze added a comment - Hi, the issue is still present in 1.2.3. Are there any plans to apply it? Kind regards,
        Jan Schütze made changes -
        Affects Version/s 1.2.3 [ 10051 ]
        Affects Version/s 1.2.1 [ 10044 ]
        Hide
        Jim Persson added a comment -

        I would like to add that this also applies to delete-queries which can cause serious data loss.
        We had a case where a table of serialized data was completely emptied which caused a database cascade that deleted quite a lot of data.

        Show
        Jim Persson added a comment - I would like to add that this also applies to delete-queries which can cause serious data loss. We had a case where a table of serialized data was completely emptied which caused a database cascade that deleted quite a lot of data.

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DC-727, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            David Jeanmonod
          • Votes:
            3 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated: