Skip to content

Commit 227dfeb

Browse files
pszalkopszalko
committed
Added parser SQL syntax check for invalid VARCHAR column definition for 5.11.x. #issue-671
1 parent 3bf4248 commit 227dfeb

6 files changed

Lines changed: 528 additions & 0 deletions

File tree

‎src/Components/AlterOperation.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,7 @@ public static function parse(Parser $parser, TokensList $list, array $options =
337337
* @var int
338338
*/
339339
$brackets = 0;
340+
$isFirstUnknownToken = true;
340341

341342
/**
342343
* The state of the parser.
@@ -434,6 +435,12 @@ public static function parse(Parser $parser, TokensList $list, array $options =
434435

435436
$state = 2;
436437
} elseif ($state === 2) {
438+
if ($isFirstUnknownToken && $ret->options->has('ADD') === true && $token->keyword === 'VARCHAR') {
439+
// Validate the column type while preserving the original tokens used to build ALTER.
440+
DataType::parse($parser, clone $list);
441+
}
442+
443+
$isFirstUnknownToken = false;
437444
if (is_string($token->value) || is_int($token->value)) {
438445
$arrayKey = $token->value;
439446
} else {

‎src/Components/DataType.php‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use PhpMyAdmin\SqlParser\TokensList;
1111

1212
use function implode;
13+
use function preg_match;
1314
use function strtolower;
1415
use function strtoupper;
1516
use function trim;
@@ -133,7 +134,39 @@ public static function parse(Parser $parser, TokensList $list, array $options =
133134
$state = 1;
134135
} elseif ($state === 1) {
135136
if (($token->type === Token::TYPE_OPERATOR) && ($token->value === '(')) {
137+
$parametersStart = $list->idx + 1;
136138
$parameters = ArrayObj::parse($parser, $list);
139+
if ($ret->name === 'VARCHAR') {
140+
$hasLength = false;
141+
for ($idx = $parametersStart; $idx < $list->idx; ++$idx) {
142+
$parameter = $list->tokens[$idx];
143+
if (
144+
$parameter->type === Token::TYPE_WHITESPACE || $parameter->type === Token::TYPE_COMMENT
145+
) {
146+
continue;
147+
}
148+
149+
// Check the original lexeme: conversion can turn 20.0 or '20' into 20.
150+
if (
151+
$hasLength || $parameter->type !== Token::TYPE_NUMBER
152+
|| preg_match('/^[0-9]+$/D', $parameter->token) !== 1
153+
) {
154+
$parser->error('VARCHAR length must be a single nonnegative integer.', $parameter);
155+
$hasLength = true;
156+
break;
157+
}
158+
159+
$hasLength = true;
160+
}
161+
162+
if (! $hasLength) {
163+
$parser->error(
164+
'VARCHAR length must be a single nonnegative integer.',
165+
$list->tokens[$list->idx] ?? null
166+
);
167+
}
168+
}
169+
137170
++$list->idx;
138171
$ret->parameters = ($ret->name === 'ENUM') || ($ret->name === 'SET') ?
139172
$parameters->raw : $parameters->values;

‎tests/Components/DataTypeTest.php‎

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PhpMyAdmin\SqlParser\Tests\Components;
6+
7+
use PhpMyAdmin\SqlParser\Components\DataType;
8+
use PhpMyAdmin\SqlParser\Exceptions\ParserException;
9+
use PhpMyAdmin\SqlParser\Parser;
10+
use PhpMyAdmin\SqlParser\Statements\AlterStatement;
11+
use PhpMyAdmin\SqlParser\Statements\CreateStatement;
12+
use PhpMyAdmin\SqlParser\Statements\SelectStatement;
13+
use PhpMyAdmin\SqlParser\Tests\TestCase;
14+
use PHPUnit\Framework\Attributes\DataProvider;
15+
16+
class DataTypeTest extends TestCase
17+
{
18+
private const LENGTH_ERROR = 'VARCHAR length must be a single nonnegative integer.';
19+
20+
/**
21+
* @dataProvider validLengthProvider
22+
*/
23+
#[DataProvider('validLengthProvider')]
24+
public function testValidLength(string $declaration, string $length): void
25+
{
26+
$parser = new Parser();
27+
$type = DataType::parse($parser, $this->getTokensList($declaration . ' COLLATE utf8mb4_bin NULL'));
28+
self::assertSame([], $parser->errors);
29+
self::assertNotNull($type);
30+
self::assertSame('VARCHAR', $type->name);
31+
self::assertSame([$length], $type->parameters);
32+
self::assertSame('utf8mb4_bin', $type->options->has('COLLATE', true));
33+
34+
$parser = new Parser('CREATE TABLE temp_users (phone ' . $declaration . ' COLLATE utf8mb4_bin NULL);');
35+
self::assertSame([], $parser->errors);
36+
self::assertInstanceOf(CreateStatement::class, $parser->statements[0]);
37+
self::assertIsArray($parser->statements[0]->fields);
38+
$field = $parser->statements[0]->fields[0];
39+
self::assertNotNull($field->type);
40+
self::assertSame([$length], $field->type->parameters);
41+
self::assertNotNull($field->options);
42+
self::assertTrue($field->options->has('NULL'));
43+
44+
$parser = new Parser('ALTER TABLE temp_users ADD COLUMN phone ' . $declaration . ' NULL;');
45+
self::assertSame([], $parser->errors);
46+
self::assertInstanceOf(AlterStatement::class, $parser->statements[0]);
47+
self::assertStringContainsString($length, $parser->statements[0]->build());
48+
self::assertStringContainsString('NULL', $parser->statements[0]->build());
49+
}
50+
51+
/** @return array<string, array{string, string}> */
52+
public static function validLengthProvider(): array
53+
{
54+
return [
55+
'ordinary' => ['VARCHAR(20)', '20'],
56+
'zero' => ['VARCHAR(0)', '0'],
57+
'leading zeros' => ['VARCHAR(020)', '20'],
58+
'mixed case and whitespace' => ["vArChAr \n ( 20\t )", '20'],
59+
'comments' => ['varchar /* type */ ( /* before */ 20 /* after */ )', '20'],
60+
'line comments' => ["varchar( -- before\n 20 # after\n )", '20'],
61+
];
62+
}
63+
64+
/**
65+
* @dataProvider invalidLengthProvider
66+
*/
67+
#[DataProvider('invalidLengthProvider')]
68+
public function testInvalidLength(string $length, string $invalidToken): void
69+
{
70+
$parser = new Parser();
71+
DataType::parse($parser, $this->getTokensList('VARCHAR(' . $length . ') NULL'));
72+
self::assertCount(1, $parser->errors);
73+
self::assertSame(self::LENGTH_ERROR, $parser->errors[0]->getMessage());
74+
self::assertInstanceOf(ParserException::class, $parser->errors[0]);
75+
self::assertNotNull($parser->errors[0]->token);
76+
self::assertSame($invalidToken, $parser->errors[0]->token->token);
77+
78+
foreach (self::columnQueries('VARCHAR(' . $length . ')') as $query) {
79+
$parser = new Parser($query . ' SELECT 1;');
80+
self::assertCount(1, $parser->errors);
81+
self::assertSame(self::LENGTH_ERROR, $parser->errors[0]->getMessage());
82+
self::assertInstanceOf(ParserException::class, $parser->errors[0]);
83+
self::assertNotNull($parser->errors[0]->token);
84+
self::assertSame($invalidToken, $parser->errors[0]->token->token);
85+
self::assertCount(2, $parser->statements);
86+
self::assertInstanceOf(SelectStatement::class, $parser->statements[1]);
87+
88+
try {
89+
new Parser($query, true);
90+
self::fail('Strict mode must reject an invalid VARCHAR length.');
91+
} catch (ParserException $exception) {
92+
self::assertSame(self::LENGTH_ERROR, $exception->getMessage());
93+
}
94+
}
95+
}
96+
97+
/** @return array<string, array{string, string}> */
98+
public static function invalidLengthProvider(): array
99+
{
100+
return [
101+
'identifier' => ['X', 'X'],
102+
'quoted string' => ["'20'", "'20'"],
103+
'double quoted string' => ['"20"', '"20"'],
104+
'quoted identifier' => ['`20`', '`20`'],
105+
'fraction' => ['20.0', '20.0'],
106+
'negative' => ['-20', '-20'],
107+
'positive sign' => ['+20', '+20'],
108+
'scientific' => ['2e1', '2e1'],
109+
'hexadecimal' => ['0x14', '0x14'],
110+
'binary' => ['0b10100', '0b10100'],
111+
'expression' => ['1+19', '+19'],
112+
'nested parentheses' => ['(20)', '('],
113+
'empty' => ['', ')'],
114+
'only comments' => [' /* empty */ ', ')'],
115+
'extra parameter' => ['20,30', ','],
116+
'trailing comma' => ['20,', ','],
117+
'separate numbers' => ['20 /* gap */ 30', '30'],
118+
];
119+
}
120+
121+
/** @return list<string> */
122+
private static function columnQueries(string $declaration): array
123+
{
124+
return [
125+
'CREATE TABLE temp_users (phone ' . $declaration . ' NULL);',
126+
'ALTER TABLE temp_users ADD COLUMN phone ' . $declaration . ' NULL;',
127+
'ALTER TABLE temp_users ADD phone ' . $declaration . ' NULL;',
128+
];
129+
}
130+
131+
/**
132+
* @param list<string> $parameters
133+
*
134+
* @dataProvider otherTypeProvider
135+
*/
136+
#[DataProvider('otherTypeProvider')]
137+
public function testOtherTypes(string $declaration, array $parameters): void
138+
{
139+
$parser = new Parser();
140+
$type = DataType::parse($parser, $this->getTokensList($declaration));
141+
self::assertSame([], $parser->errors);
142+
self::assertNotNull($type);
143+
self::assertSame($parameters, $type->parameters);
144+
}
145+
146+
/** @return array<string, array{string, list<string>}> */
147+
public static function otherTypeProvider(): array
148+
{
149+
return [
150+
'enum' => ["ENUM('a','b')", ["'a'", "'b'"]],
151+
'set' => ["SET('a','b')", ["'a'", "'b'"]],
152+
'decimal' => ['DECIMAL(10,2)', ['10', '2']],
153+
'lengthless varchar' => ['VARCHAR', []],
154+
];
155+
}
156+
157+
public function testLengthlessRoutine(): void
158+
{
159+
$parser = new Parser('CREATE PROCEDURE p(IN phone VARCHAR) SELECT phone;');
160+
self::assertSame([], $parser->errors);
161+
}
162+
163+
public function testAlterContinuation(): void
164+
{
165+
$parser = new Parser(
166+
'ALTER TABLE temp_users ADD COLUMN phone VARCHAR(X) NULL, ADD COLUMN name VARCHAR(20) NOT NULL;'
167+
);
168+
self::assertCount(1, $parser->errors);
169+
self::assertSame(self::LENGTH_ERROR, $parser->errors[0]->getMessage());
170+
self::assertInstanceOf(AlterStatement::class, $parser->statements[0]);
171+
self::assertNotNull($parser->statements[0]->altered);
172+
self::assertCount(2, $parser->statements[0]->altered);
173+
self::assertStringContainsString('VARCHAR(20) NOT NULL', $parser->statements[0]->build());
174+
}
175+
176+
public function testAlterStringParameters(): void
177+
{
178+
$parser = new Parser("ALTER TABLE temp_users ADD COLUMN e ENUM('a','b'), ADD COLUMN s SET('a','b');");
179+
self::assertSame([], $parser->errors);
180+
self::assertInstanceOf(AlterStatement::class, $parser->statements[0]);
181+
self::assertNotNull($parser->statements[0]->altered);
182+
self::assertCount(2, $parser->statements[0]->altered);
183+
self::assertStringContainsString("ENUM('a','b')", $parser->statements[0]->build());
184+
}
185+
}

‎tests/Parser/AlterStatementTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ public static function alterProvider(): array
5050
['parser/parseAlterTablePartitionByRange2'],
5151
['parser/parseAlterTableCoalescePartition'],
5252
['parser/parseAlterTableAddColumnWithCheck'],
53+
['parser/parseAlterTableAddColumnInvalidVarcharLength'],
5354
['parser/parseAlterTableAddSpatialIndex1'],
5455
['parser/parseAlterTableAddUniqueKey1'],
5556
['parser/parseAlterTableAddUniqueKey2'],

‎tests/data/parser/parseAlterTableAddColumnInvalidVarcharLength.in‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTER TABLE temp_users ADD COLUMN phone VARCHAR(X) NULL;

0 commit comments

Comments
 (0)