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.

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

Completing, refactoring and improving the WITH statements parser #363

Merged
merged 12 commits into from
Dec 29, 2021

Conversation

iifawzi
Copy link
Contributor

@iifawzi iifawzi commented Dec 5, 2021

Hi, this PR is completing #334 (comment) and fixing #353.

I've targeted the Master branch, because I couldn't find the WithStatement parser in the QA branch, which's weird.
Anyway, I've re-designed the WITH parser to be able to catch and throw more appropriate errors, and made the CreateStatement and InsertStatement aware of it.

I've Added a bunch of comments, because there're some complex operations/decisions have been taken, hopefully comments are clear, let me know if there's anything need to be more clearified. I've also added as much tests as I could.

…g other statements aware of it

Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
@codecov
Copy link

codecov bot commented Dec 5, 2021

Codecov Report

Merging #363 (c8f72ec) into master (c78df57) will increase coverage by 0.05%.
The diff coverage is 99.04%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #363      +/-   ##
============================================
+ Coverage     95.41%   95.46%   +0.05%     
- Complexity     2024     2058      +34     
============================================
  Files            67       67              
  Lines          4336     4410      +74     
============================================
+ Hits           4137     4210      +73     
- Misses          199      200       +1     
Impacted Files Coverage Δ
src/Statements/CreateStatement.php 99.52% <95.83%> (-0.48%) ⬇️
src/Statements/InsertStatement.php 100.00% <100.00%> (ø)
src/Statements/WithStatement.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c78df57...c8f72ec. Read the comment docs.

Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
@iifawzi
Copy link
Contributor Author

iifawzi commented Dec 5, 2021

working on fixing the tests issues.

Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
tests/Builder/CreateStatementTest.php Outdated Show resolved Hide resolved
tests/Builder/CreateStatementTest.php Outdated Show resolved Hide resolved
Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>

// phpcs:disable Generic.Files.LineLength.TooLong
$expected = <<<SQL
WITH categories(identifier, name, parent_id) AS (SELECT c.identifier, c.name, c.parent_id FROM category AS `c` WHERE c.identifier = 'a' UNION ALL SELECT c.identifier, c.name, c.parent_id FROM categories, category AS `c` WHERE c.identifier = categories.parent_id), foo AS (SELECT * FROM test)
SQL;
// phpcs:enable
$this->assertEquals($expected, $parser->statements[0]->build());
$this->assertEquals('SELECT * FROM categories', $parser->statements[1]->build());
Copy link
Member

Choose a reason for hiding this comment

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

why was this needed to be removed ?

Copy link
Contributor Author

@iifawzi iifawzi Dec 7, 2021

Choose a reason for hiding this comment

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

Hi, This's removed because, previously the Parser was dealing with the CTE expression as a separate statement, that's why we had two statements. Now, the CTE is included within the WITH statement, so it should be a single statement that contain both.

That's being said, the CTE statement should exist in the with statement, which isn't happening.

$expected = <<<SQL
WITH categories(identifier, name, parent_id) AS (SELECT c.identifier, c.name, c.parent_id FROM category AS `c` WHERE c.identifier = 'a' UNION ALL SELECT c.identifier, c.name, c.parent_id FROM categories, category AS `c` WHERE c.identifier = categories.parent_id), foo AS (SELECT * FROM test)
SQL;

I will work on it. I'd need to change the build functionality of the withStatement a little bit.

Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
src/Statements/WithStatement.php Outdated Show resolved Hide resolved
tests/Parser/WithStatementTest.php Show resolved Hide resolved
@iifawzi

This comment has been minimized.

Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
@@ -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

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:

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:

$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.

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.

Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

This is really awesome ! 💯
Let me know when this is finished for a final review :)

Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>
@iifawzi
Copy link
Contributor Author

iifawzi commented Dec 11, 2021

I've just noticed a test that I've duplicated by mistake, removed.
It's now totally finished :')

@williamdes williamdes added this to the 5.6.0 milestone Dec 11, 2021
@MauricioFauth MauricioFauth merged commit 403e38d into phpmyadmin:master Dec 29, 2021
@MauricioFauth
Copy link
Member

Merged, thanks for your contribution!

MauricioFauth added a commit that referenced this pull request Dec 29, 2021
Signed-off-by: Maurício Meneghini Fauth <mauricio@fauth.dev>
@MauricioFauth MauricioFauth self-assigned this Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants