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

Allow parsing KILL statement #557

Merged
merged 1 commit into from
May 23, 2024
Merged

Allow parsing KILL statement #557

merged 1 commit into from
May 23, 2024

Conversation

MoonE
Copy link
Contributor

@MoonE MoonE commented May 1, 2024

Fixes #556

@MoonE MoonE force-pushed the kill branch 2 times, most recently from ad4118c to 297929d Compare May 1, 2024 21:21
Signed-off-by: Maximilian Krög <[email protected]>
@MoonE
Copy link
Contributor Author

MoonE commented May 1, 2024

Could also use $CLAUSES to build it. Though it fails to build an invalid statement like KILL ; which may not matter.

diff --git a/src/Statements/KillStatement.php b/src/Statements/KillStatement.php
index 7040a0f3..4ef3f532 100644
--- a/src/Statements/KillStatement.php
+++ b/src/Statements/KillStatement.php
@@ -28,16 +28,22 @@ class KillStatement extends Statement
         'QUERY' => 1,
     ];
 
+    public static $CLAUSES = [
+        'KILL' => [
+            'KILL',
+            2,
+        ],
+        // Used for options.
+        '_OPTIONS' => [
+            '_OPTIONS',
+            1,
+        ],
+        'EXPRESSION' => [
+            'KILL',
+            1,
+        ],
+    ];
+
     /** @var Expression|null */
     public $processListId = null;
-
-    public function build(): string
-    {
-        $option = $this->options === null || $this->options->isEmpty()
-            ? ''
-            : ' ' . OptionsArray::build($this->options);
-        $expression = $this->processListId === null ? '' : ' ' . Expression::build($this->processListId);
-
-        return trim('KILL' . $option . $expression);
-    }
 }
diff --git a/tests/Parser/KillStatementTest.php b/tests/Parser/KillStatementTest.php
index 99b81f75..e3a1e4c8 100644
--- a/tests/Parser/KillStatementTest.php
+++ b/tests/Parser/KillStatementTest.php
@@ -53,7 +53,6 @@ class KillStatementTest extends TestCase
             ['KILL (SELECT 3 + 4)'],
             ['KILL QUERY 3'],
             ['KILL CONNECTION 3'],
-            ['KILL'],
         ];
     }
 }

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.

Looks good to me
@niconoe- or @iifawzi could you also review please ?

@williamdes williamdes changed the base branch from 5.9.x to 5.10.x May 9, 2024 08:31
@williamdes williamdes added this to the 5.10.0 milestone May 9, 2024
@niconoe-
Copy link
Contributor

niconoe- commented May 9, 2024

LGTM but I don’t know the KILL command 😅

Copy link
Contributor

@iifawzi iifawzi left a comment

Choose a reason for hiding this comment

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

Thanks for your hard work! it looks good to me, it would be awesome if we could just add the parsing logic

public static $OPTIONS = [
'CONNECTION' => 1,
'QUERY' => 1,
];
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be awesome if we could extend support of Maria KILL options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, makes sense.

*
* KILL [CONNECTION | QUERY] processlist_id
*/
class KillStatement extends Statement
Copy link
Contributor

Choose a reason for hiding this comment

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

I do believe we need some parsing logic here, so we can accurately validate the statement

public function buildKillProvider(): array
{
return [
['KILL (SELECT 3 + 4)'],
Copy link
Contributor

Choose a reason for hiding this comment

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

is it a valid use case? why would someone need to kill a query or connection that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use case may be questionable, but the query is valid and working.

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe someone can utilize this by dynamically killing queries that are locking others? by selecting their IDs from the activity? if that's the case, maybe we can also parse the nested select statements? and delegate the building of these queries to their classes? ( most probably it's only SELECT statement that would be allowed ). what do you think?

['KILL (SELECT 3 + 4)'],
['KILL QUERY 3'],
['KILL CONNECTION 3'],
['KILL'],
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be invalid, hence why the parsing logic is so important

['KILL (SELECT 3 + 4)'],
['KILL QUERY 3'],
['KILL CONNECTION 3'],
['KILL'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant KILL is what expected to be invalid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is parsed with an error, I was unsure whether it should be able to build a query that was parsed with error. It is probably not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's been a while since I interacted with the parser, so I might be mistaken. How this's parsed with an error? if there's no parse function I don't think the statement would be even parsed or validated

Copy link
Contributor

Choose a reason for hiding this comment

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

image

I believe we need the parsing logic so we can throw an error if it's incorrect, the same as we did with the CREATE statement for example

];

/** @var Expression|null */
public $processListId = null;
Copy link
Contributor

@iifawzi iifawzi May 11, 2024

Choose a reason for hiding this comment

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

I believe we should instead utilize var options instead of using custom option

    public static $OPTIONS = [
        'CONNECTION' => [
            1,
            'var',
        ],
        'QUERY' => [
            1,
            'var',
        ],
    ];

@MauricioFauth MauricioFauth merged commit 93ab20e into phpmyadmin:5.10.x May 23, 2024
12 of 17 checks passed
@MauricioFauth MauricioFauth self-assigned this May 23, 2024
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.

5 participants