Doctrine 1
  1. Doctrine 1
  2. DC-521

Empty records cannot be inserted on PostgreSQL with autoincrement identifiers

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.2.1
    • Fix Version/s: 1.2.2
    • Component/s: Connection
    • Labels:
      None
    • Environment:
      Ubuntu 9.10
      PostgreSQL 8.4
      PHP 5.2.10-2ubuntu6.4
      Doctrine 1.2 stable branch (r7206)

      Description

      If a record does not have any values set, but its identifier is defined as autoincrement, the record cannot be inserted into PostgreSQL.

      In Doctrine_Connection_UnitOfWork::processSingleInsert():

       
             $fields = $record->getPrepared();
              $table = $record->getTable();
      
             // Populate fields with a blank array so that a blank records can be inserted
              if (empty($fields)) {
                  foreach ($table->getFieldNames() as $field) {
                      $fields[$field] = null;
                  }
              }
      

      This code snippet will add null values to all fields, including to the autoincrement identifier.
      Then later in Doctrine_Connection_Pgsql::insert():

             // fix #1786 and #2327 (default values when table is just 'id' as PK)
              if(count($fields) === 1 && $table->isIdentifier(key($fields)) && $table->isIdentifierAutoincrement() )
              {
                  return $this->exec('INSERT INTO ' . $this->quoteIdentifier($tableName)
                                    . ' '
                                    . ' VALUES (DEFAULT)');
              }
      
              foreach ($fields as $fieldName => $value) {
                  $cols[] = $this->quoteIdentifier($table->getColumnName($fieldName));
                  if ($value instanceof Doctrine_Expression) {
                      $a[] = $value->getSql();
                      unset($fields[$fieldName]);
                  } else {
                      $a[] = '?';
                  }
              }
      

      The claimed fix for the referenced tickets does not work correctly for 2 reasons:
      1. It only works for tables with a single id field
      2. It only works correctly if the id field had no value assigned. If there was a value assigned, it silently bypasses it, and inserts with the autoincremented value.

      Patch with test case will be attached to the ticket.

      References:
      http://trac.doctrine-project.org/ticket/2327
      http://trac.doctrine-project.org/ticket/1786

        Activity

        Hide
        Gergely Kis added a comment -

        The patch contains:

        • Fix for the empty insert issue in PostgreSQL
        • Fix in the UnitOfWork to handle autoincrement fields correctly, if an increment field value was explicitly specified (previously it queried the lastInsertId(), which fails for postgres, since it uses a sequence, which is not updated in case an id is explicitly inserted. (It might be a good idea to also move the postgres sequence in the Pgsql::insert() to the maximum inserted value to make sure that it correctly simulates autoincrement fields, and does not generate conflicting ids -> this is not done yet)
        • Add unit test
        • Add tearDown() support for the unit test framework
        • Fix 741 ticket test (it seems to have relied on a bug in the handling of autoincrement fields: basically a second record was inserted silently) The correct state is DIRTY instead of TDIRTY.
        • Put the new unit test onto the exclusion list because it interferes with the Query test (despite using tearDown() to reset the connection). It also relies explicitly on PostgreSQL.
        Show
        Gergely Kis added a comment - The patch contains: Fix for the empty insert issue in PostgreSQL Fix in the UnitOfWork to handle autoincrement fields correctly, if an increment field value was explicitly specified (previously it queried the lastInsertId(), which fails for postgres, since it uses a sequence, which is not updated in case an id is explicitly inserted. (It might be a good idea to also move the postgres sequence in the Pgsql::insert() to the maximum inserted value to make sure that it correctly simulates autoincrement fields, and does not generate conflicting ids -> this is not done yet) Add unit test Add tearDown() support for the unit test framework Fix 741 ticket test (it seems to have relied on a bug in the handling of autoincrement fields: basically a second record was inserted silently) The correct state is DIRTY instead of TDIRTY. Put the new unit test onto the exclusion list because it interferes with the Query test (despite using tearDown() to reset the connection). It also relies explicitly on PostgreSQL.
        Hide
        Gergely Kis added a comment -

        Increasing priority to critical because this is a major issue with the current 1.2 series.

        Show
        Gergely Kis added a comment - Increasing priority to critical because this is a major issue with the current 1.2 series.
        Hide
        Enrico Stahn added a comment -

        Environment: doctrine 1.2.4, symfony 1.4

        I moved our project from doctrine 1.2.1 to 1.2.4. The build process stops because of this patch. We are using primary keys with an alias. It seems that the generation of the sequence name in the build-process is different to the one used in UnitOfWorks.

        Example:

        Authority:
        columns:
        a_id:

        { name: a_id as id, type: integer, primary: true, autoincrement: true }

        name:

        { type: string }

        This will generate a sequence called "authority_a_id", but it will try no "currval" the sequence "authority_id".

        I'll try to provide a UnitTest. The current seems broken.

        php -l Ticket/DC521TestCase.php
        PHP Parse error: syntax error, unexpected $end, expecting T_FUNCTION in Ticket/DC521TestCase.php on line 143

        Parse error: syntax error, unexpected $end, expecting T_FUNCTION in Ticket/DC521TestCase.php on line 143
        Errors parsing Ticket/DC521TestCase.php

        Show
        Enrico Stahn added a comment - Environment: doctrine 1.2.4, symfony 1.4 I moved our project from doctrine 1.2.1 to 1.2.4. The build process stops because of this patch. We are using primary keys with an alias. It seems that the generation of the sequence name in the build-process is different to the one used in UnitOfWorks. Example: Authority: columns: a_id: { name: a_id as id, type: integer, primary: true, autoincrement: true } name: { type: string } This will generate a sequence called "authority_a_id", but it will try no "currval" the sequence "authority_id". I'll try to provide a UnitTest. The current seems broken. php -l Ticket/DC521TestCase.php PHP Parse error: syntax error, unexpected $end, expecting T_FUNCTION in Ticket/DC521TestCase.php on line 143 Parse error: syntax error, unexpected $end, expecting T_FUNCTION in Ticket/DC521TestCase.php on line 143 Errors parsing Ticket/DC521TestCase.php
        Hide
        Enrico Stahn added a comment -

        @see DC-747

        Show
        Enrico Stahn added a comment - @see DC-747

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            Gergely Kis
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: