Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for backslashes in default values #2850

Merged
merged 15 commits into from
Jan 2, 2018
+122 −43
Merged

Support for backslashes in default values #2850

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bd465f7
Automatic escaping of default values
PowerKiKi Sep 22, 2017
26658ab
Functionnal tests for escaped default values
PowerKiKi Oct 14, 2017
b83ba1a
Document the default value minor BC break
PowerKiKi Oct 14, 2017
b841ba7
Support for backslashes in default values on PostgreSQL and SQLite
PowerKiKi Oct 14, 2017
4851fc0
Test double single quotes in default values
PowerKiKi Oct 15, 2017
7e65c84
Fix code style
PowerKiKi Oct 16, 2017
44637bf
Functional test of escaped default values via INSERT and SELECT
PowerKiKi Dec 2, 2017
5da4c33
Remove duplicated test case
PowerKiKi Dec 8, 2017
a50a029
More meaningful method name
PowerKiKi Dec 8, 2017
6e920f3
Test same escaped sequences across all platforms
PowerKiKi Dec 8, 2017
255e112
Remove redundant test
PowerKiKi Dec 31, 2017
87c417f
Remove unit tests in favor of functional tests
PowerKiKi Dec 31, 2017
9f32154
Independent tests for introspection and insert
PowerKiKi Dec 31, 2017
fa9e2b1
Fix code style
PowerKiKi Jan 2, 2018
2971f86
Fix code style (bis)
PowerKiKi Jan 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions UPGRADE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
# Upgrade to UNRELEASED

## MINOR BC BREAK: escaped default values

Default values will be automatically escaped. So default values must now be specified non-escaped.

Before:

$column->setDefault('Foo\\\\Bar\\\\Baz');

After:

$column->setDefault('Foo\\Bar\\Baz');

# Upgrade to 2.6

## MINOR BC BREAK: `fetch()` and `fetchAll()` method signatures in `Doctrine\DBAL\Driver\ResultStatement`
Expand Down
14 changes: 13 additions & 1 deletion lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2306,7 +2306,7 @@ public function getDefaultValueDeclarationSQL($field)
return " DEFAULT '" . $this->convertBooleans($default) . "'";
}

return " DEFAULT '" . $default . "'";
return " DEFAULT " . $this->quoteDefaultStringLiteral($default);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that quoteStringLiteral() only escapes quotes and backslashes by means of str_replace(), it may not work with encodings where the 0x27 byte (the single quote in ASCII) is part of a multi-byte sequence.

This approach to escaping is used everywhere on the platform level, so it'd be good to hear from other maintainers whether this approach is valid in general.

/cc @Ocramius @lcobucci

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to only affect schema definitions, so I'm not too worried about it for now. It is a problem if people rely on it for anything runtime-related.

}

/**
Expand Down Expand Up @@ -3570,4 +3570,16 @@ public function getStringLiteralQuoteCharacter()
{
return "'";
}

/**
* Quote a literal string for usage as DEFAULT value.
*
* @param string $str
*
* @return string
*/
protected function quoteDefaultStringLiteral(string $str) : string
{
return $this->quoteStringLiteral($str);
}
}
8 changes: 8 additions & 0 deletions lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,4 +1236,12 @@ private function isNumericType(Type $type) : bool
{
return $type instanceof IntegerType || $type instanceof BigIntType;
}

/**
* {@inheritDoc}
*/
protected function quoteDefaultStringLiteral(string $str) : string
{
return parent::quoteStringLiteral($str);
Copy link
Member

@morozov morozov Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it required to escape the default value in a different way than other literals for PostgreSQL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because PostgreSQL does not support backlash C-style escapes out of the box, the quoteStringLiteral() had to be overridden a long time ago. See this quick example:

db=# SELECT 'foo\bar', 'foo\\bar', E'foo\\bar';
 ?column? | ?column? | ?column? 
----------+----------+----------
 foo\bar  | foo\\bar | foo\bar
(1 row)

This means we cannot use PostgreSqlPlatform::quoteStringLiteral() for our use-case. And this is the only reason why we created a new quoteDefaultStringLiteral(), to be able to override it to "cancel" the original override.

I wish it was not needed, but I don't say another way around...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scratch that last comment of mine, I'll think a bit more about that...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that quoteStringLiteral() is not overridden may mean that it's not tested well enough. You could try testing other (non-DDL) cases and see if those tests fail as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean by that. AbstractPlatform::quoteStringLiteral is indeed overridden by PostgreSqlPlatform::quoteStringLiteral. But we intentionally use the non-overridden method in our code. And that is covered by tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PowerKiKi I don't remember what I meant by that 😕.

}
}
23 changes: 21 additions & 2 deletions lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,11 +384,12 @@ protected function _getPortableTableColumnDefinition($tableColumn)
$length = null;
break;
case 'text':
case '_varchar':
case 'varchar':
$tableColumn['default'] = $this->unescapeDefaultValue($tableColumn['default']);
$fixed = false;
break;
case 'varchar':
case 'interval':
case '_varchar':
$fixed = false;
break;
case 'char':
Expand Down Expand Up @@ -468,4 +469,22 @@ private function fixVersion94NegativeNumericDefaultValue($defaultValue)

return $defaultValue;
}

/**
* Un-escape a default value as given by PostgreSQL
*
* @param null|string $default
*
* @return null|string
*/
private function unescapeDefaultValue(?string $default) : ?string
{
if ($default === null) {
return $default;
}

$default = str_replace("''", "'", $default);

return $default;
}
}
5 changes: 3 additions & 2 deletions lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,9 @@ protected function _getPortableTableColumnDefinition($tableColumn)
$default = null;
}
if ($default !== null) {
// SQLite returns strings wrapped in single quotes, so we need to strip them
$default = preg_replace("/^'(.*)'$/", '\1', $default);
// SQLite returns strings wrapped in single quotes and escaped, so we need to strip them
$default = preg_replace("/^'(.*)'$/s", '\1', $default);
$default = str_replace("''", "'", $default);
}
$notnull = (bool) $tableColumn['notnull'];

Expand Down
35 changes: 0 additions & 35 deletions tests/Doctrine/Tests/DBAL/Functional/Schema/MySqlSchemaManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,39 +442,4 @@ public function testColumnDefaultValuesCurrentTimeAndDate() : void
$diff = $comparator->diffTable($table, $onlineTable);
self::assertFalse($diff, "Tables should be identical with column defauts time and date.");
}

/**
* Ensure default values (un-)escaping is properly done by mysql platforms.
* The test is voluntarily relying on schema introspection due to current
* doctrine limitations. Once #2850 is landed, this test can be removed.
* @see https://dev.mysql.com/doc/refman/5.7/en/string-literals.html
*/
public function testEnsureDefaultsAreUnescapedFromSchemaIntrospection() : void
{
$platform = $this->_sm->getDatabasePlatform();
$this->_conn->query('DROP TABLE IF EXISTS test_column_defaults_with_create');

$escapeSequences = [
"\\0", // An ASCII NUL (X'00') character
"\\'", "''", // Single quote
'\\"', '""', // Double quote
'\\b', // A backspace character
'\\n', // A new-line character
'\\r', // A carriage return character
'\\t', // A tab character
'\\Z', // ASCII 26 (Control+Z)
'\\\\', // A backslash (\) character
'\\%', // A percent (%) character
'\\_', // An underscore (_) character
];

$default = implode('+', $escapeSequences);

$sql = "CREATE TABLE test_column_defaults_with_create(
col1 VARCHAR(255) NULL DEFAULT {$platform->quoteStringLiteral($default)}
)";
$this->_conn->query($sql);
$onlineTable = $this->_sm->listTableDetails("test_column_defaults_with_create");
self::assertSame($default, $onlineTable->getColumn('col1')->getDefault());
}
}
60 changes: 60 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,4 +1397,64 @@ public function testCreateAndListSequences() : void
self::assertEquals($sequence2AllocationSize, $actualSequence2->getAllocationSize());
self::assertEquals($sequence2InitialValue, $actualSequence2->getInitialValue());
}

/**
* Returns potential escaped literals from all platforms combined.
*
* @see https://dev.mysql.com/doc/refman/5.7/en/string-literals.html
* @see http://www.sqlite.org/lang_expr.html
* @see https://www.postgresql.org/docs/9.6/static/sql-syntax-lexical.html#SQL-SYNTAX-STRINGS-ESCAPE
*
* @return array
*/
private function getEscapedLiterals() : array
{
return [
['An ASCII NUL (X\'00\')', "foo\\0bar"],
['Single quote, C-style', "foo\\'bar"],
['Single quote, doubled-style', "foo''bar"],
['Double quote, C-style', 'foo\\"bar'],
['Double quote, double-style', 'foo""bar'],
['Backspace', 'foo\\bbar'],
['New-line', 'foo\\nbar'],
['Carriage return', 'foo\\rbar'],
['Tab', 'foo\\tbar'],
['ASCII 26 (Control+Z)', 'foo\\Zbar'],
['Backslash (\)', 'foo\\\\bar'],
['Percent (%)', 'foo\\%bar'],
['Underscore (_)', 'foo\\_bar'],
];
}

private function createTableForDefaultValues() : void
{
$table = new Table('string_escaped_default_value');
foreach ($this->getEscapedLiterals() as $i => $literal) {
$table->addColumn('field' . $i, 'string', ['default' => $literal[1]]);
}

$table->addColumn('def_foo', 'string');
$this->_sm->dropAndCreateTable($table);
}

public function testEscapedDefaultValueCanBeIntrospected() : void
{
$this->createTableForDefaultValues();

$onlineTable = $this->_sm->listTableDetails('string_escaped_default_value');
foreach ($this->getEscapedLiterals() as $i => $literal) {
self::assertSame($literal[1], $onlineTable->getColumn('field' . $i)->getDefault(), 'should be able introspect the value of default for: ' . $literal[0]);
}
}

public function testEscapedDefaultValueCanBeInserted() : void
{
$this->createTableForDefaultValues();

$this->_conn->insert('string_escaped_default_value', ['def_foo' => 'foo']);
$row = $this->_conn->fetchAssoc('SELECT * FROM string_escaped_default_value');
foreach ($this->getEscapedLiterals() as $i => $literal) {
self::assertSame($literal[1], $row['field' . $i], 'inserted default value should be the configured default value for: ' . $literal[0]);
}
}
}
6 changes: 3 additions & 3 deletions tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ public function testNamedPrimaryKey()
"ALTER TABLE mytable ADD PRIMARY KEY (foo)",
), $sql);
}

public function testAlterPrimaryKeyWithNewColumn()
{
$table = new Table("yolo");
Expand All @@ -483,7 +483,7 @@ public function testAlterPrimaryKeyWithNewColumn()

$comparator = new Comparator();
$diffTable = clone $table;

$diffTable->addColumn('pkc2', 'integer');
$diffTable->dropPrimaryKey();
$diffTable->setPrimaryKey(array('pkc1', 'pkc2'));
Expand All @@ -495,7 +495,7 @@ public function testAlterPrimaryKeyWithNewColumn()
'ALTER TABLE yolo ADD PRIMARY KEY (pkc1, pkc2)',
),
$this->_platform->getAlterTableSQL($comparator->diffTable($table, $diffTable))
);
);
}

public function testInitializesDoctrineTypeMappings()
Expand Down