Merge pull request #5994 from cakephp/composite-key-insert

Fix composite primary key support when inserting data.

src/Database/Schema/PostgresSchema.php

@@ -40,17 +40,17 @@ class PostgresSchema extends BaseSchema
     {
         $sql =
         'SELECT DISTINCT table_schema AS schema, column_name AS name, data_type AS type,
-			is_nullable AS null, column_default AS default,
-			character_maximum_length AS char_length,
-			d.description as comment,
-			ordinal_position
-		FROM information_schema.columns c
-		INNER JOIN pg_catalog.pg_namespace ns ON (ns.nspname = table_schema)
-		INNER JOIN pg_catalog.pg_class cl ON (cl.relnamespace = ns.oid AND cl.relname = table_name)
-		LEFT JOIN pg_catalog.pg_index i ON (i.indrelid = cl.oid AND i.indkey[0] = c.ordinal_position)
-		LEFT JOIN pg_catalog.pg_description d on (cl.oid = d.objoid AND d.objsubid = c.ordinal_position)
-		WHERE table_name = ? AND table_schema = ? AND table_catalog = ?
-		ORDER BY ordinal_position';
+            is_nullable AS null, column_default AS default,
+            character_maximum_length AS char_length,
+            d.description as comment,
+            ordinal_position
+        FROM information_schema.columns c
+        INNER JOIN pg_catalog.pg_namespace ns ON (ns.nspname = table_schema)
+        INNER JOIN pg_catalog.pg_class cl ON (cl.relnamespace = ns.oid AND cl.relname = table_name)
+        LEFT JOIN pg_catalog.pg_index i ON (i.indrelid = cl.oid AND i.indkey[0] = c.ordinal_position)
+        LEFT JOIN pg_catalog.pg_description d on (cl.oid = d.objoid AND d.objsubid = c.ordinal_position)
+        WHERE table_name = ? AND table_schema = ? AND table_catalog = ?
+        ORDER BY ordinal_position';
 
         $schema = empty($config['schema']) ? 'public' : $config['schema'];
         return [$sql, [$tableName, $schema, $config['database']]];
@@ -139,6 +139,10 @@ class PostgresSchema extends BaseSchema
                 $row['default'] = 0;
             }
         }
+        // Sniff out serial types.
+        if (in_array($field['type'], ['integer', 'biginteger']) && strpos($row['default'], 'nextval(') === 0) {
+            $field['autoIncrement'] = true;
+        }
         $field += [
             'default' => $this->_defaultValue($row['default']),
             'null' => $row['null'] === 'YES' ? true : false,
@@ -181,24 +185,24 @@ class PostgresSchema extends BaseSchema
     public function describeIndexSql($tableName, $config)
     {
         $sql = 'SELECT
-			c2.relname,
-			i.indisprimary,
-			i.indisunique,
-			i.indisvalid,
-			pg_catalog.pg_get_indexdef(i.indexrelid, 0, true) AS statement
-		FROM pg_catalog.pg_class AS c,
-			pg_catalog.pg_class AS c2,
-			pg_catalog.pg_index AS i
-		WHERE c.oid  = (
-			SELECT c.oid
-			FROM pg_catalog.pg_class c
-			LEFT JOIN pg_catalog.pg_namespace AS n ON n.oid = c.relnamespace
-			WHERE c.relname = ?
-				AND n.nspname = ?
-		)
-		AND c.oid = i.indrelid
-		AND i.indexrelid = c2.oid
-		ORDER BY i.indisprimary DESC, i.indisunique DESC, c2.relname';
+            c2.relname,
+            i.indisprimary,
+            i.indisunique,
+            i.indisvalid,
+            pg_catalog.pg_get_indexdef(i.indexrelid, 0, true) AS statement
+        FROM pg_catalog.pg_class AS c,
+            pg_catalog.pg_class AS c2,
+            pg_catalog.pg_index AS i
+        WHERE c.oid  = (
+            SELECT c.oid
+            FROM pg_catalog.pg_class c
+            LEFT JOIN pg_catalog.pg_namespace AS n ON n.oid = c.relnamespace
+            WHERE c.relname = ?
+                AND n.nspname = ?
+        )
+        AND c.oid = i.indrelid
+        AND i.indexrelid = c2.oid
+        ORDER BY i.indisprimary DESC, i.indisunique DESC, c2.relname';
 
         $schema = 'public';
         if (!empty($config['schema'])) {
@@ -231,9 +235,10 @@ class PostgresSchema extends BaseSchema
             // If there is only one column in the primary key and it is integery,
             // make it autoincrement.
             $columnDef = $table->column($columns[0]);
-            if (count($columns) === 1 &&
-                in_array($columnDef['type'], ['integer', 'biginteger']) &&
-                $type === Table::CONSTRAINT_PRIMARY
+
+            if ($type === Table::CONSTRAINT_PRIMARY &&
+                count($columns) === 1 &&
+                in_array($columnDef['type'], ['integer', 'biginteger'])
             ) {
                 $columnDef['autoIncrement'] = true;
                 $table->addColumn($columns[0], $columnDef);

src/Database/Schema/SqliteSchema.php

@@ -122,10 +122,19 @@ class SqliteSchema extends BaseSchema
             'null' => !$row['notnull'],
             'default' => $row['dflt_value'] === null ? null : trim($row['dflt_value'], "'"),
         ];
-        if ($row['pk']) {
+        $primary = $table->constraint('primary');
+
+        if ($row['pk'] && empty($primary)) {
             $field['null'] = false;
             $field['autoIncrement'] = true;
         }
+
+        // SQLite does not support autoincrement on composite keys.
+        if ($row['pk'] && !empty($primary)) {
+            $existingColumn = $primary['columns'][0];
+            $table->addColumn($existingColumn, ['autoIncrement' => null] + $table->column($existingColumn));
+        }
+
         $table->addColumn($row['name'], $field);
         if ($row['pk']) {
             $constraint = (array)$table->constraint('primary') + [

src/Database/Schema/SqlserverSchema.php

@@ -29,12 +29,10 @@ class SqlserverSchema extends BaseSchema
      */
     public function listTablesSql($config)
     {
-        $sql = '
-			SELECT TABLE_NAME
-			FROM INFORMATION_SCHEMA.TABLES
-			WHERE TABLE_SCHEMA = ?
-			ORDER BY TABLE_NAME
-		';
+        $sql = 'SELECT TABLE_NAME
+            FROM INFORMATION_SCHEMA.TABLES
+            WHERE TABLE_SCHEMA = ?
+            ORDER BY TABLE_NAME';
         $schema = empty($config['schema']) ? static::DEFAULT_SCHEMA_NAME : $config['schema'];
         return [$sql, [$schema]];
     }
@@ -44,16 +42,20 @@ class SqlserverSchema extends BaseSchema
      */
     public function describeColumnSql($tableName, $config)
     {
-        $sql =
-        "SELECT DISTINCT TABLE_SCHEMA AS [schema], COLUMN_NAME AS [name], DATA_TYPE AS [type],
-			IS_NULLABLE AS [null], COLUMN_DEFAULT AS [default],
-			CHARACTER_MAXIMUM_LENGTH AS [char_length],
-			NUMERIC_PRECISION AS [precision],
-			NUMERIC_SCALE AS [scale],
-			'' AS [comment], ORDINAL_POSITION AS [ordinal_position]
-		FROM INFORMATION_SCHEMA.COLUMNS
-		WHERE TABLE_NAME = ? AND TABLE_SCHEMA = ?
-		ORDER BY ordinal_position";
+        $sql = "SELECT DISTINCT
+            AC.name AS [name],
+            TY.name AS [type],
+            AC.max_length AS [char_length],
+            AC.precision AS [precision],
+            AC.scale AS [scale],
+            AC.is_identity AS [autoincrement],
+            AC.is_nullable AS [null],
+            OBJECT_DEFINITION(AC.default_object_id) AS [default]
+            FROM sys.[tables] T
+            INNER JOIN sys.[schemas] S ON S.[schema_id] = T.[schema_id]
+            INNER JOIN sys.[all_columns] AC ON T.[object_id] = AC.[object_id]
+            INNER JOIN sys.[types] TY ON TY.[user_type_id] = AC.[user_type_id]
+            WHERE T.[name] = ? AND S.[name] = ?";
 
         $schema = empty($config['schema']) ? static::DEFAULT_SCHEMA_NAME : $config['schema'];
         return [$sql, [$tableName, $schema]];
@@ -149,13 +151,15 @@ class SqlserverSchema extends BaseSchema
         if (!empty($row['default'])) {
             $row['default'] = trim($row['default'], '()');
         }
-
+        if (!empty($row['autoincrement'])) {
+            $field['autoIncrement'] = true;
+        }
         if ($field['type'] === 'boolean') {
             $row['default'] = (int)$row['default'];
         }
 
         $field += [
-            'null' => $row['null'] === 'YES' ? true : false,
+            'null' => $row['null'] === '1' ? true : false,
             'default' => $row['default'],
         ];
         $table->addColumn($row['name'], $field);
@@ -166,21 +170,19 @@ class SqlserverSchema extends BaseSchema
      */
     public function describeIndexSql($tableName, $config)
     {
-        $sql = "
-			SELECT
-				I.[name] AS [index_name],
-				IC.[index_column_id] AS [index_order],
-				AC.[name] AS [column_name],
-				I.[is_unique], I.[is_primary_key],
-				I.[is_unique_constraint]
-			FROM sys.[tables] AS T
-			INNER JOIN sys.[schemas] S ON S.[schema_id] = T.[schema_id]
-			INNER JOIN sys.[indexes] I ON T.[object_id] = I.[object_id]
-			INNER JOIN sys.[index_columns] IC ON I.[object_id] = IC.[object_id] AND I.[index_id] = IC.[index_id]
-			INNER JOIN sys.[all_columns] AC ON T.[object_id] = AC.[object_id] AND IC.[column_id] = AC.[column_id]
-			WHERE T.[is_ms_shipped] = 0 AND I.[type_desc] <> 'HEAP' AND T.[name] = ? AND S.[name] = ?
-			ORDER BY I.[index_id], IC.[index_column_id]
-		";
+        $sql = "SELECT
+                I.[name] AS [index_name],
+                IC.[index_column_id] AS [index_order],
+                AC.[name] AS [column_name],
+                I.[is_unique], I.[is_primary_key],
+                I.[is_unique_constraint]
+            FROM sys.[tables] AS T
+            INNER JOIN sys.[schemas] S ON S.[schema_id] = T.[schema_id]
+            INNER JOIN sys.[indexes] I ON T.[object_id] = I.[object_id]
+            INNER JOIN sys.[index_columns] IC ON I.[object_id] = IC.[object_id] AND I.[index_id] = IC.[index_id]
+            INNER JOIN sys.[all_columns] AC ON T.[object_id] = AC.[object_id] AND IC.[column_id] = AC.[column_id]
+            WHERE T.[is_ms_shipped] = 0 AND I.[type_desc] <> 'HEAP' AND T.[name] = ? AND S.[name] = ?
+            ORDER BY I.[index_id], IC.[index_column_id]";
 
         $schema = empty($config['schema']) ? static::DEFAULT_SCHEMA_NAME : $config['schema'];
         return [$sql, [$tableName, $schema]];
@@ -229,19 +231,17 @@ class SqlserverSchema extends BaseSchema
      */
     public function describeForeignKeySql($tableName, $config)
     {
-        $sql = "
-			SELECT FK.[name] AS [foreign_key_name], FK.[delete_referential_action_desc] AS [delete_type],
-				FK.[update_referential_action_desc] AS [update_type], C.name AS [column], RT.name AS [reference_table],
-				RC.name AS [reference_column]
-			FROM sys.foreign_keys FK
-			INNER JOIN sys.foreign_key_columns FKC ON FKC.constraint_object_id = FK.object_id
-			INNER JOIN sys.tables T ON T.object_id = FKC.parent_object_id
-			INNER JOIN sys.tables RT ON RT.object_id = FKC.referenced_object_id
-			INNER JOIN sys.schemas S ON S.schema_id = T.schema_id AND S.schema_id = RT.schema_id
-			INNER JOIN sys.columns C ON C.column_id = FKC.parent_column_id AND C.object_id = FKC.parent_object_id
-			INNER JOIN sys.columns RC ON RC.column_id = FKC.referenced_column_id AND RC.object_id = FKC.referenced_object_id
-			WHERE FK.is_ms_shipped = 0 AND T.name = ? AND S.name = ?
-		";
+        $sql = "SELECT FK.[name] AS [foreign_key_name], FK.[delete_referential_action_desc] AS [delete_type],
+                FK.[update_referential_action_desc] AS [update_type], C.name AS [column], RT.name AS [reference_table],
+                RC.name AS [reference_column]
+            FROM sys.foreign_keys FK
+            INNER JOIN sys.foreign_key_columns FKC ON FKC.constraint_object_id = FK.object_id
+            INNER JOIN sys.tables T ON T.object_id = FKC.parent_object_id
+            INNER JOIN sys.tables RT ON RT.object_id = FKC.referenced_object_id
+            INNER JOIN sys.schemas S ON S.schema_id = T.schema_id AND S.schema_id = RT.schema_id
+            INNER JOIN sys.columns C ON C.column_id = FKC.parent_column_id AND C.object_id = FKC.parent_object_id
+            INNER JOIN sys.columns RC ON RC.column_id = FKC.referenced_column_id AND RC.object_id = FKC.referenced_object_id
+            WHERE FK.is_ms_shipped = 0 AND T.name = ? AND S.name = ?";
 
         $schema = empty($config['schema']) ? static::DEFAULT_SCHEMA_NAME : $config['schema'];
         return [$sql, [$tableName, $schema]];

src/ORM/Table.php

@@ -1479,8 +1479,9 @@ class Table implements RepositoryInterface, EventListenerInterface
         $data = $filteredKeys + $data;
 
         if (count($primary) > 1) {
+            $schema = $this->schema();
             foreach ($primary as $k => $v) {
-                if (!isset($data[$k])) {
+                if (!isset($data[$k]) && empty($schema->column($k)['autoIncrement'])) {
                     $msg = 'Cannot insert row, some of the primary key values are missing. ';
                     $msg .= sprintf(
                         'Got (%s), expecting (%s)',

tests/Fixture/CompositeIncrementsFixture.php

@@ -0,0 +1,41 @@
+<?php
+/**
+ * CakePHP(tm) : Rapid Development Framework (http://cakephp.org)
+ * Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * For full copyright and license information, please see the LICENSE.txt
+ * Redistributions of files must retain the above copyright notice.
+ *
+ * @copyright     Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
+ * @link          http://cakephp.org CakePHP(tm) Project
+ * @since         3.0.0
+ * @license       http://www.opensource.org/licenses/mit-license.php MIT License
+ */
+namespace Cake\Test\Fixture;
+
+use Cake\TestSuite\Fixture\TestFixture;
+
+class CompositeIncrementsFixture extends TestFixture
+{
+
+    /**
+     * fields property
+     *
+     * @var array
+     */
+    public $fields = [
+        'id' => ['type' => 'integer', 'null' => false, 'autoIncrement' => true],
+        'account_id' => ['type' => 'integer', 'null' => false],
+        'name' => ['type' => 'string', 'default' => null],
+        '_constraints' => ['primary' => ['type' => 'primary', 'columns' => ['id', 'account_id']]]
+    ];
+
+    /**
+     * records property
+     *
+     * @var array
+     */
+    public $records = [
+    ];
+}

tests/TestCase/Database/Schema/PostgresSchemaTest.php

@@ -346,6 +346,33 @@ SQL;
     }
 
     /**
+     * Test describing a table with postgres and composite keys
+     *
+     * @return void
+     */
+    public function testDescribeTableCompositeKey()
+    {
+        $this->_needsConnection();
+        $connection = ConnectionManager::get('test');
+        $sql = <<<SQL
+CREATE TABLE schema_composite (
+    "id" SERIAL,
+    "site_id" INTEGER NOT NULL,
+    "name" VARCHAR(255),
+    PRIMARY KEY("id", "site_id")
+);
+SQL;
+        $connection->execute($sql);
+        $schema = new SchemaCollection($connection);
+        $result = $schema->describe('schema_composite');
+        $connection->execute('DROP TABLE schema_composite');
+
+        $this->assertEquals(['id', 'site_id'], $result->primaryKey());
+        $this->assertNull($result->column('site_id')['autoIncrement'], 'site_id should not be autoincrement');
+        $this->assertTrue($result->column('id')['autoIncrement'], 'id should be autoincrement');
+    }
+
+    /**
      * Test describing a table containing defaults with Postgres
      *
      * @return void

tests/TestCase/Database/Schema/SqliteSchemaTest.php

@@ -153,7 +153,7 @@ class SqliteSchemaTest extends TestCase
         $dialect = new SqliteSchema($driver);
 
         $table = $this->getMock('Cake\Database\Schema\Table', [], ['table']);
-        $table->expects($this->at(0))->method('addColumn')->with('field', $expected);
+        $table->expects($this->at(1))->method('addColumn')->with('field', $expected);
 
         $dialect->convertColumnDescription($table, $field);
     }
@@ -232,6 +232,16 @@ CONSTRAINT "author_idx" FOREIGN KEY ("author_id") REFERENCES "schema_authors" ("
 SQL;
         $connection->execute($table);
         $connection->execute('CREATE INDEX "created_idx" ON "schema_articles" ("created")');
+
+        $sql = <<<SQL
+CREATE TABLE schema_composite (
+    "id" INTEGER NOT NULL,
+    "site_id" INTEGER NOT NULL,
+    "name" VARCHAR(255),
+    PRIMARY KEY("id", "site_id")
+);
+SQL;
+        $connection->execute($sql);
     }
 
     /**
@@ -327,6 +337,26 @@ SQL;
     }
 
     /**
+     * Test describing a table with Sqlite and composite keys
+     *
+     * Composite keys in SQLite are never autoincrement, and shouldn't be marked
+     * as such.
+     *
+     * @return void
+     */
+    public function testDescribeTableCompositeKey()
+    {
+        $connection = ConnectionManager::get('test');
+        $this->_createTables($connection);
+        $schema = new SchemaCollection($connection);
+        $result = $schema->describe('schema_composite');
+
+        $this->assertEquals(['id', 'site_id'], $result->primaryKey());
+        $this->assertNull($result->column('site_id')['autoIncrement'], 'site_id should not be autoincrement');
+        $this->assertNull($result->column('id')['autoIncrement'], 'id should not be autoincrement');
+    }
+
+    /**
      * Test describing a table with indexes
      *
      * @return void

tests/TestCase/Database/Schema/SqlserverSchemaTest.php

@@ -243,7 +243,7 @@ SQL;
         $field = [
             'name' => 'field',
             'type' => $type,
-            'null' => 'YES',
+            'null' => '1',
             'default' => 'Default value',
             'char_length' => $length,
             'precision' => $precision,
@@ -365,6 +365,33 @@ SQL;
     }
 
     /**
+     * Test describing a table with postgres and composite keys
+     *
+     * @return void
+     */
+    public function testDescribeTableCompositeKey()
+    {
+        $this->_needsConnection();
+        $connection = ConnectionManager::get('test');
+        $sql = <<<SQL
+CREATE TABLE schema_composite (
+    [id] INTEGER IDENTITY(1, 1),
+    [site_id] INTEGER NOT NULL,
+    [name] VARCHAR(255),
+    PRIMARY KEY([id], [site_id])
+);
+SQL;
+        $connection->execute($sql);
+        $schema = new SchemaCollection($connection);
+        $result = $schema->describe('schema_composite');
+        $connection->execute('DROP TABLE schema_composite');
+
+        $this->assertEquals(['id', 'site_id'], $result->primaryKey());
+        $this->assertNull($result->column('site_id')['autoIncrement'], 'site_id should not be autoincrement');
+        $this->assertTrue($result->column('id')['autoIncrement'], 'id should be autoincrement');
+    }
+
+    /**
      * Test that describe accepts tablenames containing `schema.table`.
      *
      * @return void

tests/TestCase/ORM/TableTest.php

@@ -54,7 +54,9 @@ class TableTest extends TestCase
         'core.articles',
         'core.authors',
         'core.tags',
-        'core.articles_tags'
+        'core.articles_tags',
+        'core.composite_increments',
+        'core.site_articles',
     ];
 
     /**
@@ -1591,10 +1593,7 @@ class TableTest extends TestCase
      */
     public function testSavePrimaryKeyEntityExists()
     {
-        $this->skipIf(
-            $this->connection->driver() instanceof \Cake\Database\Driver\Sqlserver,
-            'SQLServer does not like setting an id on IDENTITY fields'
-        );
+        $this->skipIfSqlServer();
         $table = $this->getMock(
             'Cake\ORM\Table',
             ['exists'],
@@ -1621,10 +1620,7 @@ class TableTest extends TestCase
      */
     public function testSavePrimaryKeyEntityNoExists()
     {
-        $this->skipIf(
-            $this->connection->driver() instanceof \Cake\Database\Driver\Sqlserver,
-            'SQLServer does not like setting an id on IDENTITY fields'
-        );
+        $this->skipIfSqlServer();
         $table = $this->getMock(
             'Cake\ORM\Table',
             ['exists'],
@@ -1979,6 +1975,39 @@ class TableTest extends TestCase
     }
 
     /**
+     * Test that you cannot save rows with composite keys if some columns are missing.
+     *
+     * @group save
+     * @expectedException \RuntimeException
+     * @expectedExceptionMessage Cannot insert row, some of the primary key values are missing
+     * @return void
+     */
+    public function testSaveNewErrorCompositeKeyNoIncrement()
+    {
+        $articles = TableRegistry::get('SiteArticles');
+        $article = $articles->newEntity(['site_id' => 1, 'author_id' => 1, 'title' => 'testing']);
+        $articles->save($article);
+    }
+
+    /**
+     * Test that saving into composite primary keys where one column is missing & autoIncrement works.
+     *
+     * SQLite is skipped because it doesn't support autoincrement composite keys.
+     *
+     * @group save
+     * @return void
+     */
+    public function testSaveNewCompositeKeyIncrement()
+    {
+        $this->skipIfSqlite();
+        $table = TableRegistry::get('CompositeIncrements');
+        $thing = $table->newEntity(['account_id' => 3, 'name' => 'new guy']);
+        $this->assertSame($thing, $table->save($thing));
+        $this->assertNotEmpty($thing->id, 'Primary key should have been populated');
+        $this->assertSame(3, $thing->account_id);
+    }
+
+    /**
      * Tests that save is wrapped around a transaction
      *
      * @group save
@@ -4028,4 +4057,30 @@ class TableTest extends TestCase
         $table = TableRegistry::get('TestPlugin.Comments');
         $this->assertEquals('TestPlugin.Comments', $table->newEntity()->source());
     }
+
+    /**
+     * Helper method to skip tests when connection is SQLite.
+     *
+     * @return void
+     */
+    public function skipIfSqlite()
+    {
+        $this->skipIf(
+            $this->connection->driver() instanceof \Cake\Database\Driver\Sqlite,
+            'SQLite does not support the requrirements of this test.'
+        );
+    }
+
+    /**
+     * Helper method to skip tests when connection is SQLServer.
+     *
+     * @return void
+     */
+    public function skipIfSqlServer()
+    {
+        $this->skipIf(
+            $this->connection->driver() instanceof \Cake\Database\Driver\Sqlserver,
+            'SQLServer does not support the requirements of this test.'
+        );
+    }
 }