Skip to content

Commit 15758f2

Browse files
pszalkopszalko
committed
Added parser SQL syntax check for invalid VARCHAR column definition. #issue-671
1 parent b717aa5 commit 15758f2

6 files changed

Lines changed: 553 additions & 0 deletions

File tree

‎src/Parsers/AlterOperations.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,7 @@ public static function parse(Parser $parser, TokensList $list, array $options =
262262
* Counts brackets.
263263
*/
264264
$brackets = 0;
265+
$isFirstUnknownToken = true;
265266

266267
/**
267268
* The state of the parser.
@@ -355,6 +356,12 @@ public static function parse(Parser $parser, TokensList $list, array $options =
355356

356357
$state = 2;
357358
} elseif ($state === 2) {
359+
if ($isFirstUnknownToken && $ret->options?->has('ADD') === true && $token->keyword === 'VARCHAR') {
360+
// Validate the column type while preserving the original tokens used to build ALTER.
361+
DataTypes::parse($parser, clone $list);
362+
}
363+
364+
$isFirstUnknownToken = false;
358365
if (is_string($token->value) || is_int($token->value)) {
359366
$arrayKey = $token->value;
360367
} else {

‎src/Parsers/DataTypes.php‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use PhpMyAdmin\SqlParser\TokensList;
1212
use PhpMyAdmin\SqlParser\TokenType;
1313

14+
use function preg_match;
1415
use function strtoupper;
1516

1617
/**
@@ -79,7 +80,37 @@ public static function parse(Parser $parser, TokensList $list, array $options =
7980
$state = 1;
8081
} elseif ($state === 1) {
8182
if (($token->type === TokenType::Operator) && ($token->value === '(')) {
83+
$parametersStart = $list->idx + 1;
8284
$parameters = ArrayObjs::parse($parser, $list);
85+
if ($ret->name === 'VARCHAR') {
86+
$hasLength = false;
87+
for ($idx = $parametersStart; $idx < $list->idx; ++$idx) {
88+
$parameter = $list->tokens[$idx];
89+
if ($parameter->type === TokenType::Whitespace || $parameter->type === TokenType::Comment) {
90+
continue;
91+
}
92+
93+
// Check the original lexeme: conversion can turn 20.0 or '20' into 20.
94+
if (
95+
$hasLength || $parameter->type !== TokenType::Number
96+
|| preg_match('/^[0-9]+$/D', $parameter->token) !== 1
97+
) {
98+
$parser->error('VARCHAR length must be a single nonnegative integer.', $parameter);
99+
$hasLength = true;
100+
break;
101+
}
102+
103+
$hasLength = true;
104+
}
105+
106+
if (! $hasLength) {
107+
$parser->error(
108+
'VARCHAR length must be a single nonnegative integer.',
109+
$list->tokens[$list->idx] ?? null,
110+
);
111+
}
112+
}
113+
83114
++$list->idx;
84115
$ret->parameters = ($ret->name === 'ENUM') || ($ret->name === 'SET') ?
85116
$parameters->raw : $parameters->values;

‎tests/Components/DataTypeTest.php‎

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

‎tests/Parser/AlterStatementTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ public static function alterProvider(): array
4545
['parser/parseAlterTablePartitionByRange2'],
4646
['parser/parseAlterTableCoalescePartition'],
4747
['parser/parseAlterTableAddColumnWithCheck'],
48+
['parser/parseAlterTableAddColumnInvalidVarcharLength'],
4849
['parser/parseAlterTableAddSpatialIndex1'],
4950
['parser/parseAlterTableAddUniqueKey1'],
5051
['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)