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

Jump to bottom

Completing, refactoring and improving the WITH statements parser #363

Merged
merged 12 commits into from
Dec 29, 2021
Merged

Completing, refactoring and improving the WITH statements parser #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f56b027
Refactoring and improving the support of the WITH statement and makin…
iifawzi Dec 5, 2021
835a537
Adding more test cases for withStatements
iifawzi Dec 5, 2021
75436a9
Fixing phpstan issues, and adding more test cases
iifawzi Dec 6, 2021
2961130
Adding more tests for withStatements
iifawzi Dec 6, 2021
85a6461
Improve types for Psalm and error's message improvement
iifawzi Dec 6, 2021
f5269d7
Adding more test cases
iifawzi Dec 6, 2021
3516c94
Uncommenting tests
iifawzi Dec 6, 2021
199ba66
updating comments
iifawzi Dec 6, 2021
cd49677
Explicitly check ON DUPLICATE KEY UPDATE and adding a test case
iifawzi Dec 6, 2021
f1585a9
Update the with Build statement to build the cteStatement
iifawzi Dec 7, 2021
bd86fb3
handle: cteStatementParser could be null
iifawzi Dec 7, 2021
c8f72ec
remove duplicated test
iifawzi Dec 11, 2021
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
53 changes: 45 additions & 8 deletions src/Statements/CreateStatement.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,17 @@ class CreateStatement extends Statement
*/
public $fields;

/**
* If `CREATE TABLE WITH`.
* If `CREATE TABLE AS WITH`.
* If `CREATE VIEW AS WITH`.
*
* Used by `CREATE TABLE`, `CREATE VIEW`
*
* @var WithStatement|null
*/
public $with;

/**
* If `CREATE TABLE ... SELECT`.
* If `CREATE VIEW AS ` ... SELECT`.
Expand Down Expand Up @@ -436,6 +447,13 @@ public function build()
. Expression::build($this->like);
}

if ($this->with !== null) {
return 'CREATE '
. OptionsArray::build($this->options) . ' '
. Expression::build($this->name) . ' '
. $this->with->build();
}

$partition = '';

if (! empty($this->partitionBy)) {
Expand Down Expand Up @@ -465,10 +483,17 @@ public function build()
. OptionsArray::build($this->entityOptions)
. $partition;
} elseif ($this->options->has('VIEW')) {
$builtStatement = '';
if ($this->select !== null) {
$builtStatement = $this->select->build();
Copy link
Member

Choose a reason for hiding this comment

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

do you know why it is reported non covered ?

Copy link
Contributor Author

@iifawzi iifawzi Dec 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

After 2 hours of investigation, It seems like it was always reporting false positive. it's A separate bug not related to the changes proposed in this PR, but it's now shown as uncovered because previously it wasn't in a separate branch:

sql-parser/src/Statements/CreateStatement.php

Lines 468 to 471 in ed0b4ba

return 'CREATE '
. OptionsArray::build($this->options) . ' '
. Expression::build($this->name) . ' '
. $fields . ' AS ' . ($this->select ? $this->select->build() : '')

The bug seems to be coming from this portion of code:

sql-parser/src/Statements/CreateStatement.php

Lines 699 to 718 in ed0b4ba

$token = $list->getNext(); // Skipping whitespaces and comments.
// Parsing columns list.
if (($token->type === Token::TYPE_OPERATOR) && ($token->value === '(')) {
--$list->idx; // getNext() also goes forward one field.
$this->fields = ArrayObj::parse($parser, $list);
++$list->idx; // Skipping last token from the array.
$list->getNext();
}
// Parsing the SELECT expression if the view started with it.
if (
$token->type === Token::TYPE_KEYWORD
&& $token->keyword === 'AS'
&& $list->tokens[$nextidx]->type === Token::TYPE_KEYWORD
&& $list->tokens[$nextidx]->value === 'SELECT'
) {
$list->idx = $nextidx;
$this->select = new SelectStatement($parser, $list);
}

You will notice that after parsing the array options if exists, we update the $list->idx by these two lines of code:

 ++$list->idx; // Skipping last token from the array. 
$list->getNext();

and that's totally fine, the issue is that in the code that follows, we're checking the keywords using the $token and the $nextidx but their old state, which was set before updating the idx after parsing the options.

sql-parser/src/Statements/CreateStatement.php

Lines 710 to 715 in ed0b4ba

if (
$token->type === Token::TYPE_KEYWORD
&& $token->keyword === 'AS'
&& $list->tokens[$nextidx]->type === Token::TYPE_KEYWORD
&& $list->tokens[$nextidx]->value === 'SELECT'
) {

so, we would need to update the $token and the $nextidx to their new values corresponding to the update that occured after parsing the options.

I will try to find sometime to fix it in the next weeks, but it has nothing to do with this PR.

} elseif ($this->with !== null) {
$builtStatement = $this->with->build();
}

return 'CREATE '
. OptionsArray::build($this->options) . ' '
. Expression::build($this->name) . ' '
. $fields . ' AS ' . ($this->select ? $this->select->build() : '')
. $fields . ' AS ' . $builtStatement
. (! empty($this->body) ? TokensList::build($this->body) : '') . ' '
. OptionsArray::build($this->entityOptions);
} elseif ($this->options->has('TRIGGER')) {
Expand Down Expand Up @@ -546,14 +571,22 @@ public function parse(Parser $parser, TokensList $list)
if (($token->type === Token::TYPE_KEYWORD) && ($token->keyword === 'SELECT')) {
/* CREATE TABLE ... SELECT */
$this->select = new SelectStatement($parser, $list);
} elseif ($token->type === Token::TYPE_KEYWORD && ($token->keyword === 'WITH')) {
/* CREATE TABLE WITH */
$this->with = new WithStatement($parser, $list);
} elseif (
($token->type === Token::TYPE_KEYWORD) && ($token->keyword === 'AS')
&& ($list->tokens[$nextidx]->type === Token::TYPE_KEYWORD)
&& ($list->tokens[$nextidx]->value === 'SELECT')
) {
/* CREATE TABLE ... AS SELECT */
$list->idx = $nextidx;
$this->select = new SelectStatement($parser, $list);
if ($list->tokens[$nextidx]->value === 'SELECT') {
/* CREATE TABLE ... AS SELECT */
$list->idx = $nextidx;
$this->select = new SelectStatement($parser, $list);
} elseif ($list->tokens[$nextidx]->value === 'WITH') {
/* CREATE TABLE WITH */
$list->idx = $nextidx;
$this->with = new WithStatement($parser, $list);
}
} elseif ($token->type === Token::TYPE_KEYWORD && $token->keyword === 'LIKE') {
/* CREATE TABLE `new_tbl` LIKE 'orig_tbl' */
$list->idx = $nextidx;
Expand Down Expand Up @@ -711,10 +744,14 @@ public function parse(Parser $parser, TokensList $list)
$token->type === Token::TYPE_KEYWORD
&& $token->keyword === 'AS'
&& $list->tokens[$nextidx]->type === Token::TYPE_KEYWORD
&& $list->tokens[$nextidx]->value === 'SELECT'
) {
$list->idx = $nextidx;
$this->select = new SelectStatement($parser, $list);
if ($list->tokens[$nextidx]->value === 'SELECT') {
$list->idx = $nextidx;
$this->select = new SelectStatement($parser, $list);
} elseif ($list->tokens[$nextidx]->value === 'WITH') {
++$list->idx;
$this->with = new WithStatement($parser, $list);
}
}

// Parsing all other tokens
Expand Down
10 changes: 10 additions & 0 deletions src/Statements/InsertStatement.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,14 @@ class InsertStatement extends Statement
*/
public $select;

/**
* If WITH CTE is present
* holds the WithStatement.
*
* @var WithStatement|null
*/
public $with;

/**
* If ON DUPLICATE KEY UPDATE clause is present
* holds the SetOperation.
Expand Down Expand Up @@ -210,6 +218,8 @@ public function parse(Parser $parser, TokensList $list)
$this->set = SetOperation::parse($parser, $list);
} elseif ($token->keyword === 'SELECT') {
$this->select = new SelectStatement($parser, $list);
} elseif ($token->keyword === 'WITH') {
$this->with = new WithStatement($parser, $list);
} else {
$parser->error('Unexpected keyword.', $token);
break;
Expand Down
205 changes: 168 additions & 37 deletions src/Statements/WithStatement.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,32 +60,47 @@ final class WithStatement extends Statement
/** @var WithKeyword[] */
public $withers = [];

/**
* holds the CTE parser.
*
* @var Parser|null
*/
public $cteStatementParser;

/**
* @param Parser $parser the instance that requests parsing
* @param TokensList $list the list of tokens to be parsed
*/
public function parse(Parser $parser, TokensList $list)
{
++$list->idx; // Skipping `WITH`.

// parse any options if provided
$this->options = OptionsArray::parse($parser, $list, static::$OPTIONS);
++$list->idx;

/**
* The state of the parser.
*
* Below are the states of the parser.
*
* 0 ---------------- [ name ] -----------------> 1
* 1 -------------- [( columns )] AS ----------------> 2
* 2 ------------------ [ , ] --------------------> 0
*
* 1 ------------------ [ ( ] ------------------> 2
*
* 2 ------------------ [ AS ] -----------------> 3
*
* 3 ------------------ [ ( ] ------------------> 4
*
* 4 ------------------ [ , ] ------------------> 1
*
* 4 ----- [ SELECT/UPDATE/DELETE/INSERT ] -----> 5
*
* @var int
*/
$state = 0;
$wither = null;

++$list->idx; // Skipping `WITH`.

// parse any options if provided
$this->options = OptionsArray::parse($parser, $list, static::$OPTIONS);
++$list->idx;

for (; $list->idx < $list->count; ++$list->idx) {
/**
* Token parsed at this moment.
Expand All @@ -99,57 +114,165 @@ public function parse(Parser $parser, TokensList $list)
continue;
}

if ($token->type === Token::TYPE_NONE) {
if ($state === 0) {
if ($token->type !== Token::TYPE_NONE) {
$parser->error('The name of the CTE was expected.', $token);
break;
}

$wither = $token->value;
$this->withers[$wither] = new WithKeyword($wither);
$state = 1;
continue;
}

if ($state === 1) {
if ($token->value === '(') {
} elseif ($state === 1) {
if ($token->type === Token::TYPE_OPERATOR && $token->value === '(') {
$this->withers[$wither]->columns = Array2d::parse($parser, $list);
continue;
}

if ($token->keyword === 'AS') {
++$list->idx;
$state = 2;
continue;
} elseif ($token->type === Token::TYPE_KEYWORD && $token->keyword === 'AS') {
$state = 3;
} else {
$parser->error('Unexpected token.', $token);
break;
}
} elseif ($state === 2) {
if ($token->value === '(') {
++$list->idx;
$subList = $this->getSubTokenList($list);
if ($subList instanceof ParserException) {
$parser->errors[] = $subList;
continue;
}
if (! ($token->type === Token::TYPE_KEYWORD && $token->keyword === 'AS')) {
$parser->error('AS keyword was expected.', $token);
break;
}

$subParser = new Parser($subList);
$state = 3;
} elseif ($state === 3) {
$idxBeforeGetNext = $list->idx;

if (count($subParser->errors)) {
foreach ($subParser->errors as $error) {
$parser->errors[] = $error;
}
// We want to get the next non-comment and non-space token after $token
// therefore, the first getNext call will start with the current $idx which's $token,
// will return it and increase $idx by 1, which's not guaranteed to be non-comment
// and non-space, that's why we're calling getNext again.
$list->getNext();
$nextKeyword = $list->getNext();

if (! ($token->value === '(' && ($nextKeyword && $nextKeyword->value === 'SELECT'))) {
$parser->error('Subquery of the CTE was expected.', $token);
$list->idx = $idxBeforeGetNext;
break;
}

// Restore the index
$list->idx = $idxBeforeGetNext;

++$list->idx;
$subList = $this->getSubTokenList($list);
if ($subList instanceof ParserException) {
$parser->errors[] = $subList;
break;
}

$subParser = new Parser($subList);

if (count($subParser->errors)) {
foreach ($subParser->errors as $error) {
$parser->errors[] = $error;
}

$this->withers[$wither]->statement = $subParser;
continue;
break;
}

// There's another WITH expression to parse, go back to state=0
$this->withers[$wither]->statement = $subParser;

$state = 4;
} elseif ($state === 4) {
if ($token->value === ',') {
$list->idx++;
// There's another WITH expression to parse, go back to state=0
$state = 0;
continue;
}

// No more WITH expressions, we're done with this statement
if (
$token->type === Token::TYPE_KEYWORD && (
$token->value === 'SELECT'
|| $token->value === 'INSERT'
|| $token->value === 'UPDATE'
|| $token->value === 'DELETE'
)
) {
$state = 5;
--$list->idx;
continue;
}

$parser->error('An expression was expected.', $token);
break;
} elseif ($state === 5) {
/**
* We need to parse all of the remaining tokens becuase mostly, they are only the CTE expression
* which's mostly is SELECT, or INSERT, UPDATE, or delete statement.
* e.g: INSERT .. ( SELECT 1 ) SELECT col1 FROM cte ON DUPLICATE KEY UPDATE col_name = 3.
* The issue is that, `ON DUPLICATE KEY UPDATE col_name = 3` is related to the main INSERT query
* not the cte expression (SELECT col1 FROM cte) we need to determine the end of the expression
* to parse `ON DUPLICATE KEY UPDATE` from the InsertStatement parser instead.
*/

// Index of the last parsed token by default would be the last token in the $list, because we're
// assuming that all remaining tokens at state 4, are related to the expression.
$idxOfLastParsedToken = $list->count - 1;
// Index before search to be able to restore the index.
$idxBeforeSearch = $list->idx;
// Length of expression tokens is null by default, in order for the $subList to start
// from $list->idx to the end of the $list.
$lengthOfExpressionTokens = null;

if ($list->getNextOfTypeAndValue(Token::TYPE_KEYWORD, 'ON')) {
// (-1) because getNextOfTypeAndValue returned ON and increased the index.
$idxOfOn = $list->idx - 1;
// We want to make sure that it's `ON DUPLICATE KEY UPDATE`
$dubplicateToken = $list->getNext();
$keyToken = $list->getNext();
$updateToken = $list->getNext();
if (
$dubplicateToken && $dubplicateToken->keyword === 'DUPLICATE'
&& ($keyToken && $keyToken->keyword === 'KEY')
&& ($updateToken && $updateToken->keyword === 'UPDATE')
) {
// Index of the last parsed token will be the token before the ON Keyword
$idxOfLastParsedToken = $idxOfOn - 1;
// The length of the expression tokens would be the difference
// between the first unrelated token `ON` and the idx
// before skipping the CTE tokens.
$lengthOfExpressionTokens = $idxOfOn - $idxBeforeSearch;
}
}

// Restore the index
$list->idx = $idxBeforeSearch;

$subList = new TokensList(array_slice($list->tokens, $list->idx, $lengthOfExpressionTokens));
$subParser = new Parser($subList);
if (count($subParser->errors)) {
foreach ($subParser->errors as $error) {
$parser->errors[] = $error;
}

break;
}

$this->cteStatementParser = $subParser;

$list->idx = $idxOfLastParsedToken;
break;
}
}

// 5 is the only valid end state
if ($state !== 5) {
/**
* Token parsed at this moment.
*
* @var Token
*/
$token = $list->tokens[$list->idx];

$parser->error('Unexpected end of the WITH CTE.', $token);
}

--$list->idx;
}

Expand All @@ -165,6 +288,14 @@ public function build()
$str .= WithKeyword::build($wither);
}

$str .= ' ';

if ($this->cteStatementParser) {
foreach ($this->cteStatementParser->statements as $statement) {
$str .= $statement->build();
}
}

return $str;
}

Expand Down
Loading