Skip to content

3.0 - Add Sqlite Foreign key support #1333

Merged
merged 10 commits into from Jun 7, 2013

3 participants

@markstory
CakePHP member

This add support to Sqlite for foreign key reflection and generation. I think the changes are pretty straightforward, and while they don't encompass all the various options available in Sqlite, it covers what I think are the most frequently and most portable features of foreign keys.

I'm not certain I've chosen the best names for the various delete/update options so any suggestions are more than welcome 😄.

@lorenzo lorenzo and 1 other commented on an outdated diff Jun 6, 2013
lib/Cake/Database/Schema/Table.php
@@ -92,6 +92,9 @@ class Table {
'type' => null,
'columns' => [],
'length' => [],
+ 'references' => [],
+ 'update' => 'cascade',
@lorenzo
CakePHP member
lorenzo added a note Jun 6, 2013

I think 'restrict' is a safer default

@markstory
CakePHP member
markstory added a note Jun 6, 2013

I usually use on cascade delete, but restrict does seem safer/more conservative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lorenzo
CakePHP member
lorenzo commented Jun 6, 2013

This is looking really good!

@jippi jippi and 1 other commented on an outdated diff Jun 6, 2013
lib/Cake/Database/Schema/Collection.php
+ $statement = $this->_executeSql($sql, $params);
+ foreach ($statement->fetchAll('assoc') as $row) {
+ $this->_dialect->convertForeignKey($table, $row);
+ }
+ return $table;
+ }
+
+/**
+ * Helper method to run queries and convert Exceptions to the correct types.
+ *
+ * @param string $sql The sql to run.
+ * @param array $params Parameters for the statement.
+ * @return Cake\Database\Statement Prepared statement
+ * @throws Cake\Database\Exception on query failure.
+ */
+ protected function _executeSql($sql, $params) {
try {
$statement = $this->_connection->execute($sql, $params);
@jippi
CakePHP member
jippi added a note Jun 6, 2013

why not just return $statement directly?

@markstory
CakePHP member
markstory added a note Jun 6, 2013

Good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jippi jippi commented on an outdated diff Jun 6, 2013
lib/Cake/Database/Schema/SqliteSchema.php
+ }
+
+/**
+ * Convert Sqlite on clauses to the abstract ones.
+ *
+ * @param string $clause
+ * @return string|null
+ */
+ protected function _convertOnClause($clause) {
+ if ($clause === 'CASCADE' || $clause === 'RESTRICT') {
+ return strtolower($clause);
+ }
+ if ($clause === 'NO ACTION') {
+ return 'none';
+ }
+ if ($clause === 'SET NULL') {
@jippi
CakePHP member
jippi added a note Jun 6, 2013

The if make sense though it's the same return null as if the $clause was something unrecognised which make it look a bit weird

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jippi jippi commented on the diff Jun 6, 2013
lib/Cake/Database/Schema/SqliteSchema.php
count($data['columns']) === 1 &&
$table->column($data['columns'][0])['type'] === 'integer'
) {
return '';
}
+ $clause = '';
@jippi
CakePHP member
jippi added a note Jun 6, 2013

move this to line 320 so it's closer to it's first usage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jippi jippi commented on the diff Jun 6, 2013
lib/Cake/Database/Schema/SqliteSchema.php
);
}
/**
+ * Generate an ON clause for a foreign key.
+ *
+ * @param string|null $on The on clause
+ * @return string
@jippi
CakePHP member
jippi added a note Jun 6, 2013

string|null - maybe be "exceptional" about not recognising it on type?

@markstory
CakePHP member
markstory added a note Jun 6, 2013

The exceptions would have been generated when the Table instance was created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jippi jippi and 1 other commented on an outdated diff Jun 6, 2013
lib/Cake/Database/Schema/Table.php
$this->_constraints[$name] = $attrs;
return $this;
}
/**
+ * Helper method to check/validate foreign keys.
+ *
+ * @param array $attrs Attributes to set.
+ * @return array
+ */
+ protected function _checkForeignKey($attrs) {
+ if (count($attrs['references']) < 2) {
+ throw new Exception(__d('cake_dev', 'References must contain a table and column.'));
+ }
+ $validActions = ['cascade', 'restrict', null, 'none'];
@jippi
CakePHP member
jippi added a note Jun 6, 2013

perhaps make this a type => sql name map and re-use it in _foreignOnClause too - since they seem to be the ~same

@markstory
CakePHP member
markstory added a note Jun 6, 2013

It would have to be public then, and the SQL equivalents are not stable across all platforms.

@jippi
CakePHP member
jippi added a note Jun 6, 2013

they don't share a driver or dialect anywhere ?

@markstory
CakePHP member
markstory added a note Jun 6, 2013

No.

@markstory
CakePHP member
markstory added a note Jun 6, 2013

I've intentionally avoided creating a class hierarchy here for a few reasons:

  1. There isn't much code reuse yet.
  2. I don't think there is anything to gain from inheritance here.

At best there may be an interface in the future with the methods that Schema\Table and Schema\Collection expect the Schema dialects to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lorenzo lorenzo and 2 others commented on an outdated diff Jun 6, 2013
...ke/Test/TestCase/Database/Schema/SqliteSchemaTest.php
+ ],
+ [
+ 'author_id_idx',
+ ['type' => 'foreign', 'columns' => ['author_id'], 'references' => ['authors', 'id'], 'update' => 'cascade'],
+ 'CONSTRAINT "author_id_idx" FOREIGN KEY ("author_id") ' .
+ 'REFERENCES "authors" ("id") ON UPDATE CASCADE ON DELETE RESTRICT'
+ ],
+ [
+ 'author_id_idx',
+ ['type' => 'foreign', 'columns' => ['author_id'], 'references' => ['authors', 'id'], 'update' => 'restrict'],
+ 'CONSTRAINT "author_id_idx" FOREIGN KEY ("author_id") ' .
+ 'REFERENCES "authors" ("id") ON UPDATE RESTRICT ON DELETE RESTRICT'
+ ],
+ [
+ 'author_id_idx',
+ ['type' => 'foreign', 'columns' => ['author_id'], 'references' => ['authors', 'id'], 'update' => null],
@lorenzo
CakePHP member
lorenzo added a note Jun 6, 2013

Looking at this again, it feels odd to set 'update' => null. Seems like don't care, use defaults

@markstory
CakePHP member
markstory added a note Jun 6, 2013

Would 'setNull' be a better replacement?

@jippi
CakePHP member
jippi added a note Jun 6, 2013

or make it a string - 'null' ?

@lorenzo
CakePHP member
lorenzo added a note Jun 6, 2013

'setNull' or 'null' would work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
markstory added some commits Jun 6, 2013
@markstory markstory Rename options so they are all strings.
After some discussion we came up with better names for the constraint
options.
c0f7ef8
@markstory markstory Use constants instead of bare strings.
Constants feel better in code vs. bare strings. They also
cause errors when you make a mistake.
5d29549
@lorenzo
CakePHP member
lorenzo commented Jun 7, 2013

nice!

@lorenzo lorenzo merged commit e10d2ae into cakephp:3.0 Jun 7, 2013

1 check failed

Details default The Travis CI build failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.